-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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.
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
Implemented CASEServer to fetch credentials, and wait for SigmaR1 #6791
Conversation
- Enable multiple concurrent CASE session establishmentconnectedhomeip/src/protocols/secure_channel/CASEServer.cpp Lines 82 to 92 in bb08a09
This comment was generated by todo based on a
|
- Add support of ICA certificatesconnectedhomeip/src/transport/AdminPairingTable.cpp Lines 261 to 269 in bb08a09
This comment was generated by todo based on a
|
14K text, 4K bss ... this is very large :/ |
Wow, yeah. @pan-apple ? |
Size increase report for "nrfconnect-example-build" from 8d203d7
Full report output
|
Size increase report for "esp32-example-build" from 8d203d7
Full report output
|
ChipCertificateSet mCertificates; | ||
OperationalCredentialSet mCredentials; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that we end up having to make copies of this stuff is not great. Ideally we would just hold on to the right admin and use these things in-place from there. Followup for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am expecting some refactor/cleanup once CASE state machine is hooked up on controller and device side. A lot of these overheads will iron out at that point.
ReturnErrorOnFailure(certSet.Init(kMaxNumCertsInOpCreds, kMaxChipCertSize * kMaxNumCertsInOpCreds)); | ||
|
||
ReturnErrorOnFailure( | ||
certSet.LoadCert(mRootCert, mRootCertLen, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So here, if we stored the certs as a ChipCertificateSet to start with, couldn't we just return a const ref to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am expecting some refactor/cleanup once CASE state machine is hooked up on controller and device side. A lot of these overheads will iron out at that point.
Yeah, most of this is because now some previously unused crypto code is getting linked with the final ELF. |
// Lookup the admin that corresponds to the CASE session setup request. | ||
// Each admin provisions their own credentials on the device. So it's essential to | ||
// use the correct operational certificates for CASE session setup. | ||
mAdminId = ec->GetSecureSession().GetAdminId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indexing and lookup up by Admin ID breaks scoping. Per the spec, the admin must be located based upon the root PK of the root CA for the requesting agent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. We can do that as part of multi admin feature.
Would you prefer if I updated the above comment as part of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A TODO seems appropriate. It doesn't have to happen now. I'm just pointing out that this code does not appear to be compliant with the 4.369 Validate Sigma1
algorithm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Memory usage
|
Problem
Need mechanism to
Summary of Changes
This PR adds a new class
CASEServer
, that'll perform the actions mentioned in the problem.The CASEServer will be available to device, as well as controllers.
This PR also optimizes the RAM usage for AdminPairingInfo.
Fixes #6779