Skip to content

Commit

Permalink
fix: update key usage error message to make it clear (#169)
Browse files Browse the repository at this point in the history
Resolves #164 
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

---------

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
  • Loading branch information
JeyJeyGao authored Oct 26, 2023
1 parent 3f28a1b commit c834adc
Show file tree
Hide file tree
Showing 2 changed files with 106 additions and 16 deletions.
44 changes: 30 additions & 14 deletions x509/cert_validations.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,10 @@ import (
"crypto/x509"
"errors"
"fmt"
"strings"
"time"
)

var kuLeafCertBlocked = x509.KeyUsageContentCommitment |
x509.KeyUsageKeyEncipherment |
x509.KeyUsageDataEncipherment |
x509.KeyUsageKeyAgreement |
x509.KeyUsageCertSign |
x509.KeyUsageCRLSign |
x509.KeyUsageEncipherOnly |
x509.KeyUsageDecipherOnly
var kuLeafCertBlockedString = "ContentCommitment, KeyEncipherment, DataEncipherment, KeyAgreement, " +
"CertSign, CRLSign, EncipherOnly, DecipherOnly"

// ValidateCodeSigningCertChain takes an ordered code-signing certificate chain
// and validates issuance from leaf to root
// Validates certificates according to this spec:
Expand Down Expand Up @@ -184,10 +174,36 @@ func validateLeafKeyUsage(cert *x509.Certificate) error {
return err
}
if cert.KeyUsage&x509.KeyUsageDigitalSignature == 0 {
return fmt.Errorf("certificate with subject %q: key usage must have the bit positions for digital signature set", cert.Subject)
return fmt.Errorf("The certificate with subject %q is invalid. The key usage must have the bit positions for \"Digital Signature\" set", cert.Subject)
}

var invalidKeyUsages []string
if cert.KeyUsage&x509.KeyUsageContentCommitment != 0 {
invalidKeyUsages = append(invalidKeyUsages, `"ContentCommitment"`)
}
if cert.KeyUsage&x509.KeyUsageKeyEncipherment != 0 {
invalidKeyUsages = append(invalidKeyUsages, `"KeyEncipherment"`)
}
if cert.KeyUsage&x509.KeyUsageDataEncipherment != 0 {
invalidKeyUsages = append(invalidKeyUsages, `"DataEncipherment"`)
}
if cert.KeyUsage&x509.KeyUsageKeyAgreement != 0 {
invalidKeyUsages = append(invalidKeyUsages, `"KeyAgreement"`)
}
if cert.KeyUsage&x509.KeyUsageCertSign != 0 {
invalidKeyUsages = append(invalidKeyUsages, `"CertSign"`)
}
if cert.KeyUsage&x509.KeyUsageCRLSign != 0 {
invalidKeyUsages = append(invalidKeyUsages, `"CRLSign"`)
}
if cert.KeyUsage&x509.KeyUsageEncipherOnly != 0 {
invalidKeyUsages = append(invalidKeyUsages, `"EncipherOnly"`)
}
if cert.KeyUsage&x509.KeyUsageDecipherOnly != 0 {
invalidKeyUsages = append(invalidKeyUsages, `"DecipherOnly"`)
}
if cert.KeyUsage&kuLeafCertBlocked != 0 {
return fmt.Errorf("certificate with subject %q: key usage must not have the bit positions for %s set", cert.Subject, kuLeafCertBlockedString)
if len(invalidKeyUsages) > 0 {
return fmt.Errorf("The certificate with subject %q is invalid. The key usage must be \"Digital Signature\" only, but found %s", cert.Subject, strings.Join(invalidKeyUsages, ", "))
}
return nil
}
Expand Down
78 changes: 76 additions & 2 deletions x509/cert_validations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ package x509

import (
"crypto/x509"
"crypto/x509/pkix"
_ "embed"
"encoding/asn1"
"testing"
"time"

Expand Down Expand Up @@ -510,7 +512,7 @@ var kuNoDigitalSignatureLeaf = parseCertificateFromString(kuNoDigitalSignatureLe

func TestFailKuNoDigitalSignatureLeaf(t *testing.T) {
err := validateLeafCertificate(kuNoDigitalSignatureLeaf, x509.ExtKeyUsageCodeSigning)
assertErrorEqual("certificate with subject \"CN=Hello\": key usage must have the bit positions for digital signature set", err, t)
assertErrorEqual("The certificate with subject \"CN=Hello\" is invalid. The key usage must have the bit positions for \"Digital Signature\" set", err, t)
}

var kuWrongValuesLeafPem = "-----BEGIN CERTIFICATE-----\n" +
Expand All @@ -534,7 +536,7 @@ var kuWrongValuesLeaf = parseCertificateFromString(kuWrongValuesLeafPem)

func TestFailKuWrongValuesLeaf(t *testing.T) {
err := validateLeafCertificate(kuWrongValuesLeaf, x509.ExtKeyUsageCodeSigning)
assertErrorEqual("certificate with subject \"CN=Hello\": key usage must not have the bit positions for ContentCommitment, KeyEncipherment, DataEncipherment, KeyAgreement, CertSign, CRLSign, EncipherOnly, DecipherOnly set", err, t)
assertErrorEqual("The certificate with subject \"CN=Hello\" is invalid. The key usage must be \"Digital Signature\" only, but found \"CertSign\", \"CRLSign\"", err, t)
}

var rsaKeyTooSmallLeafPem = "-----BEGIN CERTIFICATE-----\n" +
Expand Down Expand Up @@ -699,3 +701,75 @@ func assertErrorEqual(expected string, err error, t *testing.T) {
t.Fatalf("Expected error \"%v\" but was \"%v\"", expected, err)
}
}

func TestValidateLeafKeyUsage(t *testing.T) {
extensions := []pkix.Extension{{
Id: asn1.ObjectIdentifier{2, 5, 29, 15}, // OID for KeyUsage
Critical: true,
}}

tests := []struct {
name string
cert *x509.Certificate
expectedErrMsg string
}{
{
name: "Valid DigitalSignature usage",
cert: &x509.Certificate{
Subject: pkix.Name{CommonName: "Test CN"},
KeyUsage: x509.KeyUsageDigitalSignature,
Extensions: extensions,
},
expectedErrMsg: "",
},
{
name: "Valid ContentCommitment usage",
cert: &x509.Certificate{
Subject: pkix.Name{CommonName: "Test CN"},
KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageContentCommitment,
Extensions: extensions,
},
expectedErrMsg: "The certificate with subject \"CN=Test CN\" is invalid. The key usage must be \"Digital Signature\" only, but found \"ContentCommitment\"",
},
{
name: "Missing DigitalSignature usage",
cert: &x509.Certificate{
Subject: pkix.Name{CommonName: "Test CN"},
KeyUsage: x509.KeyUsageCertSign,
Extensions: extensions,
},
expectedErrMsg: "The certificate with subject \"CN=Test CN\" is invalid. The key usage must have the bit positions for \"Digital Signature\" set",
},
{
name: "Invalid KeyEncipherment usage",
cert: &x509.Certificate{
Subject: pkix.Name{CommonName: "Test CN"},
KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment,
Extensions: extensions,
},
expectedErrMsg: "The certificate with subject \"CN=Test CN\" is invalid. The key usage must be \"Digital Signature\" only, but found \"KeyEncipherment\"",
},
{
name: "Multiple Invalid usages",
cert: &x509.Certificate{
Subject: pkix.Name{CommonName: "Test CN"},
KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment | x509.KeyUsageDataEncipherment | x509.KeyUsageKeyAgreement | x509.KeyUsageCertSign | x509.KeyUsageCRLSign | x509.KeyUsageEncipherOnly | x509.KeyUsageDecipherOnly | x509.KeyUsageEncipherOnly | x509.KeyUsageDecipherOnly,
Extensions: extensions,
},
expectedErrMsg: "The certificate with subject \"CN=Test CN\" is invalid. The key usage must be \"Digital Signature\" only, but found \"KeyEncipherment\", \"DataEncipherment\", \"KeyAgreement\", \"CertSign\", \"CRLSign\", \"EncipherOnly\", \"DecipherOnly\"",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := validateLeafKeyUsage(tt.cert)
if err != nil && tt.expectedErrMsg == "" {
t.Fatalf("expected no error, but got: %s", err)
} else if err == nil && tt.expectedErrMsg != "" {
t.Fatalf("expected error %q, but got none", tt.expectedErrMsg)
} else if err != nil && err.Error() != tt.expectedErrMsg {
t.Fatalf("expected error %q, but got: %s", tt.expectedErrMsg, err)
}
})
}
}

0 comments on commit c834adc

Please sign in to comment.