advancedTLS: Swap to DenyUndetermined from AllowUndetermined in revocation settings (#7179)

* swap to `DenyUndetermined` from `AllowUndetermined`
This commit is contained in:
Gregory Cooke 2024-05-06 13:40:28 -04:00 committed by GitHub
parent befc29de93
commit 4879d51a59
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 36 additions and 26 deletions

View File

@ -427,14 +427,14 @@ func (s) TestClientServerHandshake(t *testing.T) {
return &GetRootCAsResults{TrustCerts: cs.ServerTrust3}, nil 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) rawCRL, err := os.ReadFile(crlPath)
if err != nil { if err != nil {
t.Fatalf("readFile(%v) failed err = %v", crlPath, err) t.Fatalf("readFile(%v) failed err = %v", crlPath, err)
} }
cRLProvider := NewStaticCRLProvider([][]byte{rawCRL}) cRLProvider := NewStaticCRLProvider([][]byte{rawCRL})
return &RevocationOptions{ return &RevocationOptions{
AllowUndetermined: allowUndetermined, DenyUndetermined: denyUndetermined,
CRLProvider: cRLProvider, CRLProvider: cRLProvider,
} }
} }
@ -759,7 +759,7 @@ func (s) TestClientServerHandshake(t *testing.T) {
clientVerificationType: CertVerification, clientVerificationType: CertVerification,
clientRevocationOptions: &RevocationOptions{ clientRevocationOptions: &RevocationOptions{
RootDir: testdata.Path("crl"), RootDir: testdata.Path("crl"),
AllowUndetermined: true, DenyUndetermined: false,
Cache: cache, Cache: cache,
}, },
serverMutualTLS: true, serverMutualTLS: true,
@ -768,7 +768,7 @@ func (s) TestClientServerHandshake(t *testing.T) {
serverVerificationType: CertVerification, serverVerificationType: CertVerification,
serverRevocationOptions: &RevocationOptions{ serverRevocationOptions: &RevocationOptions{
RootDir: testdata.Path("crl"), RootDir: testdata.Path("crl"),
AllowUndetermined: true, DenyUndetermined: false,
Cache: cache, Cache: cache,
}, },
}, },
@ -813,7 +813,7 @@ func (s) TestClientServerHandshake(t *testing.T) {
clientGetRoot: getRootCAsForClientCRL, clientGetRoot: getRootCAsForClientCRL,
clientVerifyFunc: clientVerifyFuncGood, clientVerifyFunc: clientVerifyFuncGood,
clientVerificationType: CertVerification, 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, serverMutualTLS: true,
serverCert: []tls.Certificate{cs.ServerCertForCRL}, serverCert: []tls.Certificate{cs.ServerCertForCRL},
serverGetRoot: getRootCAsForServerCRL, serverGetRoot: getRootCAsForServerCRL,

View File

@ -61,8 +61,13 @@ type RevocationOptions struct {
// Directory format must match OpenSSL X509_LOOKUP_hash_dir(3). // Directory format must match OpenSSL X509_LOOKUP_hash_dir(3).
// Deprecated: use CRLProvider instead. // Deprecated: use CRLProvider instead.
RootDir string RootDir string
// DenyUndetermined controls if certificate chains with RevocationUndetermined
// revocation status are allowed to complete.
DenyUndetermined bool
// AllowUndetermined controls if certificate chains with RevocationUndetermined // AllowUndetermined controls if certificate chains with RevocationUndetermined
// revocation status are allowed to complete. // revocation status are allowed to complete.
//
// Deprecated: use DenyUndetermined instead
AllowUndetermined bool AllowUndetermined bool
// Cache will store CRL files if not nil, otherwise files are reloaded for every lookup. // 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. // Only used for caching CRLs when using the RootDir setting.
@ -222,12 +227,17 @@ func checkChainRevocation(verifiedChains [][]*x509.Certificate, cfg RevocationOp
count[RevocationRevoked]++ count[RevocationRevoked]++
continue continue
case RevocationUndetermined: case RevocationUndetermined:
if cfg.AllowUndetermined {
return nil
}
count[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 continue
} }
return nil
}
} }
return fmt.Errorf("no unrevoked chains found: %v", count) return fmt.Errorf("no unrevoked chains found: %v", count)
} }

View File

@ -549,7 +549,7 @@ func TestRevokedCert(t *testing.T) {
desc string desc string
in tls.ConnectionState in tls.ConnectionState
revoked bool revoked bool
allowUndetermined bool denyUndetermined bool
}{ }{
{ {
desc: "Single unrevoked", desc: "Single unrevoked",
@ -587,6 +587,7 @@ func TestRevokedCert(t *testing.T) {
{&x509.Certificate{CRLDistributionPoints: []string{"test"}}}, {&x509.Certificate{CRLDistributionPoints: []string{"test"}}},
}}, }},
revoked: true, revoked: true,
denyUndetermined: true,
}, },
{ {
desc: "Undetermined allowed", desc: "Undetermined allowed",
@ -594,7 +595,6 @@ func TestRevokedCert(t *testing.T) {
{&x509.Certificate{CRLDistributionPoints: []string{"test"}}}, {&x509.Certificate{CRLDistributionPoints: []string{"test"}}},
}}, }},
revoked: false, revoked: false,
allowUndetermined: true,
}, },
} }
@ -602,7 +602,7 @@ func TestRevokedCert(t *testing.T) {
t.Run(fmt.Sprintf("%v with x509 crl hash dir", tt.desc), func(t *testing.T) { t.Run(fmt.Sprintf("%v with x509 crl hash dir", tt.desc), func(t *testing.T) {
err := checkRevocation(tt.in, RevocationOptions{ err := checkRevocation(tt.in, RevocationOptions{
RootDir: testdata.Path("crl"), RootDir: testdata.Path("crl"),
AllowUndetermined: tt.allowUndetermined, DenyUndetermined: tt.denyUndetermined,
Cache: cache, Cache: cache,
}) })
t.Logf("checkRevocation err = %v", err) t.Logf("checkRevocation err = %v", err)
@ -614,7 +614,7 @@ func TestRevokedCert(t *testing.T) {
}) })
t.Run(fmt.Sprintf("%v with static provider", tt.desc), func(t *testing.T) { t.Run(fmt.Sprintf("%v with static provider", tt.desc), func(t *testing.T) {
err := checkRevocation(tt.in, RevocationOptions{ err := checkRevocation(tt.in, RevocationOptions{
AllowUndetermined: tt.allowUndetermined, DenyUndetermined: tt.denyUndetermined,
CRLProvider: cRLProvider, CRLProvider: cRLProvider,
}) })
t.Logf("checkRevocation err = %v", err) t.Logf("checkRevocation err = %v", err)