implement changes from review

api/api.go

@@ -46,7 +46,8 @@ type Authority interface {
 	GetRoots() (federation []*x509.Certificate, err error)
 	GetFederation() ([]*x509.Certificate, error)
 	Version() authority.Version
-	GenerateCertificateRevocationList(force bool) ([]byte, error)
+	GenerateCertificateRevocationList() error
+	GetCertificateRevocationList() ([]byte, error)
 }
 
 // TimeDuration is an alias of provisioner.TimeDuration

api/crl.go

@@ -2,23 +2,53 @@ package api
 
 import (
 	"encoding/pem"
+	"fmt"
+	"github.com/pkg/errors"
 	"net/http"
 )
 
-// CRL is an HTTP handler that returns the current CRL in PEM format
+// CRL is an HTTP handler that returns the current CRL in DER or PEM format
 func (h *caHandler) CRL(w http.ResponseWriter, r *http.Request) {
-	crlBytes, err := h.Authority.GenerateCertificateRevocationList(false)
+	crlBytes, err := h.Authority.GetCertificateRevocationList()
+
+	_, formatAsPEM := r.URL.Query()["pem"]
 
 	if err != nil {
 		w.WriteHeader(500)
+		_, err = fmt.Fprintf(w, "%v\n", err)
+		if err != nil {
+			panic(errors.Wrap(err, "error writing http response"))
+		}
 		return
 	}
 
-	pemBytes := pem.EncodeToMemory(&pem.Block{
-		Type:  "X509 CRL",
-		Bytes: crlBytes,
-	})
+	if crlBytes == nil {
+		w.WriteHeader(404)
+		_, err = fmt.Fprintln(w, "No CRL available")
+		if err != nil {
+			panic(errors.Wrap(err, "error writing http response"))
+		}
+		return
+	}
+
+	if formatAsPEM {
+		pemBytes := pem.EncodeToMemory(&pem.Block{
+			Type:  "X509 CRL",
+			Bytes: crlBytes,
+		})
+		w.Header().Add("Content-Type", "application/x-pem-file")
+		w.Header().Add("Content-Disposition", "attachment; filename=\"crl.pem\"")
+		_, err = w.Write(pemBytes)
+	} else {
+		w.Header().Add("Content-Type", "application/pkix-crl")
+		w.Header().Add("Content-Disposition", "attachment; filename=\"crl.der\"")
+		_, err = w.Write(crlBytes)
+	}
 
 	w.WriteHeader(200)
-	_, err = w.Write(pemBytes)
+
+	if err != nil {
+		panic(errors.Wrap(err, "error writing http response"))
+	}
+
 }

authority/authority.go

@@ -64,6 +64,9 @@ type Authority struct {
 	sshCAUserFederatedCerts []ssh.PublicKey
 	sshCAHostFederatedCerts []ssh.PublicKey
 
+	// CRL vars
+	crlChannel chan int
+
 	// Do not re-initialize
 	initOnce  bool
 	startTime time.Time
@@ -550,6 +553,16 @@ func (a *Authority) init() error {
 	// not be repeated.
 	a.initOnce = true
 
+	// Start the CRL generator
+	if a.config.CRL != nil {
+		if a.config.CRL.Generate && a.config.CRL.CacheDuration.Duration > time.Duration(0) {
+			err := a.startCRLGenerator()
+			if err != nil {
+				return err
+			}
+		}
+	}
+
 	return nil
 }
 
@@ -615,3 +628,60 @@ func (a *Authority) requiresSCEPService() bool {
 func (a *Authority) GetSCEPService() *scep.Service {
 	return a.scepService
 }
+
+func (a *Authority) startCRLGenerator() error {
+
+	if a.config.CRL.CacheDuration.Duration > time.Duration(0) {
+		// Check that there is a valid CRL in the DB right now. If it doesnt exist
+		// or is expired, generated one now
+		crlDB, ok := a.db.(db.CertificateRevocationListDB)
+		if !ok {
+			return errors.Errorf("CRL Generation requested, but database does not support CRL generation")
+		}
+
+		crlInfo, err := crlDB.GetCRL()
+		if err != nil {
+			return errors.Wrap(err, "could not retrieve CRL from database")
+		}
+
+		if crlInfo == nil {
+			log.Println("No CRL exists in the DB, generating one now")
+			err = a.GenerateCertificateRevocationList()
+			if err != nil {
+				return errors.Wrap(err, "could not generate a CRL")
+			}
+		}
+
+		if crlInfo.ExpiresAt.Before(time.Now().UTC()) {
+			log.Printf("Existing CRL has expired (at %v), generating a new one", crlInfo.ExpiresAt)
+			err = a.GenerateCertificateRevocationList()
+			if err != nil {
+				return errors.Wrap(err, "could not generate a CRL")
+			}
+		}
+
+		log.Printf("CRL will be auto-generated every %v", a.config.CRL.CacheDuration)
+		tickerDuration := a.config.CRL.CacheDuration.Duration - time.Minute // generate the new CRL 1 minute before it expires
+		if tickerDuration <= 0 {
+			log.Printf("WARNING: Addition of jitter to CRL generation time %v creates a negative duration (%v). Using 1 minute cacheDuration", a.config.CRL.CacheDuration, tickerDuration)
+			tickerDuration = time.Minute
+		}
+		crlTicker := time.NewTicker(tickerDuration)
+
+		go func() {
+			for {
+				select {
+				case <-crlTicker.C:
+					log.Println("Regenerating CRL")
+					err := a.GenerateCertificateRevocationList()
+					if err != nil {
+						// TODO: log or panic here?
+						panic(errors.Wrap(err, "authority.crlGenerator encountered an error"))
+					}
+				}
+			}
+		}()
+	}
+
+	return nil
+}

authority/config/config.go

@@ -64,6 +64,7 @@ type Config struct {
 	TLS              *TLSOptions          `json:"tls,omitempty"`
 	Password         string               `json:"password,omitempty"`
 	Templates        *templates.Templates `json:"templates,omitempty"`
+	CRL              *CRLConfig           `json:"crl,omitempty"`
 }
 
 // ASN1DN contains ASN1.DN attributes that are used in Subject and Issuer
@@ -95,6 +96,12 @@ type AuthConfig struct {
 	EnableAdmin          bool                  `json:"enableAdmin,omitempty"`
 }
 
+// CRLConfig represents config options for CRL generation
+type CRLConfig struct {
+	Generate      bool                  `json:"generate,omitempty"`
+	CacheDuration *provisioner.Duration `json:"cacheDuration,omitempty"`
+}
+
 // init initializes the required fields in the AuthConfig if they are not
 // provided.
 func (c *AuthConfig) init() {

authority/tls.go

@@ -364,6 +364,16 @@ func (a *Authority) Revoke(ctx context.Context, revokeOpts *RevokeOptions) error
 		p   provisioner.Interface
 		err error
 	)
+
+	// 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
+	}
+
 	// If not mTLS then get the TokenID of the token.
 	if !revokeOpts.MTLS {
 		token, err := jose.ParseSigned(revokeOpts.OTT)
@@ -430,7 +440,10 @@ func (a *Authority) Revoke(ctx context.Context, revokeOpts *RevokeOptions) error
 		err = a.revoke(revokedCert, rci)
 
 		// Generate a new CRL so CRL requesters will always get an up-to-date CRL whenever they request it
-		_, _ = a.GenerateCertificateRevocationList(true)
+		err = a.GenerateCertificateRevocationList()
+		if err != nil {
+			return errs.Wrap(http.StatusInternalServerError, err, "authority.Revoke", opts...)
+		}
 	}
 	switch err {
 	case nil:
@@ -463,36 +476,64 @@ func (a *Authority) revokeSSH(crt *ssh.Certificate, rci *db.RevokedCertificateIn
 	return a.db.Revoke(rci)
 }
 
-// GenerateCertificateRevocationList returns a DER representation of a signed CRL.
-// It will look for a valid generated CRL in the database, check if it has expired, and generate
-// a new CRL on demand if it has expired (or a CRL does not already exist).
-//
-// force set to true will force regeneration of the CRL regardless of whether it has actually expired
-func (a *Authority) GenerateCertificateRevocationList(force bool) ([]byte, error) {
-
-	// check for an existing CRL in the database, and return that if its valid
-	crlInfo, err := a.db.GetCRL()
-
-	if err != nil {
-		return nil, err
+// 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 {
+		return nil, errs.Wrap(http.StatusInternalServerError, errors.Errorf("Certificate Revocation Lists are not enabled"), "authority.GetCertificateRevocationList")
 	}
 
-	if !force && crlInfo != nil && crlInfo.ExpiresAt.After(time.Now().UTC()) {
-		return crlInfo.DER, nil
+	crlDB, ok := a.db.(db.CertificateRevocationListDB)
+	if !ok {
+		return nil, errs.Wrap(http.StatusInternalServerError, errors.Errorf("Database does not support Certificate Revocation Lists"), "authority.GetCertificateRevocationList")
+	}
+
+	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
+}
+
+// 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
+		return nil
+	}
+
+	crlDB, ok := a.db.(db.CertificateRevocationListDB)
+	if !ok {
+		return errors.Errorf("Database does not support CRL generation")
 	}
 
 	// some CAS may not implement the CRLGenerator interface, so check before we proceed
 	caCRLGenerator, ok := a.x509CAService.(casapi.CertificateAuthorityCRLGenerator)
-
 	if !ok {
-		return nil, errors.Errorf("CRL Generator not implemented")
+		return errors.Errorf("CA does not support CRL Generation")
 	}
 
-	revokedList, err := a.db.GetRevokedCertificates()
+	crlInfo, err := crlDB.GetCRL()
+	if err != nil {
+		return errors.Wrap(err, "could not retrieve CRL from database")
+	}
+
+	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 = 0
+	var n int64
 	var bn big.Int
 
 	if crlInfo != nil {
@@ -515,37 +556,36 @@ func (a *Authority) GenerateCertificateRevocationList(force bool) ([]byte, error
 	}
 
 	// Create a RevocationList representation ready for the CAS to sign
-	// TODO: use a config value for the NextUpdate time duration
 	// TODO: allow SignatureAlgorithm to be specified?
 	revocationList := x509.RevocationList{
 		SignatureAlgorithm:  0,
 		RevokedCertificates: revokedCertificates,
 		Number:              &bn,
 		ThisUpdate:          time.Now().UTC(),
-		NextUpdate:          time.Now().UTC().Add(time.Minute * 10),
+		NextUpdate:          time.Now().UTC().Add(a.config.CRL.CacheDuration.Duration),
 		ExtraExtensions:     nil,
 	}
 
-	certificateRevocationList, err := caCRLGenerator.CreateCertificateRevocationList(&revocationList)
+	certificateRevocationList, err := caCRLGenerator.CreateCRL(&casapi.CreateCRLRequest{RevocationList: &revocationList})
 	if err != nil {
-		return nil, err
+		return errors.Wrap(err, "could not create CRL")
 	}
 
 	// Create a new db.CertificateRevocationListInfo, which stores the new Number we just generated, the
-	// expiry time, and the DER-encoded CRL - then store it in the DB
+	// expiry time, and the DER-encoded CRL
 	newCRLInfo := db.CertificateRevocationListInfo{
 		Number:    n,
 		ExpiresAt: revocationList.NextUpdate,
-		DER:       certificateRevocationList,
+		DER:       certificateRevocationList.CRL,
 	}
 
-	err = a.db.StoreCRL(&newCRLInfo)
+	// Store the CRL in the database ready for retrieval by api endpoints
+	err = crlDB.StoreCRL(&newCRLInfo)
 	if err != nil {
-		return nil, err
+		return errors.Wrap(err, "could not store CRL in database")
 	}
 
-	// Finally, return our CRL in DER
-	return certificateRevocationList, nil
+	return nil
 }
 
 // GetTLSCertificate creates a new leaf certificate to be used by the CA HTTPS server.

cas/apiv1/requests.go

@@ -144,3 +144,13 @@ type CreateCertificateAuthorityResponse struct {
 	PrivateKey       crypto.PrivateKey
 	Signer           crypto.Signer
 }
+
+// CreateCRLRequest is the request to create a Certificate Revocation List.
+type CreateCRLRequest struct {
+	RevocationList *x509.RevocationList
+}
+
+// CreateCRLResponse is the response to a Certificate Revocation List request.
+type CreateCRLResponse struct {
+	CRL []byte //the CRL in DER format
+}

cas/apiv1/services.go

@@ -17,7 +17,7 @@ type CertificateAuthorityService interface {
 // CertificateAuthorityCRLGenerator is an optional interface implemented by CertificateAuthorityService
 // that has a method to create a CRL
 type CertificateAuthorityCRLGenerator interface {
-	CreateCertificateRevocationList(crl *x509.RevocationList) ([]byte, error)
+	CreateCRL(req *CreateCRLRequest) (*CreateCRLResponse, error)
 }
 
 // CertificateAuthorityGetter is an interface implemented by a

cas/softcas/softcas.go

@@ -113,15 +113,15 @@ func (c *SoftCAS) RevokeCertificate(req *apiv1.RevokeCertificateRequest) (*apiv1
 	}, nil
 }
 
-// CreateCertificateRevocationList will create a new CRL based on the RevocationList passed to it
-func (c *SoftCAS) CreateCertificateRevocationList(crl *x509.RevocationList) ([]byte, error) {
+// CreateCRL will create a new CRL based on the RevocationList passed to it
+func (c *SoftCAS) CreateCRL(req *apiv1.CreateCRLRequest) (*apiv1.CreateCRLResponse, error) {
 
-	revocationList, err := x509.CreateRevocationList(rand.Reader, crl, c.CertificateChain[0], c.Signer)
+	revocationListBytes, err := x509.CreateRevocationList(rand.Reader, req.RevocationList, c.CertificateChain[0], c.Signer)
 	if err != nil {
 		return nil, err
 	}
 
-	return revocationList, nil
+	return &apiv1.CreateCRLResponse{CRL: revocationListBytes}, nil
 }
 
 // CreateCertificateAuthority creates a root or an intermediate certificate.

db/db.go

@@ -51,9 +51,6 @@ type AuthDB interface {
 	IsSSHRevoked(sn string) (bool, error)
 	Revoke(rci *RevokedCertificateInfo) error
 	RevokeSSH(rci *RevokedCertificateInfo) error
-	GetRevokedCertificates() (*[]RevokedCertificateInfo, error)
-	GetCRL() (*CertificateRevocationListInfo, error)
-	StoreCRL(*CertificateRevocationListInfo) error
 	GetCertificate(serialNumber string) (*x509.Certificate, error)
 	StoreCertificate(crt *x509.Certificate) error
 	UseToken(id, tok string) (bool, error)
@@ -63,6 +60,13 @@ type AuthDB interface {
 	Shutdown() error
 }
 
+// CertificateRevocationListDB is an interface to indicate whether the DB supports CRL generation
+type CertificateRevocationListDB interface {
+	GetRevokedCertificates() (*[]RevokedCertificateInfo, error)
+	GetCRL() (*CertificateRevocationListInfo, error)
+	StoreCRL(*CertificateRevocationListInfo) error
+}
+
 // DB is a wrapper over the nosql.DB interface.
 type DB struct {
 	nosql.DB
@@ -109,6 +113,7 @@ type RevokedCertificateInfo struct {
 	ReasonCode    int
 	Reason        string
 	RevokedAt     time.Time
+	ExpiresAt     time.Time
 	TokenID       string
 	MTLS          bool
 }
@@ -215,7 +220,22 @@ func (db *DB) GetRevokedCertificates() (*[]RevokedCertificateInfo, error) {
 		if err := json.Unmarshal(e.Value, &data); err != nil {
 			return nil, err
 		}
-		revokedCerts = append(revokedCerts, data)
+
+		if !data.ExpiresAt.IsZero() && data.ExpiresAt.After(time.Now().UTC()) {
+			revokedCerts = append(revokedCerts, data)
+		} else if data.ExpiresAt.IsZero() {
+			cert, err := db.GetCertificate(data.Serial)
+			if err != nil {
+				revokedCerts = append(revokedCerts, data) // a revoked certificate may not be in the database,
+				// so its expiry date is undiscoverable and will need
+				// to be added to the crl always
+				continue
+			}
+
+			if cert.NotAfter.After(time.Now().UTC()) {
+				revokedCerts = append(revokedCerts, data)
+			}
+		}
 
 	}
 	return &revokedCerts, nil