From 23c9e6f3e053893e5d5c449a9f00ad223157bae1 Mon Sep 17 00:00:00 2001 From: Jan Dittberner Date: Sun, 24 Apr 2022 12:45:22 +0200 Subject: [PATCH] Improve test coverage of X.509 revoking --- pkg/x509/revoking/repository.go | 2 + pkg/x509/revoking/revoking.go | 20 +- pkg/x509/revoking/revoking_test.go | 308 +++++++++++++++++++++++++++-- 3 files changed, 308 insertions(+), 22 deletions(-) diff --git a/pkg/x509/revoking/repository.go b/pkg/x509/revoking/repository.go index 54a9f40..a5b3705 100644 --- a/pkg/x509/revoking/repository.go +++ b/pkg/x509/revoking/repository.go @@ -19,6 +19,7 @@ package revoking import ( "crypto/x509/pkix" + "math/big" ) // A Repository for storing certificate status information @@ -26,4 +27,5 @@ type Repository interface { // StoreRevocation stores information about a revoked certificate. StoreRevocation(*pkix.RevokedCertificate) error RevokedCertificates() ([]pkix.RevokedCertificate, error) + NextCRLNumber() (*big.Int, error) } diff --git a/pkg/x509/revoking/revoking.go b/pkg/x509/revoking/revoking.go index fbbb99e..e7880f1 100644 --- a/pkg/x509/revoking/revoking.go +++ b/pkg/x509/revoking/revoking.go @@ -25,7 +25,6 @@ import ( "crypto/x509/pkix" "encoding/asn1" "fmt" - "log" "math/big" "strings" "time" @@ -71,11 +70,7 @@ func (r CRLReason) String() string { } func (r CRLReason) BuildExtension() pkix.Extension { - extBytes, err := asn1.Marshal(r) - if err != nil { - // we can panic here because all values of CRLReason must be ASN.1 marshal-able - log.Panicf("could not marshal revocation reason: %v", err) - } + extBytes, _ := asn1.Marshal(r) return pkix.Extension{Id: OidCRLReason, Value: extBytes} } @@ -103,6 +98,13 @@ type RevokeCertificate struct { reason CRLReason } +func NewRevokeCertificate(serialNumber *big.Int, reason CRLReason) *RevokeCertificate { + return &RevokeCertificate{ + serialNumber: serialNumber, + reason: reason, + } +} + type CRLInformation struct { CRL []byte // DER encoded CRL } @@ -127,9 +129,15 @@ func (r *X509Revoking) CreateCRL() (*CRLInformation, error) { return nil, fmt.Errorf("could not get revocation information: %w", err) } + nextNumber, err := r.repository.NextCRLNumber() + if err != nil { + return nil, fmt.Errorf("could not get next CRL number: %w", err) + } + list, err := x509.CreateRevocationList(rand.Reader, &x509.RevocationList{ SignatureAlgorithm: r.crlAlgorithm, RevokedCertificates: revoked, + Number: nextNumber, }, r.crlIssuer, r.signer) if err != nil { return nil, fmt.Errorf("could not sign revocation list: %w", err) diff --git a/pkg/x509/revoking/revoking_test.go b/pkg/x509/revoking/revoking_test.go index 5f73c66..cfdd086 100644 --- a/pkg/x509/revoking/revoking_test.go +++ b/pkg/x509/revoking/revoking_test.go @@ -15,22 +15,37 @@ See the License for the specific language governing permissions and limitations under the License. */ -package revoking +package revoking_test import ( "crypto/rand" "crypto/rsa" "crypto/x509" "crypto/x509/pkix" + "encoding/asn1" + "errors" + "fmt" "math/big" "testing" "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "git.cacert.org/cacert-gosigner/pkg/x509/revoking" ) type testRepo struct { - revoked []big.Int + crlNumber *big.Int + revoked []*big.Int +} + +func (t *testRepo) NextCRLNumber() (*big.Int, error) { + newNumber := new(big.Int).Add(t.crlNumber, big.NewInt(1)) + + t.crlNumber = newNumber + + return t.crlNumber, nil } func (t *testRepo) RevokedCertificates() ([]pkix.RevokedCertificate, error) { @@ -40,7 +55,7 @@ func (t *testRepo) RevokedCertificates() ([]pkix.RevokedCertificate, error) { serialNumber := s result[i] = pkix.RevokedCertificate{ - SerialNumber: &serialNumber, + SerialNumber: serialNumber, RevocationTime: time.Now(), } } @@ -49,11 +64,57 @@ func (t *testRepo) RevokedCertificates() ([]pkix.RevokedCertificate, error) { } func (t *testRepo) StoreRevocation(revoked *pkix.RevokedCertificate) error { - t.revoked = append(t.revoked, *revoked.SerialNumber) + t.revoked = append(t.revoked, revoked.SerialNumber) return nil } +type brokenRepo struct{} + +func (r *brokenRepo) NextCRLNumber() (*big.Int, error) { + return nil, errors.New("don't know") +} + +func (r *brokenRepo) RevokedCertificates() ([]pkix.RevokedCertificate, error) { + return nil, errors.New("no revocations for you") +} + +func (*brokenRepo) StoreRevocation(_ *pkix.RevokedCertificate) error { + return errors.New("cannot store") +} + +type brokenRepoNoCrlNumber struct { +} + +func (b brokenRepoNoCrlNumber) StoreRevocation(certificate *pkix.RevokedCertificate) error { + // do nothing + return nil +} + +func (b brokenRepoNoCrlNumber) RevokedCertificates() ([]pkix.RevokedCertificate, error) { + return make([]pkix.RevokedCertificate, 0), nil +} + +func (b brokenRepoNoCrlNumber) NextCRLNumber() (*big.Int, error) { + return nil, errors.New("don't know") +} + +type brokenRepoNoRevocations struct { +} + +func (b brokenRepoNoRevocations) StoreRevocation(certificate *pkix.RevokedCertificate) error { + // do nothing + return nil +} + +func (b brokenRepoNoRevocations) RevokedCertificates() ([]pkix.RevokedCertificate, error) { + return nil, errors.New("no revocations known") +} + +func (b brokenRepoNoRevocations) NextCRLNumber() (*big.Int, error) { + return nil, nil +} + func randomSerial(t *testing.T) *big.Int { t.Helper() @@ -65,15 +126,173 @@ func randomSerial(t *testing.T) *big.Int { return serial } -func TestRevoking(t *testing.T) { - testRepository := testRepo{revoked: make([]big.Int, 0)} +func TestX509Revoking_Revoke(t *testing.T) { + testRepository := testRepo{revoked: make([]*big.Int, 0), crlNumber: big.NewInt(0)} + + caKey, caCertificate := prepareTestCA(t) + + r := revoking.NewX509Revoking(&testRepository, x509.ECDSAWithSHA256, caCertificate, caKey) + + serial, err := rand.Int(rand.Reader, new(big.Int).Lsh(big.NewInt(1), 128)) + if err != nil { + t.Errorf("could not create random serial: %v", err) + } + + revoke, err := r.Revoke(revoking.NewRevokeCertificate(serial, revoking.CRLReasonKeyCompromise)) + assert.NoError(t, err) + + assert.Equal(t, revoking.CRLReasonKeyCompromise.BuildExtension(), revoke.Extensions[0]) + assert.Equal(t, serial, revoke.SerialNumber) + + assert.Contains(t, testRepository.revoked, serial) +} + +func TestX509Revoking_Revoke_BrokenRepo(t *testing.T) { + caKey, caCertificate := prepareTestCA(t) + + r := revoking.NewX509Revoking(&brokenRepo{}, x509.SHA256WithRSA, caCertificate, caKey) + + serial, err := rand.Int(rand.Reader, new(big.Int).Lsh(big.NewInt(1), 128)) + if err != nil { + t.Errorf("could not create random serial: %v", err) + } + + revoke, err := r.Revoke(revoking.NewRevokeCertificate(serial, revoking.CRLReasonKeyCompromise)) + + assert.Error(t, err) + assert.ErrorContains(t, err, "could not store revocation") + assert.Nil(t, revoke) +} + +func TestX509Revoking_CreateCRL(t *testing.T) { + key, certificate := prepareTestCA(t) + + r := revoking.NewX509Revoking( + &testRepo{revoked: make([]*big.Int, 0), crlNumber: big.NewInt(0)}, + x509.SHA256WithRSA, + certificate, + key, + ) + + serial, err := rand.Int(rand.Reader, new(big.Int).Lsh(big.NewInt(1), 128)) + if err != nil { + t.Errorf("could not create random serial: %v", err) + } + + _, err = r.Revoke(revoking.NewRevokeCertificate(serial, revoking.CRLReasonKeyCompromise)) + require.NoError(t, err) + + crl, err := r.CreateCRL() + + assert.NoError(t, err) + assert.NotNil(t, crl) + assert.NotEmpty(t, crl.CRL) + + parsedCRL, err := x509.ParseCRL(crl.CRL) + + assert.NoError(t, err) + + assert.ElementsMatch(t, certificate.Subject.ToRDNSequence(), parsedCRL.TBSCertList.Issuer) + + var found bool + + for _, item := range parsedCRL.TBSCertList.RevokedCertificates { + if item.SerialNumber.Cmp(serial) == 0 { + // standard library x509.CreateRevocationList does not support + // entry extensions according to RFC-5280 Section 5.3, therefore + // item.Extensions always is empty. + // + // otherwise the following assert would be useful + // + // assert.Contains(t, item.Extensions, revoking.CRLReasonKeyCompromise.BuildExtension()) + found = true + } + } + + assert.True(t, found) +} + +func TestX509Revoking_CreateCRL_BrokenRepoNoRevocations(t *testing.T) { + caKey, caCertificate := prepareTestCA(t) + + r := revoking.NewX509Revoking(&brokenRepoNoRevocations{}, x509.SHA256WithRSA, caCertificate, caKey) + + serial, err := rand.Int(rand.Reader, new(big.Int).Lsh(big.NewInt(1), 128)) + if err != nil { + t.Errorf("could not create random serial: %v", err) + } + + _, err = r.Revoke(revoking.NewRevokeCertificate(serial, revoking.CRLReasonKeyCompromise)) + require.NoError(t, err) + + crl, err := r.CreateCRL() + + assert.Error(t, err) + assert.ErrorContains(t, err, "could not get revocation information") + assert.Nil(t, crl) +} + +func TestX509Revoking_CreateCRL_BrokenRepoNoCRLNumber(t *testing.T) { + caKey, caCertificate := prepareTestCA(t) + + r := revoking.NewX509Revoking(&brokenRepoNoCrlNumber{}, x509.SHA256WithRSA, caCertificate, caKey) + + serial, err := rand.Int(rand.Reader, new(big.Int).Lsh(big.NewInt(1), 128)) + if err != nil { + t.Errorf("could not create random serial: %v", err) + } + + _, err = r.Revoke(revoking.NewRevokeCertificate(serial, revoking.CRLReasonKeyCompromise)) + require.NoError(t, err) + + crl, err := r.CreateCRL() + + assert.Error(t, err) + assert.ErrorContains(t, err, "could not get next CRL number") + assert.Nil(t, crl) +} + +func TestX509Revoking_CreateCRL_WrongAlgorithm(t *testing.T) { + key, certificate := prepareTestCA(t) + + r := revoking.NewX509Revoking( + &testRepo{revoked: make([]*big.Int, 0), crlNumber: big.NewInt(0)}, + x509.ECDSAWithSHA256, + certificate, + key, + ) + + serial, err := rand.Int(rand.Reader, new(big.Int).Lsh(big.NewInt(1), 128)) + if err != nil { + t.Errorf("could not create random serial: %v", err) + } + + _, err = r.Revoke(revoking.NewRevokeCertificate(serial, revoking.CRLReasonKeyCompromise)) + require.NoError(t, err) + + crl, err := r.CreateCRL() + + assert.Error(t, err) + assert.ErrorContains(t, err, "could not sign revocation list") + assert.Nil(t, crl) +} + +func prepareTestCA(t *testing.T) (*rsa.PrivateKey, *x509.Certificate) { + t.Helper() caKey, err := rsa.GenerateKey(rand.Reader, 3072) if err != nil { t.Fatalf("could not generate key pair: %v", err) } - caTemplate := &x509.Certificate{Subject: pkix.Name{CommonName: "Test CA"}, SerialNumber: randomSerial(t)} + caTemplate := &x509.Certificate{ + Subject: pkix.Name{CommonName: "Test CA"}, + SerialNumber: randomSerial(t), + IsCA: true, + MaxPathLenZero: true, + BasicConstraintsValid: true, + KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageCRLSign, + } certificateBytes, err := x509.CreateCertificate(rand.Reader, caTemplate, caTemplate, caKey.Public(), caKey) if err != nil { @@ -85,18 +304,75 @@ func TestRevoking(t *testing.T) { t.Fatalf("could not create test CA certificate: %v", err) } - r := NewX509Revoking(&testRepository, x509.ECDSAWithSHA256, caCertificate, caKey) + return caKey, caCertificate +} - serial, err := rand.Int(rand.Reader, new(big.Int).Lsh(big.NewInt(1), 128)) - if err != nil { - t.Errorf("could not create random serial: %v", err) +func TestCRLReason_BuildExtension(t *testing.T) { + reasons := []revoking.CRLReason{ + revoking.CRLReasonUnspecified, + revoking.CRLReasonKeyCompromise, + revoking.CRLReasonCACompromise, + revoking.CRLReasonAffiliationChanged, + revoking.CRLReasonSuperseded, + revoking.CRLReasonCessationOfOperation, + revoking.CRLReasonCertificateHold, + revoking.CRLReasonRemoveFromCRL, + revoking.CRLReasonPrivilegeWithdrawn, + revoking.CRLReasonAACompromise, } - revoke, err := r.Revoke(&RevokeCertificate{serialNumber: serial, reason: CRLReasonKeyCompromise}) - assert.NoError(t, err) + for _, reason := range reasons { + t.Run(fmt.Sprintf("reason %d", int(reason)), func(t *testing.T) { + ext := reason.BuildExtension() - assert.Equal(t, CRLReasonKeyCompromise.BuildExtension(), revoke.Extensions[0]) - assert.Equal(t, serial, revoke.SerialNumber) + assert.Equal(t, revoking.OidCRLReason, ext.Id) - assert.Contains(t, testRepository.revoked, *serial) + var value int + + rest, err := asn1.Unmarshal(ext.Value, &value) + assert.NoError(t, err) + assert.Len(t, rest, 0) + assert.Equal(t, value, int(reason)) + }) + } +} + +func TestCRLReason_String(t *testing.T) { + reasons := []revoking.CRLReason{ + revoking.CRLReasonUnspecified, + revoking.CRLReasonKeyCompromise, + revoking.CRLReasonCACompromise, + revoking.CRLReasonAffiliationChanged, + revoking.CRLReasonSuperseded, + revoking.CRLReasonCessationOfOperation, + revoking.CRLReasonCertificateHold, + revoking.CRLReasonRemoveFromCRL, + revoking.CRLReasonPrivilegeWithdrawn, + revoking.CRLReasonAACompromise, + } + + for _, reason := range reasons { + t.Run(fmt.Sprintf("reason %d", int(reason)), func(t *testing.T) { + assert.NotEmpty(t, reason.String()) + }) + } + + var customReason revoking.CRLReason = 40 + + assert.Equal(t, customReason.String(), revoking.CRLReasonUnspecified.String()) +} + +func TestParseReason(t *testing.T) { + expected := map[string]revoking.CRLReason{ + "keyCompromise": revoking.CRLReasonKeyCompromise, + "kEyCoMpRoMiSe": revoking.CRLReasonKeyCompromise, + "unknown": revoking.CRLReasonUnspecified, + "": revoking.CRLReasonUnspecified, + } + + for key, reason := range expected { + t.Run(fmt.Sprintf("parse '%s'", key), func(t *testing.T) { + assert.Equal(t, revoking.ParseReason(key), reason) + }) + } }