#524 Enable revocation of NOC non-root certificates

Fix bug related to removing certs from subject-key-id map Signed-off-by: Abdulbois <[email protected]> Signed-off-by: Abdulbois <[email protected]>
zigbee-alliance · Mar 20, 2024 · a0aa8a4 · a0aa8a4
1 parent f4d7477
commit a0aa8a4
Show file tree

Hide file tree

Showing 11 changed files with 83 additions and 80 deletions.
diff --git a/integration_tests/cli/pki-noc-certs.sh b/integration_tests/cli/pki-noc-certs.sh
@@ -424,7 +424,7 @@ response_does_not_contain "$result" "\"serialNumber\": \"$noc_cert_1_serial_numb
 response_does_not_contain "$result" "\"serialNumber\": \"$noc_cert_1_copy_serial_number\""
 echo $result | jq
 
-echo "Request NOC certificate by VID = $vid should contain ony leaf certificate"
+echo "Request NOC certificate by VID = $vid should contain one leaf certificate"
 result=$(dcld query pki noc-x509-certs --vid="$vid")
 echo $result | jq
 check_response "$result" "\"subject\": \"$noc_leaf_cert_1_subject\""

diff --git a/integration_tests/cli/pki-noc-revocation-with-revoking-child.sh b/integration_tests/cli/pki-noc-revocation-with-revoking-child.sh
@@ -204,7 +204,7 @@ check_response "$result" "\"serialNumber\": \"$noc_cert_2_serial_number\""
 check_response "$result" "\"serialNumber\": \"$noc_cert_2_copy_serial_number\""
 check_response "$result" "\"serialNumber\": \"$noc_leaf_cert_2_serial_number\""
 
-echo "$vendor_account Vendor revokes root NOC certificate by setting \"revoke-child\" flag to true, it should revoke child certificates too"
+echo "$vendor_account Vendor revokes non-root NOC certificate by setting \"revoke-child\" flag to true, it should revoke child certificates too"
 result=$(echo "$passphrase" | dcld tx pki revoke-noc-x509-cert --subject="$noc_cert_2_subject" --subject-key-id="$noc_cert_2_subject_key_id" --revoke-child=true --from=$vendor_account --yes)
 check_response "$result" "\"code\": 0"
 

diff --git a/x/pki/handler_revoke_noc_cert_test.go b/x/pki/handler_revoke_noc_cert_test.go
@@ -202,12 +202,6 @@ func TestHandler_RevokeNocX509Cert_RevokeDefault(t *testing.T) {
 	require.Equal(t, testconstants.NocCert1Subject, revokedNocCerts.Subject)
 	require.Equal(t, testconstants.NocCert1SubjectKeyID, revokedNocCerts.SubjectKeyId)
 
-	revokedCerts, err := queryRevokedCertificates(setup, testconstants.NocCert1Subject, testconstants.NocCert1SubjectKeyID)
-	require.NoError(t, err)
-	require.Equal(t, 2, len(revokedCerts.Certs))
-	require.Equal(t, testconstants.NocCert1Subject, revokedCerts.Subject)
-	require.Equal(t, testconstants.NocCert1SubjectKeyID, revokedCerts.SubjectKeyId)
-
 	// query noc certificate by Subject
 	_, err = queryApprovedCertificatesBySubject(setup, testconstants.NocCert1Subject)
 	require.Error(t, err)
@@ -396,11 +390,6 @@ func TestHandler_RevokeNocX509Cert_RevokeBySerialNumber(t *testing.T) {
 	require.Equal(t, 1, len(revokedNocCerts.Certs))
 	require.Equal(t, testconstants.NocCert1SerialNumber, revokedNocCerts.Certs[0].SerialNumber)
 
-	revokedCerts, err := queryRevokedCertificates(setup, testconstants.NocCert1Subject, testconstants.NocCert1SubjectKeyID)
-	require.NoError(t, err)
-	require.Equal(t, 1, len(revokedCerts.Certs))
-	require.Equal(t, testconstants.NocCert1SerialNumber, revokedCerts.Certs[0].SerialNumber)
-
 	// Child certificate should not be revoked
 	_, err = queryRevokedCertificates(setup, testconstants.NocLeafCert1Subject, testconstants.NocLeafCert1SubjectKeyID)
 	require.Equal(t, codes.NotFound, status.Code(err))

diff --git a/x/pki/keeper/approved_certificates.go b/x/pki/keeper/approved_certificates.go
@@ -155,29 +155,3 @@ func (k Keeper) verifyCertificate(ctx sdk.Context,
 		fmt.Sprintf("Certificate verification failed for certificate with subject=%v and subjectKeyID=%v",
 			x509Certificate.Subject, x509Certificate.SubjectKeyID))
 }
-
-func (k Keeper) removeCertFromList(issuer string, serialNumber string, certs *types.ApprovedCertificates) {
-	certIndex := -1
-
-	for i, cert := range certs.Certs {
-		if cert.SerialNumber == serialNumber && cert.Issuer == issuer {
-			certIndex = i
-
-			break
-		}
-	}
-	if certIndex == -1 {
-		return
-	}
-	certs.Certs = append(certs.Certs[:certIndex], certs.Certs[certIndex+1:]...)
-}
-
-func findCertificate(serialNumber string, certificates *[]*types.Certificate) (*types.Certificate, bool) {
-	for _, cert := range *certificates {
-		if cert.SerialNumber == serialNumber {
-			return cert, true
-		}
-	}
-
-	return nil, false
-}
diff --git a/x/pki/keeper/approved_certificates_by_subject_key_id.go b/x/pki/keeper/approved_certificates_by_subject_key_id.go
@@ -98,6 +98,12 @@ func (k Keeper) RemoveApprovedCertificatesBySubjectKeyID(
 	}
 }
 
+func (k Keeper) RemoveApprovedCertificatesBySubjectKeyIDAndSerialNumber(ctx sdk.Context, subject, subjectKeyID, serialNumber string) {
+	k._removeCertificatesFromSubjectKeyIDState(ctx, subjectKeyID, func(cert *types.Certificate) bool {
+		return cert.Subject == subject && cert.SubjectKeyId == subjectKeyID && cert.SerialNumber == serialNumber
+	})
+}
+
 // GetAllApprovedCertificatesBySubjectKeyID returns all approvedCertificatesBySubjectKeyId.
 func (k Keeper) GetAllApprovedCertificatesBySubjectKeyID(ctx sdk.Context) (list []types.ApprovedCertificatesBySubjectKeyId) {
 	store := prefix.NewStore(ctx.KVStore(k.storeKey), pkitypes.KeyPrefix(types.ApprovedCertificatesBySubjectKeyIDKeyPrefix))
@@ -113,3 +119,29 @@ func (k Keeper) GetAllApprovedCertificatesBySubjectKeyID(ctx sdk.Context) (list
 
 	return
 }
+
+func (k Keeper) _removeCertificatesFromSubjectKeyIDState(ctx sdk.Context, subjectKeyID string, filter func(cert *types.Certificate) bool) {
+	certs, found := k.GetApprovedCertificatesBySubjectKeyID(ctx, subjectKeyID)
+	if !found {
+		return
+	}
+
+	numCertsBefore := len(certs.Certs)
+	for i := 0; i < len(certs.Certs); {
+		cert := certs.Certs[i]
+		if filter(cert) {
+			certs.Certs = append(certs.Certs[:i], certs.Certs[i+1:]...)
+		} else {
+			i++
+		}
+	}
+
+	if len(certs.Certs) == 0 {
+		store := prefix.NewStore(ctx.KVStore(k.storeKey), pkitypes.KeyPrefix(types.ApprovedCertificatesBySubjectKeyIDKeyPrefix))
+		store.Delete(types.ApprovedCertificatesBySubjectKeyIDKey(
+			subjectKeyID,
+		))
+	} else if numCertsBefore > len(certs.Certs) { // Update state only if any certificate is removed
+		k.SetApprovedCertificatesBySubjectKeyID(ctx, certs)
+	}
+}
diff --git a/x/pki/keeper/keeper.go b/x/pki/keeper/keeper.go
@@ -76,3 +76,29 @@ func (k Keeper) EnsureVidMatches(ctx sdk.Context, owner string, signer string) e
 
 	return nil
 }
+
+func removeCertFromList(issuer string, serialNumber string, certs *[]*types.Certificate) {
+	certIndex := -1
+
+	for i, cert := range *certs {
+		if cert.SerialNumber == serialNumber && cert.Issuer == issuer {
+			certIndex = i
+
+			break
+		}
+	}
+	if certIndex == -1 {
+		return
+	}
+	*certs = append((*certs)[:certIndex], (*certs)[certIndex+1:]...)
+}
+
+func findCertificate(serialNumber string, certificates *[]*types.Certificate) (*types.Certificate, bool) {
+	for _, cert := range *certificates {
+		if cert.SerialNumber == serialNumber {
+			return cert, true
+		}
+	}
+
+	return nil, false
+}
diff --git a/x/pki/keeper/msg_server_approve_revoke_x_509_root_cert.go b/x/pki/keeper/msg_server_approve_revoke_x_509_root_cert.go
@@ -117,7 +117,7 @@ func (k msgServer) _revokeRootCertificate(
 			SubjectKeyId: cert.SubjectKeyId,
 			Certs:        []*types.Certificate{cert},
 		})
-	k.removeCertFromList(cert.Issuer, cert.SerialNumber, &certificates)
+	removeCertFromList(cert.Issuer, cert.SerialNumber, &certificates.Certs)
 
 	if len(certificates.Certs) == 0 {
 		k.RemoveApprovedCertificates(ctx, cert.Subject, cert.SubjectKeyId)
@@ -131,9 +131,6 @@ func (k msgServer) _revokeRootCertificate(
 		k.RemoveApprovedCertificatesBySubjectKeyID(ctx, cert.Subject, cert.SubjectKeyId)
 	} else {
 		k.SetApprovedCertificates(ctx, certificates)
-		k.SetApprovedCertificatesBySubjectKeyID(
-			ctx,
-			types.ApprovedCertificatesBySubjectKeyId{SubjectKeyId: cert.SubjectKeyId, Certs: certificates.Certs},
-		)
+		k.RemoveApprovedCertificatesBySubjectKeyIDAndSerialNumber(ctx, cert.Subject, cert.SubjectKeyId, serialNumber)
 	}
 }
diff --git a/x/pki/keeper/msg_server_remove_x_509_cert.go b/x/pki/keeper/msg_server_remove_x_509_cert.go
@@ -53,18 +53,19 @@ func (k msgServer) RemoveX509Cert(goCtx context.Context, msg *types.MsgRemoveX50
 		// remove from subject with serialNumber map
 		k.RemoveUniqueCertificate(ctx, certBySerialNumber.Issuer, certBySerialNumber.SerialNumber)
 
-		certs := types.ApprovedCertificates{
-			Subject:      msg.Subject,
-			SubjectKeyId: msg.SubjectKeyId,
-			Certs:        certificates,
-		}
-		k.removeCertFromList(certBySerialNumber.Issuer, certBySerialNumber.SerialNumber, &certs)
-
 		if foundApproved {
-			k._removeApprovedX509Cert(ctx, certID, certs)
+			removeCertFromList(certBySerialNumber.Issuer, certBySerialNumber.SerialNumber, &aprCerts.Certs)
+			k._removeApprovedX509Cert(ctx, certID, &aprCerts, msg.SerialNumber)
 		}
 		if foundRevoked {
-			k._removeRevokedX509Cert(ctx, certID, certs)
+			certs := types.ApprovedCertificates{
+				Subject:      revCerts.Subject,
+				SubjectKeyId: revCerts.SubjectKeyId,
+				Certs:        revCerts.Certs,
+			}
+			removeCertFromList(certBySerialNumber.Issuer, certBySerialNumber.SerialNumber, &certs.Certs)
+			revCerts.Certs = certs.Certs
+			k._removeRevokedX509Cert(ctx, certID, &revCerts)
 		}
 	} else {
 		k.RemoveApprovedCertificates(ctx, certID.Subject, certID.SubjectKeyId)
@@ -83,31 +84,24 @@ func (k msgServer) RemoveX509Cert(goCtx context.Context, msg *types.MsgRemoveX50
 	return &types.MsgRemoveX509CertResponse{}, nil
 }
 
-func (k msgServer) _removeApprovedX509Cert(ctx sdk.Context, certID types.CertificateIdentifier, certificates types.ApprovedCertificates) {
+func (k msgServer) _removeApprovedX509Cert(ctx sdk.Context, certID types.CertificateIdentifier, certificates *types.ApprovedCertificates, serialNumber string) {
 	if len(certificates.Certs) == 0 {
 		k.RemoveApprovedCertificates(ctx, certID.Subject, certID.SubjectKeyId)
 		k.RemoveApprovedCertificateBySubject(ctx, certID.Subject, certID.SubjectKeyId)
 		k.RemoveApprovedCertificatesBySubjectKeyID(ctx, certID.Subject, certID.SubjectKeyId)
 	} else {
-		k.SetApprovedCertificates(ctx, certificates)
-		k.SetApprovedCertificatesBySubjectKeyID(
-			ctx,
-			types.ApprovedCertificatesBySubjectKeyId{SubjectKeyId: certID.SubjectKeyId, Certs: certificates.Certs},
-		)
+		k.SetApprovedCertificates(ctx, *certificates)
+		k.RemoveApprovedCertificatesBySubjectKeyIDAndSerialNumber(ctx, certID.Subject, certID.SubjectKeyId, serialNumber)
 	}
 }
 
-func (k msgServer) _removeRevokedX509Cert(ctx sdk.Context, certID types.CertificateIdentifier, certificates types.ApprovedCertificates) {
+func (k msgServer) _removeRevokedX509Cert(ctx sdk.Context, certID types.CertificateIdentifier, certificates *types.RevokedCertificates) {
 	if len(certificates.Certs) == 0 {
 		k.RemoveRevokedCertificates(ctx, certID.Subject, certID.SubjectKeyId)
 	} else {
 		k.SetRevokedCertificates(
 			ctx,
-			types.RevokedCertificates{
-				Subject:      certID.Subject,
-				SubjectKeyId: certID.SubjectKeyId,
-				Certs:        certificates.Certs,
-			},
+			*certificates,
 		)
 	}
 }
diff --git a/x/pki/keeper/msg_server_revoke_noc_root_x_509_cert.go b/x/pki/keeper/msg_server_revoke_noc_root_x_509_cert.go
@@ -105,7 +105,7 @@ func (k msgServer) _revokeNocRootCertificate(
 	}
 	k.AddRevokedNocRootCertificates(ctx, revNocCerts)
 
-	k.removeCertFromList(cert.Issuer, cert.SerialNumber, &certificates)
+	removeCertFromList(cert.Issuer, cert.SerialNumber, &certificates.Certs)
 
 	if len(certificates.Certs) == 0 {
 		k.RemoveNocRootCertificate(ctx, vid, certificates.Subject, certificates.SubjectKeyId)
@@ -115,10 +115,7 @@ func (k msgServer) _revokeNocRootCertificate(
 	} else {
 		k.RemoveNocRootCertificateBySerialNumber(ctx, vid, cert.Subject, cert.SubjectKeyId, serialNumber)
 		k.SetApprovedCertificates(ctx, certificates)
-		k.SetApprovedCertificatesBySubjectKeyID(
-			ctx,
-			types.ApprovedCertificatesBySubjectKeyId{SubjectKeyId: cert.SubjectKeyId, Certs: certificates.Certs},
-		)
+		k.RemoveApprovedCertificatesBySubjectKeyIDAndSerialNumber(ctx, cert.Subject, cert.SubjectKeyId, serialNumber)
 	}
 
 	return nil

diff --git a/x/pki/keeper/msg_server_revoke_noc_x_509_cert.go b/x/pki/keeper/msg_server_revoke_noc_x_509_cert.go
@@ -80,18 +80,15 @@ func (k msgServer) _revokeNocCertificate(
 	}
 	k.AddRevokedCertificates(ctx, revCerts)
 
-	k.removeCertFromList(cert.Issuer, cert.SerialNumber, &certificates)
+	removeCertFromList(cert.Issuer, cert.SerialNumber, &certificates.Certs)
 	if len(certificates.Certs) == 0 {
 		k.RemoveNocCertificate(ctx, certificates.Subject, certificates.SubjectKeyId, vid)
 		k.RemoveApprovedCertificates(ctx, cert.Subject, cert.SubjectKeyId)
 		k.RemoveApprovedCertificateBySubject(ctx, cert.Subject, cert.SubjectKeyId)
 		k.RemoveApprovedCertificatesBySubjectKeyID(ctx, cert.Subject, cert.SubjectKeyId)
 	} else {
 		k.RemoveNocCertificateBySerialNumber(ctx, vid, cert.Subject, cert.SubjectKeyId, serialNumber)
-		k.SetApprovedCertificatesBySubjectKeyID(
-			ctx,
-			types.ApprovedCertificatesBySubjectKeyId{SubjectKeyId: cert.SubjectKeyId, Certs: certificates.Certs},
-		)
+		k.RemoveApprovedCertificatesBySubjectKeyIDAndSerialNumber(ctx, cert.Subject, cert.SubjectKeyId, serialNumber)
 		k.SetApprovedCertificates(ctx, certificates)
 	}
 

diff --git a/x/pki/keeper/msg_server_revoke_x_509_cert.go b/x/pki/keeper/msg_server_revoke_x_509_cert.go
@@ -77,7 +77,7 @@ func (k msgServer) _revokeX509Certificate(ctx sdk.Context, cert *types.Certifica
 			SubjectKeyId: cert.SubjectKeyId,
 			Certs:        []*types.Certificate{cert},
 		})
-	k.removeCertFromList(cert.Issuer, cert.SerialNumber, &certificates)
+	removeCertFromList(cert.Issuer, cert.SerialNumber, &certificates.Certs)
 	if len(certificates.Certs) == 0 {
 		k.RemoveApprovedCertificates(ctx, cert.Subject, cert.SubjectKeyId)
 		// Remove certificate identifier from issuer's ChildCertificates record
@@ -87,9 +87,6 @@ func (k msgServer) _revokeX509Certificate(ctx sdk.Context, cert *types.Certifica
 		k.RemoveApprovedCertificatesBySubjectKeyID(ctx, cert.Subject, cert.SubjectKeyId)
 	} else {
 		k.SetApprovedCertificates(ctx, certificates)
-		k.SetApprovedCertificatesBySubjectKeyID(
-			ctx,
-			types.ApprovedCertificatesBySubjectKeyId{SubjectKeyId: cert.SubjectKeyId, Certs: certificates.Certs},
-		)
+		k.RemoveApprovedCertificatesBySubjectKeyIDAndSerialNumber(ctx, cert.Subject, cert.SubjectKeyId, cert.SerialNumber)
 	}
 }