Camera TLS Client Management Cluster Impl #38895
Conversation
| namespace Clusters { | ||
| namespace Tls { | ||
|
|
||
| #if defined(TLS_CLIENT_CERTIFICATE_SIZE_PER_FABRIC) && TLS_CLIENT_CERTIFICATE_SIZE_PER_FABRIC |
There was a problem hiding this comment.
Usually this sort of config thing would get defaulted in a config header, so that over here we would just use its value and if we are not including the right config header it would fail to compile instead of silently doing the default.
| static_assert(kMaxRootCertificatesPerFabric <= 254, "Per spec, kMaxRootCertificatesPerFabric must be at most 254"); | ||
|
|
||
| // Limit is set per-fabric | ||
| static constexpr uint16_t kMaxCertificatesPerEndpoint = MAX_INT16U_VALUE; |
There was a problem hiding this comment.
| static constexpr uint16_t kMaxCertificatesPerEndpoint = MAX_INT16U_VALUE; | |
| static constexpr uint16_t kMaxCertificatesPerEndpoint = UINT16_MAX; |
is the standard thing, no?
| { | ||
| TLSEndpointStruct::Type endpoint; | ||
|
|
||
| auto err = mDelegate.GetProvisionedEndpointByIndex(i, endpoint); |
There was a problem hiding this comment.
What does the lifetime management for the Hostname field of the struct look like?
That is, how does our delegate know when it's safe to free that memory?
|
|
||
| switch (aPath.mAttributeId) | ||
| { | ||
| case MaxProvisioned::Id: { |
There was a problem hiding this comment.
This attribute does not seem to be writable in the spec?
| GetSafeAttributePersistenceProvider()->WriteScalarValue(path, mMaxProvisioned); | ||
|
|
||
| // and mark as dirty | ||
| MatterReportingAttributeChangeCallback(path); |
There was a problem hiding this comment.
This attribute is marked F in the spec, so it can only change if ConfigurationVersion is updated....
In practice, should its value just be a constructor argument for this server class?
| ChipLogDetail(Zcl, "TlsClientManagement: ProvisionEndpoint"); | ||
|
|
||
| Commands::ProvisionEndpointResponse::Type response; | ||
| Status status = mDelegate.ProvisionEndpoint(req, response.endpointID); |
There was a problem hiding this comment.
This should do constraint enforcement (Hostname length limits, CAID not allowed to be too large) before passing the data to the delegate.
| ChipLogDetail(Zcl, "TlsClientManagement: FindEndpoint"); | ||
|
|
||
| Commands::FindEndpointResponse::Type response; | ||
| Status status = mDelegate.FindProvisionedEndpointByID(req.endpointID, response.endpoint); |
There was a problem hiding this comment.
What does the lifetime management look like for the Hostname field of that endpoint struct?
| // TLS Clusters Certs Keys | ||
|
|
||
| // Number of root certs stored in table for a given endpoint, across all fabrics. | ||
| static StorageKeyName TlsRootCertEndpointCountKey(EndpointId endpoint) { return StorageKeyName::Formatted("g/tlsr/e/%x", endpoint); } |
There was a problem hiding this comment.
Do we want to have the cluster directly poke storage here, or do we want to have configurable providers that a cluster instance talks to (with a default impl that then maybe uses these keys)?
Because this setup here is not really conducive to a setup where anything that uses CHIPDeviceController implements the clusters that use these storage keys. Is having that work a goal?
Testing
CI