implement changes from review

Author: Raal Goff

api/api.go

@@ -50,7 +50,8 @@ type Authority interface {
 	GetRoots() ([]*x509.Certificate, 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

@@ -65,6 +65,9 @@ type Authority struct {
 	sshCAUserFederatedCerts []ssh.PublicKey
 	sshCAHostFederatedCerts []ssh.PublicKey
 
+	// CRL vars
+	crlChannel chan int
+
 	// Do not re-initialize
 	initOnce  bool
 	startTime time.Time
@@ -553,6 +556,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
 }
 
@@ -646,3 +659,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

@@ -68,6 +68,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
@@ -99,6 +100,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

@@ -408,6 +408,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 nor ACME, then get the TokenID of the token.
 	if !(revokeOpts.MTLS || revokeOpts.ACME) {
 		token, err := jose.ParseSigned(revokeOpts.OTT)
@@ -474,7 +484,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:
@@ -509,36 +522,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) {
+// 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")
+	}
 
-	// check for an existing CRL in the database, and return that if its valid
-	crlInfo, err := a.db.GetCRL()
+	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, err
+		return nil, errs.Wrap(http.StatusInternalServerError, err, "authority.GetCertificateRevocationList")
+
 	}
 
-	if !force && crlInfo != nil && crlInfo.ExpiresAt.After(time.Now().UTC()) {
-		return crlInfo.DER, nil
+	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")
+	}
+
+	crlInfo, err := crlDB.GetCRL()
+	if err != nil {
+		return errors.Wrap(err, "could not retrieve CRL from database")
 	}
 
-	revokedList, err := a.db.GetRevokedCertificates()
+	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 {
@@ -561,37 +602,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

@@ -130,15 +130,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.