Camera TLS Certificate Management Cluster Impl #39754
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Code Review
This pull request introduces the implementation of Camera TLS Certificate Management clusters. The changes involve adding new cluster definitions in .matter and .zap files, and providing their server-side C++ implementations. The review identified several areas for improvement, including ensuring proper handling of external storage options for attributes, addressing a critical potential null pointer dereference, resolving inconsistencies in attribute storage types, and completing the implementation of stubbed-out delegate methods. Additionally, attention should be paid to hardcoded limits and potential ID wrap-around issues.
examples/all-clusters-app/all-clusters-common/src/tls-client-management-instance.cpp
Outdated
Show resolved
Hide resolved
examples/all-clusters-app/all-clusters-common/src/tls-certificate-management-instance.cpp
Outdated
Show resolved
Hide resolved
examples/all-clusters-app/all-clusters-common/src/tls-certificate-management-instance.cpp
Show resolved
Hide resolved
examples/all-clusters-app/all-clusters-common/src/tls-client-management-instance.cpp
Outdated
Show resolved
Hide resolved
|
PR #39754: Size comparison from 06523c2 to 52a8109 Full report (26 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, qpg, stm32, telink)
|
52a8109 to
b533091
Compare
|
PR #39754: Size comparison from 2a72083 to b533091 Full report (17 builds for cc13x4_26x4, cc32xx, efr32, telink)
|
b533091 to
8bbc081
Compare
|
PR #39754: Size comparison from 2a72083 to 8bbc081 Increases above 0.2%:
Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
8bbc081 to
ca118ff
Compare
|
PR #39754: Size comparison from e0f8e39 to ca118ff Increases above 0.2%:
Full report (66 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, linux, nxp, psoc6, qpg, stm32, telink, tizen)
|
src/app/clusters/tls-certificate-management-server/CertificateTable.h
Outdated
Show resolved
Hide resolved
| ReturnErrorOnFailure(DataModel::Encode(writer, TLV::ContextTag(Fields::kClientCertificate), data.clientCertificate)); | ||
| ReturnErrorOnFailure( | ||
| DataModel::Encode(writer, TLV::ContextTag(Fields::kIntermediateCertificates), data.intermediateCertificates)); | ||
| // Fields::kFabricIndex filled from table in GetClientCertificateEntry |
There was a problem hiding this comment.
This seems to be a different encoding than the IM encoding, with different invariants.... but the naming makes it sound like the same thing.
There was a problem hiding this comment.
Sorry not sure I understood what you meant - what do you mean by "different encoding than the IM encoding, with different invariants"?
There was a problem hiding this comment.
I mean that in the normal IM wire encoding for these structs, there is always a FabricIndex, for example.
Comment on lines
+17
to
+18
| "tls-certificate-management-server.cpp", | ||
| "tls-certificate-management-server.h", |
There was a problem hiding this comment.
What is app-config-dependent in these files?
There was a problem hiding this comment.
I just copied the pattern used by other clusters so deferring to @andreilitvin, but I believe this is due to the include of app/SafeAttributePersistenceProvider.h
There was a problem hiding this comment.
I don't think SafeAttributePersistenceProvider is app-config-dependent.... and other clusters just did mass-changes to put everything in "app config dependent" until they get converted to the new setup...
Though I guess this cluster is not using "the new setup" either, but I am not sure where the dependency on app config is, if anywhere.
| switch (aPath.mAttributeId) | ||
| { | ||
| default: | ||
| // Unknown attribute |
There was a problem hiding this comment.
Why are we overriding this at all, then?
src/app/clusters/tls-certificate-management-server/tls-certificate-management-server.cpp
Show resolved
Hide resolved
| * @param[in] loadedCallback lambda to execute for each certificate. If this function returns an error result, | ||
| * iteration stops and returns that same error result. | ||
| */ | ||
| virtual CHIP_ERROR LoadedRootCerts(EndpointId matterEndpoint, FabricIndex fabric, |
There was a problem hiding this comment.
I don't quite understand why this is "Loaded" in the past tense. It sounds like a notification, but that's not what this is, right?
There was a problem hiding this comment.
Somewhat - basically it loads the cert into memory from persistence, thus it is a callback/notification of "the cert has been loaded from persistence -> memory"
But I can change it to something less weird, maybe ForEachRootCert?
| * | ||
| * @param[in] matterEndpoint The matter endpoint to query against | ||
| * @param[in] fingerprint The fingerprint of the root certificate to find. | ||
| * @param[out] loadedCallback The lambda to execute with the loaded root cert. |
There was a problem hiding this comment.
Does this lambda need to be called synchronously before LookupRootCert returns (if it's called at all)?
There was a problem hiding this comment.
Yes, there's a doc in @brief: The certificates passed to loadedCallback has a guaranteed lifetime of the method call.
| * | ||
| * @param[in] matterEndpoint The matter endpoint to query against | ||
| * @param[in] fingerprint The fingerprint of the root certificate to find. | ||
| * @param[out] loadedCallback The lambda to execute with the generated client CSR. |
There was a problem hiding this comment.
This must be called synchronously before GenerateClientCsr() returns, right?
src/app/clusters/tls-certificate-management-server/tls-certificate-management-server.h
Show resolved
Hide resolved
src/app/clusters/tls-certificate-management-server/tls-certificate-management-server.h
Show resolved
Hide resolved
d3f1ece to
72f5c17
Compare
|
PR #39754: Size comparison from 07fe740 to 2b569b1 Full report (13 builds for bl602, bl702, bl702l, cc13x4_26x4, linux, stm32)
|
|
PR #39754: Size comparison from 07fe740 to 653e399 Increases above 0.2%:
Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #39754: Size comparison from 5dc7824 to b79c696 Full report (3 builds for cc32xx, stm32)
|
samadDotDev
reviewed
Jul 10, 2025
Comment on lines
+123
to
+126
| TLSCertStruct::Type idOnlyCert; | ||
| idOnlyCert.fabricIndex = cert.fabricIndex; | ||
| idOnlyCert.caid = cert.caid; | ||
| return encoder.Encode(idOnlyCert); |
There was a problem hiding this comment.
Shouldn't this be "id only cert" when retrieved over a transport that isn't capable of large transport, otherwise encode the whole thing including the cert contents?
Comment on lines
+131
to
+140
| TlsCertificateManagementServer::EncodeProvisionedClientCertificates(EndpointId matterEndpoint, FabricIndex fabric, | ||
| const AttributeValueEncoder::ListEncodeHelper & encoder) | ||
| { | ||
| return mDelegate.LoadedClientCerts(matterEndpoint, fabric, [&](auto & cert) -> CHIP_ERROR { | ||
| TLSClientCertificateDetailStruct::Type idOnlyCert; | ||
| idOnlyCert.fabricIndex = cert.fabricIndex; | ||
| idOnlyCert.ccdid = cert.ccdid; | ||
| return encoder.Encode(idOnlyCert); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Same thing here, the client cert contents should be encoded when the underlying transport supports the large payload
There was a problem hiding this comment.
Is the test plan for this script documented somewhere? I see an open skeleton TP PR but that doesn't seem to have any of the steps exercised here
Comment on lines
+185
to
+187
| for cert in attribute_certs: | ||
| cert_ids.add(cert.caid) | ||
| asserts.assert_is_none(cert.certificate, "ProvisionedRootCertificates SHALL NOT include certificate ") |
There was a problem hiding this comment.
Shouldn't this be asserted only after a step/pre-req that ensures the cert content is only emitted when a non-TCP transport is in use? and then possibly another test step that "upgrades"/re-establishes the session to over a large payload capable one and assert that certificate contents are returned correctly
samadDotDev
reviewed
Jul 10, 2025
Comment on lines
+207
to
+209
| ProvisionRootCertificateResponse::Type response; | ||
| auto status = mDelegate.ProvisionRootCert(ctx.mRequestPath.mEndpointId, ctx.mCommandHandler.GetAccessingFabricIndex(), req, | ||
| response.caid); |
There was a problem hiding this comment.
Before forwarding to the delegate, the provisioning handler should also check if the same cert already exists which will return CertificateAlreadyInstalled error.
If the CAID is null:
...
If the Node already has the certificate installed:
Fail the command with a cluster-specific status code of CertificateAlreadyInstalled, and
end processing with no other side effects.
We could possibly load the certs here from the delegate (as also done for the read encoding above) and check for existence, or return this cluster specific error from the delegate. But I think the former may be the preferred approach just so we have it at a common server cluster handling spot and don't rely on cert tests to validate if various apps implementing the delegate did the right thing.
samadDotDev
reviewed
Jul 10, 2025
Comment on lines
+318
to
+328
| void TlsCertificateManagementServer::HandleProvisionClientCertificate(HandlerContext & ctx, | ||
| const ProvisionClientCertificate::DecodableType & req) | ||
| { | ||
| 10000 ChipLogDetail(Zcl, "TlsCertificateManagement: ProvisionClientCertificate"); | ||
|
|
||
| VerifyOrReturn(req.ccdid <= kMaxClientCertId, ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Status::ConstraintError)); | ||
| const auto & detail = req.clientCertificateDetails; | ||
| VerifyOrReturn(!detail.clientCertificate.HasValue() || detail.clientCertificate.Value().size() <= kSpecMaxCertBytes, | ||
| ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Status::ConstraintError)); | ||
|
|
||
| auto status = mDelegate.ProvisionClientCert(ctx.mRequestPath.mEndpointId, ctx.mCommandHandler.GetAccessingFabricIndex(), req); |
There was a problem hiding this comment.
Same thing with client certificate provisioning, the existing cert should be checked before forwarding to the delegate and should return the spec defined error.
In the current form, it ends up allocating duplicate certificate entries and allocating a new ID to each of them.
andy31415
reviewed
Jul 10, 2025
| @@ -69,6 +69,7 @@ class ScopedMemoryBufferBase | |||
| /** Check if a buffer is valid */ | |||
| explicit operator bool() const { return mBuffer != nullptr; } | |||
| bool operator!() const { return mBuffer == nullptr; } | |||
| inline bool IsNull() const { return mBuffer == nullptr; } | |||
There was a problem hiding this comment.
could we split out some of the support changes in separate PRs?
These seem to be independent (but required) by the TLS cluster.
That way I can move obviously see (and ask for) tests for these support class changes...
andy31415
reviewed
Jul 10, 2025
| /// RAII class for allocation that guarantees that Free() will be called. | ||
| /// This is effectively a simple unique_ptr, except calling Platform::Delete instead of delete | ||
| template <typename T, class MemoryManagement = Impl::PlatformMemoryManagement> | ||
| class AutoDelete : private Impl::ScopedMemoryBufferBase<MemoryManagement> |
bzbarsky-apple
requested changes
Jul 10, 2025
| @@ -195,6 +201,49 @@ class ScopedMemoryBuffer : public Impl::ScopedMemoryBufferBase<MemoryManagement> | |||
| } | |||
| }; | |||
|
|
|||
| /// RAII class for allocation that guarantees that Free() will be called. | |||
| /// This is effectively a simple unique_ptr, except calling Platform::Delete instead of delete | |||
There was a problem hiding this comment.
How is this different from Platform::UniquePtr?
I agree with @andy31415: can we put these core changes into a separate PR so they can be reviewed on their own?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Camera TLS Certificate Management Cluster Implementation
Related issues
#38895
Testing
CI