[C] Hub renewal stores provider-made certificates; harden managed provider storage

[Ewerton Scaboro da Silva (ewertons)]

Hub renewal stores issued certificates in the certificate provider when the provider made the CSR. The DPS operational-cert callback always gets the chain and the store result. The managed provider accepts only chains for its own key and writes files atomically with owner-only permissions.

Connection client

• send_csr(): the key owner owns the chain.
  • csr NULL: the client gets the CSR from the provider's get_csr() (subject = device id). On ISSUED it stores the chain with store_issued_certificate() before the callback; the result is in the new az_iot_csr_event.store_status. az_iot_csr_event gains _internal_size (SDK-stamped, like the state event) so fields can be appended safely. Needs a v2 provider with both hooks, else AZ_IOT_ERR_NOT_SUPPORTED before anything is published.
  • App csr: the chain is delivered, never stored (store_status = AZ_IOT_ERR_NOT_SUPPORTED).
  • The live session is kept. Subsequent connects use the stored chain; if the store fails, the previous credential stays in use.
• az_iot_operational_cert_callback gains store_result. It fires with the chain even when the store fails; the DPS registration then fails, as before. Breaking: callers must add the parameter. Breaking: az_iot_csr_event gains _internal_size as its first member, which shifts its layout; callers must rebuild.
• DPS enrollment (dps.request_operational_certificate) requires a provider with get_csr, release_csr and store_issued_certificate; open() returns AZ_IOT_ERR_NOT_SUPPORTED otherwise. The issued chain is always stored in the provider, never kept by the client. Breaking: providers without store_issued_certificate can no longer be used for DPS enrollment.
• The provider contract now says store_issued_certificate() must be all-or-nothing.
• The provider vtable has no version field (and no AZ_IOT_CERTIFICATE_PROVIDER_VTABLE_VERSION); optional hooks are gated on NULL only. Breaking: custom providers must drop .version.

Managed provider

• A store is refused unless every certificate in the chain parses (count must match) and the leaf certifies the operational key. At init, a stored chain that fails either check is ignored.
• The key and chain are written to a uniquely named, exclusively created file next to the destination (0600; owner-only DACL on Windows), flushed, then renamed over the destination. A failed write leaves the old file untouched. A symlink at the destination is replaced, not followed.
• The mode of an existing key file is not changed.

Tests: unit tests for each behavior above. The permission and symlink tests are POSIX only. On Windows, a test checks the key and chain DACLs.

Validation (Linux, gcc): unit suites pass, also under ASan/UBSan. Samples and the e2e CSR test compile with Paho. The Windows code path was syntax-checked with MinGW. clang-format 18 and the repo lint scripts pass on changed files. The hub-renewal e2e test has not been run.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Copilot review overview

🟡 Changes recommended

Public ABI compatibility and incomplete full-chain validation can cause unsafe reads or persist unusable certificate chains.

Review effort: Balanced
Findings: 4

Open (4)

• Accepts certificate files with malformed trailing chain data · New
• Accepts malformed certificate chains during atomic replacement · New
• Extending az_iot_csr_event breaks ABI compatibility · New
• Adding client state without ABI-safe layout versioning · New

[Copilot]

Copilot review overview

🟡 Changes recommended

The CSR event change breaks existing callback ABI, and v1 provider access can read beyond its declared vtable.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Gate v2 callback slot by vtable version · New
• Link directly to the named test · New

Resolved since last review (4)

• Adding client state without ABI-safe layout versioning
• Extending az_iot_csr_event breaks ABI compatibility
• Accepts malformed certificate chains during atomic replacement
• Accepts certificate files with malformed trailing chain data

Previously missed (2)

In code that hasn't changed since last review

Make sample provider credential replacement atomic

c/​inc/​azure/​iot/​az_iot_certificate_provider.h:123

This new mandatory contract is not met by the repository's complete sample provider: c/samples/common/sample_cert_provider.c:135-160 opens the destination with "wb" before writing, so a write or close failure returns an error after destroying the previous credential. Update that provider to use atomic replacement too; otherwise the public example teaches an implementation that violates this contract and can strand the next connection.

Add Windows DACL permission coverage

c/​adapters/​cert_openssl/​az_iot_certificate_provider_managed.c:133

The new owner-only Windows DACL is security-critical behavior, but the added permission tests are excluded under _WIN32; the coverage table also explicitly says this path is untested. Add a Windows unit test that creates both files and inspects their resulting DACLs, verifying access is limited to the owner and SYSTEM rather than relying only on a MinGW syntax check.

[Copilot]

Copilot review overview

🟡 Changes recommended

The event layout remains ABI-incompatible, and certificate validation still accepts empty entries and some trailing garbage.

Review effort: Balanced
Findings: 1

Open (1)

• Changing event layout breaks ABI compatibility · New

Resolved since last review (2)

• Gate v2 callback slot by vtable version
• Link directly to the named test

Previously missed (1)

In code that hasn't changed since last review

Align proposed API sketch with documented event fields

c/​docs/​eng/​certificate-management.md:360

This prose now references evt->store_status, but the immediately preceding “Proposed C surface” still defines az_iot_csr_event without either store_status or _internal_size. Update that API sketch so the design document is internally consistent with the public header.

[Copilot]

Copilot review overview

🟡 Changes recommended

The new event layout breaks ABI compatibility, and certificate-storage validation and contract consistency remain incomplete.

Review effort: Balanced
Findings: 1 · 1 · 1

Open (3)

• Changing event layout breaks ABI compatibility
• Sample provider truncates credentials before atomic replacement · New
• Documented event type lacks referenced store_status member · New

[Copilot]

Copilot review overview

🟡 Changes recommended

Persisted-chain validation can consume unbounded memory during initialization, and the documented existing-key permission behavior lacks regression coverage.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Unbounded certificate buffering can exhaust device heap · New
• Add regression test for preserving existing key file mode · New

Resolved since last review (2)

• Use exclusive unique temporary files before credential replacement
• Add Windows ACL coverage for owner-only credential access

[Copilot]

Copilot review overview

🟡 Changes recommended

The sample provider can replace a valid credential with an incomplete chain while reporting success.

Review effort: Balanced
Findings: 1 · 1 · 1

Open (3)

• Reject certificate chains containing empty entries · New
• Unbounded certificate buffering can exhaust device heap
• Add regression test for preserving existing key file mode

[Copilot]

Copilot review overview

🟡 Changes recommended

Managed storage can accept a chain that its restart path subsequently rejects due to the 64 KiB limit.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Enforce maximum chain size before storing credentials · New
• Add regression test for preserving existing key file mode

Resolved since last review (2)

• Reject certificate chains containing empty entries
• Unbounded certificate buffering can exhaust device heap

[Copilot]

Copilot review overview

🟡 Changes recommended

The managed store can allocate an arbitrarily oversized chain before enforcing its 64 KiB limit.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Enforce 64 KiB limit before appending decoded certificate entries · New
• Add regression test for preserving existing key file mode

Resolved since last review (1)

• Enforce maximum chain size before storing credentials

[Copilot]

Copilot review overview

🟡 Changes recommended

Malformed or truncated Hub responses can still trigger automatic certificate persistence.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Malformed certificate responses are persisted as valid chains · New
• Add regression test for preserving existing key file mode

Resolved since last review (1)

• Enforce 64 KiB limit before appending decoded certificate entries

[Copilot]

Copilot review overview

🔵 Needs a closer look

Security-sensitive cross-platform persistence and the unrun Hub-renewal end-to-end path require final human validation.

Review effort: Balanced
Findings: 1

Open (1)

• Add regression test for preserving existing key file mode

Resolved since last review (1)

• Malformed certificate responses are persisted as valid chains

[Ewerton Scaboro da Silva (ewertons)]

Superseded by smaller PRs: #410 (managed provider storage) and #411 (renewal CSR from the provider, chains always stored). The rest follows in separate PRs.