From 03272cd85f8fcb5af0f71865c0eeea7db5838200 Mon Sep 17 00:00:00 2001 From: kayrus Date: Tue, 19 Jul 2016 14:08:02 +0200 Subject: [PATCH] CRL: Update cfssl dependency and fixed remarks --- .../cloudflare/cfssl/revoke/revoke.go | 179 +++++++++++------- embed/etcd.go | 29 +-- pkg/tlsutil/tlsutil.go | 28 ++- 3 files changed, 136 insertions(+), 100 deletions(-) diff --git a/cmd/vendor/github.com/cloudflare/cfssl/revoke/revoke.go b/cmd/vendor/github.com/cloudflare/cfssl/revoke/revoke.go index a9e4267ba8f2..1ed5f3907c47 100644 --- a/cmd/vendor/github.com/cloudflare/cfssl/revoke/revoke.go +++ b/cmd/vendor/github.com/cloudflare/cfssl/revoke/revoke.go @@ -11,6 +11,7 @@ import ( "encoding/base64" "encoding/pem" "errors" + "fmt" "io/ioutil" "net/http" neturl "net/url" @@ -23,14 +24,21 @@ import ( "github.com/cloudflare/cfssl/log" ) -// HardFail determines whether the failure to check the revocation -// status of a certificate (i.e. due to network failure) causes -// verification to fail (a hard failure). -var HardFail = false +// Revoke type contains configuration for each new revoke instance +type Revoke struct { + // HardFail determines whether the failure to check the revocation + // status of a certificate (i.e. due to network failure) causes + // verification to fail (a hard failure). + HardFail bool + // CRLSet associates a PKIX certificate list with the URL the CRL is + // fetched from. + CRLSet map[string]*pkix.CertificateList +} -// CRLSet associates a PKIX certificate list with the URL the CRL is -// fetched from. -var CRLSet = map[string]*pkix.CertificateList{} +// NewRevokeChecker creates Revoke config structure +func NewRevokeChecker() *Revoke { + return &Revoke{false, map[string]*pkix.CertificateList{}} +} // We can't handle LDAP certificates, so this checks to see if the // URL string points to an LDAP resource so that we can ignore it. @@ -61,16 +69,29 @@ func ldapURL(url string) bool { // // true, false: failure to check revocation status causes // verification to fail -func revCheck(cert *x509.Certificate) (revoked, ok bool) { +func (r *Revoke) revCheck(cert *x509.Certificate, localCRLPath string) (revoked, ok bool) { + if localCRLPath != "" { + if revoked, ok := r.certIsRevokedCRL(cert, localCRLPath, true); !ok { + log.Warning("error checking revocation via CRL") + if r.HardFail { + return true, false + } + return false, false + } else if revoked { + log.Info("certificate is revoked via CRL") + return true, true + } + } + for _, url := range cert.CRLDistributionPoints { if ldapURL(url) { log.Infof("skipping LDAP CRL: %s", url) continue } - if revoked, ok := certIsRevokedCRL(cert, url); !ok { + if revoked, ok := r.certIsRevokedCRL(cert, url, false); !ok { log.Warning("error checking revocation via CRL") - if HardFail { + if r.HardFail { return true, false } return false, false @@ -79,9 +100,9 @@ func revCheck(cert *x509.Certificate) (revoked, ok bool) { return true, true } - if revoked, ok := certIsRevokedOCSP(cert, HardFail); !ok { + if revoked, ok := certIsRevokedOCSP(cert, r.HardFail); !ok { log.Warning("error checking revocation via OCSP") - if HardFail { + if r.HardFail { return true, false } return false, false @@ -112,21 +133,6 @@ func fetchCRL(url string) (*pkix.CertificateList, error) { return x509.ParseCRL(body) } -func revLocalCheck(cert *x509.Certificate, path string) (revoked, ok bool) { - if revoked, ok := certIsRevokedCRL(cert, path); !ok { - log.Warning("error checking revocation via CRL") - if HardFail { - return true, false - } - return false, false - } else if revoked { - log.Info("certificate is revoked via CRL") - return true, true - } - - return false, true -} - func getIssuer(cert *x509.Certificate) *x509.Certificate { var issuer *x509.Certificate var err error @@ -139,73 +145,99 @@ func getIssuer(cert *x509.Certificate) *x509.Certificate { } return issuer - } -// check a cert against a specific CRL. Returns the same bool pair -// as revCheck. -func certIsRevokedCRL(cert *x509.Certificate, url string) (revoked, ok bool) { - crl, ok := CRLSet[url] +// checks whether CRL in memory is valid +func (r *Revoke) isInMemoryCRLValid(key string) bool { + crl, ok := r.CRLSet[key] if ok && crl == nil { ok = false - delete(CRLSet, url) + delete(r.CRLSet, key) } - var shouldFetchCRL = true if ok { if !crl.HasExpired(time.Now()) { - shouldFetchCRL = false + return true } } - u, err := neturl.Parse(url) + return false +} + +// FetchLocalCRL reads CRL from the local filesystem +// force flag allows you to update the CRL +func (r *Revoke) FetchLocalCRL(path string, force bool) error { + shouldFetchCRL := !r.isInMemoryCRLValid(path) + + u, err := neturl.Parse(path) if err != nil { log.Warningf("failed to parse CRL url: %v", err) - return false, false + return err } - if u.Scheme == "" && shouldFetchCRL { - if _, err := os.Stat(url); err == nil { - tmp, err := ioutil.ReadFile(url) + + if u.Scheme == "" && (shouldFetchCRL || force) { + if _, err := os.Stat(path); err == nil { + tmp, err := ioutil.ReadFile(path) if err != nil { - log.Warningf("failed to read local CRL path: %v", err) - return false, false + return fmt.Errorf("failed to read local CRL path: %v", err) } - crl, err = x509.ParseCRL(tmp) + crl, err := x509.ParseCRL(tmp) if err != nil { - log.Warningf("failed to parse local CRL file: %v", err) - return false, false + return fmt.Errorf("failed to parse local CRL file: %v", err) } - CRLSet[url] = crl - shouldFetchCRL = false + r.CRLSet[path] = crl } else { - log.Warningf("failed to read local CRL path: %v", err) - return false, false + return fmt.Errorf("failed to read local CRL path: %v", err) } } + return nil +} + +// FetchRemoteCRL fetches remote CRL into internal map, +// force overwrites previously read CRL +func (r *Revoke) FetchRemoteCRL(url string, cert *x509.Certificate, force bool) error { + shouldFetchCRL := !r.isInMemoryCRLValid(url) + issuer := getIssuer(cert) - if shouldFetchCRL { + if force || shouldFetchCRL { var err error - crl, err = fetchCRL(url) + crl, err := fetchCRL(url) if err != nil { - log.Warningf("failed to fetch CRL: %v", err) - return false, false + return fmt.Errorf("failed to fetch CRL: %v", err) } // check CRL signature if issuer != nil { err = issuer.CheckCRLSignature(crl) if err != nil { - log.Warningf("failed to verify CRL: %v", err) - return false, false + return fmt.Errorf("failed to verify CRL: %v", err) } } - CRLSet[url] = crl + r.CRLSet[url] = crl + } + + return nil +} + +// check a cert against a specific CRL. Returns the same bool pair +// as revCheck. +func (r *Revoke) certIsRevokedCRL(cert *x509.Certificate, url string, fetchLocal bool) (revoked, ok bool) { + var err error + if fetchLocal { + err = r.FetchLocalCRL(url, false) + } else { + err = r.FetchRemoteCRL(url, cert, false) + } + + if err != nil { + log.Warningf("%v", err) + return false, false } - for _, revoked := range crl.TBSCertList.RevokedCertificates { + for _, revoked := range r.CRLSet[url].TBSCertList.RevokedCertificates { if cert.SerialNumber.Cmp(revoked.SerialNumber) == 0 { log.Info("Serial number match: intermediate is revoked.") return true, true @@ -215,32 +247,37 @@ func certIsRevokedCRL(cert *x509.Certificate, url string) (revoked, ok bool) { return false, true } -// VerifyCertificate ensures that the certificate passed in hasn't -// expired and checks the CRL for the server. -func VerifyCertificate(cert *x509.Certificate) (revoked, ok bool) { +// verifyCertTime verifies whether certificate time frames are valid +func verifyCertTime(cert *x509.Certificate) bool { if !time.Now().Before(cert.NotAfter) { - log.Infof("Certificate expired %s\n", cert.NotAfter) - return true, true + log.Infof("Certificate expired %s", cert.NotAfter) + return false } else if !time.Now().After(cert.NotBefore) { - log.Infof("Certificate isn't valid until %s\n", cert.NotBefore) + log.Infof("Certificate isn't valid until %s", cert.NotBefore) + return false + } + + return true +} + +// VerifyCertificate ensures that the certificate passed in hasn't +// expired and checks the CRL for the server. +func (r *Revoke) VerifyCertificate(cert *x509.Certificate) (revoked, ok bool) { + if !verifyCertTime(cert) { return true, true } - return revCheck(cert) + return r.revCheck(cert, "") } // VerifyCertificateByCRLPath ensures that the certificate passed in hasn't // expired and checks the local CRL path. -func VerifyCertificateByCRLPath(cert *x509.Certificate, crlPath string) (revoked, ok bool) { - if !time.Now().Before(cert.NotAfter) { - log.Infof("Certificate expired %s\n", cert.NotAfter) - return true, true - } else if !time.Now().After(cert.NotBefore) { - log.Infof("Certificate isn't valid until %s\n", cert.NotBefore) +func (r *Revoke) VerifyCertificateByCRLPath(cert *x509.Certificate, crlPath string) (revoked, ok bool) { + if !verifyCertTime(cert) { return true, true } - return revLocalCheck(cert, crlPath) + return r.revCheck(cert, crlPath) } func fetchRemote(url string) (*x509.Certificate, error) { diff --git a/embed/etcd.go b/embed/etcd.go index ca9eb9b8747b..412f5f51cb8d 100644 --- a/embed/etcd.go +++ b/embed/etcd.go @@ -16,13 +16,11 @@ package embed import ( "crypto/tls" - "errors" "fmt" "net" "net/http" "path" - "github.com/cloudflare/cfssl/revoke" "github.com/coreos/etcd/etcdserver" "github.com/coreos/etcd/etcdserver/api/v2http" "github.com/coreos/etcd/pkg/cors" @@ -282,27 +280,6 @@ func startClientListeners(cfg *Config) (sctxs map[string]*serveCtx, err error) { return sctxs, nil } -func revokeCheckHandler(req *http.Request, CRLpath string) error { - if req.TLS == nil { - return nil - } - for _, cert := range req.TLS.PeerCertificates { - var revoked, ok bool - if CRLpath != "" { - revoked, ok = revoke.VerifyCertificateByCRLPath(cert, CRLpath) - } else { - revoked, ok = revoke.VerifyCertificate(cert) - } - if !ok { - return errors.New("Cert check failed") - } - if revoked { - return errors.New("Cert if revoked") - } - } - return nil -} - func (e *Etcd) serve() (err error) { var ctlscfg *tls.Config if !e.cfg.ClientTLSInfo.Empty() { @@ -317,9 +294,8 @@ func (e *Etcd) serve() (err error) { } // Start the peer server in a goroutine - ph := tlsutil.RevocationCheck( + ph := tlsutil.NewRevokeHandler( v2http.NewPeerHandler(e.Server), - revokeCheckHandler, e.cfg.PeerTLSInfo.CRLFile) for _, l := range e.Peers { go func(l net.Listener) { @@ -327,9 +303,8 @@ func (e *Etcd) serve() (err error) { }(l) } - clientHandler := tlsutil.RevocationCheck( + clientHandler := tlsutil.NewRevokeHandler( v2http.NewClientHandler(e.Server, e.Server.Cfg.ReqTimeout()), - revokeCheckHandler, e.cfg.ClientTLSInfo.CRLFile) // Start a client server goroutine for each listen address ch := http.Handler(&cors.CORSHandler{ diff --git a/pkg/tlsutil/tlsutil.go b/pkg/tlsutil/tlsutil.go index 1cca0ae0c22a..39c754370516 100644 --- a/pkg/tlsutil/tlsutil.go +++ b/pkg/tlsutil/tlsutil.go @@ -22,6 +22,8 @@ import ( "io/ioutil" "net/http" + "github.com/cloudflare/cfssl/revoke" + etcdErr "github.com/coreos/etcd/error" ) @@ -75,9 +77,31 @@ func NewCert(certfile, keyfile string, parseFunc func([]byte, []byte) (tls.Certi return &tlsCert, nil } -func RevocationCheck(handler http.Handler, checker func(*http.Request, string) error, CRLpath string) http.Handler { +func revokeCheckHandler(req *http.Request, CRLpath string, revokeChecker *revoke.Revoke) error { + if req.TLS == nil { + return nil + } + for _, cert := range req.TLS.PeerCertificates { + var revoked, ok bool + if CRLpath != "" { + revoked, ok = revokeChecker.VerifyCertificateByCRLPath(cert, CRLpath) + } else { + revoked, ok = revokeChecker.VerifyCertificate(cert) + } + if !ok { + return fmt.Errorf("cert check failed (CN=%s, Serial: %s)", cert.Subject.CommonName, cert.SerialNumber.String()) + } + if revoked { + return fmt.Errorf("Cert is revoked (CN=%s, Serial: %s)", cert.Subject.CommonName, cert.SerialNumber.String()) + } + } + return nil +} + +func NewRevokeHandler(handler http.Handler, CRLpath string) http.Handler { + revokeChecker := revoke.NewRevokeChecker() return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - err := checker(req, CRLpath) + err := revokeCheckHandler(req, CRLpath, revokeChecker) if err == nil { handler.ServeHTTP(w, req) return