Skip to content

Commit

Permalink
crl provider: Static and FileWatcher provider implementations (#6670)
Browse files Browse the repository at this point in the history 
* rename certificateListExt to CRL

* CRLProvider file

* Add CRLProvider to RevocationConfig

* Beginning refactor of CRL handling

* Shell of StaticCRLProvider

* basic static crl provider test

* use loadCRL helper

* refactor of CRL loading

* Table tests

* Table tests

* Add tests with Static CRL provider

* New certs to be used for CRL tests. Added test for passing and failing connections based on CRL check outcomes

* Main functionality of File Watcher (Directory) CRL provider

* Refactor async go routine, validate() func, add unit tests

* Custom error callback, related unit tests

* Error callback test improvement

* Comments for StaticCRLProvider

* Comments for public API

* go mod tidy

* Comments for tests

* Fix vet errors

* Change Static provider behavior to match C Core, address other PR comments

* Data race fix

* Test helper fn change

* Address PR comments

* Address PR comments (part 2)

* Migration from context to channel for controlling crl reloading goroutine

* Align in-memory CRL updates during directory scan to C++ behavior

* Improve comments for ScanCRLDirectory

* Base test case for Scan CRL Directory file manipulations

* full set of cases for CRL directory content manipulation

* Add comment for table test structure

* Fix for go.mod and go.sum

* Empty directoru workaround

* Delete deprecated crl functionality

* Restoring deprecated crl files

* Fit to grpctest.Tester pattern

* Update readme for crl provider tests

* Address PR comments

* Revert "Restoring deprecated crl files"

This reverts commit 5643760.

* Revert "Resolve conflicts with upstream - deletion of deprecated crl"

This reverts commit e013064, reversing
changes made to 21f4301.

Revert deletion

* Update link for gRFC proposal

* Address PR comments

* Address PR comments part 1

* Address PR comments part 2

* Address PR comments part 3

* Fix for go.mod and go.sum

* Fix comment typo

* Fix for gRFC tag

* Add more details to CRL api  godoc comments.

* Address PR comments

* Address PR comments

* Delete crl_deprecated.go and crl_deprecated_test.go

* Delete testdate/crl/provider/filewatcher directory and .gitignore under it

* Race test fix

* Address PR comments

* Address PR comments

* Refactor directory reloader test from checking size of crl map to querying individual entries approach

* Add extra case for RefreshDuration config test

* Update cpmment for table test structure

* Unexport scan scanCRLDirectory, drop related mutex, update the comments

* Update API comments, clear tmp dir after the tests

---------

Co-authored-by: Gregory Cooke <gregorycooke@google.com>
  • Loading branch information
@erm-g @gtcooke94
erm-g and gtcooke94 authored Oct 31, 2023
1 parent d7ea67b commit b82468a
Show file tree
Hide file tree
Showing 22 changed files with 1,336 additions and 59 deletions.
55 changes: 54 additions & 1 deletion security/advancedtls/advancedtls_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"errors"
"fmt"
"net"
"os"
"testing"

lru "github.com/hashicorp/golang-lru"
Expand Down Expand Up @@ -371,6 +372,27 @@ func (s) TestClientServerHandshake(t *testing.T) {
getRootCAsForServerBad := func(params *GetRootCAsParams) (*GetRootCAsResults, error) {
return nil, fmt.Errorf("bad root certificate reloading")
}

getRootCAsForClientCRL := func(params *GetRootCAsParams) (*GetRootCAsResults, error) {
return &GetRootCAsResults{TrustCerts: cs.ClientTrust3}, nil
}

getRootCAsForServerCRL := func(params *GetRootCAsParams) (*GetRootCAsResults, error) {
return &GetRootCAsResults{TrustCerts: cs.ServerTrust3}, nil
}

makeStaticCRLProvider := func(crlPath string) *RevocationConfig {
rawCRL, err := os.ReadFile(crlPath)
if err != nil {
t.Fatalf("readFile(%v) failed err = %v", crlPath, err)
}
cRLProvider := NewStaticCRLProvider([][]byte{rawCRL})
return &RevocationConfig{
AllowUndetermined: true,
CRLProvider: cRLProvider,
}
}

cache, err := lru.New(5)
if err != nil {
t.Fatalf("lru.New: err = %v", err)
Expand Down Expand Up @@ -682,7 +704,7 @@ func (s) TestClientServerHandshake(t *testing.T) {
},
// Client: set valid credentials with the revocation config
// Server: set valid credentials with the revocation config
// Expected Behavior: success, because non of the certificate chains sent in the connection are revoked
// Expected Behavior: success, because none of the certificate chains sent in the connection are revoked
{
desc: "Client sets peer cert, reload root function with verifyFuncGood; Server sets peer cert, reload root function; mutualTLS",
clientCert: []tls.Certificate{cs.ClientCert1},
Expand All @@ -704,6 +726,37 @@ func (s) TestClientServerHandshake(t *testing.T) {
Cache: cache,
},
},
// Client: set valid credentials with the revocation config
// Server: set valid credentials with the revocation config
// Expected Behavior: success, because none of the certificate chains sent in the connection are revoked
{
desc: "Client sets peer cert, reload root function with verifyFuncGood; Server sets peer cert, reload root function; Client uses CRL; mutualTLS",
clientCert: []tls.Certificate{cs.ClientCert3},
clientGetRoot: getRootCAsForClientCRL,
clientVerifyFunc: clientVerifyFuncGood,
clientVType: CertVerification,
clientRevocationConfig: makeStaticCRLProvider(testdata.Path("crl/provider_crl_empty.pem")),
serverMutualTLS: true,
serverCert: []tls.Certificate{cs.ServerCert3},
serverGetRoot: getRootCAsForServerCRL,
serverVType: CertVerification,
},
// Client: set valid credentials with the revocation config
// Server: set revoked credentials with the revocation config
// Expected Behavior: fail, server creds are revoked
{
desc: "Client sets peer cert, reload root function with verifyFuncGood; Server sets revoked cert; Client uses CRL; mutualTLS",
clientCert: []tls.Certificate{cs.ClientCert3},
clientGetRoot: getRootCAsForClientCRL,
clientVerifyFunc: clientVerifyFuncGood,
clientVType: CertVerification,
clientRevocationConfig: makeStaticCRLProvider(testdata.Path("crl/provider_crl_server_revoked.pem")),
serverMutualTLS: true,
serverCert: []tls.Certificate{cs.ServerCert3},
serverGetRoot: getRootCAsForServerCRL,
serverVType: CertVerification,
serverExpectError: true,
},
} {
test := test
t.Run(test.desc, func(t *testing.T) {
Expand Down
116 changes: 86 additions & 30 deletions security/advancedtls/crl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,11 @@ type RevocationConfig struct {
AllowUndetermined bool
// Cache will store CRL files if not nil, otherwise files are reloaded for every lookup.
Cache Cache
// CRLProvider is an alternative to using RootDir directly for the
// X509_LOOKUP_hash_dir approach to CRL files. If set, the CRLProvider's CRL
// function will be called when looking up and fetching CRLs during the
// handshake.
CRLProvider CRLProvider
}

// RevocationStatus is the revocation status for a certificate or chain.
Expand All @@ -83,13 +88,46 @@ func (s RevocationStatus) String() string {
return [...]string{"RevocationUndetermined", "RevocationUnrevoked", "RevocationRevoked"}[s]
}

// certificateListExt contains a pkix.CertificateList and parsed
// extensions that aren't provided by the golang CRL parser.
type certificateListExt struct {
CertList *x509.RevocationList
// CRL contains a pkix.CertificateList and parsed extensions that aren't
// provided by the golang CRL parser.
// All CRLs should be loaded using NewCRL() for bytes directly or ReadCRLFile()
// to read directly from a filepath
type CRL struct {
certList *x509.RevocationList
// RFC5280, 5.2.1, all conforming CRLs must have a AKID with the ID method.
AuthorityKeyID []byte
RawIssuer []byte
authorityKeyID []byte
rawIssuer []byte
}

// NewCRL constructs new CRL from the provided byte array.
func NewCRL(b []byte) (*CRL, error) {
crl, err := parseRevocationList(b)
if err != nil {
return nil, fmt.Errorf("fail to parse CRL: %v", err)
}
crlExt, err := parseCRLExtensions(crl)
if err != nil {
return nil, fmt.Errorf("fail to parse CRL extensions: %v", err)
}
crlExt.rawIssuer, err = extractCRLIssuer(b)
if err != nil {
return nil, fmt.Errorf("fail to extract CRL issuer failed err= %v", err)
}
return crlExt, nil
}

// ReadCRLFile reads a file from the provided path, and returns constructed CRL
// struct from it.
func ReadCRLFile(path string) (*CRL, error) {
b, err := os.ReadFile(path)
if err != nil {
return nil, fmt.Errorf("cannot read file from provided path %q: %v", path, err)
}
crl, err := NewCRL(b)
if err != nil {
return nil, fmt.Errorf("cannot construct CRL from file %q: %v", path, err)
}
return crl, nil
}

const tagDirectoryName = 4
Expand Down Expand Up @@ -215,31 +253,31 @@ func checkChain(chain []*x509.Certificate, cfg RevocationConfig) RevocationStatu
return chainStatus
}

func cachedCrl(rawIssuer []byte, cache Cache) (*certificateListExt, bool) {
func cachedCrl(rawIssuer []byte, cache Cache) (*CRL, bool) {
val, ok := cache.Get(hex.EncodeToString(rawIssuer))
if !ok {
return nil, false
}
crl, ok := val.(*certificateListExt)
crl, ok := val.(*CRL)
if !ok {
return nil, false
}
// If the CRL is expired, force a reload.
if hasExpired(crl.CertList, time.Now()) {
if hasExpired(crl.certList, time.Now()) {
return nil, false
}
return crl, true
}

// fetchIssuerCRL fetches and verifies the CRL for rawIssuer from disk or cache if configured in cfg.
func fetchIssuerCRL(rawIssuer []byte, crlVerifyCrt []*x509.Certificate, cfg RevocationConfig) (*certificateListExt, error) {
func fetchIssuerCRL(rawIssuer []byte, crlVerifyCrt []*x509.Certificate, cfg RevocationConfig) (*CRL, error) {
if cfg.Cache != nil {
if crl, ok := cachedCrl(rawIssuer, cfg.Cache); ok {
return crl, nil
}
}

crl, err := fetchCRL(rawIssuer, cfg)
crl, err := fetchCRLOpenSSLHashDir(rawIssuer, cfg)
if err != nil {
return nil, fmt.Errorf("fetchCRL() failed: %v", err)
}
Expand All @@ -253,21 +291,39 @@ func fetchIssuerCRL(rawIssuer []byte, crlVerifyCrt []*x509.Certificate, cfg Revo
return crl, nil
}

func fetchCRL(c *x509.Certificate, crlVerifyCrt []*x509.Certificate, cfg RevocationConfig) (*CRL, error) {
if cfg.CRLProvider != nil {
crl, err := cfg.CRLProvider.CRL(c)
if err != nil {
return nil, fmt.Errorf("CrlProvider failed err = %v", err)
}
if crl == nil {
return nil, fmt.Errorf("no CRL found for certificate's issuer")
}
return crl, nil
}
return fetchIssuerCRL(c.RawIssuer, crlVerifyCrt, cfg)
}

// checkCert checks a single certificate against the CRL defined in the certificate.
// It will fetch and verify the CRL(s) defined in the root directory specified by cfg.
// If we can't load any authoritative CRL files, the status is RevocationUndetermined.
// c is the certificate to check.
// crlVerifyCrt is the group of possible certificates to verify the crl.
func checkCert(c *x509.Certificate, crlVerifyCrt []*x509.Certificate, cfg RevocationConfig) RevocationStatus {
crl, err := fetchIssuerCRL(c.RawIssuer, crlVerifyCrt, cfg)
crl, err := fetchCRL(c, crlVerifyCrt, cfg)
if err != nil {
// We couldn't load any CRL files for the certificate, so we don't know if it's RevocationUnrevoked or not.
grpclogLogger.Warningf("getIssuerCRL(%v) err = %v", c.Issuer, err)
// We couldn't load any CRL files for the certificate, so we don't know
// if it's RevocationUnrevoked or not. This is not necessarily a
// problem - it's not invalid to have no CRLs if you don't have any
// revocations for an issuer. We just return RevocationUndetermined and
// there is a setting for the user to control the handling of that.
grpclogLogger.Warningf("fetchCRL() err = %v", err)
return RevocationUndetermined
}
revocation, err := checkCertRevocation(c, crl)
if err != nil {
grpclogLogger.Warningf("checkCertRevocation(CRL %v) failed: %v", crl.CertList.Issuer, err)
grpclogLogger.Warningf("checkCertRevocation(CRL %v) failed: %v", crl.certList.Issuer, err)
// We couldn't check the CRL file for some reason, so we don't know if it's RevocationUnrevoked or not.
return RevocationUndetermined
}
Expand All @@ -277,13 +333,13 @@ func checkCert(c *x509.Certificate, crlVerifyCrt []*x509.Certificate, cfg Revoca
return revocation
}

func checkCertRevocation(c *x509.Certificate, crl *certificateListExt) (RevocationStatus, error) {
func checkCertRevocation(c *x509.Certificate, crl *CRL) (RevocationStatus, error) {
// Per section 5.3.3 we prime the certificate issuer with the CRL issuer.
// Subsequent entries use the previous entry's issuer.
rawEntryIssuer := crl.RawIssuer
rawEntryIssuer := crl.rawIssuer

// Loop through all the revoked certificates.
for _, revCert := range crl.CertList.RevokedCertificates {
for _, revCert := range crl.certList.RevokedCertificates {
// 5.3 Loop through CRL entry extensions for needed information.
for _, ext := range revCert.Extensions {
if oidCertificateIssuer.Equal(ext.Id) {
Expand Down Expand Up @@ -375,11 +431,11 @@ type issuingDistributionPoint struct {

// parseCRLExtensions parses the extensions for a CRL
// and checks that they're supported by the parser.
func parseCRLExtensions(c *x509.RevocationList) (*certificateListExt, error) {
func parseCRLExtensions(c *x509.RevocationList) (*CRL, error) {
if c == nil {
return nil, errors.New("c is nil, expected any value")
}
certList := &certificateListExt{CertList: c}
certList := &CRL{certList: c}

for _, ext := range c.Extensions {
switch {
Expand All @@ -393,7 +449,7 @@ func parseCRLExtensions(c *x509.RevocationList) (*certificateListExt, error) {
} else if len(rest) != 0 {
return nil, errors.New("trailing data after AKID extension")
}
certList.AuthorityKeyID = a.ID
certList.authorityKeyID = a.ID

case oidIssuingDistributionPoint.Equal(ext.Id):
var dp issuingDistributionPoint
Expand All @@ -418,14 +474,14 @@ func parseCRLExtensions(c *x509.RevocationList) (*certificateListExt, error) {
}
}

if len(certList.AuthorityKeyID) == 0 {
if len(certList.authorityKeyID) == 0 {
return nil, errors.New("authority key identifier extension missing")
}
return certList, nil
}

func fetchCRL(rawIssuer []byte, cfg RevocationConfig) (*certificateListExt, error) {
var parsedCRL *certificateListExt
func fetchCRLOpenSSLHashDir(rawIssuer []byte, cfg RevocationConfig) (*CRL, error) {
var parsedCRL *CRL
// 6.3.3 (a) (1) (ii)
// According to X509_LOOKUP_hash_dir the format is issuer_hash.rN where N is an increasing number.
// There are no gaps, so we break when we can't find a file.
Expand All @@ -449,7 +505,7 @@ func fetchCRL(rawIssuer []byte, cfg RevocationConfig) (*certificateListExt, erro
// Parsing errors for a CRL shouldn't happen so fail.
return nil, fmt.Errorf("parseRevocationList(%v) failed: %v", crlPath, err)
}
var certList *certificateListExt
var certList *CRL
if certList, err = parseCRLExtensions(crl); err != nil {
grpclogLogger.Infof("fetchCRL: unsupported crl %v: %v", crlPath, err)
// Continue to find a supported CRL
Expand All @@ -460,7 +516,7 @@ func fetchCRL(rawIssuer []byte, cfg RevocationConfig) (*certificateListExt, erro
if err != nil {
return nil, err
}
certList.RawIssuer = rawCRLIssuer
certList.rawIssuer = rawCRLIssuer
// RFC5280, 6.3.3 (b) Verify the issuer and scope of the complete CRL.
if bytes.Equal(rawIssuer, rawCRLIssuer) {
parsedCRL = certList
Expand All @@ -475,7 +531,7 @@ func fetchCRL(rawIssuer []byte, cfg RevocationConfig) (*certificateListExt, erro
return parsedCRL, nil
}

func verifyCRL(crl *certificateListExt, rawIssuer []byte, chain []*x509.Certificate) error {
func verifyCRL(crl *CRL, rawIssuer []byte, chain []*x509.Certificate) error {
// RFC5280, 6.3.3 (f) Obtain and validateate the certification path for the issuer of the complete CRL
// We intentionally limit our CRLs to be signed with the same certificate path as the certificate
// so we can use the chain from the connection.
Expand All @@ -487,12 +543,12 @@ func verifyCRL(crl *certificateListExt, rawIssuer []byte, chain []*x509.Certific
// "Conforming CRL issuers MUST use the key identifier method, and MUST
// include this extension in all CRLs issued."
// So, this is much simpler than RFC4158 and should be compatible.
if bytes.Equal(c.SubjectKeyId, crl.AuthorityKeyID) && bytes.Equal(c.RawSubject, crl.RawIssuer) {
if bytes.Equal(c.SubjectKeyId, crl.authorityKeyID) && bytes.Equal(c.RawSubject, crl.rawIssuer) {
// RFC5280, 6.3.3 (g) Validate signature.
return crl.CertList.CheckSignatureFrom(c)
return crl.certList.CheckSignatureFrom(c)
}
}
return fmt.Errorf("verifyCRL: No certificates mached CRL issuer (%v)", crl.CertList.Issuer)
return fmt.Errorf("verifyCRL: No certificates mached CRL issuer (%v)", crl.certList.Issuer)
}

// pemType is the type of a PEM encoded CRL.
Expand Down
Loading

0 comments on commit b82468a

Please sign in to comment.