Improve test coverage of X.509 revoking

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)
 }

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)
+	extBytes, _ := 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)
-	}
 
 	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)

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) {
+func TestX509Revoking_Revoke(t *testing.T) {
-	testRepository := testRepo{revoked: make([]big.Int, 0)}
+	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))
+func TestCRLReason_BuildExtension(t *testing.T) {
-	if err != nil {
+	reasons := []revoking.CRLReason{
-		t.Errorf("could not create random serial: %v", err)
+		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})
+	for _, reason := range reasons {
-	assert.NoError(t, err)
+		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, revoking.OidCRLReason, ext.Id)
-	assert.Equal(t, serial, revoke.SerialNumber)
 
-	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)
+		})
+	}
 }