From 4d9f4effca92523830c0dab87bce5bae4d8cd53d Mon Sep 17 00:00:00 2001 From: Seo Suchan Date: Tue, 6 Feb 2024 04:04:15 +0900 Subject: [PATCH] changes suggested from PR review --- core/objects.go | 10 +--------- policy/pa_test.go | 5 +++-- va/dns.go | 5 +++-- va/dns_test.go | 30 ++++++++++++++---------------- va/http_test.go | 4 ++-- va/tlsalpn_test.go | 18 +++++++++--------- va/va.go | 8 ++------ va/va_test.go | 4 +++- 8 files changed, 37 insertions(+), 47 deletions(-) diff --git a/core/objects.go b/core/objects.go index dd3b8389b3c..b7e5ac1140f 100644 --- a/core/objects.go +++ b/core/objects.go @@ -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 } diff --git a/policy/pa_test.go b/policy/pa_test.go index 1496e2acebb..83241b7a70a 100644 --- a/policy/pa_test.go +++ b/policy/pa_test.go @@ -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) diff --git a/va/dns.go b/va/dns.go index 16c2c98cbbe..335662e2612 100644 --- a/va/dns.go +++ b/va/dns.go @@ -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 diff --git a/va/dns_test.go b/va/dns_test.go index 3cdd077c944..fb379afe9d9 100644 --- a/va/dns_test.go +++ b/va/dns_test.go @@ -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) @@ -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) @@ -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) @@ -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) } @@ -147,7 +147,7 @@ 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) } @@ -155,7 +155,7 @@ func TestDNSValidationNoServer(t *testing.T) { 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.") } @@ -163,7 +163,7 @@ func TestDNSValidationOK(t *testing.T) { 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.") } @@ -253,11 +253,9 @@ 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.") } @@ -265,7 +263,7 @@ func TestDNSAccountValidationGood(t *testing.T) { 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) } @@ -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.") } @@ -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") @@ -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) @@ -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) @@ -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) diff --git a/va/http_test.go b/va/http_test.go index 771c703656a..a6bb71700cc 100644 --- a/va/http_test.go +++ b/va/http_test.go @@ -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") } @@ -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 "), diff --git a/va/tlsalpn_test.go b/va/tlsalpn_test.go index 20cdfe75448..430d9c996e5 100644 --- a/va/tlsalpn_test.go +++ b/va/tlsalpn_test.go @@ -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) } @@ -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") } @@ -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) @@ -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") } @@ -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") } @@ -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") } @@ -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") } @@ -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. @@ -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. diff --git a/va/va.go b/va/va.go index 510c99ae4dc..5ff7ff78c1d 100644 --- a/va/va.go +++ b/va/va.go @@ -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 @@ -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 @@ -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) @@ -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) } diff --git a/va/va_test.go b/va/va_test.go index 0555b473bea..892b1812d31 100644 --- a/va/va_test.go +++ b/va/va_test.go @@ -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) }