From 56926b9012b4e89377f923f0391db489a01280d4 Mon Sep 17 00:00:00 2001 From: Raal Goff Date: Sat, 30 Oct 2021 15:52:50 +0800 Subject: [PATCH 01/29] initial support for CRL --- api/api.go | 2 + api/crl.go | 16 +++++++ authority/tls.go | 95 ++++++++++++++++++++++++++++++++++++++++++ cas/apiv1/services.go | 6 +++ cas/softcas/softcas.go | 12 ++++++ db/db.go | 69 +++++++++++++++++++++++++++++- db/simple.go | 15 +++++++ 7 files changed, 214 insertions(+), 1 deletion(-) create mode 100644 api/crl.go diff --git a/api/api.go b/api/api.go index 30ba03f9..d8cfa15f 100644 --- a/api/api.go +++ b/api/api.go @@ -46,6 +46,7 @@ type Authority interface { GetRoots() (federation []*x509.Certificate, err error) GetFederation() ([]*x509.Certificate, error) Version() authority.Version + GenerateCertificateRevocationList(force bool) (string, error) } // TimeDuration is an alias of provisioner.TimeDuration @@ -254,6 +255,7 @@ func (h *caHandler) Route(r Router) { r.MethodFunc("POST", "/renew", h.Renew) r.MethodFunc("POST", "/rekey", h.Rekey) r.MethodFunc("POST", "/revoke", h.Revoke) + r.MethodFunc("GET", "/crl", h.CRL) r.MethodFunc("GET", "/provisioners", h.Provisioners) r.MethodFunc("GET", "/provisioners/{kid}/encrypted-key", h.ProvisionerKey) r.MethodFunc("GET", "/roots", h.Roots) diff --git a/api/crl.go b/api/crl.go new file mode 100644 index 00000000..50b29d6c --- /dev/null +++ b/api/crl.go @@ -0,0 +1,16 @@ +package api + +import "net/http" + +// CRL is an HTTP handler that returns the current CRL +func (h *caHandler) CRL(w http.ResponseWriter, r *http.Request) { + crl, err := h.Authority.GenerateCertificateRevocationList(false) + + if err != nil { + w.WriteHeader(500) + return + } + + w.WriteHeader(200) + _, err = w.Write([]byte(crl)) +} diff --git a/authority/tls.go b/authority/tls.go index 839866a2..0bd4a7e7 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -5,9 +5,12 @@ import ( "crypto" "crypto/tls" "crypto/x509" + "crypto/x509/pkix" "encoding/asn1" "encoding/base64" "encoding/pem" + "fmt" + "math/big" "net/http" "time" @@ -426,6 +429,9 @@ func (a *Authority) Revoke(ctx context.Context, revokeOpts *RevokeOptions) error // Save as revoked in the Db. 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) } switch err { case nil: @@ -458,6 +464,95 @@ func (a *Authority) revokeSSH(crt *ssh.Certificate, rci *db.RevokedCertificateIn return a.db.Revoke(rci) } +// GenerateCertificateRevocationList returns a PEM 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) (string, error) { + + // check for an existing CRL in the database, and return that if its valid + crlInfo, err := a.db.GetCRL() + + if err != nil { + return "", err + } + + if !force && crlInfo != nil && crlInfo.ExpiresAt.After(time.Now().UTC()) { + return crlInfo.PEM, nil + } + + // some CAS may not implement the CRLGenerator interface, so check before we proceed + caCRLGenerator, ok := a.x509CAService.(casapi.CertificateAuthorityCRLGenerator) + + if !ok { + return "", errors.Errorf("CRL Generator not implemented") + } + + revokedList, err := a.db.GetRevokedCertificates() + + // 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 bn big.Int + + if crlInfo != nil { + n = crlInfo.Number + 1 + } + bn.SetInt64(n) + + // Convert our database db.RevokedCertificateInfo types into the pkix representation ready for the + // CAS to sign it + var revokedCertificates []pkix.RevokedCertificate + + for _, revokedCert := range *revokedList { + var sn big.Int + sn.SetString(revokedCert.Serial, 10) + revokedCertificates = append(revokedCertificates, pkix.RevokedCertificate{ + SerialNumber: &sn, + RevocationTime: revokedCert.RevokedAt, + Extensions: nil, + }) + } + + // 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), + ExtraExtensions: nil, + } + + certificateRevocationList, err := caCRLGenerator.CreateCertificateRevocationList(&revocationList) + if err != nil { + return "", err + } + + // Quick and dirty PEM encoding + // TODO: clean this up + pemCRL := fmt.Sprintf("-----BEGIN X509 CRL-----\n%s\n-----END X509 CRL-----\n", base64.StdEncoding.EncodeToString(certificateRevocationList)) + + // Create a new db.CertificateRevocationListInfo, which stores the new Number we just generated, the + // expiry time, and the byte-encoded CRL - then store it in the DB + newCRLInfo := db.CertificateRevocationListInfo{ + Number: n, + ExpiresAt: revocationList.NextUpdate, + PEM: pemCRL, + } + + err = a.db.StoreCRL(&newCRLInfo) + if err != nil { + return "", err + } + + // Finally, return our CRL PEM + return pemCRL, nil +} + // GetTLSCertificate creates a new leaf certificate to be used by the CA HTTPS server. func (a *Authority) GetTLSCertificate() (*tls.Certificate, error) { fatal := func(err error) (*tls.Certificate, error) { diff --git a/cas/apiv1/services.go b/cas/apiv1/services.go index cf9a5470..143c1f17 100644 --- a/cas/apiv1/services.go +++ b/cas/apiv1/services.go @@ -14,6 +14,12 @@ type CertificateAuthorityService interface { RevokeCertificate(req *RevokeCertificateRequest) (*RevokeCertificateResponse, error) } +// 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) +} + // CertificateAuthorityGetter is an interface implemented by a // CertificateAuthorityService that has a method to get the root certificate. type CertificateAuthorityGetter interface { diff --git a/cas/softcas/softcas.go b/cas/softcas/softcas.go index 8e67d016..24e7dd54 100644 --- a/cas/softcas/softcas.go +++ b/cas/softcas/softcas.go @@ -3,6 +3,7 @@ package softcas import ( "context" "crypto" + "crypto/rand" "crypto/x509" "time" @@ -112,6 +113,17 @@ 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) { + + revocationList, err := x509.CreateRevocationList(rand.Reader, crl, c.CertificateChain[0], c.Signer) + if err != nil { + return nil, err + } + + return revocationList, nil +} + // CreateCertificateAuthority creates a root or an intermediate certificate. func (c *SoftCAS) CreateCertificateAuthority(req *apiv1.CreateCertificateAuthorityRequest) (*apiv1.CreateCertificateAuthorityResponse, error) { switch { diff --git a/db/db.go b/db/db.go index 2643e577..5826d7b9 100644 --- a/db/db.go +++ b/db/db.go @@ -16,6 +16,7 @@ import ( var ( certsTable = []byte("x509_certs") revokedCertsTable = []byte("revoked_x509_certs") + crlTable = []byte("x509_crl") revokedSSHCertsTable = []byte("revoked_ssh_certs") usedOTTTable = []byte("used_ott") sshCertsTable = []byte("ssh_certs") @@ -24,6 +25,9 @@ var ( sshHostPrincipalsTable = []byte("ssh_host_principals") ) +var crlKey = []byte("crl") //TODO: at the moment we store a single CRL in the database, in a dedicated table. +// is this acceptable? probably not.... + // ErrAlreadyExists can be returned if the DB attempts to set a key that has // been previously set. var ErrAlreadyExists = errors.New("already exists") @@ -47,6 +51,9 @@ 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) @@ -82,7 +89,7 @@ func New(c *Config) (AuthDB, error) { tables := [][]byte{ revokedCertsTable, certsTable, usedOTTTable, sshCertsTable, sshHostsTable, sshHostPrincipalsTable, sshUsersTable, - revokedSSHCertsTable, + revokedSSHCertsTable, crlTable, } for _, b := range tables { if err := db.CreateTable(b); err != nil { @@ -106,6 +113,14 @@ type RevokedCertificateInfo struct { MTLS bool } +// CertificateRevocationListInfo contains a CRL in PEM and associated metadata to allow a decision on whether +// to regenerate the CRL or not easier +type CertificateRevocationListInfo struct { + Number int64 + ExpiresAt time.Time + PEM string +} + // IsRevoked returns whether or not a certificate with the given identifier // has been revoked. // In the case of an X509 Certificate the `id` should be the Serial Number of @@ -188,6 +203,58 @@ func (db *DB) RevokeSSH(rci *RevokedCertificateInfo) error { } } +// GetRevokedCertificates gets a list of all revoked certificates. +func (db *DB) GetRevokedCertificates() (*[]RevokedCertificateInfo, error) { + entries, err := db.List(revokedCertsTable) + if err != nil { + return nil, err + } + var revokedCerts []RevokedCertificateInfo + for _, e := range entries { + var data RevokedCertificateInfo + if err := json.Unmarshal(e.Value, &data); err != nil { + return nil, err + } + 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") + } + + if err := db.Set(crlTable, crlKey, crlInfoBytes); err != nil { + return errors.Wrap(err, "database Set error") + } + return nil +} + +// 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") + } + + var crlInfo CertificateRevocationListInfo + err = json.Unmarshal(crlInfoBytes, &crlInfo) + if err != nil { + return nil, errors.Wrap(err, "json Unmarshal error") + } + return &crlInfo, err +} + // GetCertificate retrieves a certificate by the serial number. func (db *DB) GetCertificate(serialNumber string) (*x509.Certificate, error) { asn1Data, err := db.Get(certsTable, []byte(serialNumber)) diff --git a/db/simple.go b/db/simple.go index 0e5426ec..c34cdb5d 100644 --- a/db/simple.go +++ b/db/simple.go @@ -41,6 +41,21 @@ func (s *SimpleDB) Revoke(rci *RevokedCertificateInfo) error { return ErrNotImplemented } +// GetRevokedCertificates returns a "NotImplemented" error. +func (s *SimpleDB) GetRevokedCertificates() (*[]RevokedCertificateInfo, error) { + return nil, ErrNotImplemented +} + +// GetCRL returns a "NotImplemented" error. +func (s *SimpleDB) GetCRL() (*CertificateRevocationListInfo, error) { + return nil, ErrNotImplemented +} + +// StoreCRL returns a "NotImplemented" error. +func (s *SimpleDB) StoreCRL(crlInfo *CertificateRevocationListInfo) error { + return ErrNotImplemented +} + // RevokeSSH returns a "NotImplemented" error. func (s *SimpleDB) RevokeSSH(rci *RevokedCertificateInfo) error { return ErrNotImplemented From 8545adea92c039de2aa4d202661febfaec5f498f Mon Sep 17 00:00:00 2001 From: Raal Goff Date: Tue, 2 Nov 2021 13:26:07 +0800 Subject: [PATCH 02/29] change GenerateCertificateRevocationList to return DER, store DER in db instead of PEM, nicer PEM encoding of CRL, add Mock stubs --- api/api.go | 2 +- api/api_test.go | 4 ++++ api/crl.go | 16 ++++++++++++---- authority/tls.go | 21 ++++++++------------- db/db.go | 18 +++++++++++++++--- 5 files changed, 40 insertions(+), 21 deletions(-) diff --git a/api/api.go b/api/api.go index d8cfa15f..75a0397b 100644 --- a/api/api.go +++ b/api/api.go @@ -46,7 +46,7 @@ type Authority interface { GetRoots() (federation []*x509.Certificate, err error) GetFederation() ([]*x509.Certificate, error) Version() authority.Version - GenerateCertificateRevocationList(force bool) (string, error) + GenerateCertificateRevocationList(force bool) ([]byte, error) } // TimeDuration is an alias of provisioner.TimeDuration diff --git a/api/api_test.go b/api/api_test.go index 89596165..aef6db77 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -580,6 +580,10 @@ type mockAuthority struct { version func() authority.Version } +func (m *mockAuthority) GenerateCertificateRevocationList(force bool) ([]byte, error) { + panic("implement me") +} + // TODO: remove once Authorize is deprecated. func (m *mockAuthority) Authorize(ctx context.Context, ott string) ([]provisioner.SignOption, error) { return m.AuthorizeSign(ott) diff --git a/api/crl.go b/api/crl.go index 50b29d6c..46758777 100644 --- a/api/crl.go +++ b/api/crl.go @@ -1,16 +1,24 @@ package api -import "net/http" +import ( + "encoding/pem" + "net/http" +) -// CRL is an HTTP handler that returns the current CRL +// CRL is an HTTP handler that returns the current CRL in PEM format func (h *caHandler) CRL(w http.ResponseWriter, r *http.Request) { - crl, err := h.Authority.GenerateCertificateRevocationList(false) + crlBytes, err := h.Authority.GenerateCertificateRevocationList(false) if err != nil { w.WriteHeader(500) return } + pemBytes := pem.EncodeToMemory(&pem.Block{ + Type: "X509 CRL", + Bytes: crlBytes, + }) + w.WriteHeader(200) - _, err = w.Write([]byte(crl)) + _, err = w.Write(pemBytes) } diff --git a/authority/tls.go b/authority/tls.go index 0bd4a7e7..b46cb40d 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -9,7 +9,6 @@ import ( "encoding/asn1" "encoding/base64" "encoding/pem" - "fmt" "math/big" "net/http" "time" @@ -469,24 +468,24 @@ func (a *Authority) revokeSSH(crt *ssh.Certificate, rci *db.RevokedCertificateIn // 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) (string, error) { +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 "", err + return nil, err } if !force && crlInfo != nil && crlInfo.ExpiresAt.After(time.Now().UTC()) { - return crlInfo.PEM, nil + return crlInfo.DER, nil } // some CAS may not implement the CRLGenerator interface, so check before we proceed caCRLGenerator, ok := a.x509CAService.(casapi.CertificateAuthorityCRLGenerator) if !ok { - return "", errors.Errorf("CRL Generator not implemented") + return nil, errors.Errorf("CRL Generator not implemented") } revokedList, err := a.db.GetRevokedCertificates() @@ -529,28 +528,24 @@ func (a *Authority) GenerateCertificateRevocationList(force bool) (string, error certificateRevocationList, err := caCRLGenerator.CreateCertificateRevocationList(&revocationList) if err != nil { - return "", err + return nil, err } - // Quick and dirty PEM encoding - // TODO: clean this up - pemCRL := fmt.Sprintf("-----BEGIN X509 CRL-----\n%s\n-----END X509 CRL-----\n", base64.StdEncoding.EncodeToString(certificateRevocationList)) - // Create a new db.CertificateRevocationListInfo, which stores the new Number we just generated, the // expiry time, and the byte-encoded CRL - then store it in the DB newCRLInfo := db.CertificateRevocationListInfo{ Number: n, ExpiresAt: revocationList.NextUpdate, - PEM: pemCRL, + DER: certificateRevocationList, } err = a.db.StoreCRL(&newCRLInfo) if err != nil { - return "", err + return nil, err } // Finally, return our CRL PEM - return pemCRL, nil + return certificateRevocationList, nil } // GetTLSCertificate creates a new leaf certificate to be used by the CA HTTPS server. diff --git a/db/db.go b/db/db.go index 5826d7b9..72c4ec71 100644 --- a/db/db.go +++ b/db/db.go @@ -113,12 +113,12 @@ type RevokedCertificateInfo struct { MTLS bool } -// CertificateRevocationListInfo contains a CRL in PEM and associated metadata to allow a decision on whether -// to regenerate the CRL or not easier +// CertificateRevocationListInfo contains a CRL in DER format and associated +// metadata to allow a decision on whether to regenerate the CRL or not easier type CertificateRevocationListInfo struct { Number int64 ExpiresAt time.Time - PEM string + DER []byte } // IsRevoked returns whether or not a certificate with the given identifier @@ -378,6 +378,18 @@ type MockAuthDB struct { MShutdown func() error } +func (m *MockAuthDB) GetRevokedCertificates() (*[]RevokedCertificateInfo, error) { + panic("implement me") +} + +func (m *MockAuthDB) GetCRL() (*CertificateRevocationListInfo, error) { + panic("implement me") +} + +func (m *MockAuthDB) StoreCRL(info *CertificateRevocationListInfo) error { + panic("implement me") +} + // IsRevoked mock. func (m *MockAuthDB) IsRevoked(sn string) (bool, error) { if m.MIsRevoked != nil { From 26cb52a573bef0cc1d3c509d355ba1b2c1afb0cc Mon Sep 17 00:00:00 2001 From: Raal Goff Date: Tue, 2 Nov 2021 16:39:29 +0800 Subject: [PATCH 03/29] missed some mentions of PEM when changing the returned format to DER regarding CRL generation --- authority/tls.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/authority/tls.go b/authority/tls.go index b46cb40d..82179f13 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -463,7 +463,7 @@ func (a *Authority) revokeSSH(crt *ssh.Certificate, rci *db.RevokedCertificateIn return a.db.Revoke(rci) } -// GenerateCertificateRevocationList returns a PEM representation of a signed CRL. +// 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). // @@ -532,7 +532,7 @@ func (a *Authority) GenerateCertificateRevocationList(force bool) ([]byte, error } // Create a new db.CertificateRevocationListInfo, which stores the new Number we just generated, the - // expiry time, and the byte-encoded CRL - then store it in the DB + // expiry time, and the DER-encoded CRL - then store it in the DB newCRLInfo := db.CertificateRevocationListInfo{ Number: n, ExpiresAt: revocationList.NextUpdate, @@ -544,7 +544,7 @@ func (a *Authority) GenerateCertificateRevocationList(force bool) ([]byte, error return nil, err } - // Finally, return our CRL PEM + // Finally, return our CRL in DER return certificateRevocationList, nil } From 222b52db130543bc6b6497fecddc8c7e7f084a69 Mon Sep 17 00:00:00 2001 From: Raal Goff Date: Thu, 4 Nov 2021 14:05:07 +0800 Subject: [PATCH 04/29] implement changes from review --- api/api.go | 3 +- api/crl.go | 44 ++++++++++++++--- authority/authority.go | 70 +++++++++++++++++++++++++++ authority/config/config.go | 7 +++ authority/tls.go | 98 +++++++++++++++++++++++++++----------- cas/apiv1/requests.go | 10 ++++ cas/apiv1/services.go | 2 +- cas/softcas/softcas.go | 8 ++-- db/db.go | 28 +++++++++-- 9 files changed, 224 insertions(+), 46 deletions(-) diff --git a/api/api.go b/api/api.go index 75a0397b..fa7602e1 100644 --- a/api/api.go +++ b/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 diff --git a/api/crl.go b/api/crl.go index 46758777..4d20cfeb 100644 --- a/api/crl.go +++ b/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")) + } + } diff --git a/authority/authority.go b/authority/authority.go index aa8698d7..0b885170 100644 --- a/authority/authority.go +++ b/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 +} diff --git a/authority/config/config.go b/authority/config/config.go index 75c32994..9177dc22 100644 --- a/authority/config/config.go +++ b/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() { diff --git a/authority/tls.go b/authority/tls.go index 82179f13..8f113342 100644 --- a/authority/tls.go +++ b/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. diff --git a/cas/apiv1/requests.go b/cas/apiv1/requests.go index bf745c17..630cecce 100644 --- a/cas/apiv1/requests.go +++ b/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 +} diff --git a/cas/apiv1/services.go b/cas/apiv1/services.go index 143c1f17..ce7d0364 100644 --- a/cas/apiv1/services.go +++ b/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 diff --git a/cas/softcas/softcas.go b/cas/softcas/softcas.go index 24e7dd54..23e48d45 100644 --- a/cas/softcas/softcas.go +++ b/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. diff --git a/db/db.go b/db/db.go index 72c4ec71..21cea901 100644 --- a/db/db.go +++ b/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 From 45975b061c36b8cd3525d651635fa39c7a96a110 Mon Sep 17 00:00:00 2001 From: Raal Goff Date: Tue, 29 Mar 2022 08:51:39 +0800 Subject: [PATCH 05/29] requested changes --- api/crl.go | 2 -- authority/authority.go | 77 ++++++++++++++++++------------------------ authority/tls.go | 4 +-- 3 files changed, 34 insertions(+), 49 deletions(-) diff --git a/api/crl.go b/api/crl.go index 4d20cfeb..c90a77f9 100644 --- a/api/crl.go +++ b/api/crl.go @@ -45,8 +45,6 @@ func (h *caHandler) CRL(w http.ResponseWriter, r *http.Request) { _, err = w.Write(crlBytes) } - w.WriteHeader(200) - if err != nil { panic(errors.Wrap(err, "error writing http response")) } diff --git a/authority/authority.go b/authority/authority.go index 0b885170..5f52d04e 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -6,6 +6,7 @@ import ( "crypto/sha256" "crypto/x509" "encoding/hex" + "fmt" "log" "strings" "sync" @@ -631,57 +632,43 @@ func (a *Authority) GetSCEPService() *scep.Service { 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") - } + if a.config.CRL.CacheDuration.Duration <= 0 { + return nil + } - crlInfo, err := crlDB.GetCRL() - if err != nil { - return errors.Wrap(err, "could not retrieve CRL from database") - } + // 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) + if !ok { + return errors.Errorf("CRL Generation requested, but database does not support CRL generation") + } - 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") - } - } + // 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 { + 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 { + panic(fmt.Sprintf("ERROR: Addition of jitter to CRL generation time %v creates a negative duration (%v). Use a CRL generation time of longer than 1 minute.", a.config.CRL.CacheDuration, tickerDuration)) + } + crlTicker := time.NewTicker(tickerDuration) - 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")) - } + go func() { + for { + select { + case <-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) } } - }() - } + } + }() return nil } diff --git a/authority/tls.go b/authority/tls.go index 8f113342..ba1c4939 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -480,12 +480,12 @@ func (a *Authority) revokeSSH(crt *ssh.Certificate, rci *db.RevokedCertificateIn // 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") + return nil, errs.Wrap(http.StatusNotFound, errors.Errorf("Certificate Revocation Lists are not enabled"), "authority.GetCertificateRevocationList") } 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") + return nil, errs.Wrap(http.StatusNotImplemented, errors.Errorf("Database does not support Certificate Revocation Lists"), "authority.GetCertificateRevocationList") } crlInfo, err := crlDB.GetCRL() From 8520c861d554b31eb21e92603b690c06790ff2d4 Mon Sep 17 00:00:00 2001 From: Raal Goff Date: Tue, 5 Apr 2022 11:19:13 +0800 Subject: [PATCH 06/29] implemented some requested changes --- api/api.go | 1 - api/api_test.go | 2 +- api/crl.go | 18 +++++++++--------- authority/authority.go | 15 ++++++++++++--- authority/tls.go | 14 ++++++++------ db/db.go | 6 ++++-- 6 files changed, 34 insertions(+), 22 deletions(-) diff --git a/api/api.go b/api/api.go index fa7602e1..f20df474 100644 --- a/api/api.go +++ b/api/api.go @@ -46,7 +46,6 @@ type Authority interface { GetRoots() (federation []*x509.Certificate, err error) GetFederation() ([]*x509.Certificate, error) Version() authority.Version - GenerateCertificateRevocationList() error GetCertificateRevocationList() ([]byte, error) } diff --git a/api/api_test.go b/api/api_test.go index aef6db77..7ce54d73 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -580,7 +580,7 @@ type mockAuthority struct { version func() authority.Version } -func (m *mockAuthority) GenerateCertificateRevocationList(force bool) ([]byte, error) { +func (m *mockAuthority) GetCertificateRevocationList() ([]byte, error) { panic("implement me") } diff --git a/api/crl.go b/api/crl.go index c90a77f9..45024470 100644 --- a/api/crl.go +++ b/api/crl.go @@ -4,6 +4,7 @@ import ( "encoding/pem" "fmt" "github.com/pkg/errors" + "github.com/smallstep/certificates/errs" "net/http" ) @@ -14,6 +15,14 @@ func (h *caHandler) CRL(w http.ResponseWriter, r *http.Request) { _, formatAsPEM := r.URL.Query()["pem"] if err != nil { + + caErr, isCaErr := err.(*errs.Error) + + if isCaErr { + http.Error(w, caErr.Msg, caErr.Status) + return + } + w.WriteHeader(500) _, err = fmt.Fprintf(w, "%v\n", err) if err != nil { @@ -22,15 +31,6 @@ func (h *caHandler) CRL(w http.ResponseWriter, r *http.Request) { return } - 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", diff --git a/authority/authority.go b/authority/authority.go index 5f52d04e..7414a0d4 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -66,7 +66,7 @@ type Authority struct { sshCAHostFederatedCerts []ssh.PublicKey // CRL vars - crlChannel chan int + crlTicker *time.Ticker // Do not re-initialize initOnce bool @@ -586,6 +586,10 @@ func (a *Authority) IsAdminAPIEnabled() bool { // Shutdown safely shuts down any clients, databases, etc. held by the Authority. func (a *Authority) Shutdown() error { + if a.crlTicker != nil { + a.crlTicker.Stop() + } + if err := a.keyManager.Close(); err != nil { log.Printf("error closing the key manager: %v", err) } @@ -594,6 +598,11 @@ 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() + } + if err := a.keyManager.Close(); err != nil { log.Printf("error closing the key manager: %v", err) } @@ -655,12 +664,12 @@ func (a *Authority) startCRLGenerator() error { if tickerDuration <= 0 { panic(fmt.Sprintf("ERROR: Addition of jitter to CRL generation time %v creates a negative duration (%v). Use a CRL generation time of longer than 1 minute.", a.config.CRL.CacheDuration, tickerDuration)) } - crlTicker := time.NewTicker(tickerDuration) + a.crlTicker = time.NewTicker(tickerDuration) go func() { for { select { - case <-crlTicker.C: + case <-a.crlTicker.C: log.Println("Regenerating CRL") err := a.GenerateCertificateRevocationList() if err != nil { diff --git a/authority/tls.go b/authority/tls.go index ba1c4939..44da6b3a 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -365,13 +365,15 @@ func (a *Authority) Revoke(ctx context.Context, revokeOpts *RevokeOptions) error err error ) - // Attempt to get the certificate expiry using the serial number. - cert, err := a.db.GetCertificate(revokeOpts.Serial) + 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 + // 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. diff --git a/db/db.go b/db/db.go index 21cea901..883f54ed 100644 --- a/db/db.go +++ b/db/db.go @@ -215,13 +215,15 @@ func (db *DB) GetRevokedCertificates() (*[]RevokedCertificateInfo, error) { return nil, err } var revokedCerts []RevokedCertificateInfo + now := time.Now().UTC() + for _, e := range entries { var data RevokedCertificateInfo if err := json.Unmarshal(e.Value, &data); err != nil { return nil, err } - if !data.ExpiresAt.IsZero() && data.ExpiresAt.After(time.Now().UTC()) { + if !data.ExpiresAt.IsZero() && data.ExpiresAt.After(now) { revokedCerts = append(revokedCerts, data) } else if data.ExpiresAt.IsZero() { cert, err := db.GetCertificate(data.Serial) @@ -232,7 +234,7 @@ func (db *DB) GetRevokedCertificates() (*[]RevokedCertificateInfo, error) { continue } - if cert.NotAfter.After(time.Now().UTC()) { + if cert.NotAfter.After(now) { revokedCerts = append(revokedCerts, data) } } From e8fdb703c9a6a825d75e9dc88df0dc2e2e8ba4f2 Mon Sep 17 00:00:00 2001 From: Raal Goff Date: Sat, 30 Oct 2021 15:52:50 +0800 Subject: [PATCH 07/29] initial support for CRL --- api/api.go | 2 + api/crl.go | 16 +++++++ authority/tls.go | 94 ++++++++++++++++++++++++++++++++++++++++++ cas/apiv1/services.go | 6 +++ cas/softcas/softcas.go | 12 ++++++ db/db.go | 69 ++++++++++++++++++++++++++++++- db/simple.go | 15 +++++++ 7 files changed, 213 insertions(+), 1 deletion(-) create mode 100644 api/crl.go diff --git a/api/api.go b/api/api.go index da6309fd..7c935f3f 100644 --- a/api/api.go +++ b/api/api.go @@ -50,6 +50,7 @@ type Authority interface { GetRoots() ([]*x509.Certificate, error) GetFederation() ([]*x509.Certificate, error) Version() authority.Version + GenerateCertificateRevocationList(force bool) (string, error) } // TimeDuration is an alias of provisioner.TimeDuration @@ -258,6 +259,7 @@ func (h *caHandler) Route(r Router) { r.MethodFunc("POST", "/renew", h.Renew) r.MethodFunc("POST", "/rekey", h.Rekey) r.MethodFunc("POST", "/revoke", h.Revoke) + r.MethodFunc("GET", "/crl", h.CRL) r.MethodFunc("GET", "/provisioners", h.Provisioners) r.MethodFunc("GET", "/provisioners/{kid}/encrypted-key", h.ProvisionerKey) r.MethodFunc("GET", "/roots", h.Roots) diff --git a/api/crl.go b/api/crl.go new file mode 100644 index 00000000..50b29d6c --- /dev/null +++ b/api/crl.go @@ -0,0 +1,16 @@ +package api + +import "net/http" + +// CRL is an HTTP handler that returns the current CRL +func (h *caHandler) CRL(w http.ResponseWriter, r *http.Request) { + crl, err := h.Authority.GenerateCertificateRevocationList(false) + + if err != nil { + w.WriteHeader(500) + return + } + + w.WriteHeader(200) + _, err = w.Write([]byte(crl)) +} diff --git a/authority/tls.go b/authority/tls.go index 58a1247c..d6d6d6d1 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -5,12 +5,14 @@ import ( "crypto" "crypto/tls" "crypto/x509" + "crypto/x509/pkix" "encoding/asn1" "encoding/base64" "encoding/json" "encoding/pem" "fmt" "net" + "math/big" "net/http" "strings" "time" @@ -470,6 +472,9 @@ func (a *Authority) Revoke(ctx context.Context, revokeOpts *RevokeOptions) error // Save as revoked in the Db. 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) } switch err { case nil: @@ -504,6 +509,95 @@ func (a *Authority) revokeSSH(crt *ssh.Certificate, rci *db.RevokedCertificateIn return a.db.Revoke(rci) } +// GenerateCertificateRevocationList returns a PEM 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) (string, error) { + + // check for an existing CRL in the database, and return that if its valid + crlInfo, err := a.db.GetCRL() + + if err != nil { + return "", err + } + + if !force && crlInfo != nil && crlInfo.ExpiresAt.After(time.Now().UTC()) { + return crlInfo.PEM, nil + } + + // some CAS may not implement the CRLGenerator interface, so check before we proceed + caCRLGenerator, ok := a.x509CAService.(casapi.CertificateAuthorityCRLGenerator) + + if !ok { + return "", errors.Errorf("CRL Generator not implemented") + } + + revokedList, err := a.db.GetRevokedCertificates() + + // 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 bn big.Int + + if crlInfo != nil { + n = crlInfo.Number + 1 + } + bn.SetInt64(n) + + // Convert our database db.RevokedCertificateInfo types into the pkix representation ready for the + // CAS to sign it + var revokedCertificates []pkix.RevokedCertificate + + for _, revokedCert := range *revokedList { + var sn big.Int + sn.SetString(revokedCert.Serial, 10) + revokedCertificates = append(revokedCertificates, pkix.RevokedCertificate{ + SerialNumber: &sn, + RevocationTime: revokedCert.RevokedAt, + Extensions: nil, + }) + } + + // 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), + ExtraExtensions: nil, + } + + certificateRevocationList, err := caCRLGenerator.CreateCertificateRevocationList(&revocationList) + if err != nil { + return "", err + } + + // Quick and dirty PEM encoding + // TODO: clean this up + pemCRL := fmt.Sprintf("-----BEGIN X509 CRL-----\n%s\n-----END X509 CRL-----\n", base64.StdEncoding.EncodeToString(certificateRevocationList)) + + // Create a new db.CertificateRevocationListInfo, which stores the new Number we just generated, the + // expiry time, and the byte-encoded CRL - then store it in the DB + newCRLInfo := db.CertificateRevocationListInfo{ + Number: n, + ExpiresAt: revocationList.NextUpdate, + PEM: pemCRL, + } + + err = a.db.StoreCRL(&newCRLInfo) + if err != nil { + return "", err + } + + // Finally, return our CRL PEM + return pemCRL, nil +} + // GetTLSCertificate creates a new leaf certificate to be used by the CA HTTPS server. func (a *Authority) GetTLSCertificate() (*tls.Certificate, error) { fatal := func(err error) (*tls.Certificate, error) { diff --git a/cas/apiv1/services.go b/cas/apiv1/services.go index cf9a5470..143c1f17 100644 --- a/cas/apiv1/services.go +++ b/cas/apiv1/services.go @@ -14,6 +14,12 @@ type CertificateAuthorityService interface { RevokeCertificate(req *RevokeCertificateRequest) (*RevokeCertificateResponse, error) } +// 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) +} + // CertificateAuthorityGetter is an interface implemented by a // CertificateAuthorityService that has a method to get the root certificate. type CertificateAuthorityGetter interface { diff --git a/cas/softcas/softcas.go b/cas/softcas/softcas.go index 2a97145b..3c400080 100644 --- a/cas/softcas/softcas.go +++ b/cas/softcas/softcas.go @@ -3,6 +3,7 @@ package softcas import ( "context" "crypto" + "crypto/rand" "crypto/x509" "time" @@ -129,6 +130,17 @@ 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) { + + revocationList, err := x509.CreateRevocationList(rand.Reader, crl, c.CertificateChain[0], c.Signer) + if err != nil { + return nil, err + } + + return revocationList, nil +} + // CreateCertificateAuthority creates a root or an intermediate certificate. func (c *SoftCAS) CreateCertificateAuthority(req *apiv1.CreateCertificateAuthorityRequest) (*apiv1.CreateCertificateAuthorityResponse, error) { switch { diff --git a/db/db.go b/db/db.go index 6d48723f..a4c1fa01 100644 --- a/db/db.go +++ b/db/db.go @@ -16,6 +16,7 @@ import ( var ( certsTable = []byte("x509_certs") revokedCertsTable = []byte("revoked_x509_certs") + crlTable = []byte("x509_crl") revokedSSHCertsTable = []byte("revoked_ssh_certs") usedOTTTable = []byte("used_ott") sshCertsTable = []byte("ssh_certs") @@ -24,6 +25,9 @@ var ( sshHostPrincipalsTable = []byte("ssh_host_principals") ) +var crlKey = []byte("crl") //TODO: at the moment we store a single CRL in the database, in a dedicated table. +// is this acceptable? probably not.... + // ErrAlreadyExists can be returned if the DB attempts to set a key that has // been previously set. var ErrAlreadyExists = errors.New("already exists") @@ -47,6 +51,9 @@ 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) @@ -82,7 +89,7 @@ func New(c *Config) (AuthDB, error) { tables := [][]byte{ revokedCertsTable, certsTable, usedOTTTable, sshCertsTable, sshHostsTable, sshHostPrincipalsTable, sshUsersTable, - revokedSSHCertsTable, + revokedSSHCertsTable, crlTable, } for _, b := range tables { if err := db.CreateTable(b); err != nil { @@ -107,6 +114,14 @@ type RevokedCertificateInfo struct { ACME bool } +// CertificateRevocationListInfo contains a CRL in PEM and associated metadata to allow a decision on whether +// to regenerate the CRL or not easier +type CertificateRevocationListInfo struct { + Number int64 + ExpiresAt time.Time + PEM string +} + // IsRevoked returns whether or not a certificate with the given identifier // has been revoked. // In the case of an X509 Certificate the `id` should be the Serial Number of @@ -189,6 +204,58 @@ func (db *DB) RevokeSSH(rci *RevokedCertificateInfo) error { } } +// GetRevokedCertificates gets a list of all revoked certificates. +func (db *DB) GetRevokedCertificates() (*[]RevokedCertificateInfo, error) { + entries, err := db.List(revokedCertsTable) + if err != nil { + return nil, err + } + var revokedCerts []RevokedCertificateInfo + for _, e := range entries { + var data RevokedCertificateInfo + if err := json.Unmarshal(e.Value, &data); err != nil { + return nil, err + } + 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") + } + + if err := db.Set(crlTable, crlKey, crlInfoBytes); err != nil { + return errors.Wrap(err, "database Set error") + } + return nil +} + +// 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") + } + + var crlInfo CertificateRevocationListInfo + err = json.Unmarshal(crlInfoBytes, &crlInfo) + if err != nil { + return nil, errors.Wrap(err, "json Unmarshal error") + } + return &crlInfo, err +} + // GetCertificate retrieves a certificate by the serial number. func (db *DB) GetCertificate(serialNumber string) (*x509.Certificate, error) { asn1Data, err := db.Get(certsTable, []byte(serialNumber)) diff --git a/db/simple.go b/db/simple.go index 0e5426ec..c34cdb5d 100644 --- a/db/simple.go +++ b/db/simple.go @@ -41,6 +41,21 @@ func (s *SimpleDB) Revoke(rci *RevokedCertificateInfo) error { return ErrNotImplemented } +// GetRevokedCertificates returns a "NotImplemented" error. +func (s *SimpleDB) GetRevokedCertificates() (*[]RevokedCertificateInfo, error) { + return nil, ErrNotImplemented +} + +// GetCRL returns a "NotImplemented" error. +func (s *SimpleDB) GetCRL() (*CertificateRevocationListInfo, error) { + return nil, ErrNotImplemented +} + +// StoreCRL returns a "NotImplemented" error. +func (s *SimpleDB) StoreCRL(crlInfo *CertificateRevocationListInfo) error { + return ErrNotImplemented +} + // RevokeSSH returns a "NotImplemented" error. func (s *SimpleDB) RevokeSSH(rci *RevokedCertificateInfo) error { return ErrNotImplemented From 7d024cc4cb3f50e64fdbc62e812de6e96f55b34b Mon Sep 17 00:00:00 2001 From: Raal Goff Date: Tue, 2 Nov 2021 13:26:07 +0800 Subject: [PATCH 08/29] change GenerateCertificateRevocationList to return DER, store DER in db instead of PEM, nicer PEM encoding of CRL, add Mock stubs --- api/api.go | 2 +- api/crl.go | 16 ++++++++++++---- authority/tls.go | 20 ++++++++------------ db/db.go | 18 +++++++++++++++--- 4 files changed, 36 insertions(+), 20 deletions(-) diff --git a/api/api.go b/api/api.go index 7c935f3f..ba7d3d17 100644 --- a/api/api.go +++ b/api/api.go @@ -50,7 +50,7 @@ type Authority interface { GetRoots() ([]*x509.Certificate, error) GetFederation() ([]*x509.Certificate, error) Version() authority.Version - GenerateCertificateRevocationList(force bool) (string, error) + GenerateCertificateRevocationList(force bool) ([]byte, error) } // TimeDuration is an alias of provisioner.TimeDuration diff --git a/api/crl.go b/api/crl.go index 50b29d6c..46758777 100644 --- a/api/crl.go +++ b/api/crl.go @@ -1,16 +1,24 @@ package api -import "net/http" +import ( + "encoding/pem" + "net/http" +) -// CRL is an HTTP handler that returns the current CRL +// CRL is an HTTP handler that returns the current CRL in PEM format func (h *caHandler) CRL(w http.ResponseWriter, r *http.Request) { - crl, err := h.Authority.GenerateCertificateRevocationList(false) + crlBytes, err := h.Authority.GenerateCertificateRevocationList(false) if err != nil { w.WriteHeader(500) return } + pemBytes := pem.EncodeToMemory(&pem.Block{ + Type: "X509 CRL", + Bytes: crlBytes, + }) + w.WriteHeader(200) - _, err = w.Write([]byte(crl)) + _, err = w.Write(pemBytes) } diff --git a/authority/tls.go b/authority/tls.go index d6d6d6d1..d6d80643 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -514,24 +514,24 @@ func (a *Authority) revokeSSH(crt *ssh.Certificate, rci *db.RevokedCertificateIn // 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) (string, error) { +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 "", err + return nil, err } if !force && crlInfo != nil && crlInfo.ExpiresAt.After(time.Now().UTC()) { - return crlInfo.PEM, nil + return crlInfo.DER, nil } // some CAS may not implement the CRLGenerator interface, so check before we proceed caCRLGenerator, ok := a.x509CAService.(casapi.CertificateAuthorityCRLGenerator) if !ok { - return "", errors.Errorf("CRL Generator not implemented") + return nil, errors.Errorf("CRL Generator not implemented") } revokedList, err := a.db.GetRevokedCertificates() @@ -574,28 +574,24 @@ func (a *Authority) GenerateCertificateRevocationList(force bool) (string, error certificateRevocationList, err := caCRLGenerator.CreateCertificateRevocationList(&revocationList) if err != nil { - return "", err + return nil, err } - // Quick and dirty PEM encoding - // TODO: clean this up - pemCRL := fmt.Sprintf("-----BEGIN X509 CRL-----\n%s\n-----END X509 CRL-----\n", base64.StdEncoding.EncodeToString(certificateRevocationList)) - // Create a new db.CertificateRevocationListInfo, which stores the new Number we just generated, the // expiry time, and the byte-encoded CRL - then store it in the DB newCRLInfo := db.CertificateRevocationListInfo{ Number: n, ExpiresAt: revocationList.NextUpdate, - PEM: pemCRL, + DER: certificateRevocationList, } err = a.db.StoreCRL(&newCRLInfo) if err != nil { - return "", err + return nil, err } // Finally, return our CRL PEM - return pemCRL, nil + return certificateRevocationList, nil } // GetTLSCertificate creates a new leaf certificate to be used by the CA HTTPS server. diff --git a/db/db.go b/db/db.go index a4c1fa01..534c4000 100644 --- a/db/db.go +++ b/db/db.go @@ -114,12 +114,12 @@ type RevokedCertificateInfo struct { ACME bool } -// CertificateRevocationListInfo contains a CRL in PEM and associated metadata to allow a decision on whether -// to regenerate the CRL or not easier +// CertificateRevocationListInfo contains a CRL in DER format and associated +// metadata to allow a decision on whether to regenerate the CRL or not easier type CertificateRevocationListInfo struct { Number int64 ExpiresAt time.Time - PEM string + DER []byte } // IsRevoked returns whether or not a certificate with the given identifier @@ -379,6 +379,18 @@ type MockAuthDB struct { MShutdown func() error } +func (m *MockAuthDB) GetRevokedCertificates() (*[]RevokedCertificateInfo, error) { + panic("implement me") +} + +func (m *MockAuthDB) GetCRL() (*CertificateRevocationListInfo, error) { + panic("implement me") +} + +func (m *MockAuthDB) StoreCRL(info *CertificateRevocationListInfo) error { + panic("implement me") +} + // IsRevoked mock. func (m *MockAuthDB) IsRevoked(sn string) (bool, error) { if m.MIsRevoked != nil { From 668cb6f39c756bc17adf2814b5377ef524d6dbc4 Mon Sep 17 00:00:00 2001 From: Raal Goff Date: Tue, 2 Nov 2021 16:39:29 +0800 Subject: [PATCH 09/29] missed some mentions of PEM when changing the returned format to DER regarding CRL generation --- authority/tls.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/authority/tls.go b/authority/tls.go index d6d80643..c6dc7d27 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -509,7 +509,7 @@ func (a *Authority) revokeSSH(crt *ssh.Certificate, rci *db.RevokedCertificateIn return a.db.Revoke(rci) } -// GenerateCertificateRevocationList returns a PEM representation of a signed CRL. +// 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). // @@ -578,7 +578,7 @@ func (a *Authority) GenerateCertificateRevocationList(force bool) ([]byte, error } // Create a new db.CertificateRevocationListInfo, which stores the new Number we just generated, the - // expiry time, and the byte-encoded CRL - then store it in the DB + // expiry time, and the DER-encoded CRL - then store it in the DB newCRLInfo := db.CertificateRevocationListInfo{ Number: n, ExpiresAt: revocationList.NextUpdate, @@ -590,7 +590,7 @@ func (a *Authority) GenerateCertificateRevocationList(force bool) ([]byte, error return nil, err } - // Finally, return our CRL PEM + // Finally, return our CRL in DER return certificateRevocationList, nil } From d417ce32326067ff249f34eeff20ee82ac243d9d Mon Sep 17 00:00:00 2001 From: Raal Goff Date: Thu, 4 Nov 2021 14:05:07 +0800 Subject: [PATCH 10/29] implement changes from review --- api/api.go | 3 +- api/crl.go | 44 ++++++++++++++--- authority/authority.go | 70 +++++++++++++++++++++++++++ authority/config/config.go | 7 +++ authority/tls.go | 98 +++++++++++++++++++++++++++----------- cas/apiv1/requests.go | 10 ++++ cas/apiv1/services.go | 2 +- cas/softcas/softcas.go | 8 ++-- db/db.go | 28 +++++++++-- 9 files changed, 224 insertions(+), 46 deletions(-) diff --git a/api/api.go b/api/api.go index ba7d3d17..25b6efa7 100644 --- a/api/api.go +++ b/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 diff --git a/api/crl.go b/api/crl.go index 46758777..4d20cfeb 100644 --- a/api/crl.go +++ b/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")) + } + } diff --git a/authority/authority.go b/authority/authority.go index b6071060..2454c272 100644 --- a/authority/authority.go +++ b/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 +} diff --git a/authority/config/config.go b/authority/config/config.go index 2c437725..4bf51cfe 100644 --- a/authority/config/config.go +++ b/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() { diff --git a/authority/tls.go b/authority/tls.go index c6dc7d27..19e06659 100644 --- a/authority/tls.go +++ b/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) { - - // 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 { @@ -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. diff --git a/cas/apiv1/requests.go b/cas/apiv1/requests.go index bf745c17..630cecce 100644 --- a/cas/apiv1/requests.go +++ b/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 +} diff --git a/cas/apiv1/services.go b/cas/apiv1/services.go index 143c1f17..ce7d0364 100644 --- a/cas/apiv1/services.go +++ b/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 diff --git a/cas/softcas/softcas.go b/cas/softcas/softcas.go index 3c400080..bec1d81e 100644 --- a/cas/softcas/softcas.go +++ b/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. diff --git a/db/db.go b/db/db.go index 534c4000..bf3b97ec 100644 --- a/db/db.go +++ b/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 ACME bool @@ -216,7 +221,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 From a607ab189a8de305205313f25497e526add217a6 Mon Sep 17 00:00:00 2001 From: Raal Goff Date: Tue, 29 Mar 2022 08:51:39 +0800 Subject: [PATCH 11/29] requested changes --- api/crl.go | 2 -- authority/authority.go | 77 ++++++++++++++++++------------------------ authority/tls.go | 4 +-- 3 files changed, 34 insertions(+), 49 deletions(-) diff --git a/api/crl.go b/api/crl.go index 4d20cfeb..c90a77f9 100644 --- a/api/crl.go +++ b/api/crl.go @@ -45,8 +45,6 @@ func (h *caHandler) CRL(w http.ResponseWriter, r *http.Request) { _, err = w.Write(crlBytes) } - w.WriteHeader(200) - if err != nil { panic(errors.Wrap(err, "error writing http response")) } diff --git a/authority/authority.go b/authority/authority.go index 2454c272..025e3d39 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -6,6 +6,7 @@ import ( "crypto/sha256" "crypto/x509" "encoding/hex" + "fmt" "log" "strings" "sync" @@ -662,57 +663,43 @@ func (a *Authority) GetSCEPService() *scep.Service { 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") - } + if a.config.CRL.CacheDuration.Duration <= 0 { + return nil + } - crlInfo, err := crlDB.GetCRL() - if err != nil { - return errors.Wrap(err, "could not retrieve CRL from database") - } + // 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) + if !ok { + return errors.Errorf("CRL Generation requested, but database does not support CRL generation") + } - 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") - } - } + // 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 { + 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 { + panic(fmt.Sprintf("ERROR: Addition of jitter to CRL generation time %v creates a negative duration (%v). Use a CRL generation time of longer than 1 minute.", a.config.CRL.CacheDuration, tickerDuration)) + } + crlTicker := time.NewTicker(tickerDuration) - 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")) - } + go func() { + for { + select { + case <-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) } } - }() - } + } + }() return nil } diff --git a/authority/tls.go b/authority/tls.go index 19e06659..1d016f71 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -526,12 +526,12 @@ func (a *Authority) revokeSSH(crt *ssh.Certificate, rci *db.RevokedCertificateIn // 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") + return nil, errs.Wrap(http.StatusNotFound, errors.Errorf("Certificate Revocation Lists are not enabled"), "authority.GetCertificateRevocationList") } 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") + return nil, errs.Wrap(http.StatusNotImplemented, errors.Errorf("Database does not support Certificate Revocation Lists"), "authority.GetCertificateRevocationList") } crlInfo, err := crlDB.GetCRL() From 53dbe2309ba383c51048a6d5147e465b44f29ab0 Mon Sep 17 00:00:00 2001 From: Raal Goff Date: Tue, 5 Apr 2022 11:19:13 +0800 Subject: [PATCH 12/29] implemented some requested changes --- api/api.go | 1 - api/crl.go | 18 +++++++++--------- authority/authority.go | 15 ++++++++++++--- authority/tls.go | 14 ++++++++------ db/db.go | 6 ++++-- 5 files changed, 33 insertions(+), 21 deletions(-) diff --git a/api/api.go b/api/api.go index 25b6efa7..014a89e9 100644 --- a/api/api.go +++ b/api/api.go @@ -50,7 +50,6 @@ type Authority interface { GetRoots() ([]*x509.Certificate, error) GetFederation() ([]*x509.Certificate, error) Version() authority.Version - GenerateCertificateRevocationList() error GetCertificateRevocationList() ([]byte, error) } diff --git a/api/crl.go b/api/crl.go index c90a77f9..45024470 100644 --- a/api/crl.go +++ b/api/crl.go @@ -4,6 +4,7 @@ import ( "encoding/pem" "fmt" "github.com/pkg/errors" + "github.com/smallstep/certificates/errs" "net/http" ) @@ -14,6 +15,14 @@ func (h *caHandler) CRL(w http.ResponseWriter, r *http.Request) { _, formatAsPEM := r.URL.Query()["pem"] if err != nil { + + caErr, isCaErr := err.(*errs.Error) + + if isCaErr { + http.Error(w, caErr.Msg, caErr.Status) + return + } + w.WriteHeader(500) _, err = fmt.Fprintf(w, "%v\n", err) if err != nil { @@ -22,15 +31,6 @@ func (h *caHandler) CRL(w http.ResponseWriter, r *http.Request) { return } - 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", diff --git a/authority/authority.go b/authority/authority.go index 025e3d39..3adfc14b 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -67,7 +67,7 @@ type Authority struct { sshCAHostFederatedCerts []ssh.PublicKey // CRL vars - crlChannel chan int + crlTicker *time.Ticker // Do not re-initialize initOnce bool @@ -604,6 +604,10 @@ func (a *Authority) IsAdminAPIEnabled() bool { // Shutdown safely shuts down any clients, databases, etc. held by the Authority. func (a *Authority) Shutdown() error { + if a.crlTicker != nil { + a.crlTicker.Stop() + } + if err := a.keyManager.Close(); err != nil { log.Printf("error closing the key manager: %v", err) } @@ -612,6 +616,11 @@ 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() + } + if err := a.keyManager.Close(); err != nil { log.Printf("error closing the key manager: %v", err) } @@ -686,12 +695,12 @@ func (a *Authority) startCRLGenerator() error { if tickerDuration <= 0 { panic(fmt.Sprintf("ERROR: Addition of jitter to CRL generation time %v creates a negative duration (%v). Use a CRL generation time of longer than 1 minute.", a.config.CRL.CacheDuration, tickerDuration)) } - crlTicker := time.NewTicker(tickerDuration) + a.crlTicker = time.NewTicker(tickerDuration) go func() { for { select { - case <-crlTicker.C: + case <-a.crlTicker.C: log.Println("Regenerating CRL") err := a.GenerateCertificateRevocationList() if err != nil { diff --git a/authority/tls.go b/authority/tls.go index 1d016f71..549f0c07 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -409,13 +409,15 @@ func (a *Authority) Revoke(ctx context.Context, revokeOpts *RevokeOptions) error err error ) - // Attempt to get the certificate expiry using the serial number. - cert, err := a.db.GetCertificate(revokeOpts.Serial) + 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 + // 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. diff --git a/db/db.go b/db/db.go index bf3b97ec..070f3596 100644 --- a/db/db.go +++ b/db/db.go @@ -216,13 +216,15 @@ func (db *DB) GetRevokedCertificates() (*[]RevokedCertificateInfo, error) { return nil, err } var revokedCerts []RevokedCertificateInfo + now := time.Now().UTC() + for _, e := range entries { var data RevokedCertificateInfo if err := json.Unmarshal(e.Value, &data); err != nil { return nil, err } - if !data.ExpiresAt.IsZero() && data.ExpiresAt.After(time.Now().UTC()) { + if !data.ExpiresAt.IsZero() && data.ExpiresAt.After(now) { revokedCerts = append(revokedCerts, data) } else if data.ExpiresAt.IsZero() { cert, err := db.GetCertificate(data.Serial) @@ -233,7 +235,7 @@ func (db *DB) GetRevokedCertificates() (*[]RevokedCertificateInfo, error) { continue } - if cert.NotAfter.After(time.Now().UTC()) { + if cert.NotAfter.After(now) { revokedCerts = append(revokedCerts, data) } } From 49c41636cc4bb1970d68e6c5275fa438ca24dea5 Mon Sep 17 00:00:00 2001 From: Raal Goff Date: Wed, 6 Apr 2022 08:31:40 +0800 Subject: [PATCH 13/29] implemented some requested changes --- api/api_test.go | 4 ++++ authority/tls.go | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/api/api_test.go b/api/api_test.go index 39c77de7..06b165bf 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -201,6 +201,10 @@ type mockAuthority struct { version func() authority.Version } +func (m *mockAuthority) GetCertificateRevocationList() ([]byte, error) { + panic("implement me") +} + // TODO: remove once Authorize is deprecated. func (m *mockAuthority) Authorize(ctx context.Context, ott string) ([]provisioner.SignOption, error) { return m.AuthorizeSign(ott) diff --git a/authority/tls.go b/authority/tls.go index 549f0c07..50dc5642 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -11,8 +11,8 @@ import ( "encoding/json" "encoding/pem" "fmt" - "net" "math/big" + "net" "net/http" "strings" "time" From c8b38c0e13245b400d5f2a4c4d79726d90679ee0 Mon Sep 17 00:00:00 2001 From: Raal Goff Date: Wed, 6 Apr 2022 10:50:09 +0800 Subject: [PATCH 14/29] implemented requested changes --- cas/softcas/softcas.go | 6 +++++- db/db.go | 10 +--------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/cas/softcas/softcas.go b/cas/softcas/softcas.go index bec1d81e..0b2270bb 100644 --- a/cas/softcas/softcas.go +++ b/cas/softcas/softcas.go @@ -133,7 +133,11 @@ 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) { - revocationListBytes, err := x509.CreateRevocationList(rand.Reader, req.RevocationList, c.CertificateChain[0], c.Signer) + certChain, signer, err := c.getCertSigner() + if err != nil { + return nil, err + } + revocationListBytes, err := x509.CreateRevocationList(rand.Reader, req.RevocationList, certChain[0], signer) if err != nil { return nil, err } diff --git a/db/db.go b/db/db.go index 5b7bfc21..ccaf4056 100644 --- a/db/db.go +++ b/db/db.go @@ -127,14 +127,6 @@ type CertificateRevocationListInfo struct { DER []byte } -// CertificateRevocationListInfo contains a CRL in DER format and associated -// metadata to allow a decision on whether to regenerate the CRL or not easier -type CertificateRevocationListInfo struct { - Number int64 - ExpiresAt time.Time - DER []byte -} - // IsRevoked returns whether or not a certificate with the given identifier // has been revoked. // In the case of an X509 Certificate the `id` should be the Serial Number of @@ -224,7 +216,7 @@ func (db *DB) GetRevokedCertificates() (*[]RevokedCertificateInfo, error) { return nil, err } var revokedCerts []RevokedCertificateInfo - now := time.Now().UTC() + now := time.Now().Truncate(time.Second) for _, e := range entries { var data RevokedCertificateInfo From 9fa5f462131e04d6086d3d11be494c0c88262f8c Mon Sep 17 00:00:00 2001 From: Raal Goff Date: Wed, 13 Jul 2022 08:56:47 +0800 Subject: [PATCH 15/29] add minor doco, Test_CRLGeneration(), fix some issues from merge --- api/api_test.go | 46 ++++++++++++++++++++++++++++++++++++++++- api/crl.go | 4 ++-- db/db.go | 4 ++-- docs/GETTING_STARTED.md | 5 +++++ 4 files changed, 54 insertions(+), 5 deletions(-) diff --git a/api/api_test.go b/api/api_test.go index 485244b9..de573097 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -199,6 +199,7 @@ type mockAuthority struct { getEncryptedKey func(kid string) (string, error) getRoots func() ([]*x509.Certificate, error) getFederation func() ([]*x509.Certificate, error) + getCRL func() ([]byte, error) signSSH func(ctx context.Context, key ssh.PublicKey, opts provisioner.SignSSHOptions, signOpts ...provisioner.SignOption) (*ssh.Certificate, error) signSSHAddUser func(ctx context.Context, key ssh.PublicKey, cert *ssh.Certificate) (*ssh.Certificate, error) renewSSH func(ctx context.Context, cert *ssh.Certificate) (*ssh.Certificate, error) @@ -213,7 +214,11 @@ type mockAuthority struct { } func (m *mockAuthority) GetCertificateRevocationList() ([]byte, error) { - panic("implement me") + if m.getCRL != nil { + return m.getCRL() + } + + return m.ret1.([]byte), m.err } // TODO: remove once Authorize is deprecated. @@ -776,6 +781,45 @@ func (m *mockProvisioner) AuthorizeSSHRekey(ctx context.Context, token string) ( return m.ret1.(*ssh.Certificate), m.ret2.([]provisioner.SignOption), m.err } +func Test_CRLGeneration(t *testing.T) { + tests := []struct { + name string + err error + statusCode int + expected []byte + }{ + {"empty", nil, http.StatusOK, nil}, + } + + chiCtx := chi.NewRouteContext() + req := httptest.NewRequest("GET", "http://example.com/crl", nil) + req = req.WithContext(context.WithValue(context.Background(), chi.RouteCtxKey, chiCtx)) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockMustAuthority(t, &mockAuthority{ret1: tt.expected, err: tt.err}) + w := httptest.NewRecorder() + CRL(w, req) + res := w.Result() + + if res.StatusCode != tt.statusCode { + t.Errorf("caHandler.CRL StatusCode = %d, wants %d", res.StatusCode, tt.statusCode) + } + + body, err := io.ReadAll(res.Body) + res.Body.Close() + if err != nil { + t.Errorf("caHandler.Root unexpected error = %v", err) + } + if tt.statusCode == 200 { + if !bytes.Equal(bytes.TrimSpace(body), tt.expected) { + t.Errorf("caHandler.Root CRL = %s, wants %s", body, tt.expected) + } + } + }) + } +} + func Test_caHandler_Route(t *testing.T) { type fields struct { Authority Authority diff --git a/api/crl.go b/api/crl.go index 45024470..ce0d8f73 100644 --- a/api/crl.go +++ b/api/crl.go @@ -9,8 +9,8 @@ import ( ) // 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.GetCertificateRevocationList() +func CRL(w http.ResponseWriter, r *http.Request) { + crlBytes, err := mustAuthority(r.Context()).GetCertificateRevocationList() _, formatAsPEM := r.URL.Query()["pem"] diff --git a/db/db.go b/db/db.go index b93b23ca..ee8007c1 100644 --- a/db/db.go +++ b/db/db.go @@ -255,9 +255,9 @@ func (db *DB) GetRevokedCertificates() (*[]RevokedCertificateInfo, error) { return nil, err } - if !data.ExpiresAt.IsZero() && data.ExpiresAt.After(now) { + if !data.RevokedAt.IsZero() && data.RevokedAt.After(now) { revokedCerts = append(revokedCerts, data) - } else if data.ExpiresAt.IsZero() { + } else if data.RevokedAt.IsZero() { cert, err := db.GetCertificate(data.Serial) if err != nil { revokedCerts = append(revokedCerts, data) // a revoked certificate may not be in the database, diff --git a/docs/GETTING_STARTED.md b/docs/GETTING_STARTED.md index 67c5673d..328fba69 100644 --- a/docs/GETTING_STARTED.md +++ b/docs/GETTING_STARTED.md @@ -119,6 +119,11 @@ 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 From 924082bb492e2e13e4a26f327717fe6d26c9aaf1 Mon Sep 17 00:00:00 2001 From: Raal Goff Date: Thu, 8 Sep 2022 10:09:37 +0800 Subject: [PATCH 16/29] fix linter errors --- authority/authority.go | 13 ++++++------- authority/tls.go | 3 +++ cas/softcas/softcas.go | 2 +- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/authority/authority.go b/authority/authority.go index 31c2890e..efd7ab66 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -794,14 +794,13 @@ func (a *Authority) startCRLGenerator() error { go func() { for { - select { - case <-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) - } + <-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) } + } }() diff --git a/authority/tls.go b/authority/tls.go index 8d3bd73d..5cde341c 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -549,6 +549,9 @@ 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...) + } // Generate a new CRL so CRL requesters will always get an up-to-date CRL whenever they request it err = a.GenerateCertificateRevocationList() diff --git a/cas/softcas/softcas.go b/cas/softcas/softcas.go index 03adc667..ed909d6d 100644 --- a/cas/softcas/softcas.go +++ b/cas/softcas/softcas.go @@ -3,8 +3,8 @@ package softcas import ( "context" "crypto" - "crypto/rsa" "crypto/rand" + "crypto/rsa" "crypto/x509" "time" From 4a4f7ca9ba127e2ffc9abf18036d2265194b7958 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 14 Sep 2022 11:16:47 -0700 Subject: [PATCH 17/29] Fix panic if cacheDuration is not set --- authority/authority.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/authority/authority.go b/authority/authority.go index efd7ab66..619482bd 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -638,8 +638,8 @@ func (a *Authority) init() error { 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) { + if a.config.CRL != nil && a.config.CRL.Generate { + if v := a.config.CRL.CacheDuration; v != nil && v.Duration > 0 { err := a.startCRLGenerator() if err != nil { return err From 0829f37fe83adfdc36f0f8b7e205846c65eaef3d Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 14 Sep 2022 11:43:58 -0700 Subject: [PATCH 18/29] Define a default crl cache duration --- authority/config/config.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/authority/config/config.go b/authority/config/config.go index 721c691c..86e950cb 100644 --- a/authority/config/config.go +++ b/authority/config/config.go @@ -35,6 +35,8 @@ var ( // DefaultEnableSSHCA enable SSH CA features per provisioner or globally // for all provisioners. 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. GlobalProvisionerClaims = provisioner.Claims{ @@ -190,6 +192,9 @@ func (c *Config) Init() { if c.CommonName == "" { c.CommonName = "Step Online CA" } + if c.CRL != nil && c.CRL.Generate && c.CRL.CacheDuration == nil { + c.CRL.CacheDuration = DefaultCRLCacheDuration + } c.AuthorityConfig.init() } From 221e756f4098032b7f899497710602e862d13314 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 14 Sep 2022 11:50:11 -0700 Subject: [PATCH 19/29] Use render.Error on crl endpoint --- api/crl.go | 32 ++++++-------------------------- 1 file changed, 6 insertions(+), 26 deletions(-) diff --git a/api/crl.go b/api/crl.go index ce0d8f73..1a4d309a 100644 --- a/api/crl.go +++ b/api/crl.go @@ -2,35 +2,20 @@ package api import ( "encoding/pem" - "fmt" - "github.com/pkg/errors" - "github.com/smallstep/certificates/errs" "net/http" + + "github.com/smallstep/certificates/api/render" ) // CRL is an HTTP handler that returns the current CRL in DER or PEM format func CRL(w http.ResponseWriter, r *http.Request) { crlBytes, err := mustAuthority(r.Context()).GetCertificateRevocationList() - - _, formatAsPEM := r.URL.Query()["pem"] - if err != nil { - - caErr, isCaErr := err.(*errs.Error) - - if isCaErr { - http.Error(w, caErr.Msg, caErr.Status) - return - } - - w.WriteHeader(500) - _, err = fmt.Fprintf(w, "%v\n", err) - if err != nil { - panic(errors.Wrap(err, "error writing http response")) - } + render.Error(w, err) return } + _, formatAsPEM := r.URL.Query()["pem"] if formatAsPEM { pemBytes := pem.EncodeToMemory(&pem.Block{ Type: "X509 CRL", @@ -38,15 +23,10 @@ func CRL(w http.ResponseWriter, r *http.Request) { }) w.Header().Add("Content-Type", "application/x-pem-file") w.Header().Add("Content-Disposition", "attachment; filename=\"crl.pem\"") - _, err = w.Write(pemBytes) + 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.Write(crlBytes) } - - if err != nil { - panic(errors.Wrap(err, "error writing http response")) - } - } From 4e19aa4c52e1ee1db9f981cd3dd0024a3c36102b Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 14 Sep 2022 12:21:52 -0700 Subject: [PATCH 20/29] Add cache duration if crl is set --- authority/config/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/authority/config/config.go b/authority/config/config.go index 86e950cb..7e6922ed 100644 --- a/authority/config/config.go +++ b/authority/config/config.go @@ -192,7 +192,7 @@ func (c *Config) Init() { if c.CommonName == "" { c.CommonName = "Step Online CA" } - if c.CRL != nil && c.CRL.Generate && c.CRL.CacheDuration == nil { + if c.CRL != nil && c.CRL.CacheDuration == nil { c.CRL.CacheDuration = DefaultCRLCacheDuration } c.AuthorityConfig.init() From 40baf73dffb6c0392113ee6bf3f5579e8628de49 Mon Sep 17 00:00:00 2001 From: Raal Goff Date: Thu, 15 Sep 2022 15:03:42 +0800 Subject: [PATCH 21/29] remove incorrect check on revoked certificate dates, add mutex lock for generating CRLs, --- authority/authority.go | 1 + authority/tls.go | 3 +++ db/db.go | 2 +- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/authority/authority.go b/authority/authority.go index 619482bd..1b2faf4b 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -71,6 +71,7 @@ type Authority struct { // CRL vars crlTicker *time.Ticker + crlMutex sync.Mutex // Do not re-initialize initOnce bool diff --git a/authority/tls.go b/authority/tls.go index 5cde341c..4652735a 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -637,6 +637,9 @@ 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 + defer a.crlMutex.Unlock() + crlInfo, err := crlDB.GetCRL() if err != nil { return errors.Wrap(err, "could not retrieve CRL from database") diff --git a/db/db.go b/db/db.go index ee8007c1..a8ae23ee 100644 --- a/db/db.go +++ b/db/db.go @@ -255,7 +255,7 @@ func (db *DB) GetRevokedCertificates() (*[]RevokedCertificateInfo, error) { return nil, err } - if !data.RevokedAt.IsZero() && data.RevokedAt.After(now) { + if !data.RevokedAt.IsZero() { revokedCerts = append(revokedCerts, data) } else if data.RevokedAt.IsZero() { cert, err := db.GetCertificate(data.Serial) From f7df865687ff469204edaf7e6c95fe7d54e14b7a Mon Sep 17 00:00:00 2001 From: Raal Goff Date: Fri, 7 Oct 2022 10:30:00 +0800 Subject: [PATCH 22/29] refactor crl config, add some tests --- authority/authority.go | 45 ++++++++---- authority/authority_test.go | 6 ++ authority/config/config.go | 6 +- authority/tls.go | 27 +++++-- authority/tls_test.go | 138 ++++++++++++++++++++++++++++++++++++ db/db.go | 47 +++++++----- 6 files changed, 232 insertions(+), 37 deletions(-) diff --git a/authority/authority.go b/authority/authority.go index ede13269..b72fe61e 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -7,7 +7,6 @@ import ( "crypto/sha256" "crypto/x509" "encoding/hex" - "fmt" "log" "strings" "sync" @@ -664,12 +663,22 @@ func (a *Authority) init() error { a.initOnce = true // Start the CRL generator - if a.config.CRL != nil && a.config.CRL.Generate { - if v := a.config.CRL.CacheDuration; v != nil && v.Duration > 0 { - err := a.startCRLGenerator() - if err != nil { - return err - } + 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") + } + + 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 { + return err } } @@ -797,6 +806,12 @@ func (a *Authority) startCRLGenerator() error { 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) @@ -811,11 +826,6 @@ func (a *Authority) startCRLGenerator() error { 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 { - panic(fmt.Sprintf("ERROR: Addition of jitter to CRL generation time %v creates a negative duration (%v). Use a CRL generation time of longer than 1 minute.", a.config.CRL.CacheDuration, tickerDuration)) - } a.crlTicker = time.NewTicker(tickerDuration) go func() { @@ -832,3 +842,14 @@ func (a *Authority) startCRLGenerator() error { 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) + } +} diff --git a/authority/authority_test.go b/authority/authority_test.go index 9f35f23e..48479d5b 100644 --- a/authority/authority_test.go +++ b/authority/authority_test.go @@ -80,6 +80,12 @@ 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) diff --git a/authority/config/config.go b/authority/config/config.go index 7e6922ed..ad261cdb 100644 --- a/authority/config/config.go +++ b/authority/config/config.go @@ -111,8 +111,10 @@ type AuthConfig struct { // CRLConfig represents config options for CRL generation type CRLConfig struct { - Generate bool `json:"generate,omitempty"` - CacheDuration *provisioner.Duration `json:"cacheDuration,omitempty"` + 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 diff --git a/authority/tls.go b/authority/tls.go index d70c1558..051e09c9 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -572,10 +572,17 @@ func (a *Authority) Revoke(ctx context.Context, revokeOpts *RevokeOptions) error return errs.Wrap(http.StatusInternalServerError, err, "authority.Revoke", opts...) } - // 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 { - return errs.Wrap(http.StatusInternalServerError, err, "authority.Revoke", opts...) + 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 { + 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 { @@ -693,6 +700,13 @@ func (a *Authority) GenerateCertificateRevocationList() error { }) } + var updateDuration time.Duration + if a.config.CRL.CacheDuration != nil { + updateDuration = a.config.CRL.CacheDuration.Duration + } else if crlInfo != nil { + updateDuration = crlInfo.Duration + } + // Create a RevocationList representation ready for the CAS to sign // TODO: allow SignatureAlgorithm to be specified? revocationList := x509.RevocationList{ @@ -700,7 +714,7 @@ func (a *Authority) GenerateCertificateRevocationList() error { RevokedCertificates: revokedCertificates, Number: &bn, ThisUpdate: time.Now().UTC(), - NextUpdate: time.Now().UTC().Add(a.config.CRL.CacheDuration.Duration), + NextUpdate: time.Now().UTC().Add(updateDuration), ExtraExtensions: nil, } @@ -710,11 +724,12 @@ func (a *Authority) GenerateCertificateRevocationList() error { } // Create a new db.CertificateRevocationListInfo, which stores the new Number we just generated, the - // expiry time, and the DER-encoded CRL + // expiry time, duration, and the DER-encoded CRL newCRLInfo := db.CertificateRevocationListInfo{ Number: n, ExpiresAt: revocationList.NextUpdate, DER: certificateRevocationList.CRL, + Duration: updateDuration, } // Store the CRL in the database ready for retrieval by api endpoints diff --git a/authority/tls_test.go b/authority/tls_test.go index bc9e3e3a..6223bb3d 100644 --- a/authority/tls_test.go +++ b/authority/tls_test.go @@ -1673,3 +1673,141 @@ func TestAuthority_constraints(t *testing.T) { }) } } + +func TestAuthority_CRL(t *testing.T) { + reasonCode := 2 + reason := "bob was let go" + validIssuer := "step-cli" + validAudience := testAudiences.Revoke + now := time.Now().UTC() + // + jwk, err := jose.ReadKey("testdata/secrets/step_cli_key_priv.jwk", jose.WithPassword([]byte("pass"))) + assert.FatalError(t, err) + // + sig, err := jose.NewSigner(jose.SigningKey{Algorithm: jose.ES256, Key: jwk.Key}, + (&jose.SignerOptions{}).WithType("JWT").WithHeader("kid", jwk.KeyID)) + assert.FatalError(t, err) + + crlCtx := provisioner.NewContextWithMethod(context.Background(), provisioner.RevokeMethod) + + var crlStore db.CertificateRevocationListInfo + var revokedList []db.RevokedCertificateInfo + + type test struct { + auth *Authority + 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{ + MUseToken: func(id, tok string) (bool, error) { + return true, nil + }, + MGetCertificate: func(sn string) (*x509.Certificate, error) { + return nil, errors.New("not found") + }, + MStoreCRL: func(i *db.CertificateRevocationListInfo) error { + crlStore = *i + return nil + }, + MGetCRL: func() (*db.CertificateRevocationListInfo, error) { + return &crlStore, nil + }, + MGetRevokedCertificates: func() (*[]db.RevokedCertificateInfo, error) { + return &revokedList, nil + }, + MRevoke: func(rci *db.RevokedCertificateInfo) error { + revokedList = append(revokedList, *rci) + return nil + }, + })) + + return test{ + auth: _a, + ctx: crlCtx, + expected: nil, + } + }, + "ok/crl-full": func() test { + _a := testAuthority(t, WithDatabase(&db.MockAuthDB{ + MUseToken: func(id, tok string) (bool, error) { + return true, nil + }, + MGetCertificate: func(sn string) (*x509.Certificate, error) { + return nil, errors.New("not found") + }, + MStoreCRL: func(i *db.CertificateRevocationListInfo) error { + crlStore = *i + return nil + }, + MGetCRL: func() (*db.CertificateRevocationListInfo, error) { + return &crlStore, nil + }, + MGetRevokedCertificates: func() (*[]db.RevokedCertificateInfo, error) { + return &revokedList, nil + }, + MRevoke: func(rci *db.RevokedCertificateInfo) error { + revokedList = append(revokedList, *rci) + return nil + }, + })) + + var ex []string + + for i := 0; i < 100; i++ { + sn := fmt.Sprintf("%v", i) + + cl := jwt.Claims{ + Subject: fmt.Sprintf("sn-%v", i), + Issuer: validIssuer, + NotBefore: jwt.NewNumericDate(now), + Expiry: jwt.NewNumericDate(now.Add(time.Minute)), + Audience: validAudience, + ID: sn, + } + raw, err := jwt.Signed(sig).Claims(cl).CompactSerialize() + assert.FatalError(t, err) + err = _a.Revoke(crlCtx, &RevokeOptions{ + Serial: sn, + ReasonCode: reasonCode, + Reason: reason, + OTT: raw, + }) + + assert.FatalError(t, err) + + ex = append(ex, sn) + } + + return test{ + auth: _a, + ctx: crlCtx, + expected: ex, + } + }, + } + for name, f := range tests { + tc := f() + t.Run(name, func(t *testing.T) { + 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) + } + + var cmpList []string + for _, c := range crl.TBSCertList.RevokedCertificates { + cmpList = append(cmpList, c.SerialNumber.String()) + } + + assert.Equals(t, cmpList, tc.expected) + + } else { + assert.NotNil(t, tc.err) + } + }) + } +} diff --git a/db/db.go b/db/db.go index a8ae23ee..4db437fe 100644 --- a/db/db.go +++ b/db/db.go @@ -155,6 +155,7 @@ type RevokedCertificateInfo struct { type CertificateRevocationListInfo struct { Number int64 ExpiresAt time.Time + Duration time.Duration DER []byte } @@ -471,32 +472,44 @@ func (db *DB) Shutdown() error { // MockAuthDB mocks the AuthDB interface. // type MockAuthDB struct { - Err error - Ret1 interface{} - MIsRevoked func(string) (bool, error) - MIsSSHRevoked func(string) (bool, error) - MRevoke func(rci *RevokedCertificateInfo) error - MRevokeSSH func(rci *RevokedCertificateInfo) error - MGetCertificate func(serialNumber string) (*x509.Certificate, error) - MGetCertificateData func(serialNumber string) (*CertificateData, error) - MStoreCertificate func(crt *x509.Certificate) error - MUseToken func(id, tok string) (bool, error) - MIsSSHHost func(principal string) (bool, error) - MStoreSSHCertificate func(crt *ssh.Certificate) error - MGetSSHHostPrincipals func() ([]string, error) - MShutdown func() error + Err error + Ret1 interface{} + MIsRevoked func(string) (bool, error) + MIsSSHRevoked func(string) (bool, error) + MRevoke func(rci *RevokedCertificateInfo) error + MRevokeSSH func(rci *RevokedCertificateInfo) error + MGetCertificate func(serialNumber string) (*x509.Certificate, error) + MGetCertificateData func(serialNumber string) (*CertificateData, error) + MStoreCertificate func(crt *x509.Certificate) error + MUseToken func(id, tok string) (bool, error) + MIsSSHHost func(principal string) (bool, error) + MStoreSSHCertificate func(crt *ssh.Certificate) error + MGetSSHHostPrincipals func() ([]string, error) + MShutdown func() error + MGetRevokedCertificates func() (*[]RevokedCertificateInfo, error) + MGetCRL func() (*CertificateRevocationListInfo, error) + MStoreCRL func(*CertificateRevocationListInfo) error } func (m *MockAuthDB) GetRevokedCertificates() (*[]RevokedCertificateInfo, error) { - panic("implement me") + if m.MGetRevokedCertificates != nil { + return m.MGetRevokedCertificates() + } + return m.Ret1.(*[]RevokedCertificateInfo), m.Err } func (m *MockAuthDB) GetCRL() (*CertificateRevocationListInfo, error) { - panic("implement me") + if m.MGetCRL != nil { + return m.MGetCRL() + } + return m.Ret1.(*CertificateRevocationListInfo), m.Err } func (m *MockAuthDB) StoreCRL(info *CertificateRevocationListInfo) error { - panic("implement me") + if m.MStoreCRL != nil { + return m.MStoreCRL(info) + } + return m.Err } // IsRevoked mock. From 8200d198941b370cc197e8c06075085c1c97c08b Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 26 Oct 2022 18:55:24 -0700 Subject: [PATCH 23/29] 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 | 84 +++++++---------- authority/authority_test.go | 6 -- authority/config/config.go | 71 ++++++++++++--- authority/tls.go | 176 +++++++++++++++++++++--------------- authority/tls_test.go | 35 ++++--- cas/softcas/softcas.go | 1 - db/db.go | 7 -- docs/GETTING_STARTED.md | 28 +++--- 8 files changed, 230 insertions(+), 178 deletions(-) diff --git a/authority/authority.go b/authority/authority.go index b72fe61e..43216f1e 100644 --- a/authority/authority.go +++ b/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) - } -} diff --git a/authority/authority_test.go b/authority/authority_test.go index 48479d5b..9f35f23e 100644 --- a/authority/authority_test.go +++ b/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) diff --git a/authority/config/config.go b/authority/config/config.go index ad261cdb..79ac3e88 100644 --- a/authority/config/config.go +++ b/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()) } diff --git a/authority/tls.go b/authority/tls.go index 051e09c9..84106002 100644 --- a/authority/tls.go +++ b/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. diff --git a/authority/tls_test.go b/authority/tls_test.go index 6223bb3d..1d542cb9 100644 --- a/authority/tls_test.go +++ b/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()) } }) } diff --git a/cas/softcas/softcas.go b/cas/softcas/softcas.go index af93420d..6eae9e9e 100644 --- a/cas/softcas/softcas.go +++ b/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 diff --git a/db/db.go b/db/db.go index 4db437fe..bf1b2f48 100644 --- a/db/db.go +++ b/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") } diff --git a/docs/GETTING_STARTED.md b/docs/GETTING_STARTED.md index 328fba69..0465f0b7 100644 --- a/docs/GETTING_STARTED.md +++ b/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. From 812fee76308c716da57e00d12534fa17d8a9aef2 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 27 Oct 2022 11:38:30 -0700 Subject: [PATCH 24/29] Start crl generator before setting initOnce --- authority/authority.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/authority/authority.go b/authority/authority.go index deebdaa9..3fb6f51f 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -716,12 +716,6 @@ func (a *Authority) init() error { a.templates.Data["Step"] = tmplVars } - // JWT numeric dates are seconds. - a.startTime = time.Now().Truncate(time.Second) - // Set flag indicating that initialization has been completed, and should - // not be repeated. - a.initOnce = true - // Start the CRL generator, we can assume the configuration is validated. if a.config.CRL.IsEnabled() { // Default cache duration to the default one @@ -734,6 +728,12 @@ func (a *Authority) init() error { } } + // JWT numeric dates are seconds. + a.startTime = time.Now().Truncate(time.Second) + // Set flag indicating that initialization has been completed, and should + // not be repeated. + a.initOnce = true + return nil } From 6d4fd7d016e5e8afaa17c36d961d22d06bf6c805 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 27 Oct 2022 11:39:44 -0700 Subject: [PATCH 25/29] Update changelog with CRL support --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59829021..cb552de6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. X.509 certificates. - Added provisioner webhooks for augmenting template data and authorizing certificate requests before signing. - Added automatic migration of provisioners when enabling remote managment. +- Added experimental support for CRLs. ### Fixed - MySQL DSN parsing issues fixed with upgrade to [smallstep/nosql@v0.5.0](https://github.com/smallstep/nosql/releases/tag/v0.5.0). From 51c7f560307e332bbe5e5fc87178d61d4ec0fbf2 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 27 Oct 2022 11:57:48 -0700 Subject: [PATCH 26/29] Truncate time to the second --- authority/tls.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/authority/tls.go b/authority/tls.go index 184944a0..e80c9091 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -30,8 +30,8 @@ import ( casapi "github.com/smallstep/certificates/cas/apiv1" "github.com/smallstep/certificates/db" "github.com/smallstep/certificates/errs" - "github.com/smallstep/nosql/database" "github.com/smallstep/certificates/webhook" + "github.com/smallstep/nosql/database" ) // GetTLSOptions returns the tls options configured. @@ -689,7 +689,7 @@ func (a *Authority) GenerateCertificateRevocationList() error { return errors.Wrap(err, "could not retrieve CRL from database") } - now := time.Now().UTC() + now := time.Now().Truncate(time.Second).UTC() revokedList, err := crlDB.GetRevokedCertificates() if err != nil { return errors.Wrap(err, "could not retrieve revoked certificates list from database") From f066ac3d401339399871084721ea7b3ca6b1d068 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 27 Oct 2022 11:58:01 -0700 Subject: [PATCH 27/29] Remove buggy logic on GetRevokedCertificates() --- db/db.go | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/db/db.go b/db/db.go index bf1b2f48..784c75f4 100644 --- a/db/db.go +++ b/db/db.go @@ -248,29 +248,12 @@ func (db *DB) GetRevokedCertificates() (*[]RevokedCertificateInfo, error) { return nil, err } var revokedCerts []RevokedCertificateInfo - now := time.Now().Truncate(time.Second) - for _, e := range entries { var data RevokedCertificateInfo if err := json.Unmarshal(e.Value, &data); err != nil { return nil, err } - - if !data.RevokedAt.IsZero() { - revokedCerts = append(revokedCerts, data) - } else if data.RevokedAt.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(now) { - revokedCerts = append(revokedCerts, data) - } - } + revokedCerts = append(revokedCerts, data) } return &revokedCerts, nil } From 89c8c6d0a0d8a8d062d6b0078ba34151c8d1cb15 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 27 Oct 2022 12:06:38 -0700 Subject: [PATCH 28/29] Fix package name in tls test --- authority/tls_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/authority/tls_test.go b/authority/tls_test.go index 37066843..918adbdc 100644 --- a/authority/tls_test.go +++ b/authority/tls_test.go @@ -1903,15 +1903,15 @@ func TestAuthority_CRL(t *testing.T) { for i := 0; i < 100; i++ { sn := fmt.Sprintf("%v", i) - cl := jwt.Claims{ + cl := jose.Claims{ Subject: fmt.Sprintf("sn-%v", i), Issuer: validIssuer, - NotBefore: jwt.NewNumericDate(now), - Expiry: jwt.NewNumericDate(now.Add(time.Minute)), + NotBefore: jose.NewNumericDate(now), + Expiry: jose.NewNumericDate(now.Add(time.Minute)), Audience: validAudience, ID: sn, } - raw, err := jwt.Signed(sig).Claims(cl).CompactSerialize() + raw, err := jose.Signed(sig).Claims(cl).CompactSerialize() assert.FatalError(t, err) err = a.Revoke(crlCtx, &RevokeOptions{ Serial: sn, From 2d582e5694030434b08d6ce519f7f2a5ec322d5c Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 27 Oct 2022 12:20:13 -0700 Subject: [PATCH 29/29] Remove use of time.Duration.Abs time.Duration.Abs() was added in Go 1.19 --- authority/tls.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/authority/tls.go b/authority/tls.go index e80c9091..b5d85074 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -706,7 +706,7 @@ func (a *Authority) GenerateCertificateRevocationList() error { // 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()) + skipExpiredTime := now.Add(-config.DefaultCRLExpiredDuration) for _, revokedCert := range *revokedList { // skip expired certificates if !revokedCert.ExpiresAt.IsZero() && revokedCert.ExpiresAt.Before(skipExpiredTime) {