diff --git a/security/advancedtls/advancedtls.go b/security/advancedtls/advancedtls.go index e8ed9765b..cda5dc8ac 100644 --- a/security/advancedtls/advancedtls.go +++ b/security/advancedtls/advancedtls.go @@ -62,24 +62,12 @@ type HandshakeVerificationInfo struct { Leaf *x509.Certificate } -// VerificationFuncParams contains parameters available to users when -// implementing CustomVerificationFunc. -// The fields in this struct are read-only. -// -// Deprecated: use HandshakeVerificationInfo instead. -type VerificationFuncParams = HandshakeVerificationInfo - // PostHandshakeVerificationResults contains the information about results of // PostHandshakeVerificationFunc. // PostHandshakeVerificationResults is an empty struct for now. It may be extended in the // future to include more information. type PostHandshakeVerificationResults struct{} -// VerificationResults contains the information about results of -// PostHandshakeVerificationFunc. -// Deprecated: use PostHandshakeVerificationResults instead. -type VerificationResults = PostHandshakeVerificationResults - // PostHandshakeVerificationFunc is the function defined by users to perform // custom verification checks after chain building and regular handshake // verification has been completed. @@ -87,14 +75,6 @@ type VerificationResults = PostHandshakeVerificationResults // should fail, with the error containing information on why it failed. type PostHandshakeVerificationFunc func(params *HandshakeVerificationInfo) (*PostHandshakeVerificationResults, error) -// CustomVerificationFunc is the function defined by users to perform custom -// verification check. -// CustomVerificationFunc returns nil if the authorization fails; otherwise -// returns an empty struct. -// -// Deprecated: use PostHandshakeVerificationFunc instead. -type CustomVerificationFunc = PostHandshakeVerificationFunc - // ConnectionInfo contains the parameters available to users when // implementing GetRootCertificates. type ConnectionInfo struct { @@ -104,12 +84,6 @@ type ConnectionInfo struct { RawCerts [][]byte } -// GetRootCAsParams contains the parameters available to users when -// implementing GetRootCAs. -// -// Deprecated: use ConnectionInfo instead. -type GetRootCAsParams = ConnectionInfo - // RootCertificates is the result of GetRootCertificates. // If users want to reload the root trust certificate, it is required to return // the proper TrustCerts in GetRootCAs. @@ -118,13 +92,6 @@ type RootCertificates struct { TrustCerts *x509.CertPool } -// GetRootCAsResults contains the results of GetRootCAs. -// If users want to reload the root trust certificate, it is required to return -// the proper TrustCerts in GetRootCAs. -// -// Deprecated: use RootCertificates instead. -type GetRootCAsResults = RootCertificates - // RootCertificateOptions contains options to obtain root trust certificates // for both the client and the server. // At most one field should be set. If none of them are set, we use the system @@ -134,11 +101,6 @@ type RootCertificateOptions struct { // If RootCertificates is set, it will be used every time when verifying // the peer certificates, without performing root certificate reloading. RootCertificates *x509.CertPool - // If RootCACerts is set, it will be used every time when verifying - // the peer certificates, without performing root certificate reloading. - // - // Deprecated: use RootCertificates instead. - RootCACerts *x509.CertPool // If GetRootCertificates is set, it will be invoked to obtain root certs for // every new connection. GetRootCertificates func(params *ConnectionInfo) (*RootCertificates, error) @@ -213,14 +175,6 @@ const ( SkipVerification ) -// ClientOptions contains the fields needed to be filled by the client. -// Deprecated: use Options instead. -type ClientOptions = Options - -// ServerOptions contains the fields needed to be filled by the server. -// Deprecated: use Options instead. -type ServerOptions = Options - // Options contains the fields a user can configure when setting up TLS clients // and servers type Options struct { @@ -233,13 +187,6 @@ type Options struct { // If this is set, we will perform this customized check after doing the // normal check(s) indicated by setting VerificationType. AdditionalPeerVerification PostHandshakeVerificationFunc - // VerifyPeer is a custom verification check after certificate signature - // check. - // If this is set, we will perform this customized check after doing the - // normal check(s) indicated by setting VerificationType. - // - // Deprecated: use AdditionalPeerVerification instead. - VerifyPeer PostHandshakeVerificationFunc // RootOptions is OPTIONAL on server side. This field only needs to be set if // mutual authentication is required(RequireClientCert is true). RootOptions RootCertificateOptions @@ -251,26 +198,9 @@ type Options struct { // the `VerificationType` enum for the different options. // Default: CertAndHostVerification VerificationType VerificationType - // VType is the verification type on the server side. - // - // Deprecated: use VerificationType instead. - VType VerificationType // RevocationOptions is the configurations for certificate revocation checks. // It could be nil if such checks are not needed. RevocationOptions *RevocationOptions - // RevocationConfig is the configurations for certificate revocation checks. - // It could be nil if such checks are not needed. - // - // Deprecated: use RevocationOptions instead. - RevocationConfig *RevocationConfig - // MinVersion contains the minimum TLS version that is acceptable. - // - // Deprecated: use MinTLSVersion instead. - MinVersion uint16 - // MaxVersion contains the maximum TLS version that is acceptable. - // - // Deprecated: use MaxTLSVersion instead. - MaxVersion uint16 // MinTLSVersion contains the minimum TLS version that is acceptable. // The value should be set using tls.VersionTLSxx from https://pkg.go.dev/crypto/tls // By default, TLS 1.2 is currently used as the minimum when acting as a @@ -296,35 +226,6 @@ type Options struct { } func (o *Options) clientConfig() (*tls.Config, error) { - // TODO(gtcooke94) Remove this block when o.VerifyPeer is remoed. - // VerifyPeer is deprecated, but do this to aid the transitory migration time. - if o.AdditionalPeerVerification == nil { - o.AdditionalPeerVerification = o.VerifyPeer - } - // TODO(gtcooke94). VType is deprecated, eventually remove this block. This - // will ensure that users still explicitly setting `VType` will get the - // setting to the right place. - if o.VType != CertAndHostVerification { - o.VerificationType = o.VType - } - // TODO(gtcooke94) MinVersion and MaxVersion are deprected, eventually - // remove this block. This is a temporary fallback to ensure that if the - // refactored names aren't set we use the old names. - if o.MinTLSVersion == 0 { - o.MinTLSVersion = o.MinVersion - } - if o.MaxTLSVersion == 0 { - o.MaxTLSVersion = o.MaxVersion - } - // TODO(gtcooke94) RootCACerts is deprecated, eventually remove this block. - // This will ensure that users still explicitly setting RootCACerts will get - // the setting int the right place. - if o.RootOptions.RootCACerts != nil { - o.RootOptions.RootCertificates = o.RootOptions.RootCACerts - // There are additional checks that only 1 field of `RootOptions` is - // non-nil, so set the deprecated field to nil - o.RootOptions.RootCACerts = nil - } if o.VerificationType == SkipVerification && o.AdditionalPeerVerification == nil { return nil, fmt.Errorf("client needs to provide custom verification mechanism if choose to skip default verification") } @@ -410,35 +311,6 @@ func (o *Options) clientConfig() (*tls.Config, error) { } func (o *Options) serverConfig() (*tls.Config, error) { - // TODO(gtcooke94) Remove this block when o.VerifyPeer is remoed. - // VerifyPeer is deprecated, but do this to aid the transitory migration time. - if o.AdditionalPeerVerification == nil { - o.AdditionalPeerVerification = o.VerifyPeer - } - // TODO(gtcooke94). VType is deprecated, eventually remove this block. This - // will ensure that users still explicitly setting `VType` will get the - // setting to the right place. - if o.VType != CertAndHostVerification { - o.VerificationType = o.VType - } - // TODO(gtcooke94) MinVersion and MaxVersion are deprected, eventually - // remove this block. This is a temporary fallback to ensure that if the - // refactored names aren't set we use the old names. - if o.MinTLSVersion == 0 { - o.MinTLSVersion = o.MinVersion - } - if o.MaxTLSVersion == 0 { - o.MaxTLSVersion = o.MaxVersion - } - // TODO(gtcooke94) RootCACerts is deprecated, eventually remove this block. - // This will ensure that users still explicitly setting RootCACerts will get - // the setting int the right place. - if o.RootOptions.RootCACerts != nil { - o.RootOptions.RootCertificates = o.RootOptions.RootCACerts - // There are additional checks that only 1 field of `RootOptions` is - // non-nil, so set the deprecated field to nil - o.RootOptions.RootCACerts = nil - } if o.RequireClientCert && o.VerificationType == SkipVerification && o.AdditionalPeerVerification == nil { return nil, fmt.Errorf("server needs to provide custom verification mechanism if choose to skip default verification, but require client certificate(s)") } @@ -728,12 +600,6 @@ func buildVerifyFunc(c *advancedTLSCreds, // NewClientCreds uses ClientOptions to construct a TransportCredentials based // on TLS. func NewClientCreds(o *Options) (credentials.TransportCredentials, error) { - // TODO(gtcooke94) RevocationConfig is deprecated, eventually remove this block. - // This will ensure that users still explicitly setting RevocationConfig will get - // the setting in the right place. - if o.RevocationConfig != nil { - o.RevocationOptions = o.RevocationConfig - } conf, err := o.clientConfig() if err != nil { return nil, err @@ -753,12 +619,6 @@ func NewClientCreds(o *Options) (credentials.TransportCredentials, error) { // NewServerCreds uses ServerOptions to construct a TransportCredentials based // on TLS. func NewServerCreds(o *Options) (credentials.TransportCredentials, error) { - // TODO(gtcooke94) RevocationConfig is deprecated, eventually remove this block. - // This will ensure that users still explicitly setting RevocationConfig will get - // the setting in the right place. - if o.RevocationConfig != nil { - o.RevocationOptions = o.RevocationConfig - } conf, err := o.serverConfig() if err != nil { return nil, err diff --git a/security/advancedtls/advancedtls_test.go b/security/advancedtls/advancedtls_test.go index 7a39b241d..e7fd4458a 100644 --- a/security/advancedtls/advancedtls_test.go +++ b/security/advancedtls/advancedtls_test.go @@ -30,7 +30,6 @@ import ( "testing" "github.com/google/go-cmp/cmp" - lru "github.com/hashicorp/golang-lru" "google.golang.org/grpc/credentials" "google.golang.org/grpc/credentials/tls/certprovider" "google.golang.org/grpc/internal/grpctest" @@ -151,20 +150,6 @@ func (s) TestClientOptionsConfigErrorCases(t *testing.T) { } } -// TODO(gtcooke94) Remove when deprecated `VType` is removed. This doesn't fit nicely into other table tests since it is setting a deprecated option. -// Set VerificationType via the deprecated VType. Make sure it cascades to -// VerificationType. This should error because one cannot skip default -// verification and provide no root credentials", -func (s) TestClientOptionsWithDeprecatedVType(t *testing.T) { - clientOptions := &Options{ - VType: SkipVerification, - } - _, err := clientOptions.clientConfig() - if err == nil { - t.Fatalf("ClientOptions{%v}.config() returns no err, wantErr != nil", clientOptions) - } -} - func (s) TestClientOptionsConfigSuccessCases(t *testing.T) { tests := []struct { desc string @@ -191,13 +176,6 @@ func (s) TestClientOptionsConfigSuccessCases(t *testing.T) { MinVersion: tls.VersionTLS12, MaxVersion: tls.VersionTLS13, }, - { - desc: "Deprecated option is set and forwarded", - clientVerificationType: CertVerification, - RootOptions: RootCertificateOptions{ - RootCACerts: x509.NewCertPool(), - }, - }, { desc: "Ciphersuite plumbing through client options", cipherSuites: []uint16{ @@ -327,20 +305,6 @@ func (s) TestServerOptionsConfigErrorCases(t *testing.T) { } } -// TODO(gtcooke94) Remove when deprecated `VType` is removed. This doesn't fit nicely into other table tests since it is setting a deprecated option. -// Set VerificationType via the deprecated VType. Make sure it cascades to -// VerificationType. This should error because one cannot skip default -// verification and provide no root credentials", -func (s) TestServerOptionsWithDeprecatedVType(t *testing.T) { - serverOptions := &Options{ - VType: SkipVerification, - } - _, err := serverOptions.serverConfig() - if err == nil { - t.Fatalf("ClientOptions{%v}.config() returns no err, wantErr != nil", serverOptions) - } -} - func (s) TestServerOptionsConfigSuccessCases(t *testing.T) { tests := []struct { desc string @@ -375,22 +339,13 @@ func (s) TestServerOptionsConfigSuccessCases(t *testing.T) { MinVersion: tls.VersionTLS12, MaxVersion: tls.VersionTLS13, }, - { - desc: "Deprecated option is set and forwarded", - IdentityOptions: IdentityCertificateOptions{ - Certificates: []tls.Certificate{}, - }, - RootOptions: RootCertificateOptions{ - RootCACerts: x509.NewCertPool(), - }, - }, { desc: "Ciphersuite plumbing through server options", IdentityOptions: IdentityCertificateOptions{ Certificates: []tls.Certificate{}, }, RootOptions: RootCertificateOptions{ - RootCACerts: x509.NewCertPool(), + RootCertificates: x509.NewCertPool(), }, cipherSuites: []uint16{ tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, @@ -440,10 +395,6 @@ func (s) TestClientServerHandshake(t *testing.T) { return &RootCertificates{TrustCerts: cs.ClientTrust1}, nil } - getRootCertificatesForClientDeprecatedTypes := func(params *GetRootCAsParams) (*GetRootCAsResults, error) { - return &GetRootCAsResults{TrustCerts: cs.ClientTrust1}, nil - } - clientVerifyFuncGood := func(params *HandshakeVerificationInfo) (*PostHandshakeVerificationResults, error) { if params.ServerName == "" { return nil, errors.New("client side server name should have a value") @@ -496,10 +447,6 @@ func (s) TestClientServerHandshake(t *testing.T) { } } - cache, err := lru.New(5) - if err != nil { - t.Fatalf("lru.New: err = %v", err) - } for _, test := range []struct { desc string clientCert []tls.Certificate @@ -561,20 +508,6 @@ func (s) TestClientServerHandshake(t *testing.T) { serverVerificationType: CertVerification, serverExpectError: true, }, - // Client: set clientGetRoot with deprecated types, clientVerifyFunc and - // clientCert Server: set serverRoot and serverCert with mutual TLS on - // Expected Behavior: success - { - desc: "Client sets peer cert, reload root function with deprecatd types with verifyFuncGood; server sets peer cert and root cert; mutualTLS", - clientCert: []tls.Certificate{cs.ClientCert1}, - clientGetRoot: getRootCertificatesForClientDeprecatedTypes, - clientVerifyFunc: clientVerifyFuncGood, - clientVerificationType: CertVerification, - serverMutualTLS: true, - serverCert: []tls.Certificate{cs.ServerCert1}, - serverRoot: cs.ServerTrust1, - serverVerificationType: CertVerification, - }, // Client: set clientGetRoot, clientVerifyFunc and clientCert // Server: set serverRoot and serverCert with mutual TLS on // Expected Behavior: success @@ -822,37 +755,13 @@ 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 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}, - clientGetRoot: getRootCertificatesForClient, - clientVerifyFunc: clientVerifyFuncGood, - clientVerificationType: CertVerification, - clientRevocationOptions: &RevocationOptions{ - RootDir: testdata.Path("crl"), - DenyUndetermined: false, - Cache: cache, - }, - serverMutualTLS: true, - serverCert: []tls.Certificate{cs.ServerCert1}, - serverGetRoot: getRootCertificatesForServer, - serverVerificationType: CertVerification, - serverRevocationOptions: &RevocationOptions{ - RootDir: testdata.Path("crl"), - DenyUndetermined: false, - 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.ClientCertForCRL}, clientGetRoot: getRootCertificatesForClientCRL, clientVerifyFunc: clientVerifyFuncGood, clientVerificationType: CertVerification, - clientRevocationOptions: makeStaticCRLRevocationOptions(testdata.Path("crl/provider_crl_empty.pem"), true), + clientRevocationOptions: makeStaticCRLRevocationOptions(testdata.Path("crl/provider_crl_empty.pem"), false), serverMutualTLS: true, serverCert: []tls.Certificate{cs.ServerCertForCRL}, serverGetRoot: getRootCertificatesForServerCRL, @@ -1008,7 +917,7 @@ func (s) TestClientServerHandshake(t *testing.T) { if test.serverRoot != nil { serverRoot = test.serverRoot } else if test.serverGetRoot != nil { - result, _ := test.serverGetRoot(&GetRootCAsParams{}) + result, _ := test.serverGetRoot(&ConnectionInfo{}) serverRoot = result.TrustCerts } else if test.serverRootProvider != nil { km, _ := test.serverRootProvider.KeyMaterial(context.TODO()) @@ -1043,7 +952,7 @@ func (s) TestClientServerHandshake(t *testing.T) { if test.clientRoot != nil { clientRoot = test.clientRoot } else if test.clientGetRoot != nil { - result, _ := test.clientGetRoot(&GetRootCAsParams{}) + result, _ := test.clientGetRoot(&ConnectionInfo{}) clientRoot = result.TrustCerts } else if test.clientRootProvider != nil { km, _ := test.clientRootProvider.KeyMaterial(context.TODO()) diff --git a/security/advancedtls/crl.go b/security/advancedtls/crl.go index a865284db..327caefa0 100644 --- a/security/advancedtls/crl.go +++ b/security/advancedtls/crl.go @@ -23,20 +23,13 @@ package advancedtls import ( "bytes" - "crypto/sha1" - "crypto/tls" "crypto/x509" "crypto/x509/pkix" "encoding/asn1" - "encoding/binary" - "encoding/hex" "encoding/pem" "errors" "fmt" "os" - "path/filepath" - "strings" - "time" "golang.org/x/crypto/cryptobyte" cbasn1 "golang.org/x/crypto/cryptobyte/asn1" @@ -45,34 +38,11 @@ import ( var grpclogLogger = grpclog.Component("advancedtls") -// Cache is an interface to cache CRL files. -// The cache implementation must be concurrency safe. -// A fixed size lru cache from golang-lru is recommended. -type Cache interface { - // Add adds a value to the cache. - Add(key, value any) bool - // Get looks up a key's value from the cache. - Get(key any) (value any, ok bool) -} - // RevocationOptions allows a user to configure certificate revocation behavior. type RevocationOptions struct { - // RootDir is the directory to search for CRL files. - // Directory format must match OpenSSL X509_LOOKUP_hash_dir(3). - // Deprecated: use CRLProvider instead. - RootDir string // DenyUndetermined controls if certificate chains with RevocationUndetermined // revocation status are allowed to complete. DenyUndetermined bool - // AllowUndetermined controls if certificate chains with RevocationUndetermined - // revocation status are allowed to complete. - // - // Deprecated: use DenyUndetermined instead - AllowUndetermined bool - // Cache will store CRL files if not nil, otherwise files are reloaded for every lookup. - // Only used for caching CRLs when using the RootDir setting. - // Deprecated: use CRLProvider instead. - 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 @@ -80,11 +50,6 @@ type RevocationOptions struct { CRLProvider CRLProvider } -// RevocationConfig contains options for CRL lookup. -// -// Deprecated: use RevocationOptions instead. -type RevocationConfig = RevocationOptions - // revocationStatus is the revocation status for a certificate or chain. type revocationStatus int @@ -152,64 +117,6 @@ var ( oidAuthorityKeyIdentifier = asn1.ObjectIdentifier{2, 5, 29, 35} ) -// x509NameHash implements the OpenSSL X509_NAME_hash function for hashed directory lookups. -// -// NOTE: due to the behavior of asn1.Marshal, if the original encoding of the RDN sequence -// contains strings which do not use the ASN.1 PrintableString type, the name will not be -// re-encoded using those types, resulting in a hash which does not match that produced -// by OpenSSL. -func x509NameHash(r pkix.RDNSequence) string { - var canonBytes []byte - // First, canonicalize all the strings. - for _, rdnSet := range r { - for i, rdn := range rdnSet { - value, ok := rdn.Value.(string) - if !ok { - continue - } - // OpenSSL trims all whitespace, does a tolower, and removes extra spaces between words. - // Implemented in x509_name_canon in OpenSSL - canonStr := strings.Join(strings.Fields( - strings.TrimSpace(strings.ToLower(value))), " ") - // Then it changes everything to UTF8 strings - rdnSet[i].Value = asn1.RawValue{Tag: asn1.TagUTF8String, Bytes: []byte(canonStr)} - - } - } - - // Finally, OpenSSL drops the initial sequence tag - // so we marshal all the RDNs separately instead of as a group. - for _, canonRdn := range r { - b, err := asn1.Marshal(canonRdn) - if err != nil { - continue - } - canonBytes = append(canonBytes, b...) - } - - issuerHash := sha1.Sum(canonBytes) - // Openssl takes the first 4 bytes and encodes them as a little endian - // uint32 and then uses the hex to make the file name. - // In C++, this would be: - // (((unsigned long)md[0]) | ((unsigned long)md[1] << 8L) | - // ((unsigned long)md[2] << 16L) | ((unsigned long)md[3] << 24L) - // ) & 0xffffffffL; - fileHash := binary.LittleEndian.Uint32(issuerHash[0:4]) - return fmt.Sprintf("%08x", fileHash) -} - -// checkRevocation checks the connection for revoked certificates based on RFC5280. -// This implementation has the following major limitations: -// - Indirect CRL files are not supported. -// - CRL loading is only supported from directories in the X509_LOOKUP_hash_dir format. -// - OnlySomeReasons is not supported. -// - Delta CRL files are not supported. -// - Certificate CRLDistributionPoint must be URLs, but are then ignored and converted into a file path. -// - CRL checks are done after path building, which goes against RFC4158. -func checkRevocation(conn tls.ConnectionState, cfg RevocationOptions) error { - return checkChainRevocation(conn.VerifiedChains, cfg) -} - // checkChainRevocation checks the verified certificate chain // for revoked certificates based on RFC5280. func checkChainRevocation(verifiedChains [][]*x509.Certificate, cfg RevocationOptions) error { @@ -228,11 +135,6 @@ func checkChainRevocation(verifiedChains [][]*x509.Certificate, cfg RevocationOp continue case RevocationUndetermined: count[RevocationUndetermined]++ - // TODO(gtcooke94) Remove when deprecated AllowUndetermined is removed - // For now, if the deprecated value is explicitly set, use it - if cfg.AllowUndetermined { - cfg.DenyUndetermined = !cfg.AllowUndetermined - } if cfg.DenyUndetermined { continue } @@ -267,61 +169,23 @@ func checkChain(chain []*x509.Certificate, cfg RevocationOptions) revocationStat return chainStatus } -func cachedCrl(rawIssuer []byte, cache Cache) (*CRL, bool) { - val, ok := cache.Get(hex.EncodeToString(rawIssuer)) - if !ok { - return nil, false +func fetchCRL(c *x509.Certificate, crlVerifyCrt []*x509.Certificate, cfg RevocationOptions) (*CRL, error) { + if cfg.CRLProvider == nil { + return nil, fmt.Errorf("trying to fetch CRL but CRLProvider is nil") } - crl, ok := val.(*CRL) - if !ok { - return nil, false - } - // If the CRL is expired, force a reload. - 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 RevocationOptions) (*CRL, error) { - if cfg.Cache != nil { - if crl, ok := cachedCrl(rawIssuer, cfg.Cache); ok { - return crl, nil - } - } - - crl, err := fetchCRLOpenSSLHashDir(rawIssuer, cfg) + crl, err := cfg.CRLProvider.CRL(c) if err != nil { - return nil, fmt.Errorf("fetchCRL() failed: %v", err) + return nil, fmt.Errorf("CrlProvider failed err = %v", err) + } + if crl == nil { + return nil, fmt.Errorf("no CRL found for certificate's issuer") } - if err := verifyCRL(crl, crlVerifyCrt); err != nil { return nil, fmt.Errorf("verifyCRL() failed: %v", err) } - if cfg.Cache != nil { - cfg.Cache.Add(hex.EncodeToString(rawIssuer), crl) - } return crl, nil } -func fetchCRL(c *x509.Certificate, crlVerifyCrt []*x509.Certificate, cfg RevocationOptions) (*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") - } - if err := verifyCRL(crl, crlVerifyCrt); err != nil { - return nil, fmt.Errorf("verifyCRL() failed: %v", err) - } - 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 (or a CRLProvider) specified by cfg. If we can't load (and verify - @@ -501,57 +365,6 @@ func parseCRLExtensions(c *x509.RevocationList) (*CRL, error) { return certList, nil } -func fetchCRLOpenSSLHashDir(rawIssuer []byte, cfg RevocationOptions) (*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. - for i := 0; ; i++ { - // Unmarshal to RDNSeqence according to http://go/godoc/crypto/x509/pkix/#Name. - var r pkix.RDNSequence - rest, err := asn1.Unmarshal(rawIssuer, &r) - if len(rest) != 0 || err != nil { - return nil, fmt.Errorf("asn1.Unmarshal(Issuer) len(rest) = %d failed: %v", len(rest), err) - } - crlPath := fmt.Sprintf("%s.r%d", filepath.Join(cfg.RootDir, x509NameHash(r)), i) - crlBytes, err := os.ReadFile(crlPath) - if err != nil { - // Break when we can't read a CRL file. - grpclogLogger.Infof("readFile: %v", err) - break - } - - crl, err := parseRevocationList(crlBytes) - if err != nil { - // Parsing errors for a CRL shouldn't happen so fail. - return nil, fmt.Errorf("parseRevocationList(%v) failed: %v", crlPath, err) - } - 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 - continue - } - - rawCRLIssuer, err := extractCRLIssuer(crlBytes) - if err != nil { - return nil, err - } - certList.rawIssuer = rawCRLIssuer - // RFC5280, 6.3.3 (b) Verify the issuer and scope of the complete CRL. - if bytes.Equal(rawIssuer, rawCRLIssuer) { - parsedCRL = certList - // Continue to find the highest number in the .rN suffix. - continue - } - } - - if parsedCRL == nil { - return nil, fmt.Errorf("fetchCrls no CRLs found for issuer") - } - return parsedCRL, nil -} - func verifyCRL(crl *CRL, chain []*x509.Certificate) error { // RFC5280, 6.3.3 (f) Obtain and validate 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 @@ -598,6 +411,7 @@ func extractCRLIssuer(crlBytes []byte) ([]byte, error) { } der := cryptobyte.String(crlBytes) var issuer cryptobyte.String + // This doubled der.ReadASN1 is intentional, it modifies the input buffer if !der.ReadASN1(&der, cbasn1.SEQUENCE) || !der.ReadASN1(&der, cbasn1.SEQUENCE) || !der.SkipOptionalASN1(cbasn1.INTEGER) || @@ -608,10 +422,6 @@ func extractCRLIssuer(crlBytes []byte) ([]byte, error) { return issuer, nil } -func hasExpired(crl *x509.RevocationList, now time.Time) bool { - return !now.Before(crl.NextUpdate) -} - // parseRevocationList comes largely from here // x509.go: // https://github.com/golang/go/blob/e2f413402527505144beea443078649380e0c545/src/crypto/x509/x509.go#L1669-L1690 diff --git a/security/advancedtls/crl_test.go b/security/advancedtls/crl_test.go index 8ac9b63df..1c7b95cec 100644 --- a/security/advancedtls/crl_test.go +++ b/security/advancedtls/crl_test.go @@ -29,7 +29,6 @@ import ( "crypto/x509" "crypto/x509/pkix" "encoding/asn1" - "encoding/hex" "encoding/pem" "fmt" "math/big" @@ -40,90 +39,9 @@ import ( "testing" "time" - lru "github.com/hashicorp/golang-lru" "google.golang.org/grpc/security/advancedtls/testdata" ) -func TestX509NameHash(t *testing.T) { - nameTests := []struct { - in pkix.Name - out string - }{ - { - in: pkix.Name{ - Country: []string{"US"}, - Organization: []string{"Example"}, - }, - out: "9cdd41ff", - }, - { - in: pkix.Name{ - Country: []string{"us"}, - Organization: []string{"example"}, - }, - out: "9cdd41ff", - }, - { - in: pkix.Name{ - Country: []string{" us"}, - Organization: []string{"example"}, - }, - out: "9cdd41ff", - }, - { - in: pkix.Name{ - Country: []string{"US"}, - Province: []string{"California"}, - Locality: []string{"Mountain View"}, - Organization: []string{"BoringSSL"}, - }, - out: "c24414d9", - }, - { - in: pkix.Name{ - Country: []string{"US"}, - Province: []string{"California"}, - Locality: []string{"Mountain View"}, - Organization: []string{"BoringSSL"}, - }, - out: "c24414d9", - }, - { - in: pkix.Name{ - SerialNumber: "87f4514475ba0a2b", - }, - out: "9dc713cd", - }, - { - in: pkix.Name{ - Country: []string{"US"}, - Province: []string{"California"}, - Locality: []string{"Mountain View"}, - Organization: []string{"Google LLC"}, - OrganizationalUnit: []string{"Production", "campus-sln"}, - CommonName: "Root CA (2021-02-02T07:30:36-08:00)", - }, - out: "0b35a562", - }, - { - in: pkix.Name{ - ExtraNames: []pkix.AttributeTypeAndValue{ - {Type: asn1.ObjectIdentifier{5, 5, 5, 5}, Value: "aaaa"}, - }, - }, - out: "eea339da", - }, - } - for _, tt := range nameTests { - t.Run(tt.in.String(), func(t *testing.T) { - h := x509NameHash(tt.in.ToRDNSequence()) - if h != tt.out { - t.Errorf("x509NameHash(%v): Got %v wanted %v", tt.in, h, tt.out) - } - }) - } -} - func TestUnsupportedCRLs(t *testing.T) { crlBytesSomeReasons := []byte(`-----BEGIN X509 CRL----- MIIEeDCCA2ACAQEwDQYJKoZIhvcNAQELBQAwQjELMAkGA1UEBhMCVVMxHjAcBgNV @@ -345,98 +263,8 @@ func loadCRL(t *testing.T, path string) *CRL { return crl } -func TestCachedCRL(t *testing.T) { - cache, err := lru.New(5) - if err != nil { - t.Fatalf("lru.New: err = %v", err) - } - - tests := []struct { - desc string - val any - ok bool - }{ - { - desc: "Valid", - val: &CRL{ - certList: &x509.RevocationList{ - NextUpdate: time.Now().Add(time.Hour), - }}, - ok: true, - }, - { - desc: "Expired", - val: &CRL{ - certList: &x509.RevocationList{ - NextUpdate: time.Now().Add(-time.Hour), - }}, - ok: false, - }, - { - desc: "Wrong Type", - val: "string", - ok: false, - }, - { - desc: "Empty", - val: nil, - ok: false, - }, - } - for _, tt := range tests { - t.Run(tt.desc, func(t *testing.T) { - if tt.val != nil { - cache.Add(hex.EncodeToString([]byte(tt.desc)), tt.val) - } - _, ok := cachedCrl([]byte(tt.desc), cache) - if tt.ok != ok { - t.Errorf("Cache ok error expected %v vs %v", tt.ok, ok) - } - }) - } -} - -func TestGetIssuerCRLCache(t *testing.T) { - cache, err := lru.New(5) - if err != nil { - t.Fatalf("lru.New: err = %v", err) - } - - tests := []struct { - desc string - rawIssuer []byte - certs []*x509.Certificate - }{ - { - desc: "Valid", - rawIssuer: makeChain(t, testdata.Path("crl/unrevoked.pem"))[1].RawIssuer, - certs: makeChain(t, testdata.Path("crl/unrevoked.pem")), - }, - { - desc: "Unverified", - rawIssuer: makeChain(t, testdata.Path("crl/unrevoked.pem"))[1].RawIssuer, - }, - { - desc: "Not Found", - rawIssuer: []byte("not_found"), - }, - } - - for _, tt := range tests { - t.Run(tt.desc, func(t *testing.T) { - cache.Purge() - _, err := fetchIssuerCRL(tt.rawIssuer, tt.certs, RevocationOptions{ - RootDir: testdata.Path("."), - Cache: cache, - }) - if err == nil && cache.Len() == 0 { - t.Error("Verified CRL not added to cache") - } - if err != nil && cache.Len() != 0 { - t.Error("Unverified CRL added to cache") - } - }) - } +func checkRevocation(conn tls.ConnectionState, cfg RevocationOptions) error { + return checkChainRevocation(conn.VerifiedChains, cfg) } func TestVerifyCrl(t *testing.T) { @@ -531,7 +359,6 @@ func TestRevokedCert(t *testing.T) { revokedIntChain := makeChain(t, testdata.Path("crl/revokedInt.pem")) revokedLeafChain := makeChain(t, testdata.Path("crl/revokedLeaf.pem")) validChain := makeChain(t, testdata.Path("crl/unrevoked.pem")) - cache, err := lru.New(5) rawCRLs := make([][]byte, 6) for i := 1; i <= 6; i++ { rawCRL, err := os.ReadFile(testdata.Path(fmt.Sprintf("crl/%d.crl", i))) @@ -540,10 +367,12 @@ func TestRevokedCert(t *testing.T) { } rawCRLs = append(rawCRLs, rawCRL) } - cRLProvider := NewStaticCRLProvider(rawCRLs) + staticCRLProvider := NewStaticCRLProvider(rawCRLs) + directoryCRLProvider, err := NewFileWatcherCRLProvider(FileWatcherOptions{CRLDirectory: testdata.Path("crl")}) if err != nil { - t.Fatalf("lru.New: err = %v", err) + t.Fatalf("NewFileWatcherCRLProvider: err = %v", err) } + defer directoryCRLProvider.Close() var revocationTests = []struct { desc string @@ -599,11 +428,10 @@ func TestRevokedCert(t *testing.T) { } for _, tt := range revocationTests { - t.Run(fmt.Sprintf("%v with x509 crl hash dir", tt.desc), func(t *testing.T) { + t.Run(fmt.Sprintf("%v with x509 crl dir", tt.desc), func(t *testing.T) { err := checkRevocation(tt.in, RevocationOptions{ - RootDir: testdata.Path("crl"), + CRLProvider: directoryCRLProvider, DenyUndetermined: tt.denyUndetermined, - Cache: cache, }) t.Logf("checkRevocation err = %v", err) if tt.revoked && err == nil { @@ -615,7 +443,7 @@ func TestRevokedCert(t *testing.T) { t.Run(fmt.Sprintf("%v with static provider", tt.desc), func(t *testing.T) { err := checkRevocation(tt.in, RevocationOptions{ DenyUndetermined: tt.denyUndetermined, - CRLProvider: cRLProvider, + CRLProvider: staticCRLProvider, }) t.Logf("checkRevocation err = %v", err) if tt.revoked && err == nil { @@ -728,17 +556,22 @@ func TestVerifyConnection(t *testing.T) { t.Fatalf("templ.CreateRevocationList failed err = %v", err) } - err = os.WriteFile(path.Join(dir, fmt.Sprintf("%s.r0", x509NameHash(cert.Subject.ToRDNSequence()))), crl, 0777) + err = os.WriteFile(path.Join(dir, fmt.Sprintf("%s.r0", cert.Subject.ToRDNSequence())), crl, 0777) if err != nil { t.Fatalf("os.WriteFile failed err = %v", err) } cp := x509.NewCertPool() cp.AddCert(cert) + provider, err := NewFileWatcherCRLProvider(FileWatcherOptions{CRLDirectory: dir}) + if err != nil { + t.Errorf("NewFileWatcherCRLProvider: err = %v", err) + } + defer provider.Close() cliCfg := tls.Config{ RootCAs: cp, VerifyConnection: func(cs tls.ConnectionState) error { - return checkRevocation(cs, RevocationOptions{RootDir: dir}) + return checkRevocation(cs, RevocationOptions{CRLProvider: provider}) }, } conn, err := tls.Dial(lis.Addr().Network(), lis.Addr().String(), &cliCfg) @@ -755,54 +588,3 @@ func TestVerifyConnection(t *testing.T) { }) } } - -func TestIssuerNonPrintableString(t *testing.T) { - rawIssuer, err := hex.DecodeString("300c310a300806022a030c023a29") - if err != nil { - t.Fatalf("failed to decode issuer: %s", err) - } - _, err = fetchCRLOpenSSLHashDir(rawIssuer, RevocationOptions{RootDir: testdata.Path("crl")}) - if err != nil { - t.Fatalf("fetchCRL failed: %s", err) - } -} - -// TestCRLCacheExpirationReloading tests the basic expiration and reloading of a -// cached CRL. The setup places an empty CRL in the cache, and a corresponding -// CRL with a revocation in the CRL directory. We then validate the certificate -// to verify that the certificate is not revoked. Then, we modify the -// NextUpdate time to be in the past so that when we next check for revocation, -// the existing cache entry should be seen as expired, and the CRL in the -// directory showing `revokedInt.pem` as revoked will be loaded, resulting in -// the check returning `RevocationRevoked`. -func TestCRLCacheExpirationReloading(t *testing.T) { - cache, err := lru.New(5) - if err != nil { - t.Fatalf("Creating cache failed") - } - - var certs = makeChain(t, testdata.Path("crl/revokedInt.pem")) - // Certs[1] has the same issuer as the revoked cert - rawIssuer := certs[1].RawIssuer - - // `3.crl`` revokes `revokedInt.pem` - crl := loadCRL(t, testdata.Path("crl/3.crl")) - // Modify the crl so that the cert is NOT revoked and add it to the cache - crl.certList.RevokedCertificateEntries = nil - crl.certList.NextUpdate = time.Now().Add(time.Hour) - cache.Add(hex.EncodeToString(rawIssuer), crl) - var cfg = RevocationOptions{RootDir: testdata.Path("crl"), Cache: cache} - revocationStatus := checkChain(certs, cfg) - if revocationStatus != RevocationUnrevoked { - t.Fatalf("Certificate check should be RevocationUnrevoked, was %v", revocationStatus) - } - - // Modify the entry in the cache so that the cache will be refreshed - crl.certList.NextUpdate = time.Now() - cache.Add(hex.EncodeToString(rawIssuer), crl) - - revocationStatus = checkChain(certs, cfg) - if revocationStatus != RevocationRevoked { - t.Fatalf("A certificate should have been `RevocationRevoked` but was %v", revocationStatus) - } -} diff --git a/security/advancedtls/examples/credential_reloading_from_files/client/main.go b/security/advancedtls/examples/credential_reloading_from_files/client/main.go index 810a8bf57..854987fa3 100644 --- a/security/advancedtls/examples/credential_reloading_from_files/client/main.go +++ b/security/advancedtls/examples/credential_reloading_from_files/client/main.go @@ -72,7 +72,7 @@ func main() { if err != nil { log.Fatalf("pemfile.NewProvider(%v) failed: %v", rootOptions, err) } - options := &advancedtls.ClientOptions{ + options := &advancedtls.Options{ IdentityOptions: advancedtls.IdentityCertificateOptions{ IdentityProvider: identityProvider, }, diff --git a/security/advancedtls/examples/credential_reloading_from_files/server/main.go b/security/advancedtls/examples/credential_reloading_from_files/server/main.go index 16d389860..140c67747 100644 --- a/security/advancedtls/examples/credential_reloading_from_files/server/main.go +++ b/security/advancedtls/examples/credential_reloading_from_files/server/main.go @@ -76,7 +76,7 @@ func main() { defer rootProvider.Close() // Start a server and create a client using advancedtls API with Provider. - options := &advancedtls.ServerOptions{ + options := &advancedtls.Options{ IdentityOptions: advancedtls.IdentityCertificateOptions{ IdentityProvider: identityProvider, }, diff --git a/security/advancedtls/examples/go.sum b/security/advancedtls/examples/go.sum index 3965e085d..053a35faa 100644 --- a/security/advancedtls/examples/go.sum +++ b/security/advancedtls/examples/go.sum @@ -1,7 +1,5 @@ github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= -github.com/hashicorp/golang-lru v0.5.4 h1:YDjusn29QI/Das2iO9M0BHnIbxPeyuCHsjMW+lJfyTc= -github.com/hashicorp/golang-lru v0.5.4/go.mod h1:iADmTwqILo4mZ8BN3D2Q6+9jd8WM5uGBxy+E8yxSoD4= golang.org/x/crypto v0.23.0 h1:dIJU/v2J8Mdglj/8rJ6UUOM3Zc9zLZxVZwwxMooUSAI= golang.org/x/crypto v0.23.0/go.mod h1:CKFgDieR+mRhux2Lsu27y0fO304Db0wZe70UKqHu0v8= golang.org/x/net v0.25.0 h1:d/OCCoBEUq33pjydKrGQhw7IlUPI2Oylr+8qLx49kac= diff --git a/security/advancedtls/go.mod b/security/advancedtls/go.mod index 461a311e7..6e300bf8c 100644 --- a/security/advancedtls/go.mod +++ b/security/advancedtls/go.mod @@ -4,7 +4,6 @@ go 1.21 require ( github.com/google/go-cmp v0.6.0 - github.com/hashicorp/golang-lru v0.5.4 golang.org/x/crypto v0.23.0 google.golang.org/grpc v1.64.0 google.golang.org/grpc/examples v0.0.0-20201112215255-90f1b3ee835b diff --git a/security/advancedtls/go.sum b/security/advancedtls/go.sum index 3965e085d..053a35faa 100644 --- a/security/advancedtls/go.sum +++ b/security/advancedtls/go.sum @@ -1,7 +1,5 @@ github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= -github.com/hashicorp/golang-lru v0.5.4 h1:YDjusn29QI/Das2iO9M0BHnIbxPeyuCHsjMW+lJfyTc= -github.com/hashicorp/golang-lru v0.5.4/go.mod h1:iADmTwqILo4mZ8BN3D2Q6+9jd8WM5uGBxy+E8yxSoD4= golang.org/x/crypto v0.23.0 h1:dIJU/v2J8Mdglj/8rJ6UUOM3Zc9zLZxVZwwxMooUSAI= golang.org/x/crypto v0.23.0/go.mod h1:CKFgDieR+mRhux2Lsu27y0fO304Db0wZe70UKqHu0v8= golang.org/x/net v0.25.0 h1:d/OCCoBEUq33pjydKrGQhw7IlUPI2Oylr+8qLx49kac=