Updates for Operational Credentials cert tests (#1506)

* Updates for Operational Credentials cert tests - Modify a couple of Operational Credentials errors to return NocResponse with cluster status code rather than throwing StatusResponseError. Not sure why this is preferable but required by cert tests - Change status code to that expected by cert tests, although they seem a little arbitrary - Allow root cert to be set in an update timed op as CHIP does but fail subsequent UpdateNoc. This seems nonsensical but is required by TC_OPCREDS_3_4 * Revert to specified error handling This will fail current implementation of OPCRED_3_4 but is correct per specification. Will handle on testing side.
project-chip · Dec 12, 2024 · 21c9020 · 21c9020
1 parent a2d3d91
commit 21c9020
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 25 deletions.
diff --git a/packages/node/src/behaviors/operational-credentials/OperationalCredentialsServer.ts b/packages/node/src/behaviors/operational-credentials/OperationalCredentialsServer.ts
@@ -119,7 +119,7 @@ export class OperationalCredentialsServer extends OperationalCredentialsBehavior
 
         if (isForUpdateNoc && this.session.isPase) {
             throw new StatusResponseError(
-                "csrRequest for UpdateNoc received on a PASE session.",
+                "csrRequest for UpdateNoc received on a PASE session",
                 StatusCode.InvalidCommand,
             );
         }
@@ -128,7 +128,7 @@ export class OperationalCredentialsServer extends OperationalCredentialsBehavior
         const failsafeContext = commissioner.failsafeContext;
         if (failsafeContext.fabricIndex !== undefined) {
             throw new StatusResponseError(
-                `csrRequest received after ${failsafeContext.forUpdateNoc ? "UpdateNOC" : "AddNOC"} already invoked.`,
+                `csrRequest received after ${failsafeContext.forUpdateNoc ? "UpdateNOC" : "AddNOC"} already invoked`,
                 StatusCode.ConstraintError,
             );
         }
@@ -205,28 +205,28 @@ export class OperationalCredentialsServer extends OperationalCredentialsBehavior
 
         if (failsafeContext.fabricIndex !== undefined) {
             throw new StatusResponseError(
-                `addNoc received after ${failsafeContext.forUpdateNoc ? "UpdateNOC" : "AddNOC"} already invoked.`,
+                `AddNoc is illegal after ${failsafeContext.forUpdateNoc ? "UpdateNOC" : "AddNOC"} in the same failsafe context`,
                 StatusCode.ConstraintError,
             );
         }
 
         if (!failsafeContext.hasRootCert) {
             return {
                 statusCode: OperationalCredentials.NodeOperationalCertStatus.InvalidNoc,
-                debugText: "Root certificate not found.",
+                debugText: "Root certificate not found",
             };
         }
 
         if (failsafeContext.csrSessionId !== this.session.id) {
             return {
                 statusCode: OperationalCredentials.NodeOperationalCertStatus.MissingCsr,
-                debugText: "CSR not found in failsafe context.",
+                debugText: "CSR not found in failsafe context",
             };
         }
 
         if (failsafeContext.forUpdateNoc) {
             throw new StatusResponseError(
-                `addNoc received after csr request was invoked for UpdateNOC.`,
+                `AddNoc is illegal after CsrRequest for UpdateNOC in same failsafe context`,
                 StatusCode.ConstraintError,
             );
         }
@@ -235,7 +235,7 @@ export class OperationalCredentialsServer extends OperationalCredentialsBehavior
         if (state.commissionedFabrics >= state.supportedFabrics) {
             return {
                 statusCode: OperationalCredentials.NodeOperationalCertStatus.TableFull,
-                debugText: `No more fabrics can be added because limit ${state.supportedFabrics} reached.`,
+                debugText: `No more fabrics can be added because limit ${state.supportedFabrics} reached`,
             };
         }
 
@@ -268,7 +268,7 @@ export class OperationalCredentialsServer extends OperationalCredentialsBehavior
 
         try {
             if (this.session.isPase) {
-                logger.debug(`Add Fabric ${fabric.fabricIndex} to PASE session ${this.session.name}.`);
+                logger.debug(`Add Fabric ${fabric.fabricIndex} to PASE session ${this.session.name}`);
                 this.session.addAssociatedFabric(fabric);
             }
 
@@ -277,7 +277,7 @@ export class OperationalCredentialsServer extends OperationalCredentialsBehavior
             const existingNocIndex = this.state.nocs.findIndex(n => n.fabricIndex === fabric.fabricIndex);
             if (existingFabricIndex !== -1 || existingNocIndex !== -1) {
                 throw new MatterFlowError(
-                    `FabricIndex ${fabric.fabricIndex} already exists in state. This should not happen.`,
+                    `FabricIndex ${fabric.fabricIndex} already exists in state. This should not happen`,
                 );
             }
         } catch (e) {
@@ -308,32 +308,35 @@ export class OperationalCredentialsServer extends OperationalCredentialsBehavior
 
         if (timedOp.fabricIndex !== undefined) {
             throw new StatusResponseError(
-                `updateNoc received after ${timedOp.forUpdateNoc ? "UpdateNOC" : "AddNOC"} already invoked.`,
+                `UpdateNoc is illegal after ${timedOp.forUpdateNoc ? "UpdateNOC" : "AddNOC"} in same failsafe context`,
                 StatusCode.ConstraintError,
             );
         }
 
-        if (timedOp.forUpdateNoc) {
+        if (timedOp.forUpdateNoc === false) {
             throw new StatusResponseError(
-                `addNoc received after csr request was invoked for UpdateNOC.`,
+                "UpdateNoc is illegal after CsrRequest for AddNOC in same failsafe context",
                 StatusCode.ConstraintError,
             );
         }
 
-        if (timedOp.hasRootCert) {
+        if (timedOp.rootCertSet) {
             throw new StatusResponseError(
-                "Trusted root certificate added in this session which is now allowed for UpdateNOC.",
+                "UpdateNoc is illegal after AddTrustedRootCertificate in same failsafe context",
                 StatusCode.ConstraintError,
             );
         }
 
-        if (!timedOp.forUpdateNoc) {
-            throw new StatusResponseError("csrRequest not invoked for UpdateNOC.", StatusCode.ConstraintError);
+        if (timedOp.forUpdateNoc === undefined) {
+            throw new StatusResponseError(
+                "UpdateNoc is illegal before CsrRequest in same failsafe context",
+                StatusCode.ConstraintError,
+            );
         }
 
         if (this.session.associatedFabric.fabricIndex !== timedOp.associatedFabric?.fabricIndex) {
             throw new StatusResponseError(
-                "Fabric of this session and the failsafe context do not match.",
+                "Fabric of this session and the failsafe context do not match",
                 StatusCode.ConstraintError,
             );
         }
@@ -395,16 +398,19 @@ export class OperationalCredentialsServer extends OperationalCredentialsBehavior
     override addTrustedRootCertificate({ rootCaCertificate }: OperationalCredentials.AddTrustedRootCertificateRequest) {
         const failsafeContext = this.#failsafeContext;
 
-        if (failsafeContext.hasRootCert) {
+        // TC_OPCREDS_3_4 fails if we don't allow set of the root certificate in updates, even though that's illegal and
+        // UpdateNoc will subsequently fail.  So we can't just test for presence of root certificate; we actually need
+        // to test if it's been set
+        if (failsafeContext.rootCertSet) {
             throw new StatusResponseError(
-                "Trusted root certificate already added in this FailSafe context.",
+                "Trusted root certificate already added in this FailSafe context",
                 StatusCode.ConstraintError,
             );
         }
 
         if (failsafeContext.fabricIndex !== undefined) {
             throw new StatusResponseError(
-                `Cannot add trusted root certificates after ${failsafeContext.forUpdateNoc ? "UpdateNOC" : "AddNOC"}.`,
+                `Cannot add trusted root certificates after ${failsafeContext.forUpdateNoc ? "UpdateNOC" : "AddNOC"}`,
                 StatusCode.ConstraintError,
             );
         }

diff --git a/packages/protocol/src/common/FailsafeContext.ts b/packages/protocol/src/common/FailsafeContext.ts
@@ -33,6 +33,7 @@ export abstract class FailsafeContext {
     #csrSessionId?: number;
     #forUpdateNoc?: boolean;
     #fabricBuilder = new FabricBuilder();
+    #rootCertSet = false;
 
     #commissioned = AsyncObservable<[], void>();
 
@@ -93,8 +94,12 @@ export abstract class FailsafeContext {
         return this.#forUpdateNoc;
     }
 
+    get rootCertSet() {
+        return this.#rootCertSet;
+    }
+
     get hasRootCert() {
-        return this.#fabricBuilder.hasRootCert();
+        return this.#fabricBuilder.rootCert !== undefined;
     }
 
     get rootCert() {
@@ -183,6 +188,7 @@ export abstract class FailsafeContext {
     /** Handles adding a trusted root certificate from Operational Credentials cluster. */
     setRootCert(rootCert: Uint8Array) {
         this.#fabricBuilder.setRootCert(rootCert);
+        this.#rootCertSet = true;
     }
 
     /**

diff --git a/packages/protocol/src/fabric/Fabric.ts b/packages/protocol/src/fabric/Fabric.ts
@@ -368,10 +368,6 @@ export class FabricBuilder {
         return this.#rootCert;
     }
 
-    hasRootCert() {
-        return this.#rootCert !== undefined;
-    }
-
     setOperationalCert(operationalCert: Uint8Array, intermediateCACert?: Uint8Array) {
         if (intermediateCACert !== undefined && intermediateCACert.length === 0) {
             intermediateCACert = undefined;