Skip to content

Commit

Permalink
changes suggested from PR review
Browse files Browse the repository at this point in the history
  • Loading branch information
@orangepizza
orangepizza committed Feb 5, 2024
1 parent 59e9abb commit 4d9f4ef
Show file tree
Hide file tree
Showing 8 changed files with 37 additions and 47 deletions.
10 changes: 1 addition & 9 deletions core/objects.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,15 +242,7 @@ func (ch Challenge) RecordsSane() bool {
ch.ValidationRecord[0].AddressUsed == nil || len(ch.ValidationRecord[0].AddressesResolved) == 0 {
return false
}
case ChallengeTypeDNS01:
if len(ch.ValidationRecord) > 1 {
return false
}
if ch.ValidationRecord[0].Hostname == "" {
return false
}
return true
case ChallengeTypeDNSAccount01:
case ChallengeTypeDNS01, ChallengeTypeDNSAccount01:
if len(ch.ValidationRecord) > 1 {
return false
}
Expand Down
5 changes: 3 additions & 2 deletions policy/pa_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -397,8 +397,9 @@ func TestChallengesForWildcard(t *testing.T) {
// First try to get a challenge for the wildcard ident without the
// DNS-01 challenge type enabled. This should produce an error
var enabledChallenges = map[core.AcmeChallenge]bool{
core.ChallengeTypeHTTP01: true,
core.ChallengeTypeDNS01: false,
core.ChallengeTypeHTTP01: true,
core.ChallengeTypeDNS01: false,
core.ChallengeTypeDNSAccount01: false,
}
pa := must.Do(New(enabledChallenges, blog.NewMock()))
_, err := pa.ChallengesFor(wildcardIdent)
Expand Down
5 changes: 3 additions & 2 deletions va/dns.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,15 @@ func (va ValidationAuthorityImpl) getAddrs(ctx context.Context, hostname string)
return addrs, nil
}

// accountURLHostname takes regid and known AccountURLPrefixes and create
// possible list of dns-account-01 challenge subdomains.
func accountURLHostname(AccountURLPrefixes []string, ident identifier.ACMEIdentifier, regid int64) []string {
const dnsacc01Prefix = "_acme-challenge_"
var testdomains []string
for _, prefix := range AccountURLPrefixes {
accturl := fmt.Sprintf("%s%d", prefix, regid)
hash := sha256.Sum256([]byte(accturl))
urlhash := strings.ToLower(base32.StdEncoding.EncodeToString(hash[0:10]))
testdomain := fmt.Sprintf("%s%s.%s", dnsacc01Prefix, urlhash, ident.Value)
testdomain := fmt.Sprintf("_acme-challenge_%s.%s", urlhash, ident.Value)
testdomains = append(testdomains, testdomain)
}
return testdomains
Expand Down
30 changes: 14 additions & 16 deletions va/dns_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ func TestDNSValidationNotSane(t *testing.T) {

chall := dnsChallenge()
chall.Token = ""
_, prob := va.validateChallenge(ctx, dnsi("localhost"), chall)
_, prob := va.validateChallenge(ctx, dnsi("localhost"), chall, testregid)
if prob.Type != probs.MalformedProblem {
t.Errorf("Got wrong error type: expected %s, got %s",
prob.Type, probs.MalformedProblem)
Expand All @@ -104,7 +104,7 @@ func TestDNSValidationNotSane(t *testing.T) {
}

chall.Token = "yfCBb-bRTLz8Wd1C0lTUQK3qlKj3-t2tYGwx5Hj7r_"
_, prob = va.validateChallenge(ctx, dnsi("localhost"), chall)
_, prob = va.validateChallenge(ctx, dnsi("localhost"), chall, testregid)
if prob.Type != probs.MalformedProblem {
t.Errorf("Got wrong error type: expected %s, got %s",
prob.Type, probs.MalformedProblem)
Expand All @@ -114,7 +114,7 @@ func TestDNSValidationNotSane(t *testing.T) {
}

chall.ProvidedKeyAuthorization = "a"
_, prob = va.validateChallenge(ctx, dnsi("localhost"), chall)
_, prob = va.validateChallenge(ctx, dnsi("localhost"), chall, testregid)
if prob.Type != probs.MalformedProblem {
t.Errorf("Got wrong error type: expected %s, got %s",
prob.Type, probs.MalformedProblem)
Expand All @@ -128,7 +128,7 @@ func TestDNSValidationNotSane(t *testing.T) {
func TestDNSValidationServFail(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)

_, prob := va.validateChallenge(ctx, dnsi("servfail.com"), dnsChallenge())
_, prob := va.validateChallenge(ctx, dnsi("servfail.com"), dnsChallenge(), testregid)

test.AssertEquals(t, prob.Type, probs.DNSProblem)
}
Expand All @@ -147,23 +147,23 @@ func TestDNSValidationNoServer(t *testing.T) {
log,
nil)

_, prob := va.validateChallenge(ctx, dnsi("localhost"), dnsChallenge())
_, prob := va.validateChallenge(ctx, dnsi("localhost"), dnsChallenge(), testregid)

test.AssertEquals(t, prob.Type, probs.DNSProblem)
}

func TestDNSValidationOK(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)

_, prob := va.validateChallenge(ctx, dnsi("good-dns01.com"), dnsChallenge())
_, prob := va.validateChallenge(ctx, dnsi("good-dns01.com"), dnsChallenge(), testregid)

test.Assert(t, prob == nil, "Should be valid.")
}

func TestDNSValidationNoAuthorityOK(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)

_, prob := va.validateChallenge(ctx, dnsi("no-authority-dns01.com"), dnsChallenge())
_, prob := va.validateChallenge(ctx, dnsi("no-authority-dns01.com"), dnsChallenge(), testregid)

test.Assert(t, prob == nil, "Should be valid.")
}
Expand Down Expand Up @@ -253,19 +253,17 @@ func dnsAccountChallenge() core.Challenge {
return createChallenge(core.ChallengeTypeDNSAccount01)
}

var ctxwithaccount = context.WithValue(context.Background(), accountIDKey{}, int64(11111))

func TestDNSAccountValidationGood(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)
_, prob := va.validateChallenge(ctxwithaccount, dnsi("good-dns01.com"), dnsAccountChallenge())
_, prob := va.validateChallenge(ctx, dnsi("good-dns01.com"), dnsAccountChallenge(), testregid)

test.Assert(t, prob == nil, "Should be valid.")
}

func TestDNSAccountValidationServerFail(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)

_, prob := va.validateChallenge(ctxwithaccount, dnsi("servfail.com"), dnsAccountChallenge())
_, prob := va.validateChallenge(ctx, dnsi("servfail.com"), dnsAccountChallenge(), testregid)

test.AssertEquals(t, prob.Type, probs.DNSProblem)
}
Expand All @@ -274,7 +272,7 @@ func TestDNSAccountWithMultiAccURLGood(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)
//add another account prefix
va.accountURIPrefixes = []string{"test.invalid/acct/", accountURIPrefixes[0]}
_, prob := va.validateChallenge(ctxwithaccount, dnsi("good-dns01.com"), dnsAccountChallenge())
_, prob := va.validateChallenge(ctx, dnsi("good-dns01.com"), dnsAccountChallenge(), testregid)

test.Assert(t, prob == nil, "Should be valid.")
}
Expand All @@ -283,7 +281,7 @@ func TestDNSAccountWithMultiAccURLbothwrong(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)
//add another account prefix
va.accountURIPrefixes = []string{"test.invalid/acct/", accountURIPrefixes[0]}
_, prob := va.validateChallenge(ctxwithaccount, dnsi("wrong-dns01.com"), dnsAccountChallenge())
_, prob := va.validateChallenge(ctx, dnsi("wrong-dns01.com"), dnsAccountChallenge(), testregid)

if prob == nil {
t.Fatalf("Successful DNS validation with wrong TXT record")
Expand All @@ -296,7 +294,7 @@ func TestDNSAccountValidationNotSane(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)
chall := dnsAccountChallenge()
chall.Token = ""
_, prob := va.validateChallenge(ctxwithaccount, dnsi("localhost"), chall)
_, prob := va.validateChallenge(ctx, dnsi("localhost"), chall, testregid)
if prob.Type != probs.MalformedProblem {
t.Errorf("Got wrong error type: expected %s, got %s",
prob.Type, probs.MalformedProblem)
Expand All @@ -306,7 +304,7 @@ func TestDNSAccountValidationNotSane(t *testing.T) {
}

chall.Token = "yfCBb-bRTLz8Wd1C0lTUQK3qlKj3-t2tYGwx5Hj7r_"
_, prob = va.validateChallenge(ctxwithaccount, dnsi("localhost"), chall)
_, prob = va.validateChallenge(ctx, dnsi("localhost"), chall, testregid)
if prob.Type != probs.MalformedProblem {
t.Errorf("Got wrong error type: expected %s, got %s",
prob.Type, probs.MalformedProblem)
Expand All @@ -316,7 +314,7 @@ func TestDNSAccountValidationNotSane(t *testing.T) {
}

chall.ProvidedKeyAuthorization = "a"
_, prob = va.validateChallenge(ctxwithaccount, dnsi("localhost"), chall)
_, prob = va.validateChallenge(ctx, dnsi("localhost"), chall, testregid)
if prob.Type != probs.MalformedProblem {
t.Errorf("Got wrong error type: expected %s, got %s",
prob.Type, probs.MalformedProblem)
Expand Down
4 changes: 2 additions & 2 deletions va/http_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1516,7 +1516,7 @@ func TestValidateHTTP(t *testing.T) {

va, _ := setup(hs, 0, "", nil, nil)

_, prob := va.validateChallenge(ctx, dnsi("localhost"), chall)
_, prob := va.validateChallenge(ctx, dnsi("localhost"), chall, testregid)
test.Assert(t, prob == nil, "validation failed")
}

Expand All @@ -1528,7 +1528,7 @@ func TestLimitedReader(t *testing.T) {
va, _ := setup(hs, 0, "", nil, nil)
defer hs.Close()

_, prob := va.validateChallenge(ctx, dnsi("localhost"), chall)
_, prob := va.validateChallenge(ctx, dnsi("localhost"), chall, testregid)

test.AssertEquals(t, prob.Type, probs.UnauthorizedProblem)
test.Assert(t, strings.HasPrefix(prob.Detail, "127.0.0.1: Invalid response from "),
Expand Down
18 changes: 9 additions & 9 deletions va/tlsalpn_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ func TestTLSALPN01Success(t *testing.T) {

va, _ := setup(hs, 0, "", nil, nil)

_, prob := va.validateChallenge(ctx, dnsi("expected"), chall)
_, prob := va.validateChallenge(ctx, dnsi("expected"), chall, testregid)
if prob != nil {
t.Errorf("Validation failed: %v", prob)
}
Expand All @@ -422,7 +422,7 @@ func TestTLSALPN01ObsoleteFailure(t *testing.T) {

va, _ := setup(hs, 0, "", nil, nil)

_, prob := va.validateChallenge(ctx, dnsi("expected"), chall)
_, prob := va.validateChallenge(ctx, dnsi("expected"), chall, testregid)
test.AssertNotNil(t, prob, "expected validation to fail")
}

Expand Down Expand Up @@ -562,7 +562,7 @@ func TestTLSALPN01TLSVersion(t *testing.T) {

va, _ := setup(hs, 0, "", nil, nil)

_, prob := va.validateChallenge(ctx, dnsi("expected"), chall)
_, prob := va.validateChallenge(ctx, dnsi("expected"), chall, testregid)
if !tc.expectError {
if prob != nil {
t.Errorf("expected success, got: %v", prob)
Expand All @@ -589,7 +589,7 @@ func TestTLSALPN01WrongName(t *testing.T) {

va, _ := setup(hs, 0, "", nil, nil)

_, prob := va.validateChallenge(ctx, dnsi("expected"), chall)
_, prob := va.validateChallenge(ctx, dnsi("expected"), chall, testregid)
test.AssertError(t, prob, "validation should have failed")
}

Expand All @@ -602,7 +602,7 @@ func TestTLSALPN01ExtraNames(t *testing.T) {

va, _ := setup(hs, 0, "", nil, nil)

_, prob := va.validateChallenge(ctx, dnsi("expected"), chall)
_, prob := va.validateChallenge(ctx, dnsi("expected"), chall, testregid)
test.AssertError(t, prob, "validation should have failed")
}

Expand Down Expand Up @@ -660,7 +660,7 @@ func TestTLSALPN01NotSelfSigned(t *testing.T) {

va, _ := setup(hs, 0, "", nil, nil)

_, prob := va.validateChallenge(ctx, dnsi("expected"), chall)
_, prob := va.validateChallenge(ctx, dnsi("expected"), chall, testregid)
test.AssertError(t, prob, "validation should have failed")
test.AssertContains(t, prob.Detail, "not self-signed")
}
Expand Down Expand Up @@ -707,7 +707,7 @@ func TestTLSALPN01ExtraIdentifiers(t *testing.T) {

va, _ := setup(hs, 0, "", nil, nil)

_, prob := va.validateChallenge(ctx, dnsi("expected"), chall)
_, prob := va.validateChallenge(ctx, dnsi("expected"), chall, testregid)
test.AssertError(t, prob, "validation should have failed")
}

Expand Down Expand Up @@ -767,7 +767,7 @@ func TestTLSALPN01ExtraSANs(t *testing.T) {

va, _ := setup(hs, 0, "", nil, nil)

_, prob := va.validateChallenge(ctx, dnsi("expected"), chall)
_, prob := va.validateChallenge(ctx, dnsi("expected"), chall, testregid)
test.AssertError(t, prob, "validation should have failed")
// In go >= 1.19, the TLS client library detects that the certificate has
// a duplicate extension and terminates the connection itself.
Expand Down Expand Up @@ -822,7 +822,7 @@ func TestTLSALPN01ExtraAcmeExtensions(t *testing.T) {

va, _ := setup(hs, 0, "", nil, nil)

_, prob := va.validateChallenge(ctx, dnsi("expected"), chall)
_, prob := va.validateChallenge(ctx, dnsi("expected"), chall, testregid)
test.AssertError(t, prob, "validation should have failed")
// In go >= 1.19, the TLS client library detects that the certificate has
// a duplicate extension and terminates the connection itself.
Expand Down
8 changes: 2 additions & 6 deletions va/va.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -410,8 +410,6 @@ func detailedError(err error) *probs.ProblemDetails {
return probs.Connection("Error getting validation data")
}

type accountIDKey struct{} //empty struct for key

// validate performs a challenge validation and, in parallel,
// checks CAA and GSB for the identifier. If any of those steps fails, it
// returns a ProblemDetails plus the validation records created during the
Expand Down Expand Up @@ -449,8 +447,7 @@ func (va *ValidationAuthorityImpl) validate(
}

// TODO(#1292): send into another goroutine
ctxwithid := context.WithValue(ctx, accountIDKey{}, regid)
validationRecords, prob := va.validateChallenge(ctxwithid, baseIdentifier, challenge)
validationRecords, prob := va.validateChallenge(ctx, baseIdentifier, challenge, regid)
if prob != nil {
// The ProblemDetails will be serialized through gRPC, which requires UTF-8.
// It will also later be serialized in JSON, which defaults to UTF-8. Make
Expand Down Expand Up @@ -480,7 +477,7 @@ func (va *ValidationAuthorityImpl) validate(
return validationRecords, nil
}

func (va *ValidationAuthorityImpl) validateChallenge(ctx context.Context, identifier identifier.ACMEIdentifier, challenge core.Challenge) ([]core.ValidationRecord, *probs.ProblemDetails) {
func (va *ValidationAuthorityImpl) validateChallenge(ctx context.Context, identifier identifier.ACMEIdentifier, challenge core.Challenge, regid int64) ([]core.ValidationRecord, *probs.ProblemDetails) {
err := challenge.CheckConsistencyForValidation()
if err != nil {
return nil, probs.Malformed("Challenge failed consistency check: %s", err)
Expand All @@ -493,7 +490,6 @@ func (va *ValidationAuthorityImpl) validateChallenge(ctx context.Context, identi
case core.ChallengeTypeTLSALPN01:
return va.validateTLSALPN01(ctx, identifier, challenge)
case core.ChallengeTypeDNSAccount01:
regid, _ := ctx.Value(accountIDKey{}).(int64)
//we know regid is filled for normal caller and regid 0 is invalid
return va.validateDNSAccount01(ctx, identifier, challenge, regid)
}
Expand Down
4 changes: 3 additions & 1 deletion va/va_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -254,10 +254,12 @@ func (inmem inMemVA) IsCAAValid(ctx context.Context, req *vapb.IsCAAValidRequest
return inmem.rva.IsCAAValid(ctx, req)
}

const testregid int64 = 11111

func TestValidateMalformedChallenge(t *testing.T) {
va, _ := setup(nil, 0, "", nil, nil)

_, prob := va.validateChallenge(ctx, dnsi("example.com"), createChallenge("fake-type-01"))
_, prob := va.validateChallenge(ctx, dnsi("example.com"), createChallenge("fake-type-01"), testregid)

test.AssertEquals(t, prob.Type, probs.MalformedProblem)
}
Expand Down

0 comments on commit 4d9f4ef

Please sign in to comment.