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

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

Merged
merged 2 commits into from
May 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions security/advancedtls/advancedtls_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -409,15 +409,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,
}
}

Expand Down Expand Up @@ -740,18 +740,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
Expand Down Expand Up @@ -795,7 +795,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,
Expand Down
16 changes: 13 additions & 3 deletions security/advancedtls/crl.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to keep the current 'positive boolean' approach, potentially with 'default true' mechanism like
`
type MyConfig struct {
AllowChanges bool
}

func NewMyConfig() MyConfig {
return MyConfig{
AllowChanges: true,
}
}`
Leaving it to you&Doug to decide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like this because it requires adding a constructor layer for a relatively simple struct - in this case IMO the positive boolean approach isn't worth the complexity

@dfawley WDYT?

Copy link
Member

@dfawley dfawley May 6, 2024

Choose a reason for hiding this comment

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

Yeah there's unfortunately some conflict between having positive booleans and having default behavior = zero value.

Maybe DenyUndetermined is not too negative? It states what you will be doing, not what you will not be doing. Or we could go with something more verbose that removes the double-negative like RequireDetermined[Revocation]Status?

Copy link
Member

Choose a reason for hiding this comment

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

BTW I'm fine either way. Another option could also be to replace with an "enum" (dumb Go doesn't actually have them, but):

type RevocationStatusRequirement int

const (
    DenyUndetermined RevocationStatusRequirement = 0
    AllowUndetermined
)

(Names are just for illustrative purposes; probably pick better names.)

// 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.
Expand Down Expand Up @@ -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)
Expand Down
24 changes: 12 additions & 12 deletions security/advancedtls/crl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
Loading