#524 Enable revocation of NOC certificates

Minor refactoring due to comments of PR Signed-off-by: Abdulbois <abdulbois.tursunov@dsr-corporation.com> Signed-off-by: Abdulbois <abdulbois123@gmail.com>
zigbee-alliance · Mar 14, 2024 · 2b10f5e · 2b10f5e
1 parent bd7df90
commit 2b10f5e
Show file tree

Hide file tree

Showing 9 changed files with 96 additions and 21 deletions.
diff --git a/integration_tests/cli/pki-noc-certs.sh b/integration_tests/cli/pki-noc-certs.sh
@@ -315,9 +315,11 @@ response_does_not_contain "$result" "\"serialNumber\": \"$noc_root_cert_1_serial
 response_does_not_contain "$result" "\"serialNumber\": \"$noc_root_cert_1_copy_serial_number\""
 echo $result | jq
 
-echo "Request NOC certificate by VID must not contain revoked root certificates"
+echo "Request NOC root certificate by VID = $vid must not contain revoked root certificates"
 result=$(dcld query pki noc-x509-root-certs --vid="$vid")
-check_response "$result" "Not Found"
+check_response "$result" "\"subject\": \"$noc_cert_2_subject\""
+check_response "$result" "\"subjectKeyId\": \"$noc_cert_2_subject_key_id\""
+check_response "$result" "\"serialNumber\": \"$noc_cert_2_serial_number\""
 response_does_not_contain "$result" "\"subject\": \"$noc_root_cert_1_subject\""
 response_does_not_contain "$result" "\"subjectKeyId\": \"$noc_root_cert_1_subject_key_id\""
 response_does_not_contain "$result" "\"serialNumber\": \"$noc_root_cert_1_serial_number\""

diff --git a/integration_tests/cli/pki-noc-revocation-with-revoking-child.sh b/integration_tests/cli/pki-noc-revocation-with-revoking-child.sh
@@ -100,7 +100,7 @@ response_does_not_contain "$result" "\"serialNumber\": \"$noc_root_cert_1_serial
 response_does_not_contain "$result" "\"serialNumber\": \"$noc_root_cert_1_copy_serial_number\""
 echo $result | jq
 
-echo "Request NOC certificate by VID should be empty"
+echo "Request NOC root certificate by VID = $vid should be empty"
 result=$(dcld query pki noc-x509-root-certs --vid="$vid")
 check_response "$result" "Not Found"
 response_does_not_contain "$result" "\"subject\": \"$noc_root_cert_1_subject\""
@@ -109,14 +109,14 @@ response_does_not_contain "$result" "\"serialNumber\": \"$noc_root_cert_1_serial
 response_does_not_contain "$result" "\"serialNumber\": \"$noc_root_cert_1_copy_serial_number\""
 echo $result | jq
 
-echo "Request all certificates by subject should be empty"
+echo "Request all certificates by NOC root certificate's subject should be empty"
 result=$(dcld query pki all-subject-x509-certs --subject="$noc_root_cert_1_subject")
 check_response "$result" "Not Found"
 response_does_not_contain "$result" "\"$noc_root_cert_1_subject\""
 response_does_not_contain "$result" "\"$noc_root_cert_1_subject_key_id\""
 echo $result | jq
 
-echo "Request all certificates by subjectKeyId should be empty"
+echo "Request all certificates by NOC root certificate's subjectKeyId should be empty"
 result=$(dcld query pki x509-cert --subject-key-id="$noc_root_cert_1_subject_key_id")
 check_response "$result" "Not Found"
 response_does_not_contain "$result" "\"subject\": \"$noc_root_cert_1_subject\""

diff --git a/integration_tests/cli/pki-noc-revocation-with-serial-number.sh b/integration_tests/cli/pki-noc-revocation-with-serial-number.sh
@@ -101,7 +101,7 @@ response_does_not_contain "$result" "\"subjectKeyId\": \"$noc_root_cert_1_subjec
 response_does_not_contain "$result" "\"serialNumber\": \"$noc_root_cert_1_serial_number\""
 echo $result | jq
 
-echo "Request NOC certificate by VID should contain only one root certificate with serialNumber=$noc_root_cert_1_copy_serial_number"
+echo "Request NOC root certificate by VID = $vid should contain only one root certificate with serialNumber=$noc_root_cert_1_copy_serial_number"
 result=$(dcld query pki noc-x509-root-certs --vid="$vid")
 check_response "$result" "\"serialNumber\": \"$noc_root_cert_1_copy_serial_number\""
 check_response "$result" "\"subject\": \"$noc_root_cert_1_subject\""
@@ -192,7 +192,7 @@ response_does_not_contain "$result" "\"serialNumber\": \"$noc_root_cert_1_serial
 response_does_not_contain "$result" "\"serialNumber\": \"$noc_root_cert_1_copy_serial_number\""
 echo $result | jq
 
-echo "Request NOC certificate by VID should be empty"
+echo "Request NOC root certificate by VID = $vid should be empty"
 result=$(dcld query pki noc-x509-root-certs --vid="$vid")
 check_response "$result" "Not Found"
 response_does_not_contain "$result" "\"subject\": \"$noc_root_cert_1_subject\""

diff --git a/types/pki/errors.go b/types/pki/errors.go
@@ -357,6 +357,13 @@ func NewErrMessageRemoveRoot(subject string, subjectKeyID string) error {
 	)
 }
 
+func NewErrMessageExistingCertIsNotRoot(subject string, subjectKeyID string) error {
+	return sdkerrors.Wrapf(ErrInappropriateCertificateType,
+		"The existing certificate with the same combination of subject (%v) and subjectKeyID (%v) is not a root certificate",
+		subject, subjectKeyID,
+	)
+}
+
 func NewErrUnsupportedOperation(e interface{}) error {
 	return sdkerrors.Wrapf(ErrUnsupportedOperation, "%v", e)
 }

diff --git a/x/pki/handler_revoke_noc_root_cert_test.go b/x/pki/handler_revoke_noc_root_cert_test.go
@@ -81,7 +81,7 @@ func TestHandler_RevokeNocX509RootCert_CertificateExists(t *testing.T) {
 				Vid:           testconstants.Vid,
 			},
 			nocRoorCert: testconstants.RootCertPem,
-			err:         sdkerrors.ErrUnauthorized,
+			err:         pkitypes.ErrInappropriateCertificateType,
 		},
 		{
 			name: "ExistingNotNocCert",
@@ -235,9 +235,10 @@ func TestHandler_RevokeNocX509RootCert_RevokeDefault(t *testing.T) {
 	require.Equal(t, 0, len(aprCertsBySubjectKeyID))
 
 	// query noc root certificate by VID
-	_, err = queryNocRootCertificates(setup, testconstants.Vid)
-	require.Error(t, err)
-	require.Equal(t, codes.NotFound, status.Code(err))
+	nocRootCerts, err := queryNocRootCertificates(setup, testconstants.Vid)
+	require.NoError(t, err)
+	require.Equal(t, 1, len(nocRootCerts.Certs))
+	require.Equal(t, testconstants.NocRootCert2SubjectKeyID, nocRootCerts.Certs[0].SubjectKeyId)
 
 	// Child certificate should not be revoked
 	_, err = queryRevokedCertificates(setup, testconstants.NocCert1Subject, testconstants.NocCert1SubjectKeyID)

diff --git a/x/pki/keeper/child_certificates.go b/x/pki/keeper/child_certificates.go
@@ -111,10 +111,10 @@ func (k msgServer) RevokeChildCertificates(ctx sdk.Context, issuer string, autho
 		// Revoke certificates with this subject/subjectKeyID combination
 		certificates, _ := k.GetApprovedCertificates(ctx, certIdentifier.Subject, certIdentifier.SubjectKeyId)
 		k.AddRevokedCertificates(ctx, certificates)
-		// FIXME: Should be replaced
+		// FIXME: Below two lines is not in the context of RevokeChildCertificates method. In future current implementation must be refactored
 		if len(certificates.Certs) > 0 {
 			// If cert is NOC then remove it from NOC certificates list
-			k.RemoveNocCertificates(ctx, certificates.Certs[0].Vid)
+			k.RemoveNocCertificate(ctx, certIdentifier.Subject, certIdentifier.SubjectKeyId, certificates.Certs[0].Vid)
 		}
 		k.RemoveApprovedCertificates(ctx, certIdentifier.Subject, certIdentifier.SubjectKeyId)
 

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
@@ -30,7 +30,7 @@ func (k msgServer) RevokeNocRootX509Cert(goCtx context.Context, msg *types.MsgRe
 
 	cert := certificates.Certs[0]
 	if !cert.IsRoot {
-		return nil, pkitypes.NewErrUnauthorizedCertIssuer(cert.Subject, cert.SubjectKeyId)
+		return nil, pkitypes.NewErrMessageExistingCertIsNotRoot(cert.Subject, cert.SubjectKeyId)
 	}
 	// Existing certificate must be NOC certificate
 	if !cert.IsNoc {
@@ -71,7 +71,7 @@ func (k msgServer) _revokeNocRootCertificates(ctx sdk.Context, certificates type
 	k.AddRevokedNocRootCertificates(ctx, types.RevokedNocRootCertificates(certificates))
 
 	// Remove certs from NOC and approved lists
-	k.RemoveNocRootCertificates(ctx, vid)
+	k.RemoveNocRootCertificate(ctx, vid, certificates.Subject, certificates.SubjectKeyId)
 	k.RemoveApprovedCertificates(ctx, certificates.Subject, certificates.SubjectKeyId)
 	// remove from subject -> subject key ID map
 	k.RemoveApprovedCertificateBySubject(ctx, certificates.Subject, certificates.SubjectKeyId)
@@ -108,16 +108,12 @@ func (k msgServer) _revokeNocRootCertificate(
 	k.removeCertFromList(cert.Issuer, cert.SerialNumber, &certificates)
 
 	if len(certificates.Certs) == 0 {
+		k.RemoveNocRootCertificate(ctx, vid, certificates.Subject, certificates.SubjectKeyId)
 		k.RemoveApprovedCertificates(ctx, cert.Subject, cert.SubjectKeyId)
-		k.RemoveNocRootCertificates(ctx, vid)
 		k.RemoveApprovedCertificateBySubject(ctx, cert.Subject, cert.SubjectKeyId)
 		k.RemoveApprovedCertificatesBySubjectKeyID(ctx, cert.Subject, cert.SubjectKeyId)
 	} else {
-		certs := types.NocRootCertificates{
-			Vid:   vid,
-			Certs: certificates.Certs,
-		}
-		k.SetNocRootCertificates(ctx, certs)
+		k.RemoveNocRootCertificateBySerialNumber(ctx, vid, cert.Subject, cert.SubjectKeyId, serialNumber)
 		k.SetApprovedCertificates(ctx, certificates)
 		k.SetApprovedCertificatesBySubjectKeyID(
 			ctx,

diff --git a/x/pki/keeper/noc_certificates.go b/x/pki/keeper/noc_certificates.go
@@ -71,6 +71,27 @@ func (k Keeper) RemoveNocCertificates(
 	))
 }
 
+func (k Keeper) RemoveNocCertificate(ctx sdk.Context, subject, subjectKeyID string, vid int32) {
+	certs, found := k.GetNocCertificates(ctx, vid)
+	if !found {
+		return
+	}
+
+	for i := 0; i < len(certs.Certs); {
+		if certs.Certs[i].Subject == subject && certs.Certs[i].SubjectKeyId == subjectKeyID {
+			certs.Certs = append(certs.Certs[:i], certs.Certs[i+1:]...)
+		} else {
+			i++
+		}
+	}
+
+	if len(certs.Certs) == 0 {
+		k.RemoveNocCertificates(ctx, vid)
+	} else {
+		k.SetNocCertificates(ctx, certs)
+	}
+}
+
 // GetAllNocCertificates returns all nocCertificates.
 func (k Keeper) GetAllNocCertificates(ctx sdk.Context) (list []types.NocCertificates) {
 	store := prefix.NewStore(ctx.KVStore(k.storeKey), pkitypes.KeyPrefix(types.NocCertificatesKeyPrefix))

diff --git a/x/pki/keeper/noc_root_certificates.go b/x/pki/keeper/noc_root_certificates.go
@@ -70,6 +70,54 @@ func (k Keeper) RemoveNocRootCertificates(
 	))
 }
 
+func (k Keeper) RemoveNocRootCertificate(ctx sdk.Context, vid int32, subject, subjectKeyID string) {
+	certs, found := k.GetNocRootCertificates(ctx, vid)
+	if !found {
+		return
+	}
+
+	certsBefore := len(certs.Certs)
+	for i := 0; i < len(certs.Certs); {
+		cert := certs.Certs[i]
+		if cert.Subject == subject && cert.SubjectKeyId == subjectKeyID {
+			certs.Certs = append(certs.Certs[:i], certs.Certs[i+1:]...)
+		} else {
+			i++
+		}
+	}
+
+	if len(certs.Certs) == 0 {
+		k.RemoveNocRootCertificates(ctx, vid)
+	} else if certsBefore > len(certs.Certs) { // Update state only if any certificate is removed
+		k.SetNocRootCertificates(ctx, certs)
+	}
+}
+
+func (k Keeper) RemoveNocRootCertificateBySerialNumber(ctx sdk.Context, vid int32, subject, subjectKeyID, serialNumber string) {
+	certs, found := k.GetNocRootCertificates(ctx, vid)
+	if !found {
+		return
+	}
+
+	certsBefore := len(certs.Certs)
+	for i := 0; i < len(certs.Certs); {
+		cert := certs.Certs[i]
+		if cert.Subject == subject &&
+			cert.SubjectKeyId == subjectKeyID &&
+			cert.SerialNumber == serialNumber {
+			certs.Certs = append(certs.Certs[:i], certs.Certs[i+1:]...)
+		} else {
+			i++
+		}
+	}
+
+	if len(certs.Certs) == 0 {
+		k.RemoveNocRootCertificates(ctx, vid)
+	} else if certsBefore > len(certs.Certs) { // Update state only if any certificate is removed
+		k.SetNocRootCertificates(ctx, certs)
+	}
+}
+
 // GetAllNocRootCertificates returns all nocRootCertificates.
 func (k Keeper) GetAllNocRootCertificates(ctx sdk.Context) (list []types.NocRootCertificates) {
 	store := prefix.NewStore(ctx.KVStore(k.storeKey), pkitypes.KeyPrefix(types.NocRootCertificatesKeyPrefix))