advancedTLS: Swap to DenyUndetermined from AllowUndetermined in revoc…

…ation settings (#7179) * swap to `DenyUndetermined` from `AllowUndetermined`
grpc · May 6, 2024 · 4879d51 · 4879d51
1 parent befc29d
commit 4879d51
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 25 deletions.
diff --git a/security/advancedtls/advancedtls_test.go b/security/advancedtls/advancedtls_test.go
@@ -427,15 +427,15 @@ func (s) TestClientServerHandshake(t *testing.T) {
 		return &GetRootCAsResults{TrustCerts: cs.ServerTrust3}, nil
 	}
 
-	makeStaticCRLRevocationOptions := func(crlPath string, allowUndetermined bool) *RevocationOptions {
+	makeStaticCRLRevocationOptions := func(crlPath string, denyUndetermined bool) *RevocationOptions {
 		rawCRL, err := os.ReadFile(crlPath)
 		if err != nil {
 			t.Fatalf("readFile(%v) failed err = %v", crlPath, err)
 		}
 		cRLProvider := NewStaticCRLProvider([][]byte{rawCRL})
 		return &RevocationOptions{
-			AllowUndetermined: allowUndetermined,
-			CRLProvider:       cRLProvider,
+			DenyUndetermined: denyUndetermined,
+			CRLProvider:      cRLProvider,
 		}
 	}
 
@@ -758,18 +758,18 @@ func (s) TestClientServerHandshake(t *testing.T) {
 			clientVerifyFunc:       clientVerifyFuncGood,
 			clientVerificationType: CertVerification,
 			clientRevocationOptions: &RevocationOptions{
-				RootDir:           testdata.Path("crl"),
-				AllowUndetermined: true,
-				Cache:             cache,
+				RootDir:          testdata.Path("crl"),
+				DenyUndetermined: false,
+				Cache:            cache,
 			},
 			serverMutualTLS:        true,
 			serverCert:             []tls.Certificate{cs.ServerCert1},
 			serverGetRoot:          getRootCAsForServer,
 			serverVerificationType: CertVerification,
 			serverRevocationOptions: &RevocationOptions{
-				RootDir:           testdata.Path("crl"),
-				AllowUndetermined: true,
-				Cache:             cache,
+				RootDir:          testdata.Path("crl"),
+				DenyUndetermined: false,
+				Cache:            cache,
 			},
 		},
 		// Client: set valid credentials with the revocation config
@@ -813,7 +813,7 @@ func (s) TestClientServerHandshake(t *testing.T) {
 			clientGetRoot:           getRootCAsForClientCRL,
 			clientVerifyFunc:        clientVerifyFuncGood,
 			clientVerificationType:  CertVerification,
-			clientRevocationOptions: makeStaticCRLRevocationOptions(testdata.Path("crl/provider_malicious_crl_empty.pem"), false),
+			clientRevocationOptions: makeStaticCRLRevocationOptions(testdata.Path("crl/provider_malicious_crl_empty.pem"), true),
 			serverMutualTLS:         true,
 			serverCert:              []tls.Certificate{cs.ServerCertForCRL},
 			serverGetRoot:           getRootCAsForServerCRL,

diff --git a/security/advancedtls/crl.go b/security/advancedtls/crl.go
@@ -61,8 +61,13 @@ type RevocationOptions struct {
 	// 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.
@@ -222,11 +227,16 @@ func checkChainRevocation(verifiedChains [][]*x509.Certificate, cfg RevocationOp
 			count[RevocationRevoked]++
 			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 {
-				return nil
+				cfg.DenyUndetermined = !cfg.AllowUndetermined
 			}
-			count[RevocationUndetermined]++
-			continue
+			if cfg.DenyUndetermined {
+				continue
+			}
+			return nil
 		}
 	}
 	return fmt.Errorf("no unrevoked chains found: %v", count)

diff --git a/security/advancedtls/crl_test.go b/security/advancedtls/crl_test.go
@@ -546,10 +546,10 @@ func TestRevokedCert(t *testing.T) {
 	}
 
 	var revocationTests = []struct {
-		desc              string
-		in                tls.ConnectionState
-		revoked           bool
-		allowUndetermined bool
+		desc             string
+		in               tls.ConnectionState
+		revoked          bool
+		denyUndetermined bool
 	}{
 		{
 			desc:    "Single unrevoked",
@@ -586,24 +586,24 @@ func TestRevokedCert(t *testing.T) {
 			in: tls.ConnectionState{VerifiedChains: [][]*x509.Certificate{
 				{&x509.Certificate{CRLDistributionPoints: []string{"test"}}},
 			}},
-			revoked: true,
+			revoked:          true,
+			denyUndetermined: true,
 		},
 		{
 			desc: "Undetermined allowed",
 			in: tls.ConnectionState{VerifiedChains: [][]*x509.Certificate{
 				{&x509.Certificate{CRLDistributionPoints: []string{"test"}}},
 			}},
-			revoked:           false,
-			allowUndetermined: true,
+			revoked: false,
 		},
 	}
 
 	for _, tt := range revocationTests {
 		t.Run(fmt.Sprintf("%v with x509 crl hash dir", tt.desc), func(t *testing.T) {
 			err := checkRevocation(tt.in, RevocationOptions{
-				RootDir:           testdata.Path("crl"),
-				AllowUndetermined: tt.allowUndetermined,
-				Cache:             cache,
+				RootDir:          testdata.Path("crl"),
+				DenyUndetermined: tt.denyUndetermined,
+				Cache:            cache,
 			})
 			t.Logf("checkRevocation err = %v", err)
 			if tt.revoked && err == nil {
@@ -614,8 +614,8 @@ func TestRevokedCert(t *testing.T) {
 		})
 		t.Run(fmt.Sprintf("%v with static provider", tt.desc), func(t *testing.T) {
 			err := checkRevocation(tt.in, RevocationOptions{
-				AllowUndetermined: tt.allowUndetermined,
-				CRLProvider:       cRLProvider,
+				DenyUndetermined: tt.denyUndetermined,
+				CRLProvider:      cRLProvider,
 			})
 			t.Logf("checkRevocation err = %v", err)
 			if tt.revoked && err == nil {