Improve CRL implementation

This commit adds some changes to PR #731, some of them are:
- Add distribution point to the CRL
- Properly stop the goroutine that generates the CRLs
- CRL config validation
- Remove expired certificates from the CRL
- Require enable set to true to generate a CRL

This last point is the principal change in behaviour from the previous
implementation. The CRL will not be generated if it's not enabled, and
if it is enabled it will always be regenerated at some point, not only
if there is a revocation.

authority/authority.go

@@ -72,8 +72,9 @@ type Authority struct {
 	sshCAHostFederatedCerts []ssh.PublicKey
 
 	// CRL vars
-	crlTicker *time.Ticker
-	crlMutex  sync.Mutex
+	crlTicker  *time.Ticker
+	crlStopper chan struct{}
+	crlMutex   sync.Mutex
 
 	// Do not re-initialize
 	initOnce  bool
@@ -662,22 +663,14 @@ func (a *Authority) init() error {
 	// not be repeated.
 	a.initOnce = true
 
-	// Start the CRL generator
-	if a.config.CRL != nil && a.config.CRL.Enabled {
-		if v := a.config.CRL.CacheDuration; v != nil && v.Duration < 0 {
-			return errors.New("crl cacheDuration must be >= 0")
+	// Start the CRL generator, we can assume the configuration is validated.
+	if a.config.CRL.IsEnabled() {
+		// Default cache duration to the default one
+		if v := a.config.CRL.CacheDuration; v == nil || v.Duration <= 0 {
+			a.config.CRL.CacheDuration = config.DefaultCRLCacheDuration
 		}
-
-		if v := a.config.CRL.CacheDuration; v != nil && v.Duration == 0 {
-			a.config.CRL.CacheDuration.Duration, _ = time.ParseDuration("24h")
-		}
-
-		if a.config.CRL.CacheDuration == nil {
-			a.config.CRL.CacheDuration, _ = provisioner.NewDuration("24h")
-		}
-
-		err = a.startCRLGenerator()
-		if err != nil {
+		// Start CRL generator
+		if err := a.startCRLGenerator(); err != nil {
 			return err
 		}
 	}
@@ -736,6 +729,7 @@ func (a *Authority) IsAdminAPIEnabled() bool {
 func (a *Authority) Shutdown() error {
 	if a.crlTicker != nil {
 		a.crlTicker.Stop()
+		close(a.crlStopper)
 	}
 
 	if err := a.keyManager.Close(); err != nil {
@@ -746,9 +740,9 @@ func (a *Authority) Shutdown() error {
 
 // CloseForReload closes internal services, to allow a safe reload.
 func (a *Authority) CloseForReload() {
-
 	if a.crlTicker != nil {
 		a.crlTicker.Stop()
+		close(a.crlStopper)
 	}
 
 	if err := a.keyManager.Close(); err != nil {
@@ -791,27 +785,20 @@ func (a *Authority) requiresSCEPService() bool {
 	return false
 }
 
-// GetSCEPService returns the configured SCEP Service
-// TODO: this function is intended to exist temporarily
-// in order to make SCEP work more easily. It can be
-// made more correct by using the right interfaces/abstractions
-// after it works as expected.
+// GetSCEPService returns the configured SCEP Service.
+//
+// TODO: this function is intended to exist temporarily in order to make SCEP
+// work more easily. It can be made more correct by using the right
+// interfaces/abstractions after it works as expected.
 func (a *Authority) GetSCEPService() *scep.Service {
 	return a.scepService
 }
 
 func (a *Authority) startCRLGenerator() error {
-
-	if a.config.CRL.CacheDuration.Duration <= 0 {
+	if !a.config.CRL.IsEnabled() {
 		return nil
 	}
 
-	// Make the renewal ticker run ~2/3 of cacheDuration by default, or use renewPeriod if available
-	tickerDuration := (a.config.CRL.CacheDuration.Duration / 3) * 2
-	if v := a.config.CRL.RenewPeriod; v != nil && v.Duration > 0 {
-		tickerDuration = v.Duration
-	}
-
 	// Check that there is a valid CRL in the DB right now. If it doesn't exist
 	// or is expired, generate one now
 	_, ok := a.db.(db.CertificateRevocationListDB)
@@ -819,37 +806,28 @@ func (a *Authority) startCRLGenerator() error {
 		return errors.Errorf("CRL Generation requested, but database does not support CRL generation")
 	}
 
-	// Always create a new CRL on startup in case the CA has been down and the time to next expected CRL
-	// update is less than the cache duration.
-	err := a.GenerateCertificateRevocationList()
-	if err != nil {
+	// Always create a new CRL on startup in case the CA has been down and the
+	// time to next expected CRL update is less than the cache duration.
+	if err := a.GenerateCertificateRevocationList(); err != nil {
 		return errors.Wrap(err, "could not generate a CRL")
 	}
 
-	a.crlTicker = time.NewTicker(tickerDuration)
+	a.crlStopper = make(chan struct{}, 1)
+	a.crlTicker = time.NewTicker(a.config.CRL.TickerDuration())
 
 	go func() {
 		for {
-			<-a.crlTicker.C
-			log.Println("Regenerating CRL")
-			err := a.GenerateCertificateRevocationList()
-			if err != nil {
-				log.Printf("ERROR: authority.crlGenerator encountered an error when regenerating the CRL: %v", err)
+			select {
+			case <-a.crlTicker.C:
+				log.Println("Regenerating CRL")
+				if err := a.GenerateCertificateRevocationList(); err != nil {
+					log.Printf("error regenerating the CRL: %v", err)
+				}
+			case <-a.crlStopper:
+				return
 			}
-
 		}
 	}()
 
 	return nil
 }
-
-func (a *Authority) resetCRLGeneratorTimer() {
-	if a.crlTicker != nil {
-		tickerDuration := (a.config.CRL.CacheDuration.Duration / 3) * 2
-		if v := a.config.CRL.RenewPeriod; v != nil && v.Duration > 0 {
-			tickerDuration = v.Duration
-		}
-
-		a.crlTicker.Reset(tickerDuration)
-	}
-}

authority/authority_test.go

@@ -80,12 +80,6 @@ func testAuthority(t *testing.T, opts ...Option) *Authority {
 		AuthorityConfig: &AuthConfig{
 			Provisioners: p,
 		},
-		CRL: &config.CRLConfig{
-			Enabled:          true,
-			CacheDuration:    nil,
-			GenerateOnRevoke: true,
-			RenewPeriod:      nil,
-		},
 	}
 	a, err := New(c, opts...)
 	assert.FatalError(t, err)

authority/config/config.go

@@ -37,8 +37,11 @@ var (
 	DefaultEnableSSHCA = false
 	// DefaultCRLCacheDuration is the default cache duration for the CRL.
 	DefaultCRLCacheDuration = &provisioner.Duration{Duration: 24 * time.Hour}
-	// GlobalProvisionerClaims default claims for the Authority. Can be overridden
-	// by provisioner specific claims.
+	// DefaultCRLExpiredDuration is the default duration in which expired
+	// certificates will remain in the CRL after expiration.
+	DefaultCRLExpiredDuration = time.Hour
+	// GlobalProvisionerClaims is the default duration that expired certificates
+	// remain in the CRL after expiration.
 	GlobalProvisionerClaims = provisioner.Claims{
 		MinTLSDur:               &provisioner.Duration{Duration: 5 * time.Minute}, // TLS certs
 		MaxTLSDur:               &provisioner.Duration{Duration: 24 * time.Hour},
@@ -78,6 +81,55 @@ type Config struct {
 	SkipValidation   bool                 `json:"-"`
 }
 
+// CRLConfig represents config options for CRL generation
+type CRLConfig struct {
+	Enabled          bool                  `json:"enabled"`
+	GenerateOnRevoke bool                  `json:"generateOnRevoke,omitempty"`
+	CacheDuration    *provisioner.Duration `json:"cacheDuration,omitempty"`
+	RenewPeriod      *provisioner.Duration `json:"renewPeriod,omitempty"`
+}
+
+// IsEnabled returns if the CRL is enabled.
+func (c *CRLConfig) IsEnabled() bool {
+	return c != nil && c.Enabled
+}
+
+// Validate validates the CRL configuration.
+func (c *CRLConfig) Validate() error {
+	if c == nil {
+		return nil
+	}
+
+	if c.CacheDuration != nil && c.CacheDuration.Duration < 0 {
+		return errors.New("crl.cacheDuration must be greater than or equal to 0")
+	}
+
+	if c.RenewPeriod != nil && c.RenewPeriod.Duration < 0 {
+		return errors.New("crl.renewPeriod must be greater than or equal to 0")
+	}
+
+	if c.RenewPeriod != nil && c.CacheDuration != nil &&
+		c.RenewPeriod.Duration > c.CacheDuration.Duration {
+		return errors.New("crl.cacheDuration must be greater than or equal to crl.renewPeriod")
+	}
+
+	return nil
+}
+
+// TickerDuration the renewal ticker duration. This is set by renewPeriod, of it
+// is not set is ~2/3 of cacheDuration.
+func (c *CRLConfig) TickerDuration() time.Duration {
+	if !c.IsEnabled() {
+		return 0
+	}
+
+	if c.RenewPeriod != nil && c.RenewPeriod.Duration > 0 {
+		return c.RenewPeriod.Duration
+	}
+
+	return (c.CacheDuration.Duration / 3) * 2
+}
+
 // ASN1DN contains ASN1.DN attributes that are used in Subject and Issuer
 // x509 Certificate blocks.
 type ASN1DN struct {
@@ -109,14 +161,6 @@ type AuthConfig struct {
 	DisableGetSSHHosts   bool                  `json:"disableGetSSHHosts,omitempty"`
 }
 
-// CRLConfig represents config options for CRL generation
-type CRLConfig struct {
-	Enabled          bool                  `json:"enabled"`
-	CacheDuration    *provisioner.Duration `json:"cacheDuration,omitempty"`
-	GenerateOnRevoke bool                  `json:"generateOnRevoke,omitempty"`
-	RenewPeriod      *provisioner.Duration `json:"renewPeriod,omitempty"`
-}
-
 // init initializes the required fields in the AuthConfig if they are not
 // provided.
 func (c *AuthConfig) init() {
@@ -194,7 +238,7 @@ func (c *Config) Init() {
 	if c.CommonName == "" {
 		c.CommonName = "Step Online CA"
 	}
-	if c.CRL != nil && c.CRL.CacheDuration == nil {
+	if c.CRL != nil && c.CRL.Enabled && c.CRL.CacheDuration == nil {
 		c.CRL.CacheDuration = DefaultCRLCacheDuration
 	}
 	c.AuthorityConfig.init()
@@ -283,6 +327,11 @@ func (c *Config) Validate() error {
 		return err
 	}
 
+	// Validate crl config: nil is ok
+	if err := c.CRL.Validate(); err != nil {
+		return err
+	}
+
 	return c.AuthorityConfig.Validate(c.GetAudiences())
 }
 

authority/tls.go

@@ -30,6 +30,7 @@ import (
 	casapi "github.com/smallstep/certificates/cas/apiv1"
 	"github.com/smallstep/certificates/db"
 	"github.com/smallstep/certificates/errs"
+	"github.com/smallstep/nosql/database"
 )
 
 // GetTLSOptions returns the tls options configured.
@@ -37,8 +38,11 @@ func (a *Authority) GetTLSOptions() *config.TLSOptions {
 	return a.config.TLS
 }
 
-var oidAuthorityKeyIdentifier = asn1.ObjectIdentifier{2, 5, 29, 35}
-var oidSubjectKeyIdentifier = asn1.ObjectIdentifier{2, 5, 29, 14}
+var (
+	oidAuthorityKeyIdentifier            = asn1.ObjectIdentifier{2, 5, 29, 35}
+	oidSubjectKeyIdentifier              = asn1.ObjectIdentifier{2, 5, 29, 14}
+	oidExtensionIssuingDistributionPoint = asn1.ObjectIdentifier{2, 5, 29, 28}
+)
 
 func withDefaultASN1DN(def *config.ASN1DN) provisioner.CertificateModifierFunc {
 	return func(crt *x509.Certificate, opts provisioner.SignOptions) error {
@@ -488,19 +492,15 @@ func (a *Authority) Revoke(ctx context.Context, revokeOpts *RevokeOptions) error
 		RevokedAt:  time.Now().UTC(),
 	}
 
-	var (
-		p   provisioner.Interface
-		err error
-	)
-
-	if revokeOpts.Crt == nil {
-		// Attempt to get the certificate expiry using the serial number.
-		cert, err := a.db.GetCertificate(revokeOpts.Serial)
-
-		// Revocation of a certificate not in the database may be requested, so fill in the expiry only
-		// if we can
-		if err == nil {
-			rci.ExpiresAt = cert.NotAfter
+	// For X509 CRLs attempt to get the expiration date of the certificate.
+	if provisioner.MethodFromContext(ctx) == provisioner.RevokeMethod {
+		if revokeOpts.Crt == nil {
+			cert, err := a.db.GetCertificate(revokeOpts.Serial)
+			if err == nil {
+				rci.ExpiresAt = cert.NotAfter
+			}
+		} else {
+			rci.ExpiresAt = revokeOpts.Crt.NotAfter
 		}
 	}
 
@@ -508,8 +508,7 @@ func (a *Authority) Revoke(ctx context.Context, revokeOpts *RevokeOptions) error
 	if !(revokeOpts.MTLS || revokeOpts.ACME) {
 		token, err := jose.ParseSigned(revokeOpts.OTT)
 		if err != nil {
-			return errs.Wrap(http.StatusUnauthorized, err,
-				"authority.Revoke; error parsing token", opts...)
+			return errs.Wrap(http.StatusUnauthorized, err, "authority.Revoke; error parsing token", opts...)
 		}
 
 		// Get claims w/out verification.
@@ -519,28 +518,43 @@ func (a *Authority) Revoke(ctx context.Context, revokeOpts *RevokeOptions) error
 		}
 
 		// This method will also validate the audiences for JWK provisioners.
-		p, err = a.LoadProvisionerByToken(token, &claims.Claims)
+		p, err := a.LoadProvisionerByToken(token, &claims.Claims)
 		if err != nil {
 			return err
 		}
 		rci.ProvisionerID = p.GetID()
 		rci.TokenID, err = p.GetTokenID(revokeOpts.OTT)
 		if err != nil && !errors.Is(err, provisioner.ErrAllowTokenReuse) {
-			return errs.Wrap(http.StatusInternalServerError, err,
-				"authority.Revoke; could not get ID for token")
+			return errs.Wrap(http.StatusInternalServerError, err, "authority.Revoke; could not get ID for token")
 		}
 		opts = append(opts,
 			errs.WithKeyVal("provisionerID", rci.ProvisionerID),
 			errs.WithKeyVal("tokenID", rci.TokenID),
 		)
-	} else if p, err = a.LoadProvisionerByCertificate(revokeOpts.Crt); err == nil {
+	} else if p, err := a.LoadProvisionerByCertificate(revokeOpts.Crt); err == nil {
 		// Load the Certificate provisioner if one exists.
 		rci.ProvisionerID = p.GetID()
 		opts = append(opts, errs.WithKeyVal("provisionerID", rci.ProvisionerID))
 	}
 
+	failRevoke := func(err error) error {
+		switch {
+		case errors.Is(err, db.ErrNotImplemented):
+			return errs.NotImplemented("authority.Revoke; no persistence layer configured", opts...)
+		case errors.Is(err, db.ErrAlreadyExists):
+			return errs.ApplyOptions(
+				errs.BadRequest("certificate with serial number '%s' is already revoked", rci.Serial),
+				opts...,
+			)
+		default:
+			return errs.Wrap(http.StatusInternalServerError, err, "authority.Revoke", opts...)
+		}
+	}
+
 	if provisioner.MethodFromContext(ctx) == provisioner.SSHRevokeMethod {
-		err = a.revokeSSH(nil, rci)
+		if err := a.revokeSSH(nil, rci); err != nil {
+			return failRevoke(err)
+		}
 	} else {
 		// Revoke an X.509 certificate using CAS. If the certificate is not
 		// provided we will try to read it from the db. If the read fails we
@@ -555,7 +569,7 @@ func (a *Authority) Revoke(ctx context.Context, revokeOpts *RevokeOptions) error
 
 		// CAS operation, note that SoftCAS (default) is a noop.
 		// The revoke happens when this is stored in the db.
-		_, err = a.x509CAService.RevokeCertificate(&casapi.RevokeCertificateRequest{
+		_, err := a.x509CAService.RevokeCertificate(&casapi.RevokeCertificateRequest{
 			Certificate:  revokedCert,
 			SerialNumber: rci.Serial,
 			Reason:       rci.Reason,
@@ -567,37 +581,20 @@ func (a *Authority) Revoke(ctx context.Context, revokeOpts *RevokeOptions) error
 		}
 
 		// Save as revoked in the Db.
-		err = a.revoke(revokedCert, rci)
-		if err != nil {
-			return errs.Wrap(http.StatusInternalServerError, err, "authority.Revoke", opts...)
+		if err := a.revoke(revokedCert, rci); err != nil {
+			return failRevoke(err)
 		}
 
-		if a.config.CRL != nil && a.config.CRL.GenerateOnRevoke {
-			// Generate a new CRL so CRL requesters will always get an up-to-date CRL whenever they request it
-			err = a.GenerateCertificateRevocationList()
-			if err != nil {
+		// Generate a new CRL so CRL requesters will always get an up-to-date
+		// CRL whenever they request it.
+		if a.config.CRL.IsEnabled() && a.config.CRL.GenerateOnRevoke {
+			if err := a.GenerateCertificateRevocationList(); err != nil {
 				return errs.Wrap(http.StatusInternalServerError, err, "authority.Revoke", opts...)
 			}
-
-			// the timer only gets reset if CRL is enabled
-			if a.config.CRL.Enabled {
-				a.resetCRLGeneratorTimer()
-			}
 		}
 	}
-	switch {
-	case err == nil:
-		return nil
-	case errors.Is(err, db.ErrNotImplemented):
-		return errs.NotImplemented("authority.Revoke; no persistence layer configured", opts...)
-	case errors.Is(err, db.ErrAlreadyExists):
-		return errs.ApplyOptions(
-			errs.BadRequest("certificate with serial number '%s' is already revoked", rci.Serial),
-			opts...,
-		)
-	default:
-		return errs.Wrap(http.StatusInternalServerError, err, "authority.Revoke", opts...)
-	}
+
+	return nil
 }
 
 func (a *Authority) revoke(crt *x509.Certificate, rci *db.RevokedCertificateInfo) error {
@@ -621,7 +618,7 @@ func (a *Authority) revokeSSH(crt *ssh.Certificate, rci *db.RevokedCertificateIn
 // GetCertificateRevocationList will return the currently generated CRL from the DB, or a not implemented
 // error if the underlying AuthDB does not support CRLs
 func (a *Authority) GetCertificateRevocationList() ([]byte, error) {
-	if a.config.CRL == nil {
+	if !a.config.CRL.IsEnabled() {
 		return nil, errs.Wrap(http.StatusNotFound, errors.Errorf("Certificate Revocation Lists are not enabled"), "authority.GetCertificateRevocationList")
 	}
 
@@ -633,11 +630,6 @@ func (a *Authority) GetCertificateRevocationList() ([]byte, error) {
 	crlInfo, err := crlDB.GetCRL()
 	if err != nil {
 		return nil, errs.Wrap(http.StatusInternalServerError, err, "authority.GetCertificateRevocationList")
-
-	}
-
-	if crlInfo == nil {
-		return nil, nil
 	}
 
 	return crlInfo.DER, nil
@@ -646,9 +638,7 @@ func (a *Authority) GetCertificateRevocationList() ([]byte, error) {
 // GenerateCertificateRevocationList generates a DER representation of a signed CRL and stores it in the
 // database. Returns nil if CRL generation has been disabled in the config
 func (a *Authority) GenerateCertificateRevocationList() error {
-
-	if a.config.CRL == nil {
-		// CRL is disabled
+	if !a.config.CRL.IsEnabled() {
 		return nil
 	}
 
@@ -663,34 +653,40 @@ func (a *Authority) GenerateCertificateRevocationList() error {
 		return errors.Errorf("CA does not support CRL Generation")
 	}
 
-	a.crlMutex.Lock() // use a mutex to ensure only one CRL is generated at a time to avoid concurrency issues
+	// use a mutex to ensure only one CRL is generated at a time to avoid
+	// concurrency issues
+	a.crlMutex.Lock()
 	defer a.crlMutex.Unlock()
 
 	crlInfo, err := crlDB.GetCRL()
-	if err != nil {
+	if err != nil && !database.IsErrNotFound(err) {
 		return errors.Wrap(err, "could not retrieve CRL from database")
 	}
 
+	now := time.Now().UTC()
 	revokedList, err := crlDB.GetRevokedCertificates()
 	if err != nil {
 		return errors.Wrap(err, "could not retrieve revoked certificates list from database")
 	}
 
-	// Number is a monotonically increasing integer (essentially the CRL version number) that we need to
-	// keep track of and increase every time we generate a new CRL
-	var n int64
+	// Number is a monotonically increasing integer (essentially the CRL version
+	// number) that we need to keep track of and increase every time we generate
+	// a new CRL
 	var bn big.Int
-
 	if crlInfo != nil {
-		n = crlInfo.Number + 1
+		bn.SetInt64(crlInfo.Number + 1)
 	}
-	bn.SetInt64(n)
 
-	// Convert our database db.RevokedCertificateInfo types into the pkix representation ready for the
-	// CAS to sign it
+	// Convert our database db.RevokedCertificateInfo types into the pkix
+	// representation ready for the CAS to sign it
 	var revokedCertificates []pkix.RevokedCertificate
-
+	skipExpiredTime := now.Add(-config.DefaultCRLExpiredDuration.Abs())
 	for _, revokedCert := range *revokedList {
+		// skip expired certificates
+		if !revokedCert.ExpiresAt.IsZero() && revokedCert.ExpiresAt.Before(skipExpiredTime) {
+			continue
+		}
+
 		var sn big.Int
 		sn.SetString(revokedCert.Serial, 10)
 		revokedCertificates = append(revokedCertificates, pkix.RevokedCertificate{
@@ -713,9 +709,18 @@ func (a *Authority) GenerateCertificateRevocationList() error {
 		SignatureAlgorithm:  0,
 		RevokedCertificates: revokedCertificates,
 		Number:              &bn,
-		ThisUpdate:          time.Now().UTC(),
-		NextUpdate:          time.Now().UTC().Add(updateDuration),
-		ExtraExtensions:     nil,
+		ThisUpdate:          now,
+		NextUpdate:          now.Add(updateDuration),
+	}
+
+	// Add distribution point.
+	//
+	// Note that this is currently using the port 443 by default.
+	fullName := a.config.Audience("/1.0/crl")[0]
+	if b, err := marshalDistributionPoint(fullName, false); err == nil {
+		revocationList.ExtraExtensions = []pkix.Extension{
+			{Id: oidExtensionIssuingDistributionPoint, Value: b},
+		}
 	}
 
 	certificateRevocationList, err := caCRLGenerator.CreateCRL(&casapi.CreateCRLRequest{RevocationList: &revocationList})
@@ -726,7 +731,7 @@ func (a *Authority) GenerateCertificateRevocationList() error {
 	// Create a new db.CertificateRevocationListInfo, which stores the new Number we just generated, the
 	// expiry time, duration, and the DER-encoded CRL
 	newCRLInfo := db.CertificateRevocationListInfo{
-		Number:    n,
+		Number:    bn.Int64(),
 		ExpiresAt: revocationList.NextUpdate,
 		DER:       certificateRevocationList.CRL,
 		Duration:  updateDuration,
@@ -834,6 +839,33 @@ func (a *Authority) GetTLSCertificate() (*tls.Certificate, error) {
 	return &tlsCrt, nil
 }
 
+// RFC 5280, 5.2.5
+type distributionPoint struct {
+	DistributionPoint          distributionPointName `asn1:"optional,tag:0"`
+	OnlyContainsUserCerts      bool                  `asn1:"optional,tag:1"`
+	OnlyContainsCACerts        bool                  `asn1:"optional,tag:2"`
+	OnlySomeReasons            asn1.BitString        `asn1:"optional,tag:3"`
+	IndirectCRL                bool                  `asn1:"optional,tag:4"`
+	OnlyContainsAttributeCerts bool                  `asn1:"optional,tag:5"`
+}
+
+type distributionPointName struct {
+	FullName     []asn1.RawValue  `asn1:"optional,tag:0"`
+	RelativeName pkix.RDNSequence `asn1:"optional,tag:1"`
+}
+
+func marshalDistributionPoint(fullName string, isCA bool) ([]byte, error) {
+	return asn1.Marshal(distributionPoint{
+		DistributionPoint: distributionPointName{
+			FullName: []asn1.RawValue{
+				{Class: 2, Tag: 6, Bytes: []byte(fullName)},
+			},
+		},
+		OnlyContainsUserCerts: !isCA,
+		OnlyContainsCACerts:   isCA,
+	})
+}
+
 // templatingError tries to extract more information about the cause of
 // an error related to (most probably) malformed template data and adds
 // this to the error message.

authority/tls_test.go

@@ -28,11 +28,13 @@ import (
 
 	"github.com/smallstep/assert"
 	"github.com/smallstep/certificates/api/render"
+	"github.com/smallstep/certificates/authority/config"
 	"github.com/smallstep/certificates/authority/policy"
 	"github.com/smallstep/certificates/authority/provisioner"
 	"github.com/smallstep/certificates/cas/softcas"
 	"github.com/smallstep/certificates/db"
 	"github.com/smallstep/certificates/errs"
+	"github.com/smallstep/nosql/database"
 )
 
 var (
@@ -1364,7 +1366,7 @@ func TestAuthority_Revoke(t *testing.T) {
 					return true, nil
 				},
 				MGetCertificate: func(sn string) (*x509.Certificate, error) {
-					return nil, nil
+					return nil, errors.New("not found")
 				},
 				Err: errors.New("force"),
 			}))
@@ -1404,7 +1406,7 @@ func TestAuthority_Revoke(t *testing.T) {
 					return true, nil
 				},
 				MGetCertificate: func(sn string) (*x509.Certificate, error) {
-					return nil, nil
+					return nil, errors.New("not found")
 				},
 				Err: db.ErrAlreadyExists,
 			}))
@@ -1698,11 +1700,10 @@ func TestAuthority_CRL(t *testing.T) {
 		ctx      context.Context
 		expected []string
 		err      error
-		code     int
 	}
 	tests := map[string]func() test{
-		"ok/empty-crl": func() test {
-			_a := testAuthority(t, WithDatabase(&db.MockAuthDB{
+		"fail/empty-crl": func() test {
+			a := testAuthority(t, WithDatabase(&db.MockAuthDB{
 				MUseToken: func(id, tok string) (bool, error) {
 					return true, nil
 				},
@@ -1714,7 +1715,7 @@ func TestAuthority_CRL(t *testing.T) {
 					return nil
 				},
 				MGetCRL: func() (*db.CertificateRevocationListInfo, error) {
-					return &crlStore, nil
+					return nil, database.ErrNotFound
 				},
 				MGetRevokedCertificates: func() (*[]db.RevokedCertificateInfo, error) {
 					return &revokedList, nil
@@ -1724,15 +1725,19 @@ func TestAuthority_CRL(t *testing.T) {
 					return nil
 				},
 			}))
+			a.config.CRL = &config.CRLConfig{
+				Enabled: true,
+			}
 
 			return test{
-				auth:     _a,
+				auth:     a,
 				ctx:      crlCtx,
 				expected: nil,
+				err:      database.ErrNotFound,
 			}
 		},
 		"ok/crl-full": func() test {
-			_a := testAuthority(t, WithDatabase(&db.MockAuthDB{
+			a := testAuthority(t, WithDatabase(&db.MockAuthDB{
 				MUseToken: func(id, tok string) (bool, error) {
 					return true, nil
 				},
@@ -1754,6 +1759,10 @@ func TestAuthority_CRL(t *testing.T) {
 					return nil
 				},
 			}))
+			a.config.CRL = &config.CRLConfig{
+				Enabled:          true,
+				GenerateOnRevoke: true,
+			}
 
 			var ex []string
 
@@ -1770,7 +1779,7 @@ func TestAuthority_CRL(t *testing.T) {
 				}
 				raw, err := jwt.Signed(sig).Claims(cl).CompactSerialize()
 				assert.FatalError(t, err)
-				err = _a.Revoke(crlCtx, &RevokeOptions{
+				err = a.Revoke(crlCtx, &RevokeOptions{
 					Serial:     sn,
 					ReasonCode: reasonCode,
 					Reason:     reason,
@@ -1783,7 +1792,7 @@ func TestAuthority_CRL(t *testing.T) {
 			}
 
 			return test{
-				auth:     _a,
+				auth:     a,
 				ctx:      crlCtx,
 				expected: ex,
 			}
@@ -1792,10 +1801,11 @@ func TestAuthority_CRL(t *testing.T) {
 	for name, f := range tests {
 		tc := f()
 		t.Run(name, func(t *testing.T) {
-			if crlBytes, _err := tc.auth.GetCertificateRevocationList(); _err == nil {
+			if crlBytes, err := tc.auth.GetCertificateRevocationList(); err == nil {
 				crl, parseErr := x509.ParseCRL(crlBytes)
 				if parseErr != nil {
 					t.Errorf("x509.ParseCertificateRequest() error = %v, wantErr %v", parseErr, nil)
+					return
 				}
 
 				var cmpList []string
@@ -1804,9 +1814,8 @@ func TestAuthority_CRL(t *testing.T) {
 				}
 
 				assert.Equals(t, cmpList, tc.expected)
-
 			} else {
-				assert.NotNil(t, tc.err)
+				assert.NotNil(t, tc.err, err.Error())
 			}
 		})
 	}

cas/softcas/softcas.go

@@ -135,7 +135,6 @@ func (c *SoftCAS) RevokeCertificate(req *apiv1.RevokeCertificateRequest) (*apiv1
 
 // CreateCRL will create a new CRL based on the RevocationList passed to it
 func (c *SoftCAS) CreateCRL(req *apiv1.CreateCRLRequest) (*apiv1.CreateCRLResponse, error) {
-
 	certChain, signer, err := c.getCertSigner()
 	if err != nil {
 		return nil, err

db/db.go

@@ -271,14 +271,12 @@ func (db *DB) GetRevokedCertificates() (*[]RevokedCertificateInfo, error) {
 				revokedCerts = append(revokedCerts, data)
 			}
 		}
-
 	}
 	return &revokedCerts, nil
 }
 
 // StoreCRL stores a CRL in the DB
 func (db *DB) StoreCRL(crlInfo *CertificateRevocationListInfo) error {
-
 	crlInfoBytes, err := json.Marshal(crlInfo)
 	if err != nil {
 		return errors.Wrap(err, "json Marshal error")
@@ -293,11 +291,6 @@ func (db *DB) StoreCRL(crlInfo *CertificateRevocationListInfo) error {
 // GetCRL gets the existing CRL from the database
 func (db *DB) GetCRL() (*CertificateRevocationListInfo, error) {
 	crlInfoBytes, err := db.Get(crlTable, crlKey)
-
-	if database.IsErrNotFound(err) {
-		return nil, nil
-	}
-
 	if err != nil {
 		return nil, errors.Wrap(err, "database Get error")
 	}

docs/GETTING_STARTED.md

@@ -119,11 +119,6 @@ starting the CA.
 * `address`: e.g. `127.0.0.1:8080` - address and port on which the CA will bind
 and respond to requests.
 
-* `crl`: Certificate Revocation List settings:
-    - generate: Enable/Disable CRL generation (`true` to generate, `false` to disable)
-
-    - cacheDuration: Time between CRL regeneration task. E.g if set to `5m`, step-ca will regenerate the CRL every 5 minutes.
-
 * `dnsNames`: comma separated list of DNS Name(s) for the CA.
 
 * `logger`: the default logging format for the CA is `text`. The other option
@@ -131,17 +126,20 @@ is `json`.
 
 * `db`: data persistence layer. See [database documentation](./database.md) for more
 info.
+  * `type`: `badger`, `bbolt`, `mysql`, etc.
+  * `dataSource`: string that can be interpreted differently depending on the
+    type of the database. Usually a path to where the data is stored. See the
+    [database configuration docs](./database.md#configuration) for more info.
+  * `database`: name of the database. Used for backends that may have multiple
+    databases. e.g. MySQL
+  * `valueDir`: directory to store the value log in (Badger specific).
 
-    - type: `badger`, `bbolt`, `mysql`, etc.
-
-    - dataSource: `string` that can be interpreted differently depending on the
-    type of the database. Usually a path to where the data is stored. See
-    the [database configuration docs](./database.md#configuration) for more info.
-
-    - database: name of the database. Used for backends that may have
-    multiple databases. e.g. MySQL
-
-    - valueDir: directory to store the value log in (Badger specific).
+* `crl`: Certificate Revocation List settings:
+  * `enable`: enables CRL generation (`true` to generate, `false` to disable)
+  * `generateOnRevoke`: a revoke will generate a new CRL if the crl is enabled.
+  * `cacheDuration`: the duration until next update of the CRL, defaults to 24h.
+  * `renewPeriod`: the time between CRL regeneration. If not set ~2/3 of the
+    cacheDuration will be used.
 
 * `tls`: settings for negotiating communication with the CA; includes acceptable
 ciphersuites, min/max TLS version, etc.