Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crl provider: Static and FileWatcher provider implementations #6670

Merged
merged 63 commits into from Oct 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
63 commits
Select commit Hold shift + click to select a range
978cb44
rename certificateListExt to CRL
gtcooke94 Jun 29, 2023
7d032e0
CRLProvider file
gtcooke94 Jun 29, 2023
cdbc298
Add CRLProvider to RevocationConfig
gtcooke94 Jun 29, 2023
95991d8
Beginning refactor of CRL handling
gtcooke94 Jun 29, 2023
32e3158
Shell of StaticCRLProvider
gtcooke94 Jun 29, 2023
00de36e
basic static crl provider test
gtcooke94 Jun 29, 2023
8033cab
use loadCRL helper
gtcooke94 Jun 30, 2023
338a7f4
refactor of CRL loading
gtcooke94 Jun 30, 2023
d1f63fe
Table tests
gtcooke94 Jun 30, 2023
01afa97
Table tests
gtcooke94 Jun 30, 2023
401eb79
Add tests with Static CRL provider
gtcooke94 Jun 30, 2023
c88d12d
New certs to be used for CRL tests. Added test for passing and failin…
erm-g Sep 26, 2023
1feaae3
Main functionality of File Watcher (Directory) CRL provider
erm-g Sep 28, 2023
a9a84f1
Refactor async go routine, validate() func, add unit tests
erm-g Sep 29, 2023
5a0acad
Custom error callback, related unit tests
erm-g Sep 29, 2023
f3c830b
Error callback test improvement
erm-g Oct 1, 2023
4ea1b34
Comments for StaticCRLProvider
erm-g Oct 1, 2023
aeebd4e
Comments for public API
erm-g Oct 2, 2023
735ac20
go mod tidy
erm-g Oct 2, 2023
5c76a60
Comments for tests
erm-g Oct 2, 2023
0bc7757
Fix vet errors
erm-g Oct 2, 2023
f844c8c
Change Static provider behavior to match C Core, address other PR com…
erm-g Oct 3, 2023
a4da85e
Data race fix
erm-g Oct 4, 2023
c3ba07e
Test helper fn change
gtcooke94 Oct 4, 2023
ffe5c34
Address PR comments
erm-g Oct 8, 2023
6d28181
Address PR comments (part 2)
erm-g Oct 9, 2023
7814373
Migration from context to channel for controlling crl reloading gorou…
erm-g Oct 9, 2023
1a46b65
Align in-memory CRL updates during directory scan to C++ behavior
erm-g Oct 10, 2023
d7f1555
Improve comments for ScanCRLDirectory
erm-g Oct 10, 2023
2f1935d
Base test case for Scan CRL Directory file manipulations
erm-g Oct 10, 2023
0a7b086
full set of cases for CRL directory content manipulation
erm-g Oct 10, 2023
8d05f28
Add comment for table test structure
erm-g Oct 10, 2023
ccbf7f6
Fix for go.mod and go.sum
erm-g Oct 11, 2023
99ecab0
Empty directoru workaround
erm-g Oct 11, 2023
9e5a70d
Delete deprecated crl functionality
erm-g Oct 11, 2023
5643760
Restoring deprecated crl files
erm-g Oct 11, 2023
8898959
Fit to grpctest.Tester pattern
erm-g Oct 12, 2023
b16af8b
Update readme for crl provider tests
erm-g Oct 16, 2023
21f4301
Address PR comments
erm-g Oct 16, 2023
51b42aa
Revert "Restoring deprecated crl files"
gtcooke94 Oct 16, 2023
f0c1ca4
Merge remote-tracking branch 'upstream/master' into CrlTemp
gtcooke94 Oct 16, 2023
08188d1
Revert "Resolve conflicts with upstream - deletion of deprecated crl"
erm-g Oct 16, 2023
9b8d07e
Update link for gRFC proposal
erm-g Oct 19, 2023
ad15e23
Address PR comments
erm-g Oct 19, 2023
8e02546
Address PR comments part 1
erm-g Oct 20, 2023
e6a690d
Address PR comments part 2
erm-g Oct 20, 2023
340757d
Address PR comments part 3
erm-g Oct 20, 2023
1e4c5ac
Fix for go.mod and go.sum
erm-g Oct 20, 2023
f654d18
Fix comment typo
erm-g Oct 23, 2023
53d6b05
Fix for gRFC tag
erm-g Oct 23, 2023
1f398eb
Add more details to CRL api godoc comments.
erm-g Oct 23, 2023
f3dcca1
Address PR comments
erm-g Oct 24, 2023
131e6e7
Address PR comments
erm-g Oct 24, 2023
bc14ea8
Delete crl_deprecated.go and crl_deprecated_test.go
erm-g Oct 24, 2023
96bf905
Delete testdate/crl/provider/filewatcher directory and .gitignore und…
erm-g Oct 24, 2023
0ce6a2c
Race test fix
erm-g Oct 27, 2023
c57a08a
Address PR comments
erm-g Oct 27, 2023
4c53c56
Address PR comments
erm-g Oct 27, 2023
7fedab5
Refactor directory reloader test from checking size of crl map to que…
erm-g Oct 28, 2023
1025333
Add extra case for RefreshDuration config test
erm-g Oct 28, 2023
d9ba363
Update cpmment for table test structure
erm-g Oct 30, 2023
150e585
Unexport scan scanCRLDirectory, drop related mutex, update the comments
erm-g Oct 30, 2023
d7cf48f
Update API comments, clear tmp dir after the tests
erm-g Oct 31, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
55 changes: 54 additions & 1 deletion security/advancedtls/advancedtls_test.go
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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding serverExpectError: false here to make the test case definition easier to parse at a glance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It fits the current pattern, if serverExpectError is missing from test struct it's false.

},
// 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
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type is newly exported. The fields were already named as though they were exported. Are they supposed to be read (or set) by the user directly? If not, they should be unexported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After an offline discussion with @gtcooke94 we made this info private for both C++ and Go

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done - unexported

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