Repository navigation
Conversation
…the CSRNonce The commissioner did not compare the CSRNonce in the NOCSR elements with the nonce it sent in CSRRequest, which Core § 6.4.6.1.1 requires. Without the check, a CSRResponse to an earlier CSRRequest in the same session was accepted, and the commissioner built the NOC from that older key. The CSRResponse checks move from the commissioning flow into NocsrValidator, next to DeviceAttestationValidator: empty response, DAC signature, NOCSR decoding and the CSR itself. The CSRNonce check is new. Each failure throws NocsrError with the check that failed. No attestation policy can accept these failures. Both validators use one rule to tell a defect of our code from bad device data. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Tick the box to add this pull request to the merge queue (same as
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The validator throws a raw TypeError when mandatory fields are omitted and accepts signed NOCSRElements exceeding the 900-byte limit, requiring explicit validation and regression tests.
2 open issues
What changed in this PR
This PR closes a same-session CSR replay gap by checking the returned CSRNonce before the commissioner issues operational credentials.
Changes:
- Adds and exports
NocsrValidatorwith classified validation failures. - Integrates validation into commissioning and shares local-defect detection with device attestation.
- Adds unit tests, a commissioning regression test, and changelog entries.
Build, lint, formatting, and tests were not run in this review environment.
| File | Description |
|---|---|
packages/protocol/test/certificate/NocsrValidatorTest.ts |
Tests valid responses, validation failures, and local-error propagation. |
packages/protocol/src/peer/ControllerCommissioningFlow.ts |
Retains the request nonce and delegates CSRResponse validation. |
packages/protocol/src/certificate/PeerData.ts |
Adds shared local-defect classification. |
packages/protocol/src/certificate/NocsrValidator.ts |
Implements CSRResponse validation and classified errors. |
packages/protocol/src/certificate/index.ts |
Exports the new validator API. |
packages/protocol/src/certificate/DeviceAttestationValidator.ts |
Uses the shared error-classification helper. |
packages/node/test/node/CsrNonceCommissioningTest.ts |
Tests commissioning rejection of a signed mismatched nonce. |
CHANGELOG.md |
Documents the validator and commissioning behavior changes. |
Review effort: Balanced
| { crypto, attestationChallenge, csrNonce, dacPublicKey }: Context, | ||
| { nocsrElements, attestationSignature }: Response, | ||
| ): Promise<Bytes> { | ||
| if (nocsrElements.byteLength === 0 || attestationSignature.byteLength === 0) { |
| { crypto, attestationChallenge, csrNonce, dacPublicKey }: Context, | ||
| { nocsrElements, attestationSignature }: Response, | ||
| ): Promise<Bytes> { | ||
| if (nocsrElements.byteLength === 0 || attestationSignature.byteLength === 0) { |
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
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.
Type of change
Description
The commissioner sent a random CSRNonce in CSRRequest but never compared it with the CSRNonce in the NOCSR elements of the CSRResponse. Core § 6.4.6.1.1 says: "The CSR Nonce in NOCSR Information SHALL match the Commissioner's CSR Nonce." Without the check, a CSRResponse to an earlier CSRRequest in the same session was accepted, and the commissioner built the NOC from that older key. A response from another session already failed, because the DAC signature covers the session's attestation challenge.
Why a validator. The commissioning flow checked the CSRResponse inline. The checks for the AttestationResponse live in
DeviceAttestationValidator, with a named check per failure and their own unit tests.NocsrValidatordoes the same for the CSRResponse: empty response, the DAC signature, decoding the NOCSR elements, the CSRNonce (new), and the CSR itself. A failed check throwsNocsrErrorwith theNocsrCheckthat failed. Before, an empty or undecodable response threwUnexpectedDataErroror a TLV error, and the cause of a failed signature check was dropped. LikeDeviceAttestationValidator, the validator passesInternalErrorandImplementationErrorthrough and reports any other error as a failure of the device's data; both validators now use one function for that rule.All these checks are hard failures. Unlike device attestation findings, the attestation policy cannot accept them. The flow now throws
InternalErrorif the DAC public key is missing at this step; before, it skipped the signature check without notice. The attestation step always sets the key or fails, so this does not happen today.When a check fails, the commissioner tries to end the device's fail-safe, as it does when other errors stop commissioning, unless the device stopped responding.
Tests: unit tests for each check, and a commissioning test with a device that signs its NOCSR elements over a changed nonce. Removing any single
NocsrCheckcheck makes its test fail.Checklist
npm testpassesnpm run format-verifyandnpm run lintpass🤖 Generated with Claude Code