Skip to content

Commit dcd2b43

Browse files
rolandshoemakerrolandshoemaker
committed
Fix previous impl, add valid authz reuse fix and existing authz validation fix
1 parent 5ca646c commit dcd2b43

File tree

8 files changed

+87
-29
lines changed

8 files changed

+87
-29
lines changed

‎core/interfaces.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ type PolicyAuthority interface {
108108
WillingToIssue(domain AcmeIdentifier) error
109109
WillingToIssueWildcard(domain AcmeIdentifier) error
110110
ChallengesFor(domain AcmeIdentifier) (challenges []Challenge, validCombinations [][]int, err error)
111-
ChallengeStillAllowed(authz *Authorization) bool
111+
ChallengeTypeEnabled(t string) bool
112112
}
113113

114114
// StorageGetter are the Boulder SA's read-only methods

‎csr/csr_test.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,9 @@ func (pa *mockPA) WillingToIssue(id core.AcmeIdentifier) error {
3737
func (pa *mockPA) WillingToIssueWildcard(id core.AcmeIdentifier) error {
3838
return nil
3939
}
40+
func (pa *mockPA) ChallengeTypeEnabled(t string) bool {
41+
return true
42+
}
4043

4144
func TestVerifyCSR(t *testing.T) {
4245
private, err := rsa.GenerateKey(rand.Reader, 2048)

‎mocks/mocks.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -368,6 +368,13 @@ func (sa *StorageAuthority) GetValidAuthorizations(_ context.Context, regID int6
368368
Type: "dns",
369369
Value: name,
370370
},
371+
Challenges: []core.Challenge{
372+
{
373+
Status: core.StatusValid,
374+
ID: 23,
375+
Type: core.ChallengeTypeDNS01,
376+
},
377+
},
371378
}
372379
}
373380
}

‎policy/pa.go‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -454,13 +454,7 @@ func extractDomainIANASuffix(name string) (string, error) {
454454
return suffix, nil
455455
}
456456

457-
// AuthzStillValid checks if the challenge type that was used in a valid authorization
458-
// is still enabled
459-
func (pa *AuthorityImpl) ChallengeStillAllowed(authz *core.Authorization) bool {
460-
for _, chall := range authz.Challenges {
461-
if chall.Status == core.StatusValid {
462-
return pa.enabledChallenges[chall.Type]
463-
}
464-
}
465-
return false
457+
// ChallengeTypeEnabled returns whether the specified challenge type is enabled
458+
func (pa *AuthorityImpl) ChallengeTypeEnabled(t string) bool {
459+
return pa.enabledChallenges[t]
466460
}

‎policy/pa_test.go‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -461,13 +461,3 @@ func TestMalformedExactBlacklist(t *testing.T) {
461461
test.AssertError(t, err, "Loaded invalid exact blacklist content without error")
462462
test.AssertEquals(t, err.Error(), "Malformed exact blacklist entry, only one label: \"com\"")
463463
}
464-
465-
func TestChallengeStillAllowed(t *testing.T) {
466-
pa := paImpl(t)
467-
pa.enabledChallenges[core.ChallengeTypeHTTP01] = false
468-
test.Assert(t, !pa.ChallengeStillAllowed(&core.Authorization{}), "pa.ChallengeStillAllowed didn't fail with empty authorization")
469-
test.Assert(t, !pa.ChallengeStillAllowed(&core.Authorization{Challenges: []core.Challenge{{Status: core.StatusPending}}}), "pa.ChallengeStillAllowed didn't fail with no valid challenges")
470-
test.Assert(t, !pa.ChallengeStillAllowed(&core.Authorization{Challenges: []core.Challenge{{Status: core.StatusValid, Type: core.ChallengeTypeHTTP01}}}), "pa.ChallengeStillAllowed didn't fail with disabled challenge")
471-
472-
test.Assert(t, pa.ChallengeStillAllowed(&core.Authorization{Challenges: []core.Challenge{{Status: core.StatusValid, Type: core.ChallengeTypeTLSSNI01}}}), "pa.ChallengeStillAllowed failed with enabled challenge")
473-
}

‎ra/ra.go‎

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -532,13 +532,14 @@ func (ra *RegistrationAuthorityImpl) NewAuthorization(ctx context.Context, reque
532532
ra.log.Warning(fmt.Sprintf("%s: %s", outErr.Error(), existingAuthz.ID))
533533
return core.Authorization{}, outErr
534534
}
535-
536-
// The existing authorization must not expire within the next 24 hours for
537-
// it to be OK for reuse
538-
reuseCutOff := ra.clk.Now().Add(time.Hour * 24)
539-
if populatedAuthz.Expires.After(reuseCutOff) {
540-
ra.stats.Inc("ReusedValidAuthz", 1)
541-
return populatedAuthz, nil
535+
if ra.validChallengeStillGood(&populatedAuthz) {
536+
// The existing authorization must not expire within the next 24 hours for
537+
// it to be OK for reuse
538+
reuseCutOff := ra.clk.Now().Add(time.Hour * 24)
539+
if populatedAuthz.Expires.After(reuseCutOff) {
540+
ra.stats.Inc("ReusedValidAuthz", 1)
541+
return populatedAuthz, nil
542+
}
542543
}
543544
}
544545
}
@@ -721,7 +722,7 @@ func (ra *RegistrationAuthorityImpl) checkAuthorizationsCAA(
721722
// Ensure that CAA is rechecked for this name
722723
recheckNames = append(recheckNames, name)
723724
}
724-
if authz != nil && !ra.PA.ChallengeStillAllowed(authz) {
725+
if authz != nil && !ra.validChallengeStillGood(authz) {
725726
return berrors.UnauthorizedError("challenge used to validate authorization with ID %q no longer allowed", authz.ID)
726727
}
727728
}
@@ -1335,6 +1336,10 @@ func (ra *RegistrationAuthorityImpl) UpdateAuthorization(ctx context.Context, ba
13351336
// )
13361337
}
13371338

1339+
if !ra.PA.ChallengeTypeEnabled(ch.Type) {
1340+
return core.Authorization{}, berrors.MalformedError("challenge type %q no longer allowed", ch.Type)
1341+
}
1342+
13381343
// When configured with `reuseValidAuthz` we can expect some clients to try
13391344
// and update a challenge for an authorization that is already valid. In this
13401345
// case we don't need to process the challenge update. It wouldn't be helpful,
@@ -1751,3 +1756,14 @@ func (ra *RegistrationAuthorityImpl) createPendingAuthz(ctx context.Context, reg
17511756
authz.Combinations = comboBytes
17521757
return authz, nil
17531758
}
1759+
1760+
// validChallengeStillGood checks whether the valid challenge in an authorization uses a type
1761+
// which is still enabled
1762+
func (ra *RegistrationAuthorityImpl) validChallengeStillGood(authz *core.Authorization) bool {
1763+
for _, chall := range authz.Challenges {
1764+
if chall.Status == core.StatusValid {
1765+
return ra.PA.ChallengeTypeEnabled(chall.Type)
1766+
}
1767+
}
1768+
return false
1769+
}

‎ra/ra_test.go‎

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ var (
7171
SupportedChallenges = map[string]bool{
7272
core.ChallengeTypeHTTP01: true,
7373
core.ChallengeTypeTLSSNI01: true,
74+
core.ChallengeTypeDNS01: true,
7475
}
7576

7677
// These values we simulate from the client
@@ -622,6 +623,7 @@ func TestReuseValidAuthorization(t *testing.T) {
622623
exp := ra.clk.Now().Add(365 * 24 * time.Hour)
623624
finalAuthz.Expires = &exp
624625
finalAuthz.Challenges[0].Status = "valid"
626+
finalAuthz.Challenges[0].Type = core.ChallengeTypeHTTP01
625627
finalAuthz.RegistrationID = Registration.ID
626628
finalAuthz, err := sa.NewPendingAuthorization(ctx, finalAuthz)
627629
test.AssertNotError(t, err, "Could not store test pending authorization")
@@ -668,6 +670,21 @@ func TestReuseValidAuthorization(t *testing.T) {
668670
test.AssertNotError(t, err, "UpdateAuthorization on secondAuthz sni failed")
669671
test.AssertEquals(t, finalAuthz.ID, secondAuthz.ID)
670672
test.AssertEquals(t, secondAuthz.Status, core.StatusValid)
673+
674+
// Test that a valid authorization that used a challenge which has been disabled
675+
// is not reused
676+
pa, err := policy.New(map[string]bool{
677+
core.ChallengeTypeHTTP01: false,
678+
core.ChallengeTypeTLSSNI01: true,
679+
core.ChallengeTypeDNS01: true,
680+
})
681+
test.AssertNotError(t, err, "Couldn't create PA")
682+
err = pa.SetHostnamePolicyFile("../test/hostname-policy.json")
683+
test.AssertNotError(t, err, "Couldn't set hostname policy")
684+
ra.PA = pa
685+
newAuthz, err := ra.NewAuthorization(ctx, AuthzRequest, Registration.ID)
686+
test.AssertNotError(t, err, "NewAuthorization for secondAuthz failed")
687+
test.Assert(t, finalAuthz.ID != newAuthz.ID, "NewAuthorization reused a valid authz with a disabled challenge type")
671688
}
672689

673690
func TestReusePendingAuthorization(t *testing.T) {
@@ -902,7 +919,7 @@ func TestUpdateAuthorizationAlreadyValid(t *testing.T) {
902919

903920
response, err := makeResponse(finalAuthz.Challenges[ResponseIndex])
904921
test.AssertNotError(t, err, "Unable to construct response to challenge")
905-
finalAuthz.Challenges[ResponseIndex].Type = core.ChallengeTypeDNS01
922+
finalAuthz.Challenges[ResponseIndex].Type = core.ChallengeTypeHTTP01
906923
finalAuthz.Challenges[ResponseIndex].Status = core.StatusPending
907924
va.RecordsReturn = []core.ValidationRecord{
908925
{Hostname: "example.com"}}
@@ -2909,6 +2926,33 @@ func TestDisabledChallengeValidAuthz(t *testing.T) {
29092926
test.AssertNotError(t, err, "RA prevented use of an authorization which used an enabled challenge type")
29102927
}
29112928

2929+
func TestValidChallengeStillGood(t *testing.T) {
2930+
_, _, ra, _, cleanUp := initAuthorities(t)
2931+
defer cleanUp()
2932+
pa, err := policy.New(map[string]bool{
2933+
core.ChallengeTypeTLSSNI01: true,
2934+
})
2935+
test.AssertNotError(t, err, "Couldn't create PA")
2936+
ra.PA = pa
2937+
2938+
test.Assert(t, !ra.validChallengeStillGood(&core.Authorization{}), "ra.validChallengeStillGood didn't fail with empty authorization")
2939+
test.Assert(t, !ra.validChallengeStillGood(&core.Authorization{Challenges: []core.Challenge{{Status: core.StatusPending}}}), "ra.validChallengeStillGood didn't fail with no valid challenges")
2940+
test.Assert(t, !ra.validChallengeStillGood(&core.Authorization{Challenges: []core.Challenge{{Status: core.StatusValid, Type: core.ChallengeTypeHTTP01}}}), "ra.validChallengeStillGood didn't fail with disabled challenge")
2941+
2942+
test.Assert(t, ra.validChallengeStillGood(&core.Authorization{Challenges: []core.Challenge{{Status: core.StatusValid, Type: core.ChallengeTypeTLSSNI01}}}), "ra.validChallengeStillGood failed with enabled challenge")
2943+
}
2944+
2945+
func TestUpdateAuthorizationBadChallengeType(t *testing.T) {
2946+
_, _, ra, _, cleanUp := initAuthorities(t)
2947+
defer cleanUp()
2948+
pa, err := policy.New(map[string]bool{})
2949+
test.AssertNotError(t, err, "Couldn't create PA")
2950+
ra.PA = pa
2951+
2952+
_, err = ra.UpdateAuthorization(context.Background(), core.Authorization{}, 0, core.Challenge{})
2953+
test.AssertError(t, err, "ra.UpdateAuthorization allowed a update to a authorization")
2954+
}
2955+
29122956
var CAkeyPEM = `
29132957
-----BEGIN RSA PRIVATE KEY-----
29142958
MIIJKQIBAAKCAgEAqmM0dEf/J9MCk2ItzevL0dKJ84lVUtf/vQ7AXFi492vFXc3b

‎wfe/wfe_test.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -280,6 +280,10 @@ func (pa *mockPA) WillingToIssueWildcard(id core.AcmeIdentifier) error {
280280
return nil
281281
}
282282

283+
func (pa *mockPA) ChallengeTypeEnabled(string) bool {
284+
return true
285+
}
286+
283287
func makeBody(s string) io.ReadCloser {
284288
return ioutil.NopCloser(strings.NewReader(s))
285289
}

0 commit comments

Comments
 (0)