Skip to content

Commit dd98906

Browse files
AbdulboisAbdulbois
authored
Merge pull request #571 from zigbee-alliance/improve-error-message-texts
Improve error message texts
2 parents ac58d9f + e50f3df commit dd98906

12 files changed

+71
-30
lines changed

‎types/pki/errors.go

+22-4
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ func NewErrProposedCertificateDoesNotExist(subject string, subjectKeyID string)
7575
return errors.Wrapf(ErrProposedCertificateDoesNotExist,
7676
"No proposed X509 root certificate associated "+
7777
"with the combination of subject=%v and subjectKeyID=%v on the ledger. "+
78-
"The cerificate either does not exists or already approved.",
78+
"The certificate either does not exists, already approved or rejected",
7979
subject, subjectKeyID)
8080
}
8181

@@ -115,11 +115,15 @@ func NewErrProposedCertificateRevocationAlreadyExists(subject string, subjectKey
115115
subject, subjectKeyID)
116116
}
117117

118-
func NewErrProposedCertificateRevocationDoesNotExist(subject string, subjectKeyID string) error {
118+
func NewErrProposedCertificateRevocationDoesNotExist(subject string, subjectKeyID string, serialNumber string) error {
119+
if serialNumber != "" {
120+
serialNumber = " and serialNumber=" + serialNumber
121+
}
122+
119123
return errors.Wrapf(ErrProposedCertificateRevocationDoesNotExist,
120124
"No proposed X509 root certificate revocation associated "+
121-
"with the combination of subject=%v and subjectKeyID=%v on the ledger.",
122-
subject, subjectKeyID)
125+
"with the combination of subject=%v, subjectKeyID=%v%v on the ledger.",
126+
subject, subjectKeyID, serialNumber)
123127
}
124128

125129
func NewErrRevokedCertificateDoesNotExist(subject string, subjectKeyID string) error {
@@ -235,6 +239,20 @@ func NewErrRootCertVidNotEqualToAccountVid(rootVID int32, accountVID int32) erro
235239
rootVID, accountVID)
236240
}
237241

242+
func NewErrRevokeRootCertVidNotEqualToAccountVid(rootVID int32, accountVID int32) error {
243+
return errors.Wrapf(ErrCertVidNotEqualAccountVid,
244+
"Only a Vendor associated with VID of root certificate can revoke certificate: "+
245+
"Root certificate's VID = %v, Account VID = %v",
246+
rootVID, accountVID)
247+
}
248+
249+
func NewErrRevokeCertVidNotEqualToAccountVid(rootVID int32, accountVID int32) error {
250+
return errors.Wrapf(ErrCertVidNotEqualAccountVid,
251+
"Only a Vendor associated with VID of certificate can revoke certificate: "+
252+
"Certificate's VID = %v, Account VID = %v",
253+
rootVID, accountVID)
254+
}
255+
238256
func NewErrCRLSignerCertificateInvalidFormat(description string) error {
239257
return errors.Wrapf(
240258
ErrCRLSignerCertificateInvalidFormat, "Invalid CRL Signer Certificate format: %v",

‎x/pki/handler_add_noc_cert_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ func TestHandler_AddXNoc509Cert_WhenNocRootCertIsAbsent(t *testing.T) {
224224
addNocX509Cert := types.NewMsgAddNocX509IcaCert(accAddress.String(), testconstants.NocCert1, testconstants.CertSchemaVersion, testconstants.SchemaVersion)
225225
_, err := setup.Handler(setup.Ctx, addNocX509Cert)
226226

227-
require.ErrorIs(t, err, pkitypes.ErrInvalidCertificate)
227+
require.ErrorIs(t, err, pkitypes.ErrCertificateDoesNotExist)
228228
}
229229

230230
func TestHandler_AddNocX509Cert_CertificateExist(t *testing.T) {

‎x/pki/handler_add_non_root_cert_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,7 @@ func TestHandler_AddX509Cert_ForAbsentDirectParentCert(t *testing.T) {
235235
// add intermediate x509 certificate
236236
addX509Cert := types.NewMsgAddX509Cert(vendorAccAddress.String(), testconstants.IntermediateCertPem, testconstants.CertSchemaVersion, testconstants.SchemaVersion)
237237
_, err := setup.Handler(setup.Ctx, addX509Cert)
238-
require.ErrorIs(t, err, pkitypes.ErrInvalidCertificate)
238+
require.ErrorIs(t, err, pkitypes.ErrCertificateDoesNotExist)
239239
}
240240

241241
func TestHandler_AddX509Cert_ForFailedCertificateVerification(t *testing.T) {

‎x/pki/handler_remove_noc_ica_cert_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -436,7 +436,7 @@ func TestHandler_RemoveNocX509IcaCert_ByOtherVendor(t *testing.T) {
436436
vendorAccAddress2.String(), testconstants.NocCert1Subject, testconstants.NocCert1SubjectKeyID, testconstants.NocCert1SerialNumber)
437437
_, err = setup.Handler(setup.Ctx, removeIcaCert)
438438
require.Error(t, err)
439-
require.True(t, pkitypes.ErrCertificateDoesNotExist.Is(err))
439+
require.True(t, pkitypes.ErrCertVidNotEqualAccountVid.Is(err))
440440
}
441441

442442
func TestHandler_RemoveNocX509IcaCert_SenderNotVendor(t *testing.T) {

‎x/pki/handler_remove_noc_root_cert_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -491,7 +491,7 @@ func TestHandler_RemoveNocX509RootCert_ByOtherVendor(t *testing.T) {
491491
vendorAccAddress2.String(), testconstants.NocRootCert1Subject, testconstants.NocRootCert1SubjectKeyID, testconstants.NocRootCert1SerialNumber)
492492
_, err := setup.Handler(setup.Ctx, removeIcaCert)
493493
require.Error(t, err)
494-
require.True(t, pkitypes.ErrCertificateDoesNotExist.Is(err))
494+
require.True(t, pkitypes.ErrCertVidNotEqualAccountVid.Is(err))
495495
}
496496

497497
func TestHandler_RemoveNocX509RootCert_SenderNotVendor(t *testing.T) {

‎x/pki/keeper/approved_certificates.go

+1-3
Original file line numberDiff line numberDiff line change
@@ -129,9 +129,7 @@ func (k Keeper) verifyCertificate(ctx sdk.Context,
129129
} else {
130130
parentCertificates, found := k.GetApprovedCertificates(ctx, x509Certificate.Issuer, x509Certificate.AuthorityKeyID)
131131
if !found {
132-
return nil, pkitypes.NewErrInvalidCertificate(
133-
fmt.Sprintf("Certificate verification failed for certificate with subject=%v and subjectKeyID=%v",
134-
x509Certificate.Subject, x509Certificate.SubjectKeyID))
132+
return nil, pkitypes.NewErrRootCertificateDoesNotExist(x509Certificate.Issuer, x509Certificate.AuthorityKeyID)
135133
}
136134

137135
for _, cert := range parentCertificates.Certs {

‎x/pki/keeper/msg_server_add_noc_x_509_ica_cert.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ func (k msgServer) AddNocX509IcaCert(goCtx context.Context, msg *types.MsgAddNoc
8282
}
8383
// Check VID scoping
8484
if nocRootCert.Vid != accountVid {
85-
return nil, pkitypes.NewErrRootCertVidNotEqualToAccountVid(accountVid, nocRootCert.Vid)
85+
return nil, pkitypes.NewErrRootCertVidNotEqualToAccountVid(nocRootCert.Vid, accountVid)
8686
}
8787

8888
// create new certificate

‎x/pki/keeper/msg_server_approve_revoke_x_509_root_cert.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ func (k msgServer) ApproveRevokeX509RootCert(goCtx context.Context, msg *types.M
2929
// get proposed certificate revocation
3030
revocation, found := k.GetProposedCertificateRevocation(ctx, msg.Subject, msg.SubjectKeyId, msg.SerialNumber)
3131
if !found {
32-
return nil, pkitypes.NewErrProposedCertificateRevocationDoesNotExist(msg.Subject, msg.SubjectKeyId)
32+
return nil, pkitypes.NewErrProposedCertificateRevocationDoesNotExist(msg.Subject, msg.SubjectKeyId, msg.SerialNumber)
3333
}
3434

3535
// check if proposed certificate revocation already has approval form signer

‎x/pki/keeper/msg_server_remove_noc_x_509_ica_cert.go

+17-7
Original file line numberDiff line numberDiff line change
@@ -26,19 +26,30 @@ func (k msgServer) RemoveNocX509IcaCert(goCtx context.Context, msg *types.MsgRem
2626
signerAccount, _ := k.dclauthKeeper.GetAccountO(ctx, signerAddr)
2727
accountVid := signerAccount.VendorID
2828

29-
icaCerts, foundActive := k.GetNocIcaCertificatesBySubjectAndSKID(ctx, accountVid, msg.Subject, msg.SubjectKeyId)
29+
icaCerts, foundActive := k.GetApprovedCertificates(ctx, msg.Subject, msg.SubjectKeyId)
3030
revCerts, foundRevoked := k.GetRevokedCertificates(ctx, msg.Subject, msg.SubjectKeyId)
3131
certificates := icaCerts.Certs
3232
certificates = append(certificates, revCerts.Certs...)
3333
if len(certificates) == 0 {
3434
return nil, pkitypes.NewErrCertificateDoesNotExist(msg.Subject, msg.SubjectKeyId)
3535
}
3636

37+
cert := certificates[0]
38+
// Existing certificate must be Root certificate
39+
if cert.IsRoot {
40+
return nil, pkitypes.NewErrMessageExpectedNonRoot(cert.Subject, cert.SubjectKeyId)
41+
}
42+
3743
// Existing certificate must be NOC certificate
38-
if !certificates[0].IsNoc {
44+
if !cert.IsNoc {
3945
return nil, pkitypes.NewErrProvidedNocCertButExistingNotNoc(msg.Subject, msg.SubjectKeyId)
4046
}
4147

48+
// account VID must be same as VID of existing certificates
49+
if accountVid != cert.Vid {
50+
return nil, pkitypes.NewErrRevokeCertVidNotEqualToAccountVid(cert.Vid, accountVid)
51+
}
52+
4253
if err = k.EnsureVidMatches(ctx, certificates[0].Owner, msg.Signer); err != nil {
4354
return nil, err
4455
}
@@ -59,19 +70,18 @@ func (k msgServer) RemoveNocX509IcaCert(goCtx context.Context, msg *types.MsgRem
5970

6071
if foundActive {
6172
// Remove from Approved lists
62-
aprCerts, _ := k.GetApprovedCertificates(ctx, msg.Subject, msg.SubjectKeyId)
63-
removeCertFromList(certBySerialNumber.Issuer, certBySerialNumber.SerialNumber, &aprCerts.Certs)
64-
k.removeApprovedX509Cert(ctx, certID, &aprCerts, msg.SerialNumber)
73+
removeCertFromList(certBySerialNumber.Issuer, certBySerialNumber.SerialNumber, &icaCerts.Certs)
74+
k.removeApprovedX509Cert(ctx, certID, &icaCerts, msg.SerialNumber)
6575

6676
// Remove from ICA lists
67-
k.RemoveNocIcaCertificateBySerialNumber(ctx, icaCerts.Vid, certID.Subject, certID.SubjectKeyId, msg.SerialNumber)
77+
k.RemoveNocIcaCertificateBySerialNumber(ctx, accountVid, certID.Subject, certID.SubjectKeyId, msg.SerialNumber)
6878
}
6979
if foundRevoked {
7080
removeCertFromList(certBySerialNumber.Issuer, certBySerialNumber.SerialNumber, &revCerts.Certs)
7181
k.removeOrUpdateRevokedX509Cert(ctx, certID, &revCerts)
7282
}
7383
} else {
74-
k.RemoveNocIcaCertificate(ctx, certID.Subject, certID.SubjectKeyId, icaCerts.Vid)
84+
k.RemoveNocIcaCertificate(ctx, certID.Subject, certID.SubjectKeyId, accountVid)
7585
// remove from approved list
7686
k.RemoveApprovedCertificates(ctx, certID.Subject, certID.SubjectKeyId)
7787
// remove from subject -> subject key ID map

‎x/pki/keeper/msg_server_remove_noc_x_509_root_cert.go

+23-8
Original file line numberDiff line numberDiff line change
@@ -26,14 +26,30 @@ func (k msgServer) RemoveNocX509RootCert(goCtx context.Context, msg *types.MsgRe
2626
signerAccount, _ := k.dclauthKeeper.GetAccountO(ctx, signerAddr)
2727
accountVid := signerAccount.VendorID
2828

29-
nocCerts, foundActive := k.GetNocRootCertificatesByVidAndSkid(ctx, accountVid, msg.SubjectKeyId)
29+
nocCerts, foundActive := k.GetApprovedCertificates(ctx, msg.Subject, msg.SubjectKeyId)
3030
revCerts, foundRevoked := k.GetRevokedNocRootCertificates(ctx, msg.Subject, msg.SubjectKeyId)
3131
certificates := nocCerts.Certs
3232
certificates = append(certificates, revCerts.Certs...)
3333
if len(certificates) == 0 {
3434
return nil, pkitypes.NewErrCertificateDoesNotExist(msg.Subject, msg.SubjectKeyId)
3535
}
3636

37+
cert := certificates[0]
38+
// Existing certificate must be Root certificate
39+
if !cert.IsRoot {
40+
return nil, pkitypes.NewErrMessageExistingCertIsNotRoot(cert.Subject, cert.SubjectKeyId)
41+
}
42+
43+
// Existing certificate must be NOC certificate
44+
if !cert.IsNoc {
45+
return nil, pkitypes.NewErrProvidedNocCertButExistingNotNoc(msg.Subject, msg.SubjectKeyId)
46+
}
47+
48+
// account VID must be same as VID of existing certificates
49+
if accountVid != cert.Vid {
50+
return nil, pkitypes.NewErrRevokeCertVidNotEqualToAccountVid(cert.Vid, accountVid)
51+
}
52+
3753
certID := types.CertificateIdentifier{
3854
Subject: msg.Subject,
3955
SubjectKeyId: msg.SubjectKeyId,
@@ -50,23 +66,22 @@ func (k msgServer) RemoveNocX509RootCert(goCtx context.Context, msg *types.MsgRe
5066

5167
if foundActive {
5268
// Remove from Approved lists
53-
aprCerts, _ := k.GetApprovedCertificates(ctx, msg.Subject, msg.SubjectKeyId)
54-
removeCertFromList(certBySerialNumber.Issuer, certBySerialNumber.SerialNumber, &aprCerts.Certs)
55-
k.removeApprovedX509Cert(ctx, certID, &aprCerts, msg.SerialNumber)
69+
removeCertFromList(certBySerialNumber.Issuer, certBySerialNumber.SerialNumber, &nocCerts.Certs)
70+
k.removeApprovedX509Cert(ctx, certID, &nocCerts, msg.SerialNumber)
5671

5772
// Remove from NOC lists
58-
k.RemoveNocRootCertificateBySerialNumber(ctx, nocCerts.Vid, certID.Subject, certID.SubjectKeyId, msg.SerialNumber)
59-
k.RemoveNocRootCertificateByVidSubjectSkidAndSerialNumber(ctx, nocCerts.Vid, certID.Subject, certID.SubjectKeyId, msg.SerialNumber)
73+
k.RemoveNocRootCertificateBySerialNumber(ctx, accountVid, certID.Subject, certID.SubjectKeyId, msg.SerialNumber)
74+
k.RemoveNocRootCertificateByVidSubjectSkidAndSerialNumber(ctx, accountVid, certID.Subject, certID.SubjectKeyId, msg.SerialNumber)
6075
}
6176

6277
if foundRevoked {
6378
removeCertFromList(certBySerialNumber.Issuer, certBySerialNumber.SerialNumber, &revCerts.Certs)
6479
k._removeRevokedNocX509RootCert(ctx, certID, &revCerts)
6580
}
6681
} else {
67-
k.RemoveNocRootCertificate(ctx, nocCerts.Vid, certID.Subject, certID.SubjectKeyId)
82+
k.RemoveNocRootCertificate(ctx, accountVid, certID.Subject, certID.SubjectKeyId)
6883
// remove from vid, subject key id map
69-
k.RemoveNocRootCertificatesByVidAndSkid(ctx, nocCerts.Vid, certID.SubjectKeyId)
84+
k.RemoveNocRootCertificatesByVidAndSkid(ctx, accountVid, certID.SubjectKeyId)
7085
// remove from revoked noc root certs
7186
k.RemoveRevokedNocRootCertificates(ctx, certID.Subject, certID.SubjectKeyId)
7287
// remove from revoked list

‎x/pki/keeper/msg_server_revoke_noc_x_509_ica_cert.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ func (k msgServer) RevokeNocX509IcaCert(goCtx context.Context, msg *types.MsgRev
4040
signerVid := signerAccount.VendorID
4141
// signer VID must be same as VID of existing certificates
4242
if signerVid != cert.Vid {
43-
return nil, pkitypes.NewErrRootCertVidNotEqualToAccountVid(cert.Vid, signerVid)
43+
return nil, pkitypes.NewErrRevokeCertVidNotEqualToAccountVid(cert.Vid, signerVid)
4444
}
4545

4646
if msg.SerialNumber != "" {

‎x/pki/keeper/msg_server_revoke_noc_x_509_root_cert.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ func (k msgServer) RevokeNocX509RootCert(goCtx context.Context, msg *types.MsgRe
4141
signerVid := signerAccount.VendorID
4242
// signer VID must be same as VID of existing certificates
4343
if signerVid != cert.Vid {
44-
return nil, pkitypes.NewErrRootCertVidNotEqualToAccountVid(cert.Vid, signerVid)
44+
return nil, pkitypes.NewErrRevokeRootCertVidNotEqualToAccountVid(cert.Vid, signerVid)
4545
}
4646

4747
if msg.SerialNumber != "" {

0 commit comments

Comments
 (0)