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

TLSSNIRevalidation=false should not allow issuance with a valid pending_authzs #4118

Closed
jkarner opened this issue Mar 15, 2019 · 1 comment

Comments

Projects
None yet
2 participants
@jkarner
Copy link

commented Mar 15, 2019

TLSSNIRevalidation has been set to false in the boulder production environment, but there are still a small amount of successful issuances using the TLS-SNI-01 challenge type.

It looks like users that successfully posted to /acme/new-authz but didn't complete the challenge before fully disabling TLS-SNI-01 are able to complete issuance.

@jsha

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

This is an interesting one. It turns out it was fixed recently in #4114, which will go out in the next release.

Here's the code that was supposed to prevent pending challenges from being validated if their challenge type was disabled since they were created:

boulder/ra/ra.go

Lines 1436 to 1452 in 57fc8a4

// If TLSSNIRevalidation is enabled, find out whether this was a revalidation
// (previous certificate existed) or not. If it is a revalidation, we can
// proceed with validation even though the challenge type is currently
// disabled.
if !ra.PA.ChallengeTypeEnabled(ch.Type, authz.RegistrationID) && features.Enabled(features.TLSSNIRevalidation) {
existsResp, err := ra.SA.PreviousCertificateExists(ctx, &sapb.PreviousCertificateExistsRequest{
Domain: &authz.Identifier.Value,
RegID: &authz.RegistrationID,
})
if err != nil {
return nil, err
}
if !*existsResp.Exists {
return nil,
berrors.MalformedError("challenge type %q no longer allowed", ch.Type)
}
}

In particular this:

berrors.MalformedError("challenge type %q no longer allowed", ch.Type)

Was gated by features.TLSSNIRevalidation being true. So disabling that flag meant we could no longer hit this check.

There was a unittest that was supposed to test this path:

boulder/ra/ra_test.go

Lines 3618 to 3644 in 57fc8a4

func TestPerformValidationBadChallengeType(t *testing.T) {
_, _, ra, fc, cleanUp := initAuthorities(t)
defer cleanUp()
pa, err := policy.New(map[string]bool{})
test.AssertNotError(t, err, "Couldn't create PA")
ra.PA = pa
exp := fc.Now().Add(10 * time.Hour)
authz := core.Authorization{
Challenges: []core.Challenge{
core.Challenge{
Status: core.StatusValid,
Type: core.ChallengeTypeTLSSNI01},
},
Expires: &exp,
}
authzPB, err := bgrpc.AuthzToPB(authz)
test.AssertNotError(t, err, "AuthzToPB failed")
var challIndex int64
_, err = ra.PerformValidation(context.Background(), &rapb.PerformValidationRequest{
Authz: authzPB,
ChallengeIndex: &challIndex,
})
test.AssertError(t, err, "ra.PerformValidation allowed a update to a authorization")
test.AssertEquals(t, err.Error(), "challenge type \"tls-sni-01\" no longer allowed")
}
. That test was passing.

However, that test was unexpectedly running with TLSSNIRevalidation=true, because a previous test had set the feature flag and forgot to call defer features.Reset():

boulder/ra/ra_test.go

Lines 3538 to 3540 in 57fc8a4

_ = features.Set(map[string]bool{
"TLSSNIRevalidation": true,
})

So the test was always passing even though it shouldn't have. Since #4114 deleted the offending test case, there's no cleanup to do there. However I've filed #4120 to change how we call features.Reset in our unittests so we are less likely to miss cases like this in the future.

@jsha jsha closed this Mar 15, 2019

jsha added a commit that referenced this issue Mar 20, 2019

Put features.Reset in unitest setup functions.
Previously we relied on each instance of `features.Set` to have a
corresponding `defer features.Reset()`. If we forget that, we can wind
up with unexpected behavior where features set in one test case leak
into another test case. This led to the bug in
#4118 going undetected.

rolandshoemaker added a commit that referenced this issue Apr 2, 2019

Put features.Reset in unitest setup functions. (#4129)
Previously we relied on each instance of `features.Set` to have a
corresponding `defer features.Reset()`. If we forget that, we can wind
up with unexpected behavior where features set in one test case leak
into another test case. This led to the bug in
#4118 going undetected.

Fix #4120
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.