Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Need to rename Darwin AttestationInfo and CSRInfo interfaces #23439

Closed
bzbarsky-apple opened this issue Nov 2, 2022 · 3 comments · Fixed by #23893
Closed

Need to rename Darwin AttestationInfo and CSRInfo interfaces #23439

bzbarsky-apple opened this issue Nov 2, 2022 · 3 comments · Fixed by #23893
Labels
api darwin v1.1

Comments

@bzbarsky-apple
Copy link
Contributor

bzbarsky-apple commented Nov 2, 2022 •
edited
Loading

They should be called MTRAttestationInfo and MTRCSRInfo. That was asked for during review, but apparently the PR author chose to ignore the review comments...
@bzbarsky-apple bzbarsky-apple changed the title Need to rename Darwin AttestationInfo interface Need to rename Darwin AttestationInfo and CSRInfo interfaces Nov 2, 2022
@bzbarsky-apple
Copy link
Contributor Author

In fact, the code around src/darwin/Framework/CHIP/MTRNOCChainIssuer.h and its callers needs revamping in general: right now it can pretty trivially lead to deadlock.

@bzbarsky-apple
Copy link
Contributor Author

bzbarsky-apple commented Dec 2, 2022 •
edited
Loading

Current plan is:

  1. Rename MTRNOCChainIssuer to MTROperationalCertificateIssuer, including renaming the file.
  2. Rename onNOCChainGenerationNeeded to issueOperationalCertificateForRequest:attestionInfo:completion
  3. Remove the NSError outparam argument from MTRNOCChainGenerationCompleteHandler (and probably rename the type to avoid "NOC").
  4. Fix the broken nullability bits (e.g. not allowing a nil intermediate).
  5. Make all the queuing async.

@bzbarsky-apple
Copy link
Contributor Author

Alternately, s/CertificateIssuer/CA/ (e.g. MTROperationalCA).

bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Dec 3, 2022
@bzbarsky-apple
Fix APIs involved in the MTROperationalCredentialsDelegate.
3b5e74d 
* Rename CSRInfo to MTROperationalCSRInfo
* Rename the properties of MTROperationalCSRInfo to match the spec more closely.
* Rename AttestationInfo to MTRAttestationInfo.
* Fix naming of the API on MTRDeviceController to follow conventions better,
  document the API more clearly, and have it return errors as needed.
* Rename MTRNOCChainIssuer to MTROperationalCertificateIssuer
* Change signature of the one method on MTROperationalCertificateIssuer to have
  better naming, take a controller, and name the completion block "completion".
* Change the completion signature on MTROperationalCertificateIssuer to allow
  the external certificate issuer to return an error (e.g. if its device
  attestation checks failed).
* Don't ask the external issuer for the IPK, since we should already have that
  anyway (from our controller init).
* Allow the external issuer to return nil for the intermediate certificate, to
  indicate that there isn't one.
* Make sure that all our access to mOnNOCCompletionCallback happens on the
  Matter queue, so we don't have thread races on that member.
* Make all the dispatch we do as part of the credential-issuing process async.
* Introduce backwards-compat shims for all the API changes for now.

Fixes project-chip#23439
@bzbarsky-apple bzbarsky-apple mentioned this issue Dec 3, 2022
Merged
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Dec 5, 2022
@bzbarsky-apple
Fix APIs involved in the MTROperationalCredentialsDelegate.
9a7a315 
* Rename CSRInfo to MTROperationalCSRInfo
* Rename the properties of MTROperationalCSRInfo to match the spec more closely.
* Rename AttestationInfo to MTRAttestationInfo.
* Fix naming of the API on MTRDeviceController to follow conventions better,
  document the API more clearly, and have it return errors as needed.
* Rename MTRNOCChainIssuer to MTROperationalCertificateIssuer
* Change signature of the one method on MTROperationalCertificateIssuer to have
  better naming, take a controller, and name the completion block "completion".
* Change the completion signature on MTROperationalCertificateIssuer to allow
  the external certificate issuer to return an error (e.g. if its device
  attestation checks failed).
* Don't ask the external issuer for the IPK, since we should already have that
  anyway (from our controller init).
* Allow the external issuer to return nil for the intermediate certificate, to
  indicate that there isn't one.
* Make sure that all our access to mOnNOCCompletionCallback happens on the
  Matter queue, so we don't have thread races on that member.
* Make all the dispatch we do as part of the credential-issuing process async.
* Introduce backwards-compat shims for all the API changes for now.

Fixes project-chip#23439
bzbarsky-apple added a commit that referenced this issue Dec 12, 2022
@bzbarsky-apple
Fix APIs involved in the MTROperationalCredentialsDelegate. (#23893)
6fc4beb 
* Fix APIs involved in the MTROperationalCredentialsDelegate.

* Rename CSRInfo to MTROperationalCSRInfo
* Rename the properties of MTROperationalCSRInfo to match the spec more closely.
* Rename AttestationInfo to MTRAttestationInfo.
* Fix naming of the API on MTRDeviceController to follow conventions better,
  document the API more clearly, and have it return errors as needed.
* Rename MTRNOCChainIssuer to MTROperationalCertificateIssuer
* Change signature of the one method on MTROperationalCertificateIssuer to have
  better naming, take a controller, and name the completion block "completion".
* Change the completion signature on MTROperationalCertificateIssuer to allow
  the external certificate issuer to return an error (e.g. if its device
  attestation checks failed).
* Don't ask the external issuer for the IPK, since we should already have that
  anyway (from our controller init).
* Allow the external issuer to return nil for the intermediate certificate, to
  indicate that there isn't one.
* Make sure that all our access to mOnNOCCompletionCallback happens on the
  Matter queue, so we don't have thread races on that member.
* Make all the dispatch we do as part of the credential-issuing process async.
* Introduce backwards-compat shims for all the API changes for now.

Fixes #23439

* Address review comment.

* Address review comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
api darwin v1.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix APIs involved in the MTROperationalCredentialsDelegate. bzbarsky-apple/connectedhomeip
1 participant
@bzbarsky-apple