From 4aa827594b28f45f68e3bef6b78df206a6bda5b4 Mon Sep 17 00:00:00 2001 From: James Renken Date: Thu, 20 Feb 2025 17:57:46 -0800 Subject: [PATCH 01/27] va: Use Identifier in PerformValidationRequest & support IP address identifiers --- bdns/mocks.go | 6 ++ core/objects.go | 2 + identifier/identifier.go | 7 ++ va/caa.go | 2 +- va/dns.go | 2 +- va/dns_test.go | 2 +- va/http.go | 88 +++++++++------- va/http_test.go | 220 +++++++++++++++++++++------------------ va/proto/va.pb.go | 154 +++++++++++++++------------ va/proto/va.proto | 6 ++ va/tlsalpn.go | 81 +++++++++----- va/tlsalpn_test.go | 70 +++++++------ va/va.go | 19 ++-- va/va_test.go | 134 +++++++++++++++++++++--- va/vampic.go | 18 ++-- 15 files changed, 513 insertions(+), 298 deletions(-) diff --git a/bdns/mocks.go b/bdns/mocks.go index 36bf2e88d29..2bcd3f44965 100644 --- a/bdns/mocks.go +++ b/bdns/mocks.go @@ -28,6 +28,12 @@ func (mock *MockClient) LookupTXT(_ context.Context, hostname string) ([]string, // expected token + test account jwk thumbprint return []string{"LPsIwTo7o8BoG0-vjCyGQGBWSVIPxI-i_X336eUOQZo"}, ResolverAddrs{"MockClient"}, nil } + if hostname == "_acme-challenge.good-dns02.com" { + // base64(sha256("LoqXcYV8q5ONbJQxbmR7SCTNo3tiAXDfowyjxAjEuX0" + // + "." + "9jg46WB3rR_AHD-EBXdN7cBkH1WOu0tA3M9fm21mqTI")) + // expected token + test account jwk thumbprint + return []string{"LPsIwTo7o8BoG0-vjCyGQGBWSVIPxI-i_X336eUOQZo"}, ResolverAddrs{"MockClient"}, nil + } if hostname == "_acme-challenge.wrong-dns01.com" { return []string{"a"}, ResolverAddrs{"MockClient"}, nil } diff --git a/core/objects.go b/core/objects.go index 33f14d92d98..b5a2c4a7b62 100644 --- a/core/objects.go +++ b/core/objects.go @@ -122,6 +122,8 @@ type ValidationRecord struct { URL string `json:"url,omitempty"` // Shared + // + // TODO(#7311): Replace DnsName with Identifier. DnsName string `json:"hostname,omitempty"` Port string `json:"port,omitempty"` AddressesResolved []net.IP `json:"addressesResolved,omitempty"` diff --git a/identifier/identifier.go b/identifier/identifier.go index 1b48d1c5afb..24453547cde 100644 --- a/identifier/identifier.go +++ b/identifier/identifier.go @@ -39,6 +39,13 @@ func (i ACMEIdentifier) AsProto() *corepb.Identifier { } } +func FromProto(ident *corepb.Identifier) ACMEIdentifier { + return ACMEIdentifier{ + Type: IdentifierType(ident.Type), + Value: ident.Value, + } +} + // NewDNS is a convenience function for creating an ACMEIdentifier with Type // "dns" for a given domain name. func NewDNS(domain string) ACMEIdentifier { diff --git a/va/caa.go b/va/caa.go index 0b27c90cac1..6faf1a62cb3 100644 --- a/va/caa.go +++ b/va/caa.go @@ -37,7 +37,7 @@ func (va *ValidationAuthorityImpl) IsCAAValid(ctx context.Context, req *vapb.IsC // TODO(#7061) Plumb req.Authz.Id as "AuthzID:" through from the RA to // correlate which authz triggered this request. Requester: req.AccountURIID, - Identifier: req.Domain, + Identifier: identifier.NewDNS(req.Domain), } challType := core.AcmeChallenge(req.ValidationMethod) diff --git a/va/dns.go b/va/dns.go index 273f1afb71c..7445d4fdd18 100644 --- a/va/dns.go +++ b/va/dns.go @@ -51,7 +51,7 @@ func availableAddresses(allAddrs []net.IP) (v4 []net.IP, v6 []net.IP) { func (va *ValidationAuthorityImpl) validateDNS01(ctx context.Context, ident identifier.ACMEIdentifier, keyAuthorization string) ([]core.ValidationRecord, error) { if ident.Type != identifier.TypeDNS { va.log.Infof("Identifier type for DNS challenge was not DNS: %s", ident) - return nil, berrors.MalformedError("Identifier type for DNS was not itself DNS") + return nil, berrors.MalformedError("Identifier type for DNS challenge was not DNS") } // Compute the digest of the key authorization file diff --git a/va/dns_test.go b/va/dns_test.go index 58f157647e8..0c2d5102f3b 100644 --- a/va/dns_test.go +++ b/va/dns_test.go @@ -23,7 +23,7 @@ func TestDNSValidationEmpty(t *testing.T) { // This test calls PerformValidation directly, because that is where the // metrics checked below are incremented. - req := createValidationRequest("empty-txts.com", core.ChallengeTypeDNS01) + req := createValidationRequest(identifier.NewDNS("empty-txts.com"), core.ChallengeTypeDNS01) res, _ := va.PerformValidation(context.Background(), req) test.AssertEquals(t, res.Problem.ProblemType, "unauthorized") test.AssertEquals(t, res.Problem.Detail, "No TXT record found at _acme-challenge.empty-txts.com") diff --git a/va/http.go b/va/http.go index b56e73bce60..d0593aa7b8b 100644 --- a/va/http.go +++ b/va/http.go @@ -8,6 +8,7 @@ import ( "io" "net" "net/http" + "net/netip" "net/url" "strconv" "strings" @@ -159,7 +160,7 @@ func httpTransport(df dialerFunc) *http.Transport { // httpValidationTarget bundles all of the information needed to make an HTTP-01 // validation request against a target. type httpValidationTarget struct { - // the hostname being validated + // the host being validated host string // the port for the validation request port int @@ -201,20 +202,32 @@ func (vt *httpValidationTarget) nextIP() error { // port, and path. This involves querying DNS for the IP addresses for the host. // An error is returned if there are no usable IP addresses or if the DNS // lookups fail. +// +// TODO(#7311): This needs testing with IP address identifiers. func (va *ValidationAuthorityImpl) newHTTPValidationTarget( ctx context.Context, - host string, + ident identifier.ACMEIdentifier, port int, path string, query string) (*httpValidationTarget, error) { - // Resolve IP addresses for the hostname - addrs, resolvers, err := va.getAddrs(ctx, host) - if err != nil { - return nil, err + var addrs []net.IP + var resolvers bdns.ResolverAddrs + switch ident.Type { + case identifier.TypeDNS: + // Resolve IP addresses for the identifier + dnsAddrs, dnsResolvers, err := va.getAddrs(ctx, ident.Value) + if err != nil { + return nil, err + } + addrs, resolvers = dnsAddrs, dnsResolvers + case identifier.TypeIP: + addrs = []net.IP{net.ParseIP(ident.Value)} + default: + return nil, fmt.Errorf("Unknown identifier type: %s", ident.Type) } target := &httpValidationTarget{ - host: host, + host: ident.Value, port: port, path: path, query: query, @@ -230,7 +243,7 @@ func (va *ValidationAuthorityImpl) newHTTPValidationTarget( if !hasV6Addrs && !hasV4Addrs { // If there are no v6 addrs and no v4addrs there was a bug with getAddrs or // availableAddresses and we need to return an error. - return nil, fmt.Errorf("host %q has no IPv4 or IPv6 addresses", host) + return nil, fmt.Errorf("host %q has no IPv4 or IPv6 addresses", ident.Value) } else if !hasV6Addrs && hasV4Addrs { // If there are no v6 addrs and there are v4 addrs then use the first v4 // address. There's no fallback address. @@ -250,23 +263,21 @@ func (va *ValidationAuthorityImpl) newHTTPValidationTarget( return target, nil } -// extractRequestTarget extracts the hostname and port specified in the provided +// extractRequestTarget extracts the host and port specified in the provided // HTTP redirect request. If the request's URL's protocol schema is not HTTP or // HTTPS an error is returned. If an explicit port is specified in the request's -// URL and it isn't the VA's HTTP or HTTPS port, an error is returned. If the -// request's URL's Host is a bare IPv4 or IPv6 address and not a domain name an -// error is returned. -func (va *ValidationAuthorityImpl) extractRequestTarget(req *http.Request) (string, int, error) { +// URL and it isn't the VA's HTTP or HTTPS port, an error is returned. +func (va *ValidationAuthorityImpl) extractRequestTarget(req *http.Request) (identifier.ACMEIdentifier, int, error) { // A nil request is certainly not a valid redirect and has no port to extract. if req == nil { - return "", 0, fmt.Errorf("redirect HTTP request was nil") + return identifier.ACMEIdentifier{}, 0, fmt.Errorf("redirect HTTP request was nil") } reqScheme := req.URL.Scheme // The redirect request must use HTTP or HTTPs protocol schemes regardless of the port.. if reqScheme != "http" && reqScheme != "https" { - return "", 0, berrors.ConnectionFailureError( + return identifier.ACMEIdentifier{}, 0, berrors.ConnectionFailureError( "Invalid protocol scheme in redirect target. "+ `Only "http" and "https" protocol schemes are supported, not %q`, reqScheme) } @@ -280,12 +291,12 @@ func (va *ValidationAuthorityImpl) extractRequestTarget(req *http.Request) (stri reqHost = h reqPort, err = strconv.Atoi(p) if err != nil { - return "", 0, err + return identifier.ACMEIdentifier{}, 0, err } // The explicit port must match the VA's configured HTTP or HTTPS port. if reqPort != va.httpPort && reqPort != va.httpsPort { - return "", 0, berrors.ConnectionFailureError( + return identifier.ACMEIdentifier{}, 0, berrors.ConnectionFailureError( "Invalid port in redirect target. Only ports %d and %d are supported, not %d", va.httpPort, va.httpsPort, reqPort) } @@ -296,17 +307,11 @@ func (va *ValidationAuthorityImpl) extractRequestTarget(req *http.Request) (stri } else { // This shouldn't happen but defensively return an internal server error in // case it does. - return "", 0, fmt.Errorf("unable to determine redirect HTTP request port") + return identifier.ACMEIdentifier{}, 0, fmt.Errorf("unable to determine redirect HTTP request port") } if reqHost == "" { - return "", 0, berrors.ConnectionFailureError("Invalid empty hostname in redirect target") - } - - // Check that the request host isn't a bare IP address. We only follow - // redirects to hostnames. - if net.ParseIP(reqHost) != nil { - return "", 0, berrors.ConnectionFailureError("Invalid host in redirect target %q. Only domain names are supported, not IP addresses", reqHost) + return identifier.ACMEIdentifier{}, 0, berrors.ConnectionFailureError("Invalid empty host in redirect target") } // Often folks will misconfigure their webserver to send an HTTP redirect @@ -319,17 +324,28 @@ func (va *ValidationAuthorityImpl) extractRequestTarget(req *http.Request) (stri // This happens frequently enough we want to return a distinct error message // for this case by detecting the reqHost ending in ".well-known". if strings.HasSuffix(reqHost, ".well-known") { - return "", 0, berrors.ConnectionFailureError( + return identifier.ACMEIdentifier{}, 0, berrors.ConnectionFailureError( "Invalid host in redirect target %q. Check webserver config for missing '/' in redirect target.", reqHost, ) } + // We use net.ParseIP to check whether the host is an IP address; otherwise, + // netip.ParseAddr would happily ingest a hostname, returning a garbage IP + // and no error. + if net.ParseIP(reqHost) != nil { + reqIP, err := netip.ParseAddr(reqHost) + if err != nil { + return identifier.ACMEIdentifier{}, 0, berrors.ConnectionFailureError("Invalid IP address in redirect target %q", reqHost) + } + return identifier.NewIP(reqIP), reqPort, nil + } + if _, err := iana.ExtractSuffix(reqHost); err != nil { - return "", 0, berrors.ConnectionFailureError("Invalid hostname in redirect target, must end in IANA registered TLD") + return identifier.ACMEIdentifier{}, 0, berrors.ConnectionFailureError("Invalid host in redirect target, must end in IANA registered TLD") } - return reqHost, reqPort, nil + return identifier.NewDNS(reqHost), reqPort, nil } // setupHTTPValidation sets up a preresolvedDialer and a validation record for @@ -403,10 +419,11 @@ func fallbackErr(err error) bool { // a non-nil error and potentially some ValidationRecords are returned. func (va *ValidationAuthorityImpl) processHTTPValidation( ctx context.Context, - host string, + ident identifier.ACMEIdentifier, + port int, path string) ([]byte, []core.ValidationRecord, error) { // Create a target for the host, port and path with no query parameters - target, err := va.newHTTPValidationTarget(ctx, host, va.httpPort, path, "") + target, err := va.newHTTPValidationTarget(ctx, ident, port, path, "") if err != nil { return nil, nil, err } @@ -414,7 +431,7 @@ func (va *ValidationAuthorityImpl) processHTTPValidation( // Create an initial GET Request initialURL := url.URL{ Scheme: "http", - Host: host, + Host: ident.Value, Path: path, } initialReq, err := http.NewRequest("GET", initialURL.String(), nil) @@ -626,14 +643,15 @@ func (va *ValidationAuthorityImpl) processHTTPValidation( } func (va *ValidationAuthorityImpl) validateHTTP01(ctx context.Context, ident identifier.ACMEIdentifier, token string, keyAuthorization string) ([]core.ValidationRecord, error) { - if ident.Type != identifier.TypeDNS { - va.log.Infof("Got non-DNS identifier for HTTP validation: %s", ident) - return nil, berrors.MalformedError("Identifier type for HTTP validation was not DNS") + // TODO(#7311): This needs testing. + if ident.Type != identifier.TypeDNS && ident.Type != identifier.TypeIP { + va.log.Info(fmt.Sprintf("Identifier type for HTTP-01 challenge was not DNS or IP: %s", ident)) + return nil, berrors.MalformedError("Identifier type for HTTP-01 challenge was not DNS or IP") } // Perform the fetch path := fmt.Sprintf(".well-known/acme-challenge/%s", token) - body, validationRecords, err := va.processHTTPValidation(ctx, ident.Value, "/"+path) + body, validationRecords, err := va.processHTTPValidation(ctx, ident, va.httpPort, "/"+path) if err != nil { return validationRecords, err } diff --git a/va/http_test.go b/va/http_test.go index 67b1224e64e..3ee9a081645 100644 --- a/va/http_test.go +++ b/va/http_test.go @@ -89,7 +89,7 @@ func TestDialerTimeout(t *testing.T) { var took time.Duration for range 20 { started := time.Now() - _, _, err = va.processHTTPValidation(ctx, "unroutable.invalid", "/.well-known/acme-challenge/whatever") + _, _, err = va.processHTTPValidation(ctx, identifier.NewDNS("unroutable.invalid"), 80, "/.well-known/acme-challenge/whatever") took = time.Since(started) if err != nil && strings.Contains(err.Error(), "network is unreachable") { continue @@ -134,28 +134,28 @@ func TestHTTPValidationTarget(t *testing.T) { // hostnames used in this test. testCases := []struct { Name string - Host string + Ident identifier.ACMEIdentifier ExpectedError error ExpectedIPs []string }{ { Name: "No IPs for host", - Host: "always.invalid", + Ident: identifier.NewDNS("always.invalid"), ExpectedError: berrors.DNSError("No valid IP addresses found for always.invalid"), }, { Name: "Only IPv4 addrs for host", - Host: "some.example.com", + Ident: identifier.NewDNS("some.example.com"), ExpectedIPs: []string{"127.0.0.1"}, }, { Name: "Only IPv6 addrs for host", - Host: "ipv6.localhost", + Ident: identifier.NewDNS("ipv6.localhost"), ExpectedIPs: []string{"::1"}, }, { - Name: "Both IPv6 and IPv4 addrs for host", - Host: "ipv4.and.ipv6.localhost", + Name: "Both IPv6 and IPv4 addrs for host", + Ident: identifier.NewDNS("ipv4.and.ipv6.localhost"), // In this case we expect 1 IPv6 address first, and then 1 IPv4 address ExpectedIPs: []string{"::1", "127.0.0.1"}, }, @@ -172,7 +172,7 @@ func TestHTTPValidationTarget(t *testing.T) { t.Run(tc.Name, func(t *testing.T) { target, err := va.newHTTPValidationTarget( context.Background(), - tc.Host, + tc.Ident, examplePort, examplePath, exampleQuery) @@ -211,7 +211,7 @@ func TestExtractRequestTarget(t *testing.T) { Name string Req *http.Request ExpectedError error - ExpectedHost string + ExpectedHost identifier.ACMEIdentifier ExpectedPort int }{ { @@ -236,11 +236,11 @@ func TestExtractRequestTarget(t *testing.T) { "and 443 are supported, not 9999"), }, { - Name: "invalid empty hostname", + Name: "invalid empty host", Req: &http.Request{ URL: mustURL("https:///who/needs/a/hostname?not=me"), }, - ExpectedError: errors.New("Invalid empty hostname in redirect target"), + ExpectedError: errors.New("Invalid empty host in redirect target"), }, { Name: "invalid .well-known hostname", @@ -254,22 +254,22 @@ func TestExtractRequestTarget(t *testing.T) { Req: &http.Request{ URL: mustURL("https://my.tld.is.cpu/pretty/cool/right?yeah=Ithoughtsotoo"), }, - ExpectedError: errors.New("Invalid hostname in redirect target, must end in IANA registered TLD"), + ExpectedError: errors.New("Invalid host in redirect target, must end in IANA registered TLD"), }, { Name: "bare IP", Req: &http.Request{ URL: mustURL("https://10.10.10.10"), }, - ExpectedError: fmt.Errorf(`Invalid host in redirect target "10.10.10.10". ` + - "Only domain names are supported, not IP addresses"), + ExpectedHost: identifier.NewIP(netip.MustParseAddr("10.10.10.10")), + ExpectedPort: 443, }, { Name: "valid HTTP redirect, explicit port", Req: &http.Request{ URL: mustURL("http://cpu.letsencrypt.org:80"), }, - ExpectedHost: "cpu.letsencrypt.org", + ExpectedHost: identifier.NewDNS("cpu.letsencrypt.org"), ExpectedPort: 80, }, { @@ -277,7 +277,7 @@ func TestExtractRequestTarget(t *testing.T) { Req: &http.Request{ URL: mustURL("http://cpu.letsencrypt.org"), }, - ExpectedHost: "cpu.letsencrypt.org", + ExpectedHost: identifier.NewDNS("cpu.letsencrypt.org"), ExpectedPort: 80, }, { @@ -285,7 +285,7 @@ func TestExtractRequestTarget(t *testing.T) { Req: &http.Request{ URL: mustURL("https://cpu.letsencrypt.org:443/hello.world"), }, - ExpectedHost: "cpu.letsencrypt.org", + ExpectedHost: identifier.NewDNS("cpu.letsencrypt.org"), ExpectedPort: 443, }, { @@ -293,7 +293,7 @@ func TestExtractRequestTarget(t *testing.T) { Req: &http.Request{ URL: mustURL("https://cpu.letsencrypt.org/hello.world"), }, - ExpectedHost: "cpu.letsencrypt.org", + ExpectedHost: identifier.NewDNS("cpu.letsencrypt.org"), ExpectedPort: 443, }, } @@ -322,7 +322,7 @@ func TestExtractRequestTarget(t *testing.T) { func TestHTTPValidationDNSError(t *testing.T) { va, mockLog := setup(nil, "", nil, nil) - _, _, prob := va.processHTTPValidation(ctx, "always.error", "/.well-known/acme-challenge/whatever") + _, _, prob := va.processHTTPValidation(ctx, identifier.NewDNS("always.error"), 80, "/.well-known/acme-challenge/whatever") test.AssertError(t, prob, "Expected validation fetch to fail") matchingLines := mockLog.GetAllMatching(`read udp: some net error`) if len(matchingLines) != 1 { @@ -338,7 +338,7 @@ func TestHTTPValidationDNSError(t *testing.T) { func TestHTTPValidationDNSIdMismatchError(t *testing.T) { va, mockLog := setup(nil, "", nil, nil) - _, _, prob := va.processHTTPValidation(ctx, "id.mismatch", "/.well-known/acme-challenge/whatever") + _, _, prob := va.processHTTPValidation(ctx, identifier.NewDNS("id.mismatch"), 80, "/.well-known/acme-challenge/whatever") test.AssertError(t, prob, "Expected validation fetch to fail") matchingLines := mockLog.GetAllMatching(`logDNSError ID mismatch`) if len(matchingLines) != 1 { @@ -377,7 +377,7 @@ func TestHTTPValidationDNSIdMismatchError(t *testing.T) { func TestSetupHTTPValidation(t *testing.T) { va, _ := setup(nil, "", nil, nil) - mustTarget := func(t *testing.T, host string, port int, path string) *httpValidationTarget { + mustTarget := func(t *testing.T, host identifier.ACMEIdentifier, port int, path string) *httpValidationTarget { target, err := va.newHTTPValidationTarget( context.Background(), host, @@ -429,7 +429,7 @@ func TestSetupHTTPValidation(t *testing.T) { }, { Name: "HTTP input req", - InputTarget: mustTarget(t, "ipv4.and.ipv6.localhost", va.httpPort, "/yellow/brick/road"), + InputTarget: mustTarget(t, identifier.NewDNS("ipv4.and.ipv6.localhost"), va.httpPort, "/yellow/brick/road"), InputURL: httpInputURL, ExpectedRecord: core.ValidationRecord{ DnsName: "ipv4.and.ipv6.localhost", @@ -447,7 +447,7 @@ func TestSetupHTTPValidation(t *testing.T) { }, { Name: "HTTPS input req", - InputTarget: mustTarget(t, "ipv4.and.ipv6.localhost", va.httpsPort, "/yellow/brick/road"), + InputTarget: mustTarget(t, identifier.NewDNS("ipv4.and.ipv6.localhost"), va.httpsPort, "/yellow/brick/road"), InputURL: httpsInputURL, ExpectedRecord: core.ValidationRecord{ DnsName: "ipv4.and.ipv6.localhost", @@ -556,7 +556,7 @@ func httpTestSrv(t *testing.T) *httptest.Server { }) // A path that always redirects to a URL with a bare IP address - mux.HandleFunc("/redir-bad-host", func(resp http.ResponseWriter, req *http.Request) { + mux.HandleFunc("/redir-bare-ip", func(resp http.ResponseWriter, req *http.Request) { http.Redirect( resp, req, @@ -803,16 +803,18 @@ func TestFetchHTTP(t *testing.T) { testCases := []struct { Name string - Host string + Ident identifier.ACMEIdentifier + Port int Path string ExpectedBody string ExpectedRecords []core.ValidationRecord ExpectedProblem *probs.ProblemDetails }{ { - Name: "No IPs for host", - Host: "always.invalid", - Path: "/.well-known/whatever", + Name: "No IPs for host", + Ident: identifier.NewDNS("always.invalid"), + Port: httpPort, + Path: "/.well-known/whatever", ExpectedProblem: probs.DNS( "No valid IP addresses found for always.invalid"), // There are no validation records in this case because the base record @@ -820,9 +822,10 @@ func TestFetchHTTP(t *testing.T) { ExpectedRecords: nil, }, { - Name: "Timeout for host with standard ACME allowed port", - Host: "example.com", - Path: "/timeout", + Name: "Timeout for host with standard ACME allowed port", + Ident: identifier.NewDNS("example.com"), + Port: httpPort, + Path: "/timeout", ExpectedProblem: probs.Connection( "127.0.0.1: Fetching http://example.com/timeout: " + "Timeout after connect (your server may be slow or overloaded)"), @@ -838,9 +841,10 @@ func TestFetchHTTP(t *testing.T) { }, }, { - Name: "Connecting to bad port", - Host: "example.com:" + strconv.Itoa(httpPort), - Path: "/timeout", + Name: "Connecting to bad port", + Ident: identifier.NewDNS("example.com"), + Port: 1023, + Path: "/timeout", ExpectedProblem: probs.Connection( "127.0.0.1: Fetching http://example.com:" + strconv.Itoa(httpPort) + "/timeout: " + "Error getting validation data"), @@ -856,25 +860,28 @@ func TestFetchHTTP(t *testing.T) { }, }, { - Name: "Redirect loop", - Host: "example.com", - Path: "/loop", + Name: "Redirect loop", + Ident: identifier.NewDNS("example.com"), + Port: httpPort, + Path: "/loop", ExpectedProblem: probs.Connection(fmt.Sprintf( "127.0.0.1: Fetching http://example.com:%d/loop: Redirect loop detected", httpPort)), ExpectedRecords: expectedLoopRecords, }, { - Name: "Too many redirects", - Host: "example.com", - Path: "/max-redirect/0", + Name: "Too many redirects", + Ident: identifier.NewDNS("example.com"), + Port: httpPort, + Path: "/max-redirect/0", ExpectedProblem: probs.Connection(fmt.Sprintf( "127.0.0.1: Fetching http://example.com:%d/max-redirect/12: Too many redirects", httpPort)), ExpectedRecords: expectedTooManyRedirRecords, }, { - Name: "Redirect to bad protocol", - Host: "example.com", - Path: "/redir-bad-proto", + Name: "Redirect to bad protocol", + Ident: identifier.NewDNS("example.com"), + Port: httpPort, + Path: "/redir-bad-proto", ExpectedProblem: probs.Connection( "127.0.0.1: Fetching gopher://example.com: Invalid protocol scheme in " + `redirect target. Only "http" and "https" protocol schemes ` + @@ -891,9 +898,10 @@ func TestFetchHTTP(t *testing.T) { }, }, { - Name: "Redirect to bad port", - Host: "example.com", - Path: "/redir-bad-port", + Name: "Redirect to bad port", + Ident: identifier.NewDNS("example.com"), + Port: httpPort, + Path: "/redir-bad-port", ExpectedProblem: probs.Connection(fmt.Sprintf( "127.0.0.1: Fetching https://example.com:1987: Invalid port in redirect target. "+ "Only ports %d and 443 are supported, not 1987", httpPort)), @@ -909,12 +917,11 @@ func TestFetchHTTP(t *testing.T) { }, }, { - Name: "Redirect to bad host (bare IP address)", - Host: "example.com", - Path: "/redir-bad-host", - ExpectedProblem: probs.Connection( - "127.0.0.1: Fetching https://127.0.0.1: Invalid host in redirect target " + - `"127.0.0.1". Only domain names are supported, not IP addresses`), + Name: "Redirect to bare IP address", + Ident: identifier.NewDNS("example.com"), + Port: httpPort, + Path: "/redir-bare-ip", + ExpectedBody: "ok", ExpectedRecords: []core.ValidationRecord{ { DnsName: "example.com", @@ -927,9 +934,10 @@ func TestFetchHTTP(t *testing.T) { }, }, { - Name: "Redirect to long path", - Host: "example.com", - Path: "/redir-path-too-long", + Name: "Redirect to long path", + Ident: identifier.NewDNS("example.com"), + Port: httpPort, + Path: "/redir-path-too-long", ExpectedProblem: probs.Connection( "127.0.0.1: Fetching https://example.com/this-is-too-long-01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789: Redirect target too long"), ExpectedRecords: []core.ValidationRecord{ @@ -944,9 +952,10 @@ func TestFetchHTTP(t *testing.T) { }, }, { - Name: "Wrong HTTP status code", - Host: "example.com", - Path: "/bad-status-code", + Name: "Wrong HTTP status code", + Ident: identifier.NewDNS("example.com"), + Port: httpPort, + Path: "/bad-status-code", ExpectedProblem: probs.Unauthorized( "127.0.0.1: Invalid response from http://example.com/bad-status-code: 410"), ExpectedRecords: []core.ValidationRecord{ @@ -961,9 +970,10 @@ func TestFetchHTTP(t *testing.T) { }, }, { - Name: "HTTP status code 303 redirect", - Host: "example.com", - Path: "/303-see-other", + Name: "HTTP status code 303 redirect", + Ident: identifier.NewDNS("example.com"), + Port: httpPort, + Path: "/303-see-other", ExpectedProblem: probs.Connection( "127.0.0.1: Fetching http://example.org/303-see-other: received disallowed redirect status code"), ExpectedRecords: []core.ValidationRecord{ @@ -978,9 +988,10 @@ func TestFetchHTTP(t *testing.T) { }, }, { - Name: "Response too large", - Host: "example.com", - Path: "/resp-too-big", + Name: "Response too large", + Ident: identifier.NewDNS("example.com"), + Port: httpPort, + Path: "/resp-too-big", ExpectedProblem: probs.Unauthorized(fmt.Sprintf( "127.0.0.1: Invalid response from http://example.com/resp-too-big: %q", expectedTruncatedResp.String(), )), @@ -996,9 +1007,10 @@ func TestFetchHTTP(t *testing.T) { }, }, { - Name: "Broken IPv6 only", - Host: "ipv6.localhost", - Path: "/ok", + Name: "Broken IPv6 only", + Ident: identifier.NewDNS("ipv6.localhost"), + Port: httpPort, + Path: "/ok", ExpectedProblem: probs.Connection( "::1: Fetching http://ipv6.localhost/ok: Connection refused"), ExpectedRecords: []core.ValidationRecord{ @@ -1014,7 +1026,8 @@ func TestFetchHTTP(t *testing.T) { }, { Name: "Dual homed w/ broken IPv6, working IPv4", - Host: "ipv4.and.ipv6.localhost", + Ident: identifier.NewDNS("ipv4.and.ipv6.localhost"), + Port: httpPort, Path: "/ok", ExpectedBody: "ok", ExpectedRecords: []core.ValidationRecord{ @@ -1040,7 +1053,8 @@ func TestFetchHTTP(t *testing.T) { }, { Name: "Working IPv4 only", - Host: "example.com", + Ident: identifier.NewDNS("example.com"), + Port: httpPort, Path: "/ok", ExpectedBody: "ok", ExpectedRecords: []core.ValidationRecord{ @@ -1056,7 +1070,8 @@ func TestFetchHTTP(t *testing.T) { }, { Name: "Redirect to uppercase Public Suffix", - Host: "example.com", + Ident: identifier.NewDNS("example.com"), + Port: httpPort, Path: "/redir-uppercase-publicsuffix", ExpectedBody: "ok", ExpectedRecords: []core.ValidationRecord{ @@ -1079,9 +1094,10 @@ func TestFetchHTTP(t *testing.T) { }, }, { - Name: "Reflected response body containing printf verbs", - Host: "example.com", - Path: "/printf-verbs", + Name: "Reflected response body containing printf verbs", + Ident: identifier.NewDNS("example.com"), + Port: httpPort, + Path: "/printf-verbs", ExpectedProblem: &probs.ProblemDetails{ Type: probs.UnauthorizedProblem, Detail: fmt.Sprintf("127.0.0.1: Invalid response from http://example.com/printf-verbs: %q", @@ -1105,7 +1121,7 @@ func TestFetchHTTP(t *testing.T) { t.Run(tc.Name, func(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*500) defer cancel() - body, records, err := va.processHTTPValidation(ctx, tc.Host, tc.Path) + body, records, err := va.processHTTPValidation(ctx, tc.Ident, tc.Port, tc.Path) if tc.ExpectedProblem == nil { test.AssertNotError(t, err, "expected nil prob") } else { @@ -1179,7 +1195,7 @@ func httpSrv(t *testing.T, token string) *httptest.Server { port := getPort(server) http.Redirect(w, r, fmt.Sprintf("http://other.valid.com:%d/path", port), http.StatusFound) } else if strings.HasSuffix(r.URL.Path, pathReLookupInvalid) { - t.Logf("HTTPSRV: Got a redirect req to an invalid hostname\n") + t.Logf("HTTPSRV: Got a redirect req to an invalid host\n") http.Redirect(w, r, "http://invalid.invalid/path", http.StatusFound) } else if strings.HasSuffix(r.URL.Path, pathRedirectToFailingURL) { t.Logf("HTTPSRV: Redirecting to a URL that will fail\n") @@ -1224,7 +1240,7 @@ func TestHTTPBadPort(t *testing.T) { badPort := 40000 + mrand.IntN(25000) va.httpPort = badPort - _, err := va.validateHTTP01(ctx, dnsi("localhost"), expectedToken, expectedKeyAuthorization) + _, err := va.validateHTTP01(ctx, identifier.NewDNS("localhost"), expectedToken, expectedKeyAuthorization) if err == nil { t.Fatalf("Server's down; expected refusal. Where did we connect?") } @@ -1244,7 +1260,7 @@ func TestHTTPKeyAuthorizationFileMismatch(t *testing.T) { hs.Start() va, _ := setup(hs, "", nil, nil) - _, err := va.validateHTTP01(ctx, dnsi("localhost.com"), expectedToken, expectedKeyAuthorization) + _, err := va.validateHTTP01(ctx, identifier.NewDNS("localhost.com"), expectedToken, expectedKeyAuthorization) if err == nil { t.Fatalf("Expected validation to fail when file mismatched.") @@ -1261,14 +1277,20 @@ func TestHTTP(t *testing.T) { va, log := setup(hs, "", nil, nil) - _, err := va.validateHTTP01(ctx, dnsi("localhost.com"), expectedToken, expectedKeyAuthorization) + _, err := va.validateHTTP01(ctx, identifier.NewDNS("localhost.com"), expectedToken, expectedKeyAuthorization) + if err != nil { + t.Errorf("Unexpected failure in HTTP validation for DNS: %s", err) + } + test.AssertEquals(t, len(log.GetAllMatching(`\[AUDIT\] `)), 1) + + _, err = va.validateHTTP01(ctx, identifier.NewIP(netip.MustParseAddr("127.0.0.1")), expectedToken, expectedKeyAuthorization) if err != nil { - t.Errorf("Unexpected failure in HTTP validation: %s", err) + t.Errorf("Unexpected failure in HTTP validation for IP: %s", err) } test.AssertEquals(t, len(log.GetAllMatching(`\[AUDIT\] `)), 1) log.Clear() - _, err = va.validateHTTP01(ctx, dnsi("localhost.com"), path404, ka(path404)) + _, err = va.validateHTTP01(ctx, identifier.NewDNS("localhost.com"), path404, ka(path404)) if err == nil { t.Fatalf("Should have found a 404 for the challenge.") } @@ -1278,7 +1300,7 @@ func TestHTTP(t *testing.T) { log.Clear() // The "wrong token" will actually be the expectedToken. It's wrong // because it doesn't match pathWrongToken. - _, err = va.validateHTTP01(ctx, dnsi("localhost.com"), pathWrongToken, ka(pathWrongToken)) + _, err = va.validateHTTP01(ctx, identifier.NewDNS("localhost.com"), pathWrongToken, ka(pathWrongToken)) if err == nil { t.Fatalf("Should have found the wrong token value.") } @@ -1287,7 +1309,7 @@ func TestHTTP(t *testing.T) { test.AssertEquals(t, len(log.GetAllMatching(`\[AUDIT\] `)), 1) log.Clear() - _, err = va.validateHTTP01(ctx, dnsi("localhost.com"), pathMoved, ka(pathMoved)) + _, err = va.validateHTTP01(ctx, identifier.NewDNS("localhost.com"), pathMoved, ka(pathMoved)) if err != nil { t.Fatalf("Failed to follow http.StatusMovedPermanently redirect") } @@ -1296,7 +1318,7 @@ func TestHTTP(t *testing.T) { test.AssertEquals(t, len(matchedValidRedirect), 1) log.Clear() - _, err = va.validateHTTP01(ctx, dnsi("localhost.com"), pathFound, ka(pathFound)) + _, err = va.validateHTTP01(ctx, identifier.NewDNS("localhost.com"), pathFound, ka(pathFound)) if err != nil { t.Fatalf("Failed to follow http.StatusFound redirect") } @@ -1305,12 +1327,6 @@ func TestHTTP(t *testing.T) { test.AssertEquals(t, len(matchedValidRedirect), 1) test.AssertEquals(t, len(matchedMovedRedirect), 1) - _, err = va.validateHTTP01(ctx, identifier.NewIP(netip.MustParseAddr("127.0.0.1")), pathFound, ka(pathFound)) - if err == nil { - t.Fatalf("IdentifierType IP shouldn't have worked.") - } - test.AssertErrorIs(t, err, berrors.Malformed) - _, err = va.validateHTTP01(ctx, identifier.NewDNS("always.invalid"), pathFound, ka(pathFound)) if err == nil { t.Fatalf("Domain name is invalid.") @@ -1329,7 +1345,7 @@ func TestHTTPTimeout(t *testing.T) { timeout := 250 * time.Millisecond ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() - _, err := va.validateHTTP01(ctx, dnsi("localhost"), pathWaitLong, ka(pathWaitLong)) + _, err := va.validateHTTP01(ctx, identifier.NewDNS("localhost"), pathWaitLong, ka(pathWaitLong)) if err == nil { t.Fatalf("Connection should've timed out") } @@ -1353,7 +1369,7 @@ func TestHTTPRedirectLookup(t *testing.T) { defer hs.Close() va, log := setup(hs, "", nil, nil) - _, err := va.validateHTTP01(ctx, dnsi("localhost.com"), pathMoved, ka(pathMoved)) + _, err := va.validateHTTP01(ctx, identifier.NewDNS("localhost.com"), pathMoved, ka(pathMoved)) if err != nil { t.Fatalf("Unexpected failure in redirect (%s): %s", pathMoved, err) } @@ -1363,7 +1379,7 @@ func TestHTTPRedirectLookup(t *testing.T) { test.AssertEquals(t, len(log.GetAllMatching(`Resolved addresses for localhost.com: \[127.0.0.1\]`)), 2) log.Clear() - _, err = va.validateHTTP01(ctx, dnsi("localhost.com"), pathFound, ka(pathFound)) + _, err = va.validateHTTP01(ctx, identifier.NewDNS("localhost.com"), pathFound, ka(pathFound)) if err != nil { t.Fatalf("Unexpected failure in redirect (%s): %s", pathFound, err) } @@ -1373,14 +1389,14 @@ func TestHTTPRedirectLookup(t *testing.T) { test.AssertEquals(t, len(log.GetAllMatching(`Resolved addresses for localhost.com: \[127.0.0.1\]`)), 3) log.Clear() - _, err = va.validateHTTP01(ctx, dnsi("localhost.com"), pathReLookupInvalid, ka(pathReLookupInvalid)) + _, err = va.validateHTTP01(ctx, identifier.NewDNS("localhost.com"), pathReLookupInvalid, ka(pathReLookupInvalid)) test.AssertError(t, err, "error for pathReLookupInvalid should not be nil") test.AssertEquals(t, len(log.GetAllMatching(`Resolved addresses for localhost.com: \[127.0.0.1\]`)), 1) prob := detailedError(err) - test.AssertDeepEquals(t, prob, probs.Connection(`127.0.0.1: Fetching http://invalid.invalid/path: Invalid hostname in redirect target, must end in IANA registered TLD`)) + test.AssertDeepEquals(t, prob, probs.Connection(`127.0.0.1: Fetching http://invalid.invalid/path: Invalid host in redirect target, must end in IANA registered TLD`)) log.Clear() - _, err = va.validateHTTP01(ctx, dnsi("localhost.com"), pathReLookup, ka(pathReLookup)) + _, err = va.validateHTTP01(ctx, identifier.NewDNS("localhost.com"), pathReLookup, ka(pathReLookup)) if err != nil { t.Fatalf("Unexpected error in redirect (%s): %s", pathReLookup, err) } @@ -1390,7 +1406,7 @@ func TestHTTPRedirectLookup(t *testing.T) { test.AssertEquals(t, len(log.GetAllMatching(`Resolved addresses for other.valid.com: \[127.0.0.1\]`)), 1) log.Clear() - _, err = va.validateHTTP01(ctx, dnsi("localhost.com"), pathRedirectInvalidPort, ka(pathRedirectInvalidPort)) + _, err = va.validateHTTP01(ctx, identifier.NewDNS("localhost.com"), pathRedirectInvalidPort, ka(pathRedirectInvalidPort)) test.AssertNotNil(t, err, "error for pathRedirectInvalidPort should not be nil") prob = detailedError(err) test.AssertEquals(t, prob.Detail, fmt.Sprintf( @@ -1401,7 +1417,7 @@ func TestHTTPRedirectLookup(t *testing.T) { // HTTP 500 errors. The test case is ensuring that the connection error // is referencing the redirected to host, instead of the original host. log.Clear() - _, err = va.validateHTTP01(ctx, dnsi("localhost.com"), pathRedirectToFailingURL, ka(pathRedirectToFailingURL)) + _, err = va.validateHTTP01(ctx, identifier.NewDNS("localhost.com"), pathRedirectToFailingURL, ka(pathRedirectToFailingURL)) test.AssertNotNil(t, err, "err should not be nil") prob = detailedError(err) test.AssertDeepEquals(t, prob, @@ -1415,7 +1431,7 @@ func TestHTTPRedirectLoop(t *testing.T) { defer hs.Close() va, _ := setup(hs, "", nil, nil) - _, prob := va.validateHTTP01(ctx, dnsi("localhost"), "looper", ka("looper")) + _, prob := va.validateHTTP01(ctx, identifier.NewDNS("localhost"), "looper", ka("looper")) if prob == nil { t.Fatalf("Challenge should have failed for looper") } @@ -1427,12 +1443,12 @@ func TestHTTPRedirectUserAgent(t *testing.T) { va, _ := setup(hs, "", nil, nil) va.userAgent = rejectUserAgent - _, prob := va.validateHTTP01(ctx, dnsi("localhost"), pathMoved, ka(pathMoved)) + _, prob := va.validateHTTP01(ctx, identifier.NewDNS("localhost"), pathMoved, ka(pathMoved)) if prob == nil { t.Fatalf("Challenge with rejectUserAgent should have failed (%s).", pathMoved) } - _, prob = va.validateHTTP01(ctx, dnsi("localhost"), pathFound, ka(pathFound)) + _, prob = va.validateHTTP01(ctx, identifier.NewDNS("localhost"), pathFound, ka(pathFound)) if prob == nil { t.Fatalf("Challenge with rejectUserAgent should have failed (%s).", pathFound) } @@ -1462,7 +1478,7 @@ func TestValidateHTTP(t *testing.T) { va, _ := setup(hs, "", nil, nil) - _, prob := va.validateHTTP01(ctx, dnsi("localhost"), token, ka(token)) + _, prob := va.validateHTTP01(ctx, identifier.NewDNS("localhost"), token, ka(token)) test.Assert(t, prob == nil, "validation failed") } @@ -1473,7 +1489,7 @@ func TestLimitedReader(t *testing.T) { va, _ := setup(hs, "", nil, nil) defer hs.Close() - _, err := va.validateHTTP01(ctx, dnsi("localhost"), token, ka(token)) + _, err := va.validateHTTP01(ctx, identifier.NewDNS("localhost"), token, ka(token)) prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.UnauthorizedProblem) diff --git a/va/proto/va.pb.go b/va/proto/va.pb.go index abc195705a3..d9e660822a9 100644 --- a/va/proto/va.pb.go +++ b/va/proto/va.pb.go @@ -162,10 +162,13 @@ type PerformValidationRequest struct { sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields - DnsName string `protobuf:"bytes,1,opt,name=dnsName,proto3" json:"dnsName,omitempty"` - Challenge *proto.Challenge `protobuf:"bytes,2,opt,name=challenge,proto3" json:"challenge,omitempty"` - Authz *AuthzMeta `protobuf:"bytes,3,opt,name=authz,proto3" json:"authz,omitempty"` - ExpectedKeyAuthorization string `protobuf:"bytes,4,opt,name=expectedKeyAuthorization,proto3" json:"expectedKeyAuthorization,omitempty"` + // Next unused field number: 6 + // TODO(#7311): dnsNames are being deprecated in favour of identifiers. + DnsName string `protobuf:"bytes,1,opt,name=dnsName,proto3" json:"dnsName,omitempty"` + Identifier *proto.Identifier `protobuf:"bytes,5,opt,name=identifier,proto3" json:"identifier,omitempty"` + Challenge *proto.Challenge `protobuf:"bytes,2,opt,name=challenge,proto3" json:"challenge,omitempty"` + Authz *AuthzMeta `protobuf:"bytes,3,opt,name=authz,proto3" json:"authz,omitempty"` + ExpectedKeyAuthorization string `protobuf:"bytes,4,opt,name=expectedKeyAuthorization,proto3" json:"expectedKeyAuthorization,omitempty"` } func (x *PerformValidationRequest) Reset() { @@ -207,6 +210,13 @@ func (x *PerformValidationRequest) GetDnsName() string { return "" } +func (x *PerformValidationRequest) GetIdentifier() *proto.Identifier { + if x != nil { + return x.Identifier + } + return nil +} + func (x *PerformValidationRequest) GetChallenge() *proto.Challenge { if x != nil { return x.Challenge @@ -376,53 +386,57 @@ var file_va_proto_rawDesc = []byte{ 0x6c, 0x65, 0x6d, 0x12, 0x20, 0x0a, 0x0b, 0x70, 0x65, 0x72, 0x73, 0x70, 0x65, 0x63, 0x74, 0x69, 0x76, 0x65, 0x18, 0x03, 0x20, 0x01, 0x28, 0x09, 0x52, 0x0b, 0x70, 0x65, 0x72, 0x73, 0x70, 0x65, 0x63, 0x74, 0x69, 0x76, 0x65, 0x12, 0x10, 0x0a, 0x03, 0x72, 0x69, 0x72, 0x18, 0x04, 0x20, 0x01, - 0x28, 0x09, 0x52, 0x03, 0x72, 0x69, 0x72, 0x22, 0xc4, 0x01, 0x0a, 0x18, 0x50, 0x65, 0x72, 0x66, + 0x28, 0x09, 0x52, 0x03, 0x72, 0x69, 0x72, 0x22, 0xf6, 0x01, 0x0a, 0x18, 0x50, 0x65, 0x72, 0x66, 0x6f, 0x72, 0x6d, 0x56, 0x61, 0x6c, 0x69, 0x64, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x12, 0x18, 0x0a, 0x07, 0x64, 0x6e, 0x73, 0x4e, 0x61, 0x6d, 0x65, 0x18, - 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, 0x07, 0x64, 0x6e, 0x73, 0x4e, 0x61, 0x6d, 0x65, 0x12, 0x2d, - 0x0a, 0x09, 0x63, 0x68, 0x61, 0x6c, 0x6c, 0x65, 0x6e, 0x67, 0x65, 0x18, 0x02, 0x20, 0x01, 0x28, - 0x0b, 0x32, 0x0f, 0x2e, 0x63, 0x6f, 0x72, 0x65, 0x2e, 0x43, 0x68, 0x61, 0x6c, 0x6c, 0x65, 0x6e, - 0x67, 0x65, 0x52, 0x09, 0x63, 0x68, 0x61, 0x6c, 0x6c, 0x65, 0x6e, 0x67, 0x65, 0x12, 0x23, 0x0a, - 0x05, 0x61, 0x75, 0x74, 0x68, 0x7a, 0x18, 0x03, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x0d, 0x2e, 0x76, - 0x61, 0x2e, 0x41, 0x75, 0x74, 0x68, 0x7a, 0x4d, 0x65, 0x74, 0x61, 0x52, 0x05, 0x61, 0x75, 0x74, - 0x68, 0x7a, 0x12, 0x3a, 0x0a, 0x18, 0x65, 0x78, 0x70, 0x65, 0x63, 0x74, 0x65, 0x64, 0x4b, 0x65, - 0x79, 0x41, 0x75, 0x74, 0x68, 0x6f, 0x72, 0x69, 0x7a, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x18, 0x04, - 0x20, 0x01, 0x28, 0x09, 0x52, 0x18, 0x65, 0x78, 0x70, 0x65, 0x63, 0x74, 0x65, 0x64, 0x4b, 0x65, - 0x79, 0x41, 0x75, 0x74, 0x68, 0x6f, 0x72, 0x69, 0x7a, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x22, 0x31, - 0x0a, 0x09, 0x41, 0x75, 0x74, 0x68, 0x7a, 0x4d, 0x65, 0x74, 0x61, 0x12, 0x0e, 0x0a, 0x02, 0x69, - 0x64, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, 0x02, 0x69, 0x64, 0x12, 0x14, 0x0a, 0x05, 0x72, - 0x65, 0x67, 0x49, 0x44, 0x18, 0x02, 0x20, 0x01, 0x28, 0x03, 0x52, 0x05, 0x72, 0x65, 0x67, 0x49, - 0x44, 0x22, 0xa8, 0x01, 0x0a, 0x10, 0x56, 0x61, 0x6c, 0x69, 0x64, 0x61, 0x74, 0x69, 0x6f, 0x6e, - 0x52, 0x65, 0x73, 0x75, 0x6c, 0x74, 0x12, 0x30, 0x0a, 0x07, 0x72, 0x65, 0x63, 0x6f, 0x72, 0x64, - 0x73, 0x18, 0x01, 0x20, 0x03, 0x28, 0x0b, 0x32, 0x16, 0x2e, 0x63, 0x6f, 0x72, 0x65, 0x2e, 0x56, - 0x61, 0x6c, 0x69, 0x64, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x52, 0x65, 0x63, 0x6f, 0x72, 0x64, 0x52, - 0x07, 0x72, 0x65, 0x63, 0x6f, 0x72, 0x64, 0x73, 0x12, 0x2e, 0x0a, 0x07, 0x70, 0x72, 0x6f, 0x62, - 0x6c, 0x65, 0x6d, 0x18, 0x02, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x14, 0x2e, 0x63, 0x6f, 0x72, 0x65, - 0x2e, 0x50, 0x72, 0x6f, 0x62, 0x6c, 0x65, 0x6d, 0x44, 0x65, 0x74, 0x61, 0x69, 0x6c, 0x73, 0x52, - 0x07, 0x70, 0x72, 0x6f, 0x62, 0x6c, 0x65, 0x6d, 0x12, 0x20, 0x0a, 0x0b, 0x70, 0x65, 0x72, 0x73, - 0x70, 0x65, 0x63, 0x74, 0x69, 0x76, 0x65, 0x18, 0x03, 0x20, 0x01, 0x28, 0x09, 0x52, 0x0b, 0x70, - 0x65, 0x72, 0x73, 0x70, 0x65, 0x63, 0x74, 0x69, 0x76, 0x65, 0x12, 0x10, 0x0a, 0x03, 0x72, 0x69, - 0x72, 0x18, 0x04, 0x20, 0x01, 0x28, 0x09, 0x52, 0x03, 0x72, 0x69, 0x72, 0x32, 0x8e, 0x01, 0x0a, - 0x02, 0x56, 0x41, 0x12, 0x49, 0x0a, 0x11, 0x50, 0x65, 0x72, 0x66, 0x6f, 0x72, 0x6d, 0x56, 0x61, - 0x6c, 0x69, 0x64, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x12, 0x1c, 0x2e, 0x76, 0x61, 0x2e, 0x50, 0x65, - 0x72, 0x66, 0x6f, 0x72, 0x6d, 0x56, 0x61, 0x6c, 0x69, 0x64, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x52, - 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x14, 0x2e, 0x76, 0x61, 0x2e, 0x56, 0x61, 0x6c, 0x69, - 0x64, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x52, 0x65, 0x73, 0x75, 0x6c, 0x74, 0x22, 0x00, 0x12, 0x3d, - 0x0a, 0x05, 0x44, 0x6f, 0x44, 0x43, 0x56, 0x12, 0x1c, 0x2e, 0x76, 0x61, 0x2e, 0x50, 0x65, 0x72, - 0x66, 0x6f, 0x72, 0x6d, 0x56, 0x61, 0x6c, 0x69, 0x64, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x52, 0x65, - 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x14, 0x2e, 0x76, 0x61, 0x2e, 0x56, 0x61, 0x6c, 0x69, 0x64, - 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x52, 0x65, 0x73, 0x75, 0x6c, 0x74, 0x22, 0x00, 0x32, 0x7e, 0x0a, - 0x03, 0x43, 0x41, 0x41, 0x12, 0x3d, 0x0a, 0x0a, 0x49, 0x73, 0x43, 0x41, 0x41, 0x56, 0x61, 0x6c, - 0x69, 0x64, 0x12, 0x15, 0x2e, 0x76, 0x61, 0x2e, 0x49, 0x73, 0x43, 0x41, 0x41, 0x56, 0x61, 0x6c, - 0x69, 0x64, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x16, 0x2e, 0x76, 0x61, 0x2e, 0x49, - 0x73, 0x43, 0x41, 0x41, 0x56, 0x61, 0x6c, 0x69, 0x64, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, - 0x65, 0x22, 0x00, 0x12, 0x38, 0x0a, 0x05, 0x44, 0x6f, 0x43, 0x41, 0x41, 0x12, 0x15, 0x2e, 0x76, - 0x61, 0x2e, 0x49, 0x73, 0x43, 0x41, 0x41, 0x56, 0x61, 0x6c, 0x69, 0x64, 0x52, 0x65, 0x71, 0x75, - 0x65, 0x73, 0x74, 0x1a, 0x16, 0x2e, 0x76, 0x61, 0x2e, 0x49, 0x73, 0x43, 0x41, 0x41, 0x56, 0x61, - 0x6c, 0x69, 0x64, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x22, 0x00, 0x42, 0x29, 0x5a, - 0x27, 0x67, 0x69, 0x74, 0x68, 0x75, 0x62, 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x6c, 0x65, 0x74, 0x73, - 0x65, 0x6e, 0x63, 0x72, 0x79, 0x70, 0x74, 0x2f, 0x62, 0x6f, 0x75, 0x6c, 0x64, 0x65, 0x72, 0x2f, - 0x76, 0x61, 0x2f, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, + 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, 0x07, 0x64, 0x6e, 0x73, 0x4e, 0x61, 0x6d, 0x65, 0x12, 0x30, + 0x0a, 0x0a, 0x69, 0x64, 0x65, 0x6e, 0x74, 0x69, 0x66, 0x69, 0x65, 0x72, 0x18, 0x05, 0x20, 0x01, + 0x28, 0x0b, 0x32, 0x10, 0x2e, 0x63, 0x6f, 0x72, 0x65, 0x2e, 0x49, 0x64, 0x65, 0x6e, 0x74, 0x69, + 0x66, 0x69, 0x65, 0x72, 0x52, 0x0a, 0x69, 0x64, 0x65, 0x6e, 0x74, 0x69, 0x66, 0x69, 0x65, 0x72, + 0x12, 0x2d, 0x0a, 0x09, 0x63, 0x68, 0x61, 0x6c, 0x6c, 0x65, 0x6e, 0x67, 0x65, 0x18, 0x02, 0x20, + 0x01, 0x28, 0x0b, 0x32, 0x0f, 0x2e, 0x63, 0x6f, 0x72, 0x65, 0x2e, 0x43, 0x68, 0x61, 0x6c, 0x6c, + 0x65, 0x6e, 0x67, 0x65, 0x52, 0x09, 0x63, 0x68, 0x61, 0x6c, 0x6c, 0x65, 0x6e, 0x67, 0x65, 0x12, + 0x23, 0x0a, 0x05, 0x61, 0x75, 0x74, 0x68, 0x7a, 0x18, 0x03, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x0d, + 0x2e, 0x76, 0x61, 0x2e, 0x41, 0x75, 0x74, 0x68, 0x7a, 0x4d, 0x65, 0x74, 0x61, 0x52, 0x05, 0x61, + 0x75, 0x74, 0x68, 0x7a, 0x12, 0x3a, 0x0a, 0x18, 0x65, 0x78, 0x70, 0x65, 0x63, 0x74, 0x65, 0x64, + 0x4b, 0x65, 0x79, 0x41, 0x75, 0x74, 0x68, 0x6f, 0x72, 0x69, 0x7a, 0x61, 0x74, 0x69, 0x6f, 0x6e, + 0x18, 0x04, 0x20, 0x01, 0x28, 0x09, 0x52, 0x18, 0x65, 0x78, 0x70, 0x65, 0x63, 0x74, 0x65, 0x64, + 0x4b, 0x65, 0x79, 0x41, 0x75, 0x74, 0x68, 0x6f, 0x72, 0x69, 0x7a, 0x61, 0x74, 0x69, 0x6f, 0x6e, + 0x22, 0x31, 0x0a, 0x09, 0x41, 0x75, 0x74, 0x68, 0x7a, 0x4d, 0x65, 0x74, 0x61, 0x12, 0x0e, 0x0a, + 0x02, 0x69, 0x64, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, 0x02, 0x69, 0x64, 0x12, 0x14, 0x0a, + 0x05, 0x72, 0x65, 0x67, 0x49, 0x44, 0x18, 0x02, 0x20, 0x01, 0x28, 0x03, 0x52, 0x05, 0x72, 0x65, + 0x67, 0x49, 0x44, 0x22, 0xa8, 0x01, 0x0a, 0x10, 0x56, 0x61, 0x6c, 0x69, 0x64, 0x61, 0x74, 0x69, + 0x6f, 0x6e, 0x52, 0x65, 0x73, 0x75, 0x6c, 0x74, 0x12, 0x30, 0x0a, 0x07, 0x72, 0x65, 0x63, 0x6f, + 0x72, 0x64, 0x73, 0x18, 0x01, 0x20, 0x03, 0x28, 0x0b, 0x32, 0x16, 0x2e, 0x63, 0x6f, 0x72, 0x65, + 0x2e, 0x56, 0x61, 0x6c, 0x69, 0x64, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x52, 0x65, 0x63, 0x6f, 0x72, + 0x64, 0x52, 0x07, 0x72, 0x65, 0x63, 0x6f, 0x72, 0x64, 0x73, 0x12, 0x2e, 0x0a, 0x07, 0x70, 0x72, + 0x6f, 0x62, 0x6c, 0x65, 0x6d, 0x18, 0x02, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x14, 0x2e, 0x63, 0x6f, + 0x72, 0x65, 0x2e, 0x50, 0x72, 0x6f, 0x62, 0x6c, 0x65, 0x6d, 0x44, 0x65, 0x74, 0x61, 0x69, 0x6c, + 0x73, 0x52, 0x07, 0x70, 0x72, 0x6f, 0x62, 0x6c, 0x65, 0x6d, 0x12, 0x20, 0x0a, 0x0b, 0x70, 0x65, + 0x72, 0x73, 0x70, 0x65, 0x63, 0x74, 0x69, 0x76, 0x65, 0x18, 0x03, 0x20, 0x01, 0x28, 0x09, 0x52, + 0x0b, 0x70, 0x65, 0x72, 0x73, 0x70, 0x65, 0x63, 0x74, 0x69, 0x76, 0x65, 0x12, 0x10, 0x0a, 0x03, + 0x72, 0x69, 0x72, 0x18, 0x04, 0x20, 0x01, 0x28, 0x09, 0x52, 0x03, 0x72, 0x69, 0x72, 0x32, 0x8e, + 0x01, 0x0a, 0x02, 0x56, 0x41, 0x12, 0x49, 0x0a, 0x11, 0x50, 0x65, 0x72, 0x66, 0x6f, 0x72, 0x6d, + 0x56, 0x61, 0x6c, 0x69, 0x64, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x12, 0x1c, 0x2e, 0x76, 0x61, 0x2e, + 0x50, 0x65, 0x72, 0x66, 0x6f, 0x72, 0x6d, 0x56, 0x61, 0x6c, 0x69, 0x64, 0x61, 0x74, 0x69, 0x6f, + 0x6e, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x14, 0x2e, 0x76, 0x61, 0x2e, 0x56, 0x61, + 0x6c, 0x69, 0x64, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x52, 0x65, 0x73, 0x75, 0x6c, 0x74, 0x22, 0x00, + 0x12, 0x3d, 0x0a, 0x05, 0x44, 0x6f, 0x44, 0x43, 0x56, 0x12, 0x1c, 0x2e, 0x76, 0x61, 0x2e, 0x50, + 0x65, 0x72, 0x66, 0x6f, 0x72, 0x6d, 0x56, 0x61, 0x6c, 0x69, 0x64, 0x61, 0x74, 0x69, 0x6f, 0x6e, + 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x14, 0x2e, 0x76, 0x61, 0x2e, 0x56, 0x61, 0x6c, + 0x69, 0x64, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x52, 0x65, 0x73, 0x75, 0x6c, 0x74, 0x22, 0x00, 0x32, + 0x7e, 0x0a, 0x03, 0x43, 0x41, 0x41, 0x12, 0x3d, 0x0a, 0x0a, 0x49, 0x73, 0x43, 0x41, 0x41, 0x56, + 0x61, 0x6c, 0x69, 0x64, 0x12, 0x15, 0x2e, 0x76, 0x61, 0x2e, 0x49, 0x73, 0x43, 0x41, 0x41, 0x56, + 0x61, 0x6c, 0x69, 0x64, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x16, 0x2e, 0x76, 0x61, + 0x2e, 0x49, 0x73, 0x43, 0x41, 0x41, 0x56, 0x61, 0x6c, 0x69, 0x64, 0x52, 0x65, 0x73, 0x70, 0x6f, + 0x6e, 0x73, 0x65, 0x22, 0x00, 0x12, 0x38, 0x0a, 0x05, 0x44, 0x6f, 0x43, 0x41, 0x41, 0x12, 0x15, + 0x2e, 0x76, 0x61, 0x2e, 0x49, 0x73, 0x43, 0x41, 0x41, 0x56, 0x61, 0x6c, 0x69, 0x64, 0x52, 0x65, + 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x16, 0x2e, 0x76, 0x61, 0x2e, 0x49, 0x73, 0x43, 0x41, 0x41, + 0x56, 0x61, 0x6c, 0x69, 0x64, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x22, 0x00, 0x42, + 0x29, 0x5a, 0x27, 0x67, 0x69, 0x74, 0x68, 0x75, 0x62, 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x6c, 0x65, + 0x74, 0x73, 0x65, 0x6e, 0x63, 0x72, 0x79, 0x70, 0x74, 0x2f, 0x62, 0x6f, 0x75, 0x6c, 0x64, 0x65, + 0x72, 0x2f, 0x76, 0x61, 0x2f, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, + 0x6f, 0x33, } var ( @@ -445,28 +459,30 @@ var file_va_proto_goTypes = []interface{}{ (*AuthzMeta)(nil), // 3: va.AuthzMeta (*ValidationResult)(nil), // 4: va.ValidationResult (*proto.ProblemDetails)(nil), // 5: core.ProblemDetails - (*proto.Challenge)(nil), // 6: core.Challenge - (*proto.ValidationRecord)(nil), // 7: core.ValidationRecord + (*proto.Identifier)(nil), // 6: core.Identifier + (*proto.Challenge)(nil), // 7: core.Challenge + (*proto.ValidationRecord)(nil), // 8: core.ValidationRecord } var file_va_proto_depIdxs = []int32{ - 5, // 0: va.IsCAAValidResponse.problem:type_name -> core.ProblemDetails - 6, // 1: va.PerformValidationRequest.challenge:type_name -> core.Challenge - 3, // 2: va.PerformValidationRequest.authz:type_name -> va.AuthzMeta - 7, // 3: va.ValidationResult.records:type_name -> core.ValidationRecord - 5, // 4: va.ValidationResult.problem:type_name -> core.ProblemDetails - 2, // 5: va.VA.PerformValidation:input_type -> va.PerformValidationRequest - 2, // 6: va.VA.DoDCV:input_type -> va.PerformValidationRequest - 0, // 7: va.CAA.IsCAAValid:input_type -> va.IsCAAValidRequest - 0, // 8: va.CAA.DoCAA:input_type -> va.IsCAAValidRequest - 4, // 9: va.VA.PerformValidation:output_type -> va.ValidationResult - 4, // 10: va.VA.DoDCV:output_type -> va.ValidationResult - 1, // 11: va.CAA.IsCAAValid:output_type -> va.IsCAAValidResponse - 1, // 12: va.CAA.DoCAA:output_type -> va.IsCAAValidResponse - 9, // [9:13] is the sub-list for method output_type - 5, // [5:9] is the sub-list for method input_type - 5, // [5:5] is the sub-list for extension type_name - 5, // [5:5] is the sub-list for extension extendee - 0, // [0:5] is the sub-list for field type_name + 5, // 0: va.IsCAAValidResponse.problem:type_name -> core.ProblemDetails + 6, // 1: va.PerformValidationRequest.identifier:type_name -> core.Identifier + 7, // 2: va.PerformValidationRequest.challenge:type_name -> core.Challenge + 3, // 3: va.PerformValidationRequest.authz:type_name -> va.AuthzMeta + 8, // 4: va.ValidationResult.records:type_name -> core.ValidationRecord + 5, // 5: va.ValidationResult.problem:type_name -> core.ProblemDetails + 2, // 6: va.VA.PerformValidation:input_type -> va.PerformValidationRequest + 2, // 7: va.VA.DoDCV:input_type -> va.PerformValidationRequest + 0, // 8: va.CAA.IsCAAValid:input_type -> va.IsCAAValidRequest + 0, // 9: va.CAA.DoCAA:input_type -> va.IsCAAValidRequest + 4, // 10: va.VA.PerformValidation:output_type -> va.ValidationResult + 4, // 11: va.VA.DoDCV:output_type -> va.ValidationResult + 1, // 12: va.CAA.IsCAAValid:output_type -> va.IsCAAValidResponse + 1, // 13: va.CAA.DoCAA:output_type -> va.IsCAAValidResponse + 10, // [10:14] is the sub-list for method output_type + 6, // [6:10] is the sub-list for method input_type + 6, // [6:6] is the sub-list for extension type_name + 6, // [6:6] is the sub-list for extension extendee + 0, // [0:6] is the sub-list for field type_name } func init() { file_va_proto_init() } diff --git a/va/proto/va.proto b/va/proto/va.proto index 44fa5c6e6e1..aa70aaea11b 100644 --- a/va/proto/va.proto +++ b/va/proto/va.proto @@ -16,6 +16,9 @@ service CAA { } message IsCAAValidRequest { + // TODO(#7311): Accept an identifier instead of a domain (purely for + // consistency, because only DNS identifiers support CAA checks). + // // NOTE: Domain may be a name with a wildcard prefix (e.g. `*.example.com`) string domain = 1; string validationMethod = 2; @@ -31,7 +34,10 @@ message IsCAAValidResponse { } message PerformValidationRequest { + // Next unused field number: 6 + // TODO(#7311): dnsNames are being deprecated in favour of identifiers. string dnsName = 1; + core.Identifier identifier = 5; core.Challenge challenge = 2; AuthzMeta authz = 3; string expectedKeyAuthorization = 4; diff --git a/va/tlsalpn.go b/va/tlsalpn.go index 034f7e0975a..b70426573f5 100644 --- a/va/tlsalpn.go +++ b/va/tlsalpn.go @@ -16,6 +16,7 @@ import ( "strconv" "strings" + "github.com/letsencrypt/boulder/bdns" "github.com/letsencrypt/boulder/core" berrors "github.com/letsencrypt/boulder/errors" "github.com/letsencrypt/boulder/identifier" @@ -56,30 +57,40 @@ func certAltNames(cert *x509.Certificate) []string { return names } +// TODO(#7311): Identifiers need testing here. func (va *ValidationAuthorityImpl) tryGetChallengeCert( ctx context.Context, - identifier identifier.ACMEIdentifier, + ident identifier.ACMEIdentifier, tlsConfig *tls.Config, ) (*x509.Certificate, *tls.ConnectionState, core.ValidationRecord, error) { - - allAddrs, resolvers, err := va.getAddrs(ctx, identifier.Value) validationRecord := core.ValidationRecord{ - DnsName: identifier.Value, - AddressesResolved: allAddrs, + DnsName: ident.Value, + AddressesResolved: []net.IP{}, Port: strconv.Itoa(va.tlsPort), - ResolverAddrs: resolvers, + ResolverAddrs: bdns.ResolverAddrs{}, } - if err != nil { - return nil, nil, validationRecord, err + + switch ident.Type { + case identifier.TypeDNS: + // Resolve IP addresses for the identifier + addrs, resolvers, err := va.getAddrs(ctx, ident.Value) + if err != nil { + return nil, nil, validationRecord, err + } + validationRecord.AddressesResolved, validationRecord.ResolverAddrs = addrs, resolvers + case identifier.TypeIP: + validationRecord.AddressesResolved = []net.IP{net.ParseIP(ident.Value)} + default: + return nil, nil, validationRecord, fmt.Errorf("Unknown identifier type: %s", ident.Type) } // Split the available addresses into v4 and v6 addresses - v4, v6 := availableAddresses(allAddrs) + v4, v6 := availableAddresses(validationRecord.AddressesResolved) addresses := append(v4, v6...) // This shouldn't happen, but be defensive about it anyway if len(addresses) < 1 { - return nil, nil, validationRecord, berrors.MalformedError("no IP addresses found for %q", identifier.Value) + return nil, nil, validationRecord, berrors.MalformedError("no IP addresses found for %q", ident.Value) } // If there is at least one IPv6 address then try it first @@ -87,7 +98,7 @@ func (va *ValidationAuthorityImpl) tryGetChallengeCert( address := net.JoinHostPort(v6[0].String(), validationRecord.Port) validationRecord.AddressUsed = v6[0] - cert, cs, err := va.getChallengeCert(ctx, address, identifier, tlsConfig) + cert, cs, err := va.getChallengeCert(ctx, address, ident, tlsConfig) // If there is no problem, return immediately if err == nil { @@ -114,7 +125,7 @@ func (va *ValidationAuthorityImpl) tryGetChallengeCert( // talking to the first IPv6 address, try the first IPv4 address validationRecord.AddressUsed = v4[0] address := net.JoinHostPort(v4[0].String(), validationRecord.Port) - cert, cs, err := va.getChallengeCert(ctx, address, identifier, tlsConfig) + cert, cs, err := va.getChallengeCert(ctx, address, ident, tlsConfig) return cert, cs, validationRecord, err } @@ -160,15 +171,30 @@ func (va *ValidationAuthorityImpl) getChallengeCert( return certs[0], &cs, nil } -func checkExpectedSAN(cert *x509.Certificate, name identifier.ACMEIdentifier) error { - if len(cert.DNSNames) != 1 { - return errors.New("wrong number of dNSNames") +// TODO(#7311): Identifiers need testing here. +func checkExpectedSAN(cert *x509.Certificate, ident identifier.ACMEIdentifier) error { + var certSAN []byte + switch ident.Type { + case identifier.TypeDNS: + if len(cert.DNSNames) != 1 || len(cert.IPAddresses) != 0 { + return errors.New("wrong number of identifiers") + } + certSAN = []byte(cert.DNSNames[0]) + case identifier.TypeIP: + if len(cert.IPAddresses) != 1 || len(cert.DNSNames) != 0 { + return errors.New("wrong number of identifiers") + } + certSAN = cert.IPAddresses[0] + default: + // This should never happen. The calling function should check the + // identifier type. + return errors.New("unsupported identifier type") } for _, ext := range cert.Extensions { if IdCeSubjectAltName.Equal(ext.Id) { expectedSANs, err := asn1.Marshal([]asn1.RawValue{ - {Tag: 2, Class: 2, Bytes: []byte(cert.DNSNames[0])}, + {Tag: 2, Class: 2, Bytes: certSAN}, }) if err != nil || !bytes.Equal(expectedSANs, ext.Value) { return errors.New("SAN extension does not match expected bytes") @@ -176,8 +202,8 @@ func checkExpectedSAN(cert *x509.Certificate, name identifier.ACMEIdentifier) er } } - if !strings.EqualFold(cert.DNSNames[0], name.Value) { - return errors.New("dNSName does not match expected identifier") + if !strings.EqualFold(string(certSAN), ident.Value) { + return errors.New("identifier does not match expected identifier") } return nil @@ -205,16 +231,17 @@ func checkAcceptableExtensions(exts []pkix.Extension, requiredOIDs []asn1.Object return nil } -func (va *ValidationAuthorityImpl) validateTLSALPN01(ctx context.Context, identifier identifier.ACMEIdentifier, keyAuthorization string) ([]core.ValidationRecord, error) { - if identifier.Type != "dns" { - va.log.Info(fmt.Sprintf("Identifier type for TLS-ALPN-01 was not DNS: %s", identifier)) - return nil, berrors.MalformedError("Identifier type for TLS-ALPN-01 was not DNS") +func (va *ValidationAuthorityImpl) validateTLSALPN01(ctx context.Context, ident identifier.ACMEIdentifier, keyAuthorization string) ([]core.ValidationRecord, error) { + // TODO(#7311): This needs testing. + if ident.Type != identifier.TypeDNS && ident.Type != identifier.TypeIP { + va.log.Info(fmt.Sprintf("Identifier type for TLS-ALPN-01 challenge was not DNS or IP: %s", ident)) + return nil, berrors.MalformedError("Identifier type for TLS-ALPN-01 challenge was not DNS or IP") } - cert, cs, tvr, problem := va.tryGetChallengeCert(ctx, identifier, &tls.Config{ + cert, cs, tvr, problem := va.tryGetChallengeCert(ctx, ident, &tls.Config{ MinVersion: tls.VersionTLS12, NextProtos: []string{ACMETLS1Protocol}, - ServerName: identifier.Value, + ServerName: ident.Value, }) // Copy the single validationRecord into the slice that we have to return, and // get a reference to it so we can modify it if we have to. @@ -237,7 +264,7 @@ func (va *ValidationAuthorityImpl) validateTLSALPN01(ctx context.Context, identi return berrors.UnauthorizedError( "Incorrect validation certificate for %s challenge. "+ "Requested %s from %s. %s", - core.ChallengeTypeTLSALPN01, identifier.Value, hostPort, msg) + core.ChallengeTypeTLSALPN01, ident.Value, hostPort, msg) } // The certificate must be self-signed. @@ -259,8 +286,8 @@ func (va *ValidationAuthorityImpl) validateTLSALPN01(ctx context.Context, identi } // The certificate returned must have a subjectAltName extension containing - // only the dNSName being validated and no other entries. - err = checkExpectedSAN(cert, identifier) + // only the identifier being validated and no other entries. + err = checkExpectedSAN(cert, ident) if err != nil { names := strings.Join(certAltNames(cert), ", ") return validationRecords, badCertErr( diff --git a/va/tlsalpn_test.go b/va/tlsalpn_test.go index fc8d5385997..7d4e00e4cb9 100644 --- a/va/tlsalpn_test.go +++ b/va/tlsalpn_test.go @@ -29,7 +29,7 @@ import ( "github.com/letsencrypt/boulder/test" ) -func tlsCertTemplate(names []string) *x509.Certificate { +func tlsCertTemplate(names []string, ips []net.IP) *x509.Certificate { return &x509.Certificate{ SerialNumber: big.NewInt(1337), Subject: pkix.Name{ @@ -42,12 +42,13 @@ func tlsCertTemplate(names []string) *x509.Certificate { ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, BasicConstraintsValid: true, - DNSNames: names, + DNSNames: names, + IPAddresses: ips, } } -func makeACert(names []string) *tls.Certificate { - template := tlsCertTemplate(names) +func makeACert(names []string, ips []net.IP) *tls.Certificate { + template := tlsCertTemplate(names, ips) certBytes, _ := x509.CreateCertificate(rand.Reader, template, template, &TheKey.PublicKey, &TheKey) return &tls.Certificate{ Certificate: [][]byte{certBytes}, @@ -59,7 +60,7 @@ func makeACert(names []string) *tls.Certificate { func tlssniSrvWithNames(t *testing.T, names ...string) *httptest.Server { t.Helper() - cert := makeACert(names) + cert := makeACert(names, nil) tlsConfig := &tls.Config{ Certificates: []tls.Certificate{*cert}, ClientAuth: tls.NoClientCert, @@ -105,8 +106,9 @@ func tlsalpn01Srv( keyAuthorization string, oid asn1.ObjectIdentifier, tlsVersion uint16, - names ...string) (*httptest.Server, error) { - template := tlsCertTemplate(names) + names []string, + ips []net.IP) (*httptest.Server, error) { + template := tlsCertTemplate(names, ips) shasum := sha256.Sum256([]byte(keyAuthorization)) encHash, err := asn1.Marshal(shasum[:]) @@ -133,27 +135,13 @@ func tlsalpn01Srv( return tlsalpn01SrvWithCert(t, acmeCert, tlsVersion), nil } -func TestTLSALPN01FailIP(t *testing.T) { - hs, err := tlsalpn01Srv(t, expectedKeyAuthorization, IdPeAcmeIdentifier, 0, "expected") - test.AssertNotError(t, err, "Error creating test server") - - va, _ := setup(hs, "", nil, nil) - - _, err = va.validateTLSALPN01(ctx, identifier.NewIP(netip.MustParseAddr("127.0.0.1")), expectedKeyAuthorization) - if err == nil { - t.Fatalf("IdentifierType IP shouldn't have worked.") - } - prob := detailedError(err) - test.AssertEquals(t, prob.Type, probs.MalformedProblem) -} - func slowTLSSrv() *httptest.Server { server := httptest.NewUnstartedServer(http.DefaultServeMux) server.TLS = &tls.Config{ NextProtos: []string{"http/1.1", ACMETLS1Protocol}, GetCertificate: func(*tls.ClientHelloInfo) (*tls.Certificate, error) { time.Sleep(100 * time.Millisecond) - return makeACert([]string{"nomatter"}), nil + return makeACert([]string{"nomatter"}, nil), nil }, } server.StartTLS() @@ -246,7 +234,7 @@ func TestTLSALPN01DialTimeout(t *testing.T) { } func TestTLSALPN01Refused(t *testing.T) { - hs, err := tlsalpn01Srv(t, expectedKeyAuthorization, IdPeAcmeIdentifier, 0, "expected") + hs, err := tlsalpn01Srv(t, expectedKeyAuthorization, IdPeAcmeIdentifier, 0, []string{"expected"}, nil) test.AssertNotError(t, err, "Error creating test server") va, _ := setup(hs, "", nil, nil) @@ -265,7 +253,7 @@ func TestTLSALPN01Refused(t *testing.T) { } func TestTLSALPN01TalkingToHTTP(t *testing.T) { - hs, err := tlsalpn01Srv(t, expectedKeyAuthorization, IdPeAcmeIdentifier, 0, "expected") + hs, err := tlsalpn01Srv(t, expectedKeyAuthorization, IdPeAcmeIdentifier, 0, []string{"expected"}, nil) test.AssertNotError(t, err, "Error creating test server") va, _ := setup(hs, "", nil, nil) @@ -382,8 +370,8 @@ func TestCertNames(t *testing.T) { test.AssertDeepEquals(t, actual, expected) } -func TestTLSALPN01Success(t *testing.T) { - hs, err := tlsalpn01Srv(t, expectedKeyAuthorization, IdPeAcmeIdentifier, 0, "expected") +func TestTLSALPN01SuccessDNS(t *testing.T) { + hs, err := tlsalpn01Srv(t, expectedKeyAuthorization, IdPeAcmeIdentifier, 0, []string{"expected"}, nil) test.AssertNotError(t, err, "Error creating test server") va, _ := setup(hs, "", nil, nil) @@ -398,6 +386,22 @@ func TestTLSALPN01Success(t *testing.T) { hs.Close() } +func TestTLSALPN01SuccessIP(t *testing.T) { + hs, err := tlsalpn01Srv(t, expectedKeyAuthorization, IdPeAcmeIdentifier, 0, nil, []net.IP{net.ParseIP("127.0.0.1")}) + test.AssertNotError(t, err, "Error creating test server") + + va, _ := setup(hs, "", nil, nil) + + _, prob := va.validateTLSALPN01(ctx, identifier.NewIP(netip.MustParseAddr("127.0.0.1")), expectedKeyAuthorization) + if prob != nil { + t.Errorf("Validation failed: %v", prob) + } + test.AssertMetricWithLabelsEquals( + t, va.metrics.tlsALPNOIDCounter, prometheus.Labels{"oid": IdPeAcmeIdentifier.String()}, 1) + + hs.Close() +} + func TestTLSALPN01ObsoleteFailure(t *testing.T) { // NOTE: unfortunately another document claimed the OID we were using in // draft-ietf-acme-tls-alpn-01 for their own extension and IANA chose to @@ -408,7 +412,7 @@ func TestTLSALPN01ObsoleteFailure(t *testing.T) { // id-pe OID + 30 (acmeIdentifier) + 1 (v1) IdPeAcmeIdentifierV1Obsolete := asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 1, 30, 1} - hs, err := tlsalpn01Srv(t, expectedKeyAuthorization, IdPeAcmeIdentifierV1Obsolete, 0, "expected") + hs, err := tlsalpn01Srv(t, expectedKeyAuthorization, IdPeAcmeIdentifierV1Obsolete, 0, []string{"expected"}, nil) test.AssertNotError(t, err, "Error creating test server") va, _ := setup(hs, "", nil, nil) @@ -420,7 +424,7 @@ func TestTLSALPN01ObsoleteFailure(t *testing.T) { func TestValidateTLSALPN01BadChallenge(t *testing.T) { badKeyAuthorization := ka("bad token") - hs, err := tlsalpn01Srv(t, badKeyAuthorization, IdPeAcmeIdentifier, 0, "expected") + hs, err := tlsalpn01Srv(t, badKeyAuthorization, IdPeAcmeIdentifier, 0, []string{"expected"}, nil) test.AssertNotError(t, err, "Error creating test server") va, _ := setup(hs, "", nil, nil) @@ -472,7 +476,7 @@ func TestValidateTLSALPN01UnawareSrv(t *testing.T) { // a host that returns a certificate with a SAN/CN that contains invalid UTF-8 // will result in a problem with the invalid UTF-8. func TestValidateTLSALPN01BadUTFSrv(t *testing.T) { - _, err := tlsalpn01Srv(t, expectedKeyAuthorization, IdPeAcmeIdentifier, 0, "expected", "\xf0\x28\x8c\xbc") + _, err := tlsalpn01Srv(t, expectedKeyAuthorization, IdPeAcmeIdentifier, 0, []string{"expected", "\xf0\x28\x8c\xbc"}, nil) test.AssertContains(t, err.Error(), "cannot be encoded as an IA5String") } @@ -482,7 +486,7 @@ func TestValidateTLSALPN01BadUTFSrv(t *testing.T) { // will result in an Unauthorized problem func TestValidateTLSALPN01MalformedExtnValue(t *testing.T) { names := []string{"expected"} - template := tlsCertTemplate(names) + template := tlsCertTemplate(names, nil) wrongTypeDER, _ := asn1.Marshal("a string") wrongLengthDER, _ := asn1.Marshal(make([]byte, 31)) @@ -544,7 +548,7 @@ func TestTLSALPN01TLSVersion(t *testing.T) { }, } { // Create a server that only negotiates the given TLS version - hs, err := tlsalpn01Srv(t, expectedKeyAuthorization, IdPeAcmeIdentifier, tc.version, "expected") + hs, err := tlsalpn01Srv(t, expectedKeyAuthorization, IdPeAcmeIdentifier, tc.version, []string{"expected"}, nil) test.AssertNotError(t, err, "Error creating test server") va, _ := setup(hs, "", nil, nil) @@ -569,7 +573,7 @@ func TestTLSALPN01TLSVersion(t *testing.T) { func TestTLSALPN01WrongName(t *testing.T) { // Create a cert with a different name from what we're validating - hs, err := tlsalpn01Srv(t, expectedKeyAuthorization, IdPeAcmeIdentifier, tls.VersionTLS12, "incorrect") + hs, err := tlsalpn01Srv(t, expectedKeyAuthorization, IdPeAcmeIdentifier, tls.VersionTLS12, []string{"incorrect"}, nil) test.AssertNotError(t, err, "failed to set up tls-alpn-01 server") va, _ := setup(hs, "", nil, nil) @@ -580,7 +584,7 @@ func TestTLSALPN01WrongName(t *testing.T) { func TestTLSALPN01ExtraNames(t *testing.T) { // Create a cert with two names when we only want to validate one. - hs, err := tlsalpn01Srv(t, expectedKeyAuthorization, IdPeAcmeIdentifier, tls.VersionTLS12, "expected", "extra") + hs, err := tlsalpn01Srv(t, expectedKeyAuthorization, IdPeAcmeIdentifier, tls.VersionTLS12, []string{"expected", "extra"}, nil) test.AssertNotError(t, err, "failed to set up tls-alpn-01 server") va, _ := setup(hs, "", nil, nil) diff --git a/va/va.go b/va/va.go index a1e2cd4492e..1bcbb0b1529 100644 --- a/va/va.go +++ b/va/va.go @@ -299,7 +299,7 @@ func maxAllowedFailures(perspectiveCount int) int { type verificationRequestEvent struct { AuthzID string Requester int64 - Identifier string + Identifier identifier.ACMEIdentifier Challenge core.Challenge Error string `json:",omitempty"` InternalError string `json:",omitempty"` @@ -422,8 +422,10 @@ func (va *ValidationAuthorityImpl) validateChallenge( token string, keyAuthorization string, ) ([]core.ValidationRecord, error) { - // Strip a (potential) leading wildcard token from the identifier. - ident.Value = strings.TrimPrefix(ident.Value, "*.") + if ident.Type == identifier.TypeDNS { + // Strip a (potential) leading wildcard token from the identifier. + ident.Value = strings.TrimPrefix(ident.Value, "*.") + } switch kind { case core.ChallengeTypeHTTP01: @@ -676,7 +678,12 @@ func (va *ValidationAuthorityImpl) performLocalValidation( // also contains Problems. This method does NOT implement Multi-Perspective // Issuance Corroboration as defined in BRs Sections 3.2.2.9 and 5.4.1. func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *vapb.PerformValidationRequest) (*vapb.ValidationResult, error) { - if core.IsAnyNilOrZero(req, req.DnsName, req.Challenge, req.Authz, req.ExpectedKeyAuthorization) { + ident := req.Identifier + if ident == nil { + ident = identifier.NewDNS(req.DnsName).AsProto() + } + + if core.IsAnyNilOrZero(req, ident, req.Challenge, req.Authz, req.ExpectedKeyAuthorization) { return nil, berrors.InternalServerError("Incomplete validation request") } @@ -699,7 +706,7 @@ func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *v logEvent := verificationRequestEvent{ AuthzID: req.Authz.Id, Requester: req.Authz.RegID, - Identifier: req.DnsName, + Identifier: identifier.FromProto(ident), Challenge: chall, } defer func() { @@ -732,7 +739,7 @@ func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *v // was successful or not, and cannot themselves fail. records, err := va.performLocalValidation( ctx, - identifier.NewDNS(req.DnsName), + identifier.FromProto(ident), req.Authz.RegID, chall.Type, chall.Token, diff --git a/va/va_test.go b/va/va_test.go index c54fa680b99..cf36fdfd2c9 100644 --- a/va/va_test.go +++ b/va/va_test.go @@ -85,9 +85,9 @@ func TestMain(m *testing.M) { var accountURIPrefixes = []string{"http://boulder.service.consul:4000/acme/reg/"} -func createValidationRequest(domain string, challengeType core.AcmeChallenge) *vapb.PerformValidationRequest { +func createValidationRequest(ident identifier.ACMEIdentifier, challengeType core.AcmeChallenge) *vapb.PerformValidationRequest { return &vapb.PerformValidationRequest{ - DnsName: domain, + Identifier: ident.AsProto(), Challenge: &corepb.Challenge{ Type: string(challengeType), Status: string(core.StatusPending), @@ -402,7 +402,7 @@ func TestPerformValidationWithMismatchedRemoteVAPerspectives(t *testing.T) { t.Parallel() va, mockLog := setup(nil, "", remoteVAs, nil) - req := createValidationRequest("good-dns01.com", core.ChallengeTypeDNS01) + req := createValidationRequest(identifier.NewDNS("good-dns01.com"), core.ChallengeTypeDNS01) res, _ := tc.validationFunc(context.Background(), va, req) test.AssertNotNil(t, res.GetProblem(), "validation succeeded with mismatched remote VA perspectives") test.AssertEquals(t, len(mockLog.GetAllMatching("Expected perspective")), 2) @@ -445,7 +445,7 @@ func TestPerformValidationWithMismatchedRemoteVARIRs(t *testing.T) { t.Parallel() va, mockLog := setup(nil, "", remoteVAs, nil) - req := createValidationRequest("good-dns01.com", core.ChallengeTypeDNS01) + req := createValidationRequest(identifier.NewDNS("good-dns01.com"), core.ChallengeTypeDNS01) res, _ := tc.validationFunc(context.Background(), va, req) test.AssertNotNil(t, res.GetProblem(), "validation succeeded with mismatched remote VA perspectives") test.AssertEquals(t, len(mockLog.GetAllMatching("Expected perspective")), 2) @@ -456,7 +456,7 @@ func TestPerformValidationWithMismatchedRemoteVARIRs(t *testing.T) { func TestValidateMalformedChallenge(t *testing.T) { va, _ := setup(nil, "", nil, nil) - _, err := va.validateChallenge(ctx, dnsi("example.com"), "fake-type-01", expectedToken, expectedKeyAuthorization) + _, err := va.validateChallenge(ctx, identifier.NewDNS("example.com"), "fake-type-01", expectedToken, expectedKeyAuthorization) prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.MalformedProblem) @@ -484,7 +484,7 @@ func TestPerformValidationInvalid(t *testing.T) { t.Run(tc.validationFuncName, func(t *testing.T) { t.Parallel() - req := createValidationRequest("foo.com", core.ChallengeTypeDNS01) + req := createValidationRequest(identifier.NewDNS("foo.com"), core.ChallengeTypeDNS01) res, _ := tc.validationFunc(context.Background(), va, req) test.Assert(t, res.Problem != nil, "validation succeeded") if tc.validationFuncName == "PerformValidation" { @@ -533,7 +533,7 @@ func TestInternalErrorLogged(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 1*time.Millisecond) defer cancel() - req := createValidationRequest("nonexistent.com", core.ChallengeTypeHTTP01) + req := createValidationRequest(identifier.NewDNS("nonexistent.com"), core.ChallengeTypeHTTP01) _, err := tc.validationFunc(ctx, va, req) test.AssertNotError(t, err, "failed validation should not be an error") matchingLogs := mockLog.GetAllMatching( @@ -567,7 +567,7 @@ func TestPerformValidationValid(t *testing.T) { va, mockLog := setup(nil, "", nil, nil) // create a challenge with well known token - req := createValidationRequest("good-dns01.com", core.ChallengeTypeDNS01) + req := createValidationRequest(identifier.NewDNS("good-dns01.com"), core.ChallengeTypeDNS01) res, _ := tc.validationFunc(context.Background(), va, req) test.Assert(t, res.Problem == nil, fmt.Sprintf("validation failed: %#v", res.Problem)) if tc.validationFuncName == "PerformValidation" { @@ -591,7 +591,7 @@ func TestPerformValidationValid(t *testing.T) { if len(resultLog) != 1 { t.Fatalf("Wrong number of matching lines for 'Validation result'") } - if !strings.Contains(resultLog[0], `"Identifier":"good-dns01.com"`) { + if !strings.Contains(resultLog[0], `"Identifier":{"type":"dns","value":"good-dns01.com"}`) { t.Error("PerformValidation didn't log validation identifier.") } }) @@ -600,6 +600,9 @@ func TestPerformValidationValid(t *testing.T) { // TestPerformValidationWildcard tests that the VA properly strips the `*.` // prefix from a wildcard name provided to the PerformValidation function. +// +// TODO(#7311): This or another test should check that an IP address identifier +// with a wildcard is rejected. func TestPerformValidationWildcard(t *testing.T) { t.Parallel() @@ -624,7 +627,7 @@ func TestPerformValidationWildcard(t *testing.T) { va, mockLog := setup(nil, "", nil, nil) // create a challenge with well known token - req := createValidationRequest("*.good-dns01.com", core.ChallengeTypeDNS01) + req := createValidationRequest(identifier.NewDNS("*.good-dns01.com"), core.ChallengeTypeDNS01) // perform a validation for a wildcard name res, _ := tc.validationFunc(context.Background(), va, req) test.Assert(t, res.Problem == nil, fmt.Sprintf("validation failed: %#v", res.Problem)) @@ -651,7 +654,7 @@ func TestPerformValidationWildcard(t *testing.T) { } // We expect that the top level Identifier reflect the wildcard name - if !strings.Contains(resultLog[0], `"Identifier":"*.good-dns01.com"`) { + if !strings.Contains(resultLog[0], `"Identifier":{"type":"dns","value":"*.good-dns01.com"}`) { t.Errorf("PerformValidation didn't log correct validation identifier.") } // We expect that the ValidationRecord contain the correct non-wildcard @@ -668,7 +671,7 @@ func TestDCVAndCAASequencing(t *testing.T) { // When validation succeeds, CAA should be checked. mockLog.Clear() - req := createValidationRequest("good-dns01.com", core.ChallengeTypeDNS01) + req := createValidationRequest(identifier.NewDNS("good-dns01.com"), core.ChallengeTypeDNS01) res, err := va.PerformValidation(context.Background(), req) test.AssertNotError(t, err, "performing validation") test.Assert(t, res.Problem == nil, fmt.Sprintf("validation failed: %#v", res.Problem)) @@ -677,7 +680,7 @@ func TestDCVAndCAASequencing(t *testing.T) { // When validation fails, CAA should be skipped. mockLog.Clear() - req = createValidationRequest("bad-dns01.com", core.ChallengeTypeDNS01) + req = createValidationRequest(identifier.NewDNS("bad-dns01.com"), core.ChallengeTypeDNS01) res, err = va.PerformValidation(context.Background(), req) test.AssertNotError(t, err, "performing validation") test.Assert(t, res.Problem != nil, "validation succeeded") @@ -753,7 +756,7 @@ func TestMultiVA(t *testing.T) { t.Parallel() // Create a new challenge to use for the httpSrv - req := createValidationRequest("localhost", core.ChallengeTypeHTTP01) + req := createValidationRequest(identifier.NewDNS("localhost"), core.ChallengeTypeHTTP01) brokenVA := RemoteClients{ VAClient: brokenRemoteVA{}, @@ -1032,7 +1035,7 @@ func TestMultiVAEarlyReturn(t *testing.T) { // Perform all validations start := time.Now() - req := createValidationRequest("localhost", core.ChallengeTypeHTTP01) + req := createValidationRequest(identifier.NewDNS("localhost"), core.ChallengeTypeHTTP01) res, _ := testFunc.impl(ctx, localVA, req) // It should always fail @@ -1090,7 +1093,7 @@ func TestMultiVAPolicy(t *testing.T) { localVA, _ := setupWithRemotes(ms.Server, pass, remoteConfs, nil) // Perform validation for a domain not in the disabledDomains list - req := createValidationRequest("letsencrypt.org", core.ChallengeTypeHTTP01) + req := createValidationRequest(identifier.NewDNS("letsencrypt.org"), core.ChallengeTypeHTTP01) res, _ := tc.validationFunc(ctx, localVA, req) // It should fail if res.Problem == nil { @@ -1131,7 +1134,7 @@ func TestMultiVALogging(t *testing.T) { defer ms.Close() va, _ := setupWithRemotes(ms.Server, pass, remoteConfs, nil) - req := createValidationRequest("letsencrypt.org", core.ChallengeTypeHTTP01) + req := createValidationRequest(identifier.NewDNS("letsencrypt.org"), core.ChallengeTypeHTTP01) res, err := tc.validationFunc(ctx, va, req) test.Assert(t, res.Problem == nil, fmt.Sprintf("validation failed with: %#v", res.Problem)) test.AssertNotError(t, err, "performing validation") @@ -1253,3 +1256,100 @@ func TestLogRemoteDifferentials(t *testing.T) { }) } } + +// TestPerformValidationDnsName modifies the PerformValidationRequest to test +// backward compatibility during the transition to using an Identifier instead +// of a DnsName. +func TestPerformValidationDnsName(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + validationFuncName string + validationFunc validationFuncRunner + identDomain string + transmogrifier func(*vapb.PerformValidationRequest) + expectLog string + }{ + { + name: "Identifier overrides DnsName for PerformValidation", + validationFuncName: "PerformValidation", + validationFunc: runPerformValidation, + identDomain: "good-dns01.com", + transmogrifier: func(req *vapb.PerformValidationRequest) { + req.DnsName = "good-dns02.com" + }, + expectLog: `"Identifier":{"type":"dns","value":"good-dns01.com"}`, + }, + { + name: "Identifier overrides DnsName for DoDCV", + validationFuncName: "DoDCV", + validationFunc: runDoDCV, + identDomain: "good-dns01.com", + transmogrifier: func(req *vapb.PerformValidationRequest) { + req.DnsName = "good-dns02.com" + }, + expectLog: `"Identifier":{"type":"dns","value":"good-dns01.com"}`, + }, + { + name: "No Identifier for PerformValidation", + validationFuncName: "PerformValidation", + validationFunc: runPerformValidation, + identDomain: "good-dns01.com", + transmogrifier: func(req *vapb.PerformValidationRequest) { + req.DnsName = "good-dns02.com" + req.Identifier = nil + }, + expectLog: `"Identifier":{"type":"dns","value":"good-dns02.com"}`, + }, + { + name: "No Identifier for DoDCV", + validationFuncName: "DoDCV", + validationFunc: runDoDCV, + identDomain: "good-dns01.com", + transmogrifier: func(req *vapb.PerformValidationRequest) { + req.DnsName = "good-dns02.com" + req.Identifier = nil + }, + expectLog: `"Identifier":{"type":"dns","value":"good-dns02.com"}`, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + va, mockLog := setup(nil, "", nil, nil) + + // create a challenge with well known token + req := createValidationRequest(identifier.NewDNS(tc.identDomain), core.ChallengeTypeDNS01) + tc.transmogrifier(req) + res, _ := tc.validationFunc(context.Background(), va, req) + test.Assert(t, res.Problem == nil, fmt.Sprintf("validation failed: %#v", res.Problem)) + if tc.validationFuncName == "PerformValidation" { + test.AssertMetricWithLabelsEquals(t, va.metrics.validationLatency, prometheus.Labels{ + "operation": opDCVAndCAA, + "perspective": va.perspective, + "challenge_type": string(core.ChallengeTypeDNS01), + "problem_type": "", + "result": pass, + }, 1) + } else { + test.AssertMetricWithLabelsEquals(t, va.metrics.validationLatency, prometheus.Labels{ + "operation": opDCV, + "perspective": va.perspective, + "challenge_type": string(core.ChallengeTypeDNS01), + "problem_type": "", + "result": pass, + }, 1) + } + resultLog := mockLog.GetAllMatching(`Validation result`) + if len(resultLog) != 1 { + t.Fatalf("Wrong number of matching lines for 'Validation result'") + } + if !strings.Contains(resultLog[0], tc.expectLog) { + t.Error("PerformValidation didn't log correct validation identifier.") + } + }) + } +} diff --git a/va/vampic.go b/va/vampic.go index 28b6036f7d8..79c80607850 100644 --- a/va/vampic.go +++ b/va/vampic.go @@ -9,6 +9,8 @@ import ( "slices" "time" + "google.golang.org/protobuf/proto" + "github.com/letsencrypt/boulder/core" corepb "github.com/letsencrypt/boulder/core/proto" berrors "github.com/letsencrypt/boulder/errors" @@ -16,7 +18,6 @@ import ( "github.com/letsencrypt/boulder/identifier" "github.com/letsencrypt/boulder/probs" vapb "github.com/letsencrypt/boulder/va/proto" - "google.golang.org/protobuf/proto" ) const ( @@ -199,7 +200,7 @@ func (va *ValidationAuthorityImpl) doRemoteOperation(ctx context.Context, op rem type validationLogEvent struct { AuthzID string Requester int64 - Identifier string + Identifier identifier.ACMEIdentifier Challenge core.Challenge Error string `json:",omitempty"` InternalError string `json:",omitempty"` @@ -218,7 +219,12 @@ type validationLogEvent struct { // implements the DCV portion of Multi-Perspective Issuance Corroboration as // defined in BRs Sections 3.2.2.9 and 5.4.1. func (va *ValidationAuthorityImpl) DoDCV(ctx context.Context, req *vapb.PerformValidationRequest) (*vapb.ValidationResult, error) { - if core.IsAnyNilOrZero(req, req.DnsName, req.Challenge, req.Authz, req.ExpectedKeyAuthorization) { + ident := req.Identifier + if ident == nil { + ident = identifier.NewDNS(req.DnsName).AsProto() + } + + if core.IsAnyNilOrZero(req, ident, req.Challenge, req.Authz, req.ExpectedKeyAuthorization) { return nil, berrors.InternalServerError("Incomplete validation request") } @@ -242,7 +248,7 @@ func (va *ValidationAuthorityImpl) DoDCV(ctx context.Context, req *vapb.PerformV logEvent := validationLogEvent{ AuthzID: req.Authz.Id, Requester: req.Authz.RegID, - Identifier: req.DnsName, + Identifier: identifier.FromProto(ident), Challenge: chall, } defer func() { @@ -276,7 +282,7 @@ func (va *ValidationAuthorityImpl) DoDCV(ctx context.Context, req *vapb.PerformV // was successful or not, and cannot themselves fail. records, err := va.validateChallenge( ctx, - identifier.NewDNS(req.DnsName), + identifier.FromProto(ident), chall.Type, chall.Token, req.ExpectedKeyAuthorization, @@ -330,7 +336,7 @@ func (va *ValidationAuthorityImpl) DoCAA(ctx context.Context, req *vapb.IsCAAVal logEvent := validationLogEvent{ AuthzID: req.AuthzID, Requester: req.AccountURIID, - Identifier: req.Domain, + Identifier: identifier.NewDNS(req.Domain), } challType := core.AcmeChallenge(req.ValidationMethod) From a96107fdbf65153951f3d98cdfdd216e64ec3233 Mon Sep 17 00:00:00 2001 From: James Renken Date: Fri, 21 Feb 2025 13:07:29 -0800 Subject: [PATCH 02/27] Fix tests: ASN.1 tag for IPs, err var name, bare IP redir --- va/http_test.go | 11 +++++++++-- va/tlsalpn.go | 5 ++++- va/tlsalpn_test.go | 6 +++--- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/va/http_test.go b/va/http_test.go index b5f9f3edf2b..3592eabef0b 100644 --- a/va/http_test.go +++ b/va/http_test.go @@ -560,7 +560,7 @@ func httpTestSrv(t *testing.T) *httptest.Server { http.Redirect( resp, req, - "https://127.0.0.1", + "http://127.0.0.1/ok", http.StatusMovedPermanently, ) }) @@ -926,11 +926,18 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com/redir-bad-host", + URL: "http://example.com/redir-bare-ip", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, }, + { + DnsName: "127.0.0.1", + Port: strconv.Itoa(httpPort), + URL: "http://127.0.0.1/ok", + AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, + AddressUsed: net.ParseIP("127.0.0.1"), + }, }, }, { diff --git a/va/tlsalpn.go b/va/tlsalpn.go index b70426573f5..41b321921cd 100644 --- a/va/tlsalpn.go +++ b/va/tlsalpn.go @@ -173,17 +173,20 @@ func (va *ValidationAuthorityImpl) getChallengeCert( // TODO(#7311): Identifiers need testing here. func checkExpectedSAN(cert *x509.Certificate, ident identifier.ACMEIdentifier) error { + var tagType int var certSAN []byte switch ident.Type { case identifier.TypeDNS: if len(cert.DNSNames) != 1 || len(cert.IPAddresses) != 0 { return errors.New("wrong number of identifiers") } + tagType = 2 certSAN = []byte(cert.DNSNames[0]) case identifier.TypeIP: if len(cert.IPAddresses) != 1 || len(cert.DNSNames) != 0 { return errors.New("wrong number of identifiers") } + tagType = 7 certSAN = cert.IPAddresses[0] default: // This should never happen. The calling function should check the @@ -194,7 +197,7 @@ func checkExpectedSAN(cert *x509.Certificate, ident identifier.ACMEIdentifier) e for _, ext := range cert.Extensions { if IdCeSubjectAltName.Equal(ext.Id) { expectedSANs, err := asn1.Marshal([]asn1.RawValue{ - {Tag: 2, Class: 2, Bytes: certSAN}, + {Tag: tagType, Class: 2, Bytes: certSAN}, }) if err != nil || !bytes.Equal(expectedSANs, ext.Value) { return errors.New("SAN extension does not match expected bytes") diff --git a/va/tlsalpn_test.go b/va/tlsalpn_test.go index 271bbf7ec2c..4ad8f05a8f0 100644 --- a/va/tlsalpn_test.go +++ b/va/tlsalpn_test.go @@ -377,9 +377,9 @@ func TestTLSALPN01SuccessIP(t *testing.T) { va, _ := setup(hs, "", nil, nil) - _, prob := va.validateTLSALPN01(ctx, identifier.NewIP(netip.MustParseAddr("127.0.0.1")), expectedKeyAuthorization) - if prob != nil { - t.Errorf("Validation failed: %v", prob) + _, err := va.validateTLSALPN01(ctx, identifier.NewIP(netip.MustParseAddr("127.0.0.1")), expectedKeyAuthorization) + if err != nil { + t.Errorf("Validation failed: %v", err) } test.AssertMetricWithLabelsEquals( t, va.metrics.tlsALPNOIDCounter, prometheus.Labels{"oid": IdPeAcmeIdentifier.String()}, 1) From 9bd276325eeb01131c5dc63b0ef59e43da9b5957 Mon Sep 17 00:00:00 2001 From: James Renken Date: Fri, 21 Feb 2025 13:18:22 -0800 Subject: [PATCH 03/27] Update TODOs, clarify var names --- va/http.go | 4 ++-- va/tlsalpn.go | 22 +++++++++++----------- va/va_test.go | 2 +- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/va/http.go b/va/http.go index d0593aa7b8b..99f2c4da87c 100644 --- a/va/http.go +++ b/va/http.go @@ -203,7 +203,7 @@ func (vt *httpValidationTarget) nextIP() error { // An error is returned if there are no usable IP addresses or if the DNS // lookups fail. // -// TODO(#7311): This needs testing with IP address identifiers. +// TODO(#8020): This needs testing with IP address identifiers. func (va *ValidationAuthorityImpl) newHTTPValidationTarget( ctx context.Context, ident identifier.ACMEIdentifier, @@ -643,7 +643,7 @@ func (va *ValidationAuthorityImpl) processHTTPValidation( } func (va *ValidationAuthorityImpl) validateHTTP01(ctx context.Context, ident identifier.ACMEIdentifier, token string, keyAuthorization string) ([]core.ValidationRecord, error) { - // TODO(#7311): This needs testing. + // TODO(#8020): This needs testing. if ident.Type != identifier.TypeDNS && ident.Type != identifier.TypeIP { va.log.Info(fmt.Sprintf("Identifier type for HTTP-01 challenge was not DNS or IP: %s", ident)) return nil, berrors.MalformedError("Identifier type for HTTP-01 challenge was not DNS or IP") diff --git a/va/tlsalpn.go b/va/tlsalpn.go index 41b321921cd..d7e557b5517 100644 --- a/va/tlsalpn.go +++ b/va/tlsalpn.go @@ -57,7 +57,7 @@ func certAltNames(cert *x509.Certificate) []string { return names } -// TODO(#7311): Identifiers need testing here. +// TODO(#8020): Identifiers need testing here. func (va *ValidationAuthorityImpl) tryGetChallengeCert( ctx context.Context, ident identifier.ACMEIdentifier, @@ -171,22 +171,22 @@ func (va *ValidationAuthorityImpl) getChallengeCert( return certs[0], &cs, nil } -// TODO(#7311): Identifiers need testing here. +// TODO(#8020): Identifiers need testing here. func checkExpectedSAN(cert *x509.Certificate, ident identifier.ACMEIdentifier) error { - var tagType int + var asn1Tag int var certSAN []byte switch ident.Type { case identifier.TypeDNS: if len(cert.DNSNames) != 1 || len(cert.IPAddresses) != 0 { return errors.New("wrong number of identifiers") } - tagType = 2 + asn1Tag = 2 certSAN = []byte(cert.DNSNames[0]) case identifier.TypeIP: if len(cert.IPAddresses) != 1 || len(cert.DNSNames) != 0 { return errors.New("wrong number of identifiers") } - tagType = 7 + asn1Tag = 7 certSAN = cert.IPAddresses[0] default: // This should never happen. The calling function should check the @@ -197,7 +197,7 @@ func checkExpectedSAN(cert *x509.Certificate, ident identifier.ACMEIdentifier) e for _, ext := range cert.Extensions { if IdCeSubjectAltName.Equal(ext.Id) { expectedSANs, err := asn1.Marshal([]asn1.RawValue{ - {Tag: tagType, Class: 2, Bytes: certSAN}, + {Tag: asn1Tag, Class: 2, Bytes: certSAN}, }) if err != nil || !bytes.Equal(expectedSANs, ext.Value) { return errors.New("SAN extension does not match expected bytes") @@ -235,13 +235,13 @@ func checkAcceptableExtensions(exts []pkix.Extension, requiredOIDs []asn1.Object } func (va *ValidationAuthorityImpl) validateTLSALPN01(ctx context.Context, ident identifier.ACMEIdentifier, keyAuthorization string) ([]core.ValidationRecord, error) { - // TODO(#7311): This needs testing. + // TODO(#8020): This needs testing. if ident.Type != identifier.TypeDNS && ident.Type != identifier.TypeIP { va.log.Info(fmt.Sprintf("Identifier type for TLS-ALPN-01 challenge was not DNS or IP: %s", ident)) return nil, berrors.MalformedError("Identifier type for TLS-ALPN-01 challenge was not DNS or IP") } - cert, cs, tvr, problem := va.tryGetChallengeCert(ctx, ident, &tls.Config{ + cert, cs, tvr, err := va.tryGetChallengeCert(ctx, ident, &tls.Config{ MinVersion: tls.VersionTLS12, NextProtos: []string{ACMETLS1Protocol}, ServerName: ident.Value, @@ -250,8 +250,8 @@ func (va *ValidationAuthorityImpl) validateTLSALPN01(ctx context.Context, ident // get a reference to it so we can modify it if we have to. validationRecords := []core.ValidationRecord{tvr} validationRecord := &validationRecords[0] - if problem != nil { - return validationRecords, problem + if err != nil { + return validationRecords, err } if cs.NegotiatedProtocol != ACMETLS1Protocol { @@ -271,7 +271,7 @@ func (va *ValidationAuthorityImpl) validateTLSALPN01(ctx context.Context, ident } // The certificate must be self-signed. - err := cert.CheckSignature(cert.SignatureAlgorithm, cert.RawTBSCertificate, cert.Signature) + err = cert.CheckSignature(cert.SignatureAlgorithm, cert.RawTBSCertificate, cert.Signature) if err != nil || !bytes.Equal(cert.RawSubject, cert.RawIssuer) { return validationRecords, badCertErr( "Received certificate which is not self-signed.") diff --git a/va/va_test.go b/va/va_test.go index 128a62d61bb..153f45e131c 100644 --- a/va/va_test.go +++ b/va/va_test.go @@ -603,7 +603,7 @@ func TestPerformValidationValid(t *testing.T) { // TestPerformValidationWildcard tests that the VA properly strips the `*.` // prefix from a wildcard name provided to the PerformValidation function. // -// TODO(#7311): This or another test should check that an IP address identifier +// TODO(#8020): This or another test should check that an IP address identifier // with a wildcard is rejected. func TestPerformValidationWildcard(t *testing.T) { t.Parallel() From d8e90bcd31297177d68514f5cac92b2d2ea05a76 Mon Sep 17 00:00:00 2001 From: James Renken Date: Fri, 21 Feb 2025 14:09:47 -0800 Subject: [PATCH 04/27] Fix lint, bring bad port test closer to original --- va/http_test.go | 2 +- va/tlsalpn_test.go | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/va/http_test.go b/va/http_test.go index 3592eabef0b..801d64356ab 100644 --- a/va/http_test.go +++ b/va/http_test.go @@ -843,7 +843,7 @@ func TestFetchHTTP(t *testing.T) { { Name: "Connecting to bad port", Ident: identifier.NewDNS("example.com"), - Port: 1023, + Port: httpPort, Path: "/timeout", ExpectedProblem: probs.Connection( "127.0.0.1: Fetching http://example.com:" + strconv.Itoa(httpPort) + "/timeout: " + diff --git a/va/tlsalpn_test.go b/va/tlsalpn_test.go index 4ad8f05a8f0..6fdfdb69f1d 100644 --- a/va/tlsalpn_test.go +++ b/va/tlsalpn_test.go @@ -78,8 +78,8 @@ func testTLSCert(names []string, ips []net.IP, extensions []pkix.Extension) *tls // testACMECert returns a certificate with the correctly-formed ACME TLS-ALPN-01 // extension with our default test values. Use acmeExtension and testCert if you // need to customize the contents of that extension. -func testACMECert(names []string, ips []net.IP) *tls.Certificate { - return testTLSCert(names, ips, []pkix.Extension{testACMEExt}) +func testACMECert(names []string) *tls.Certificate { + return testTLSCert(names, nil, []pkix.Extension{testACMEExt}) } // tlsalpn01SrvWithCert creates a test server which will present the given @@ -113,7 +113,7 @@ func tlsalpn01SrvWithCert(t *testing.T, acmeCert *tls.Certificate, tlsVersion ui // that don't need to customize specific names or extensions in the certificate // served by the TLS server. func testTLSALPN01Srv(t *testing.T) *httptest.Server { - return tlsalpn01SrvWithCert(t, testACMECert([]string{"expected"}, nil), 0) + return tlsalpn01SrvWithCert(t, testACMECert([]string{"expected"}), 0) } func slowTLSSrv() *httptest.Server { @@ -508,7 +508,7 @@ func TestValidateTLSALPN01MalformedExtnValue(t *testing.T) { } func TestTLSALPN01TLSVersion(t *testing.T) { - cert := testACMECert([]string{"expected"}, nil) + cert := testACMECert([]string{"expected"}) for _, tc := range []struct { version uint16 @@ -553,7 +553,7 @@ func TestTLSALPN01TLSVersion(t *testing.T) { func TestTLSALPN01WrongName(t *testing.T) { // Create a cert with a different name from what we're validating - hs := tlsalpn01SrvWithCert(t, testACMECert([]string{"incorrect"}, nil), 0) + hs := tlsalpn01SrvWithCert(t, testACMECert([]string{"incorrect"}), 0) va, _ := setup(hs, "", nil, nil) @@ -564,7 +564,7 @@ func TestTLSALPN01WrongName(t *testing.T) { func TestTLSALPN01ExtraNames(t *testing.T) { // Create a cert with two names when we only want to validate one. - hs := tlsalpn01SrvWithCert(t, testACMECert([]string{"expected", "extra"}, nil), 0) + hs := tlsalpn01SrvWithCert(t, testACMECert([]string{"expected", "extra"}), 0) va, _ := setup(hs, "", nil, nil) From b8aed4445e568c32040f842a8e36e23f8c9034b0 Mon Sep 17 00:00:00 2001 From: James Renken Date: Fri, 21 Feb 2025 15:19:32 -0800 Subject: [PATCH 05/27] Fix initialURL composition; update test strings to expect explicit port number --- va/http.go | 4 ++-- va/http_test.go | 62 +++++++++++++++++++++---------------------------- va/va_test.go | 2 +- 3 files changed, 30 insertions(+), 38 deletions(-) diff --git a/va/http.go b/va/http.go index 99f2c4da87c..c95496680f0 100644 --- a/va/http.go +++ b/va/http.go @@ -431,7 +431,7 @@ func (va *ValidationAuthorityImpl) processHTTPValidation( // Create an initial GET Request initialURL := url.URL{ Scheme: "http", - Host: ident.Value, + Host: net.JoinHostPort(ident.Value, strconv.Itoa(port)), Path: path, } initialReq, err := http.NewRequest("GET", initialURL.String(), nil) @@ -481,7 +481,7 @@ func (va *ValidationAuthorityImpl) processHTTPValidation( transport := httpTransport(dialer.DialContext) va.log.AuditInfof("Attempting to validate HTTP-01 for %q with GET to %q", - initialReq.Host, initialReq.URL.String()) + initialReq.URL.Hostname(), initialReq.URL.String()) // Create a closure around records & numRedirects we can use with a HTTP // client to process redirects per our own policy (e.g. resolving IP diff --git a/va/http_test.go b/va/http_test.go index 801d64356ab..264be29e3ac 100644 --- a/va/http_test.go +++ b/va/http_test.go @@ -758,12 +758,8 @@ func TestFetchHTTP(t *testing.T) { // redirect to the url with a port definition and on i=1 it will encounter // the second redirect to the url with the port and get an expected error. expectedLoopRecords := []core.ValidationRecord{} - for i := range 2 { - // The first request will not have a port # in the URL. - url := "http://example.com/loop" - if i != 0 { - url = fmt.Sprintf("http://example.com:%d/loop", httpPort) - } + for range 2 { + url := fmt.Sprintf("http://example.com:%d/loop", httpPort) expectedLoopRecords = append(expectedLoopRecords, core.ValidationRecord{ DnsName: "example.com", @@ -780,11 +776,7 @@ func TestFetchHTTP(t *testing.T) { // base lookup, giving a termination criteria of > maxRedirect+1 expectedTooManyRedirRecords := []core.ValidationRecord{} for i := range maxRedirect + 2 { - // The first request will not have a port # in the URL. - url := "http://example.com/max-redirect/0" - if i != 0 { - url = fmt.Sprintf("http://example.com:%d/max-redirect/%d", httpPort, i) - } + url := fmt.Sprintf("http://example.com:%d/max-redirect/%d", httpPort, i) expectedTooManyRedirRecords = append(expectedTooManyRedirRecords, core.ValidationRecord{ DnsName: "example.com", @@ -827,13 +819,13 @@ func TestFetchHTTP(t *testing.T) { Port: httpPort, Path: "/timeout", ExpectedProblem: probs.Connection( - "127.0.0.1: Fetching http://example.com/timeout: " + + "127.0.0.1: Fetching http://example.com:" + strconv.Itoa(httpPort) + "/timeout: " + "Timeout after connect (your server may be slow or overloaded)"), ExpectedRecords: []core.ValidationRecord{ { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com/timeout", + URL: "http://example.com:" + strconv.Itoa(httpPort) + "/timeout", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -890,7 +882,7 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com/redir-bad-proto", + URL: "http://example.com:" + strconv.Itoa(httpPort) + "/redir-bad-proto", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -909,7 +901,7 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com/redir-bad-port", + URL: "http://example.com:" + strconv.Itoa(httpPort) + "/redir-bad-port", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -926,7 +918,7 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com/redir-bare-ip", + URL: "http://example.com:" + strconv.Itoa(httpPort) + "/redir-bare-ip", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -951,7 +943,7 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com/redir-path-too-long", + URL: "http://example.com:" + strconv.Itoa(httpPort) + "/redir-path-too-long", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -964,12 +956,12 @@ func TestFetchHTTP(t *testing.T) { Port: httpPort, Path: "/bad-status-code", ExpectedProblem: probs.Unauthorized( - "127.0.0.1: Invalid response from http://example.com/bad-status-code: 410"), + "127.0.0.1: Invalid response from http://example.com:" + strconv.Itoa(httpPort) + "/bad-status-code: 410"), ExpectedRecords: []core.ValidationRecord{ { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com/bad-status-code", + URL: "http://example.com:" + strconv.Itoa(httpPort) + "/bad-status-code", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -987,7 +979,7 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com/303-see-other", + URL: "http://example.com:" + strconv.Itoa(httpPort) + "/303-see-other", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -1000,13 +992,13 @@ func TestFetchHTTP(t *testing.T) { Port: httpPort, Path: "/resp-too-big", ExpectedProblem: probs.Unauthorized(fmt.Sprintf( - "127.0.0.1: Invalid response from http://example.com/resp-too-big: %q", expectedTruncatedResp.String(), + "127.0.0.1: Invalid response from http://example.com:"+strconv.Itoa(httpPort)+"/resp-too-big: %q", expectedTruncatedResp.String(), )), ExpectedRecords: []core.ValidationRecord{ { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com/resp-too-big", + URL: "http://example.com:" + strconv.Itoa(httpPort) + "/resp-too-big", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -1019,12 +1011,12 @@ func TestFetchHTTP(t *testing.T) { Port: httpPort, Path: "/ok", ExpectedProblem: probs.Connection( - "::1: Fetching http://ipv6.localhost/ok: Connection refused"), + "::1: Fetching http://ipv6.localhost:" + strconv.Itoa(httpPort) + "/ok: Connection refused"), ExpectedRecords: []core.ValidationRecord{ { DnsName: "ipv6.localhost", Port: strconv.Itoa(httpPort), - URL: "http://ipv6.localhost/ok", + URL: "http://ipv6.localhost:" + strconv.Itoa(httpPort) + "/ok", AddressesResolved: []net.IP{net.ParseIP("::1")}, AddressUsed: net.ParseIP("::1"), ResolverAddrs: []string{"MockClient"}, @@ -1041,7 +1033,7 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "ipv4.and.ipv6.localhost", Port: strconv.Itoa(httpPort), - URL: "http://ipv4.and.ipv6.localhost/ok", + URL: "http://ipv4.and.ipv6.localhost:" + strconv.Itoa(httpPort) + "/ok", AddressesResolved: []net.IP{net.ParseIP("::1"), net.ParseIP("127.0.0.1")}, // The first validation record should have used the IPv6 addr AddressUsed: net.ParseIP("::1"), @@ -1050,7 +1042,7 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "ipv4.and.ipv6.localhost", Port: strconv.Itoa(httpPort), - URL: "http://ipv4.and.ipv6.localhost/ok", + URL: "http://ipv4.and.ipv6.localhost:" + strconv.Itoa(httpPort) + "/ok", AddressesResolved: []net.IP{net.ParseIP("::1"), net.ParseIP("127.0.0.1")}, // The second validation record should have used the IPv4 addr as a fallback AddressUsed: net.ParseIP("127.0.0.1"), @@ -1068,7 +1060,7 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com/ok", + URL: "http://example.com:" + strconv.Itoa(httpPort) + "/ok", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -1085,7 +1077,7 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com/redir-uppercase-publicsuffix", + URL: "http://example.com:" + strconv.Itoa(httpPort) + "/redir-uppercase-publicsuffix", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -1107,7 +1099,7 @@ func TestFetchHTTP(t *testing.T) { Path: "/printf-verbs", ExpectedProblem: &probs.ProblemDetails{ Type: probs.UnauthorizedProblem, - Detail: fmt.Sprintf("127.0.0.1: Invalid response from http://example.com/printf-verbs: %q", + Detail: fmt.Sprintf("127.0.0.1: Invalid response from http://example.com:"+strconv.Itoa(httpPort)+"/printf-verbs: %q", ("%2F.well-known%2F" + expectedTruncatedResp.String())[:maxResponseSize]), HTTPStatus: http.StatusForbidden, }, @@ -1115,7 +1107,7 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com/printf-verbs", + URL: "http://example.com:" + strconv.Itoa(httpPort) + "/printf-verbs", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -1320,7 +1312,7 @@ func TestHTTP(t *testing.T) { if err != nil { t.Fatalf("Failed to follow http.StatusMovedPermanently redirect") } - redirectValid := `following redirect to host "" url "http://localhost.com/.well-known/acme-challenge/` + pathValid + `"` + redirectValid := `following redirect to host "" url "http://localhost.com:` + strconv.Itoa(getPort(hs)) + `/.well-known/acme-challenge/` + pathValid + `"` matchedValidRedirect := log.GetAllMatching(redirectValid) test.AssertEquals(t, len(matchedValidRedirect), 1) @@ -1329,7 +1321,7 @@ func TestHTTP(t *testing.T) { if err != nil { t.Fatalf("Failed to follow http.StatusFound redirect") } - redirectMoved := `following redirect to host "" url "http://localhost.com/.well-known/acme-challenge/` + pathMoved + `"` + redirectMoved := `following redirect to host "" url "http://localhost.com:` + strconv.Itoa(getPort(hs)) + `/.well-known/acme-challenge/` + pathMoved + `"` matchedMovedRedirect := log.GetAllMatching(redirectMoved) test.AssertEquals(t, len(matchedValidRedirect), 1) test.AssertEquals(t, len(matchedMovedRedirect), 1) @@ -1368,7 +1360,7 @@ func TestHTTPTimeout(t *testing.T) { } prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.ConnectionProblem) - test.AssertEquals(t, prob.Detail, "127.0.0.1: Fetching http://localhost/.well-known/acme-challenge/wait-long: Timeout after connect (your server may be slow or overloaded)") + test.AssertEquals(t, prob.Detail, "127.0.0.1: Fetching http://localhost:"+strconv.Itoa(getPort(hs))+"/.well-known/acme-challenge/wait-long: Timeout after connect (your server may be slow or overloaded)") } func TestHTTPRedirectLookup(t *testing.T) { @@ -1380,7 +1372,7 @@ func TestHTTPRedirectLookup(t *testing.T) { if err != nil { t.Fatalf("Unexpected failure in redirect (%s): %s", pathMoved, err) } - redirectValid := `following redirect to host "" url "http://localhost.com/.well-known/acme-challenge/` + pathValid + `"` + redirectValid := `following redirect to host "" url "http://localhost.com:` + strconv.Itoa(getPort(hs)) + `/.well-known/acme-challenge/` + pathValid + `"` matchedValidRedirect := log.GetAllMatching(redirectValid) test.AssertEquals(t, len(matchedValidRedirect), 1) test.AssertEquals(t, len(log.GetAllMatching(`Resolved addresses for localhost.com: \[127.0.0.1\]`)), 2) @@ -1390,7 +1382,7 @@ func TestHTTPRedirectLookup(t *testing.T) { if err != nil { t.Fatalf("Unexpected failure in redirect (%s): %s", pathFound, err) } - redirectMoved := `following redirect to host "" url "http://localhost.com/.well-known/acme-challenge/` + pathMoved + `"` + redirectMoved := `following redirect to host "" url "http://localhost.com:` + strconv.Itoa(getPort(hs)) + `/.well-known/acme-challenge/` + pathMoved + `"` matchedMovedRedirect := log.GetAllMatching(redirectMoved) test.AssertEquals(t, len(matchedMovedRedirect), 1) test.AssertEquals(t, len(log.GetAllMatching(`Resolved addresses for localhost.com: \[127.0.0.1\]`)), 3) diff --git a/va/va_test.go b/va/va_test.go index 153f45e131c..39521734222 100644 --- a/va/va_test.go +++ b/va/va_test.go @@ -539,7 +539,7 @@ func TestInternalErrorLogged(t *testing.T) { _, err := tc.validationFunc(ctx, va, req) test.AssertNotError(t, err, "failed validation should not be an error") matchingLogs := mockLog.GetAllMatching( - `Validation result JSON=.*"InternalError":"127.0.0.1: Get.*nonexistent.com/\.well-known.*: context deadline exceeded`) + `Validation result JSON=.*"InternalError":"127.0.0.1: Get.*nonexistent.com:80/\.well-known.*: context deadline exceeded`) test.AssertEquals(t, len(matchingLogs), 1) }) } From 76d40b75df01f2318a5f9a064fa25b74b0f43385 Mon Sep 17 00:00:00 2001 From: James Renken Date: Fri, 21 Feb 2025 15:38:58 -0800 Subject: [PATCH 06/27] Fix redirect loop test for the new era of explicit port numbers --- va/http_test.go | 32 ++++++++++---------------------- 1 file changed, 10 insertions(+), 22 deletions(-) diff --git a/va/http_test.go b/va/http_test.go index 264be29e3ac..4629b749623 100644 --- a/va/http_test.go +++ b/va/http_test.go @@ -750,27 +750,6 @@ func TestFetchHTTP(t *testing.T) { // We need to know the randomly assigned HTTP port for testcases as well httpPort := getPort(testSrv) - // For the looped test case we expect one validation record per redirect - // until boulder detects that a url has been used twice indicating a - // redirect loop. Because it is hitting the /loop endpoint it will encounter - // this scenario after the base url and fail on the second time hitting the - // redirect with a port definition. On i=0 it will encounter the first - // redirect to the url with a port definition and on i=1 it will encounter - // the second redirect to the url with the port and get an expected error. - expectedLoopRecords := []core.ValidationRecord{} - for range 2 { - url := fmt.Sprintf("http://example.com:%d/loop", httpPort) - expectedLoopRecords = append(expectedLoopRecords, - core.ValidationRecord{ - DnsName: "example.com", - Port: strconv.Itoa(httpPort), - URL: url, - AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, - AddressUsed: net.ParseIP("127.0.0.1"), - ResolverAddrs: []string{"MockClient"}, - }) - } - // For the too many redirect test case we expect one validation record per // redirect up to maxRedirect (inclusive). There is also +1 record for the // base lookup, giving a termination criteria of > maxRedirect+1 @@ -858,7 +837,16 @@ func TestFetchHTTP(t *testing.T) { Path: "/loop", ExpectedProblem: probs.Connection(fmt.Sprintf( "127.0.0.1: Fetching http://example.com:%d/loop: Redirect loop detected", httpPort)), - ExpectedRecords: expectedLoopRecords, + ExpectedRecords: []core.ValidationRecord{ + { + DnsName: "example.com", + Port: strconv.Itoa(httpPort), + URL: fmt.Sprintf("http://example.com:%d/loop", httpPort), + AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, + AddressUsed: net.ParseIP("127.0.0.1"), + ResolverAddrs: []string{"MockClient"}, + }, + }, }, { Name: "Too many redirects", From 8a96db7e20b00a8a7bee275c7f8ad79387adecc0 Mon Sep 17 00:00:00 2001 From: James Renken Date: Fri, 21 Feb 2025 17:14:55 -0800 Subject: [PATCH 07/27] Revert explicit port number; hardcode port in processHTTPValidation instead; remove invalid bad port test --- va/http.go | 9 ++-- va/http_test.go | 117 ++++++++++++++++++++++++------------------------ va/va_test.go | 2 +- 3 files changed, 64 insertions(+), 64 deletions(-) diff --git a/va/http.go b/va/http.go index c95496680f0..6e13f22ecde 100644 --- a/va/http.go +++ b/va/http.go @@ -420,10 +420,9 @@ func fallbackErr(err error) bool { func (va *ValidationAuthorityImpl) processHTTPValidation( ctx context.Context, ident identifier.ACMEIdentifier, - port int, path string) ([]byte, []core.ValidationRecord, error) { // Create a target for the host, port and path with no query parameters - target, err := va.newHTTPValidationTarget(ctx, ident, port, path, "") + target, err := va.newHTTPValidationTarget(ctx, ident, va.httpPort, path, "") if err != nil { return nil, nil, err } @@ -431,7 +430,7 @@ func (va *ValidationAuthorityImpl) processHTTPValidation( // Create an initial GET Request initialURL := url.URL{ Scheme: "http", - Host: net.JoinHostPort(ident.Value, strconv.Itoa(port)), + Host: ident.Value, Path: path, } initialReq, err := http.NewRequest("GET", initialURL.String(), nil) @@ -481,7 +480,7 @@ func (va *ValidationAuthorityImpl) processHTTPValidation( transport := httpTransport(dialer.DialContext) va.log.AuditInfof("Attempting to validate HTTP-01 for %q with GET to %q", - initialReq.URL.Hostname(), initialReq.URL.String()) + initialReq.Host, initialReq.URL.String()) // Create a closure around records & numRedirects we can use with a HTTP // client to process redirects per our own policy (e.g. resolving IP @@ -651,7 +650,7 @@ func (va *ValidationAuthorityImpl) validateHTTP01(ctx context.Context, ident ide // Perform the fetch path := fmt.Sprintf(".well-known/acme-challenge/%s", token) - body, validationRecords, err := va.processHTTPValidation(ctx, ident, va.httpPort, "/"+path) + body, validationRecords, err := va.processHTTPValidation(ctx, ident, "/"+path) if err != nil { return validationRecords, err } diff --git a/va/http_test.go b/va/http_test.go index 4629b749623..394dd0ad3b9 100644 --- a/va/http_test.go +++ b/va/http_test.go @@ -89,7 +89,7 @@ func TestDialerTimeout(t *testing.T) { var took time.Duration for range 20 { started := time.Now() - _, _, err = va.processHTTPValidation(ctx, identifier.NewDNS("unroutable.invalid"), 80, "/.well-known/acme-challenge/whatever") + _, _, err = va.processHTTPValidation(ctx, identifier.NewDNS("unroutable.invalid"), "/.well-known/acme-challenge/whatever") took = time.Since(started) if err != nil && strings.Contains(err.Error(), "network is unreachable") { continue @@ -322,7 +322,7 @@ func TestExtractRequestTarget(t *testing.T) { func TestHTTPValidationDNSError(t *testing.T) { va, mockLog := setup(nil, "", nil, nil) - _, _, prob := va.processHTTPValidation(ctx, identifier.NewDNS("always.error"), 80, "/.well-known/acme-challenge/whatever") + _, _, prob := va.processHTTPValidation(ctx, identifier.NewDNS("always.error"), "/.well-known/acme-challenge/whatever") test.AssertError(t, prob, "Expected validation fetch to fail") matchingLines := mockLog.GetAllMatching(`read udp: some net error`) if len(matchingLines) != 1 { @@ -338,7 +338,7 @@ func TestHTTPValidationDNSError(t *testing.T) { func TestHTTPValidationDNSIdMismatchError(t *testing.T) { va, mockLog := setup(nil, "", nil, nil) - _, _, prob := va.processHTTPValidation(ctx, identifier.NewDNS("id.mismatch"), 80, "/.well-known/acme-challenge/whatever") + _, _, prob := va.processHTTPValidation(ctx, identifier.NewDNS("id.mismatch"), "/.well-known/acme-challenge/whatever") test.AssertError(t, prob, "Expected validation fetch to fail") matchingLines := mockLog.GetAllMatching(`logDNSError ID mismatch`) if len(matchingLines) != 1 { @@ -750,12 +750,41 @@ func TestFetchHTTP(t *testing.T) { // We need to know the randomly assigned HTTP port for testcases as well httpPort := getPort(testSrv) + // For the looped test case we expect one validation record per redirect + // until boulder detects that a url has been used twice indicating a + // redirect loop. Because it is hitting the /loop endpoint it will encounter + // this scenario after the base url and fail on the second time hitting the + // redirect with a port definition. On i=0 it will encounter the first + // redirect to the url with a port definition and on i=1 it will encounter + // the second redirect to the url with the port and get an expected error. + expectedLoopRecords := []core.ValidationRecord{} + for i := range 2 { + // The first request will not have a port # in the URL. + url := "http://example.com/loop" + if i != 0 { + url = fmt.Sprintf("http://example.com:%d/loop", httpPort) + } + expectedLoopRecords = append(expectedLoopRecords, + core.ValidationRecord{ + DnsName: "example.com", + Port: strconv.Itoa(httpPort), + URL: url, + AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, + AddressUsed: net.ParseIP("127.0.0.1"), + ResolverAddrs: []string{"MockClient"}, + }) + } + // For the too many redirect test case we expect one validation record per // redirect up to maxRedirect (inclusive). There is also +1 record for the // base lookup, giving a termination criteria of > maxRedirect+1 expectedTooManyRedirRecords := []core.ValidationRecord{} for i := range maxRedirect + 2 { - url := fmt.Sprintf("http://example.com:%d/max-redirect/%d", httpPort, i) + // The first request will not have a port # in the URL. + url := "http://example.com/max-redirect/0" + if i != 0 { + url = fmt.Sprintf("http://example.com:%d/max-redirect/%d", httpPort, i) + } expectedTooManyRedirRecords = append(expectedTooManyRedirRecords, core.ValidationRecord{ DnsName: "example.com", @@ -798,32 +827,13 @@ func TestFetchHTTP(t *testing.T) { Port: httpPort, Path: "/timeout", ExpectedProblem: probs.Connection( - "127.0.0.1: Fetching http://example.com:" + strconv.Itoa(httpPort) + "/timeout: " + + "127.0.0.1: Fetching http://example.com/timeout: " + "Timeout after connect (your server may be slow or overloaded)"), ExpectedRecords: []core.ValidationRecord{ { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com:" + strconv.Itoa(httpPort) + "/timeout", - AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, - AddressUsed: net.ParseIP("127.0.0.1"), - ResolverAddrs: []string{"MockClient"}, - }, - }, - }, - { - Name: "Connecting to bad port", - Ident: identifier.NewDNS("example.com"), - Port: httpPort, - Path: "/timeout", - ExpectedProblem: probs.Connection( - "127.0.0.1: Fetching http://example.com:" + strconv.Itoa(httpPort) + "/timeout: " + - "Error getting validation data"), - ExpectedRecords: []core.ValidationRecord{ - { - DnsName: "example.com:" + strconv.Itoa(httpPort), - Port: strconv.Itoa(httpPort), - URL: "http://example.com:" + strconv.Itoa(httpPort) + "/timeout", + URL: "http://example.com/timeout", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -837,16 +847,7 @@ func TestFetchHTTP(t *testing.T) { Path: "/loop", ExpectedProblem: probs.Connection(fmt.Sprintf( "127.0.0.1: Fetching http://example.com:%d/loop: Redirect loop detected", httpPort)), - ExpectedRecords: []core.ValidationRecord{ - { - DnsName: "example.com", - Port: strconv.Itoa(httpPort), - URL: fmt.Sprintf("http://example.com:%d/loop", httpPort), - AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, - AddressUsed: net.ParseIP("127.0.0.1"), - ResolverAddrs: []string{"MockClient"}, - }, - }, + ExpectedRecords: expectedLoopRecords, }, { Name: "Too many redirects", @@ -870,7 +871,7 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com:" + strconv.Itoa(httpPort) + "/redir-bad-proto", + URL: "http://example.com/redir-bad-proto", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -889,7 +890,7 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com:" + strconv.Itoa(httpPort) + "/redir-bad-port", + URL: "http://example.com/redir-bad-port", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -906,7 +907,7 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com:" + strconv.Itoa(httpPort) + "/redir-bare-ip", + URL: "http://example.com/redir-bare-ip", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -931,7 +932,7 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com:" + strconv.Itoa(httpPort) + "/redir-path-too-long", + URL: "http://example.com/redir-path-too-long", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -944,12 +945,12 @@ func TestFetchHTTP(t *testing.T) { Port: httpPort, Path: "/bad-status-code", ExpectedProblem: probs.Unauthorized( - "127.0.0.1: Invalid response from http://example.com:" + strconv.Itoa(httpPort) + "/bad-status-code: 410"), + "127.0.0.1: Invalid response from http://example.com/bad-status-code: 410"), ExpectedRecords: []core.ValidationRecord{ { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com:" + strconv.Itoa(httpPort) + "/bad-status-code", + URL: "http://example.com/bad-status-code", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -967,7 +968,7 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com:" + strconv.Itoa(httpPort) + "/303-see-other", + URL: "http://example.com/303-see-other", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -980,13 +981,13 @@ func TestFetchHTTP(t *testing.T) { Port: httpPort, Path: "/resp-too-big", ExpectedProblem: probs.Unauthorized(fmt.Sprintf( - "127.0.0.1: Invalid response from http://example.com:"+strconv.Itoa(httpPort)+"/resp-too-big: %q", expectedTruncatedResp.String(), + "127.0.0.1: Invalid response from http://example.com/resp-too-big: %q", expectedTruncatedResp.String(), )), ExpectedRecords: []core.ValidationRecord{ { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com:" + strconv.Itoa(httpPort) + "/resp-too-big", + URL: "http://example.com/resp-too-big", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -999,12 +1000,12 @@ func TestFetchHTTP(t *testing.T) { Port: httpPort, Path: "/ok", ExpectedProblem: probs.Connection( - "::1: Fetching http://ipv6.localhost:" + strconv.Itoa(httpPort) + "/ok: Connection refused"), + "::1: Fetching http://ipv6.localhost/ok: Connection refused"), ExpectedRecords: []core.ValidationRecord{ { DnsName: "ipv6.localhost", Port: strconv.Itoa(httpPort), - URL: "http://ipv6.localhost:" + strconv.Itoa(httpPort) + "/ok", + URL: "http://ipv6.localhost/ok", AddressesResolved: []net.IP{net.ParseIP("::1")}, AddressUsed: net.ParseIP("::1"), ResolverAddrs: []string{"MockClient"}, @@ -1021,7 +1022,7 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "ipv4.and.ipv6.localhost", Port: strconv.Itoa(httpPort), - URL: "http://ipv4.and.ipv6.localhost:" + strconv.Itoa(httpPort) + "/ok", + URL: "http://ipv4.and.ipv6.localhost/ok", AddressesResolved: []net.IP{net.ParseIP("::1"), net.ParseIP("127.0.0.1")}, // The first validation record should have used the IPv6 addr AddressUsed: net.ParseIP("::1"), @@ -1030,7 +1031,7 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "ipv4.and.ipv6.localhost", Port: strconv.Itoa(httpPort), - URL: "http://ipv4.and.ipv6.localhost:" + strconv.Itoa(httpPort) + "/ok", + URL: "http://ipv4.and.ipv6.localhost/ok", AddressesResolved: []net.IP{net.ParseIP("::1"), net.ParseIP("127.0.0.1")}, // The second validation record should have used the IPv4 addr as a fallback AddressUsed: net.ParseIP("127.0.0.1"), @@ -1048,7 +1049,7 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com:" + strconv.Itoa(httpPort) + "/ok", + URL: "http://example.com/ok", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -1065,7 +1066,7 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com:" + strconv.Itoa(httpPort) + "/redir-uppercase-publicsuffix", + URL: "http://example.com/redir-uppercase-publicsuffix", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -1087,7 +1088,7 @@ func TestFetchHTTP(t *testing.T) { Path: "/printf-verbs", ExpectedProblem: &probs.ProblemDetails{ Type: probs.UnauthorizedProblem, - Detail: fmt.Sprintf("127.0.0.1: Invalid response from http://example.com:"+strconv.Itoa(httpPort)+"/printf-verbs: %q", + Detail: fmt.Sprintf("127.0.0.1: Invalid response from http://example.com/printf-verbs: %q", ("%2F.well-known%2F" + expectedTruncatedResp.String())[:maxResponseSize]), HTTPStatus: http.StatusForbidden, }, @@ -1095,7 +1096,7 @@ func TestFetchHTTP(t *testing.T) { { DnsName: "example.com", Port: strconv.Itoa(httpPort), - URL: "http://example.com:" + strconv.Itoa(httpPort) + "/printf-verbs", + URL: "http://example.com/printf-verbs", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, @@ -1108,7 +1109,7 @@ func TestFetchHTTP(t *testing.T) { t.Run(tc.Name, func(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*500) defer cancel() - body, records, err := va.processHTTPValidation(ctx, tc.Ident, tc.Port, tc.Path) + body, records, err := va.processHTTPValidation(ctx, tc.Ident, tc.Path) if tc.ExpectedProblem == nil { test.AssertNotError(t, err, "expected nil prob") } else { @@ -1300,7 +1301,7 @@ func TestHTTP(t *testing.T) { if err != nil { t.Fatalf("Failed to follow http.StatusMovedPermanently redirect") } - redirectValid := `following redirect to host "" url "http://localhost.com:` + strconv.Itoa(getPort(hs)) + `/.well-known/acme-challenge/` + pathValid + `"` + redirectValid := `following redirect to host "" url "http://localhost.com/.well-known/acme-challenge/` + pathValid + `"` matchedValidRedirect := log.GetAllMatching(redirectValid) test.AssertEquals(t, len(matchedValidRedirect), 1) @@ -1309,7 +1310,7 @@ func TestHTTP(t *testing.T) { if err != nil { t.Fatalf("Failed to follow http.StatusFound redirect") } - redirectMoved := `following redirect to host "" url "http://localhost.com:` + strconv.Itoa(getPort(hs)) + `/.well-known/acme-challenge/` + pathMoved + `"` + redirectMoved := `following redirect to host "" url "http://localhost.com/.well-known/acme-challenge/` + pathMoved + `"` matchedMovedRedirect := log.GetAllMatching(redirectMoved) test.AssertEquals(t, len(matchedValidRedirect), 1) test.AssertEquals(t, len(matchedMovedRedirect), 1) @@ -1348,7 +1349,7 @@ func TestHTTPTimeout(t *testing.T) { } prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.ConnectionProblem) - test.AssertEquals(t, prob.Detail, "127.0.0.1: Fetching http://localhost:"+strconv.Itoa(getPort(hs))+"/.well-known/acme-challenge/wait-long: Timeout after connect (your server may be slow or overloaded)") + test.AssertEquals(t, prob.Detail, "127.0.0.1: Fetching http://localhost/.well-known/acme-challenge/wait-long: Timeout after connect (your server may be slow or overloaded)") } func TestHTTPRedirectLookup(t *testing.T) { @@ -1360,7 +1361,7 @@ func TestHTTPRedirectLookup(t *testing.T) { if err != nil { t.Fatalf("Unexpected failure in redirect (%s): %s", pathMoved, err) } - redirectValid := `following redirect to host "" url "http://localhost.com:` + strconv.Itoa(getPort(hs)) + `/.well-known/acme-challenge/` + pathValid + `"` + redirectValid := `following redirect to host "" url "http://localhost.com/.well-known/acme-challenge/` + pathValid + `"` matchedValidRedirect := log.GetAllMatching(redirectValid) test.AssertEquals(t, len(matchedValidRedirect), 1) test.AssertEquals(t, len(log.GetAllMatching(`Resolved addresses for localhost.com: \[127.0.0.1\]`)), 2) @@ -1370,7 +1371,7 @@ func TestHTTPRedirectLookup(t *testing.T) { if err != nil { t.Fatalf("Unexpected failure in redirect (%s): %s", pathFound, err) } - redirectMoved := `following redirect to host "" url "http://localhost.com:` + strconv.Itoa(getPort(hs)) + `/.well-known/acme-challenge/` + pathMoved + `"` + redirectMoved := `following redirect to host "" url "http://localhost.com/.well-known/acme-challenge/` + pathMoved + `"` matchedMovedRedirect := log.GetAllMatching(redirectMoved) test.AssertEquals(t, len(matchedMovedRedirect), 1) test.AssertEquals(t, len(log.GetAllMatching(`Resolved addresses for localhost.com: \[127.0.0.1\]`)), 3) diff --git a/va/va_test.go b/va/va_test.go index 39521734222..153f45e131c 100644 --- a/va/va_test.go +++ b/va/va_test.go @@ -539,7 +539,7 @@ func TestInternalErrorLogged(t *testing.T) { _, err := tc.validationFunc(ctx, va, req) test.AssertNotError(t, err, "failed validation should not be an error") matchingLogs := mockLog.GetAllMatching( - `Validation result JSON=.*"InternalError":"127.0.0.1: Get.*nonexistent.com:80/\.well-known.*: context deadline exceeded`) + `Validation result JSON=.*"InternalError":"127.0.0.1: Get.*nonexistent.com/\.well-known.*: context deadline exceeded`) test.AssertEquals(t, len(matchingLogs), 1) }) } From 1423162aa7be98d1fd2c3e57afbf9e89a6254594 Mon Sep 17 00:00:00 2001 From: James Renken Date: Fri, 21 Feb 2025 17:23:26 -0800 Subject: [PATCH 08/27] Remove superfluous Port from TestFetchHTTP struct --- va/http_test.go | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/va/http_test.go b/va/http_test.go index 394dd0ad3b9..48dd8149e52 100644 --- a/va/http_test.go +++ b/va/http_test.go @@ -804,7 +804,6 @@ func TestFetchHTTP(t *testing.T) { testCases := []struct { Name string Ident identifier.ACMEIdentifier - Port int Path string ExpectedBody string ExpectedRecords []core.ValidationRecord @@ -813,7 +812,6 @@ func TestFetchHTTP(t *testing.T) { { Name: "No IPs for host", Ident: identifier.NewDNS("always.invalid"), - Port: httpPort, Path: "/.well-known/whatever", ExpectedProblem: probs.DNS( "No valid IP addresses found for always.invalid"), @@ -824,7 +822,6 @@ func TestFetchHTTP(t *testing.T) { { Name: "Timeout for host with standard ACME allowed port", Ident: identifier.NewDNS("example.com"), - Port: httpPort, Path: "/timeout", ExpectedProblem: probs.Connection( "127.0.0.1: Fetching http://example.com/timeout: " + @@ -843,7 +840,6 @@ func TestFetchHTTP(t *testing.T) { { Name: "Redirect loop", Ident: identifier.NewDNS("example.com"), - Port: httpPort, Path: "/loop", ExpectedProblem: probs.Connection(fmt.Sprintf( "127.0.0.1: Fetching http://example.com:%d/loop: Redirect loop detected", httpPort)), @@ -852,7 +848,6 @@ func TestFetchHTTP(t *testing.T) { { Name: "Too many redirects", Ident: identifier.NewDNS("example.com"), - Port: httpPort, Path: "/max-redirect/0", ExpectedProblem: probs.Connection(fmt.Sprintf( "127.0.0.1: Fetching http://example.com:%d/max-redirect/12: Too many redirects", httpPort)), @@ -861,7 +856,6 @@ func TestFetchHTTP(t *testing.T) { { Name: "Redirect to bad protocol", Ident: identifier.NewDNS("example.com"), - Port: httpPort, Path: "/redir-bad-proto", ExpectedProblem: probs.Connection( "127.0.0.1: Fetching gopher://example.com: Invalid protocol scheme in " + @@ -881,7 +875,6 @@ func TestFetchHTTP(t *testing.T) { { Name: "Redirect to bad port", Ident: identifier.NewDNS("example.com"), - Port: httpPort, Path: "/redir-bad-port", ExpectedProblem: probs.Connection(fmt.Sprintf( "127.0.0.1: Fetching https://example.com:1987: Invalid port in redirect target. "+ @@ -900,7 +893,6 @@ func TestFetchHTTP(t *testing.T) { { Name: "Redirect to bare IP address", Ident: identifier.NewDNS("example.com"), - Port: httpPort, Path: "/redir-bare-ip", ExpectedBody: "ok", ExpectedRecords: []core.ValidationRecord{ @@ -924,7 +916,6 @@ func TestFetchHTTP(t *testing.T) { { Name: "Redirect to long path", Ident: identifier.NewDNS("example.com"), - Port: httpPort, Path: "/redir-path-too-long", ExpectedProblem: probs.Connection( "127.0.0.1: Fetching https://example.com/this-is-too-long-01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789: Redirect target too long"), @@ -942,7 +933,6 @@ func TestFetchHTTP(t *testing.T) { { Name: "Wrong HTTP status code", Ident: identifier.NewDNS("example.com"), - Port: httpPort, Path: "/bad-status-code", ExpectedProblem: probs.Unauthorized( "127.0.0.1: Invalid response from http://example.com/bad-status-code: 410"), @@ -960,7 +950,6 @@ func TestFetchHTTP(t *testing.T) { { Name: "HTTP status code 303 redirect", Ident: identifier.NewDNS("example.com"), - Port: httpPort, Path: "/303-see-other", ExpectedProblem: probs.Connection( "127.0.0.1: Fetching http://example.org/303-see-other: received disallowed redirect status code"), @@ -978,7 +967,6 @@ func TestFetchHTTP(t *testing.T) { { Name: "Response too large", Ident: identifier.NewDNS("example.com"), - Port: httpPort, Path: "/resp-too-big", ExpectedProblem: probs.Unauthorized(fmt.Sprintf( "127.0.0.1: Invalid response from http://example.com/resp-too-big: %q", expectedTruncatedResp.String(), @@ -997,7 +985,6 @@ func TestFetchHTTP(t *testing.T) { { Name: "Broken IPv6 only", Ident: identifier.NewDNS("ipv6.localhost"), - Port: httpPort, Path: "/ok", ExpectedProblem: probs.Connection( "::1: Fetching http://ipv6.localhost/ok: Connection refused"), @@ -1015,7 +1002,6 @@ func TestFetchHTTP(t *testing.T) { { Name: "Dual homed w/ broken IPv6, working IPv4", Ident: identifier.NewDNS("ipv4.and.ipv6.localhost"), - Port: httpPort, Path: "/ok", ExpectedBody: "ok", ExpectedRecords: []core.ValidationRecord{ @@ -1042,7 +1028,6 @@ func TestFetchHTTP(t *testing.T) { { Name: "Working IPv4 only", Ident: identifier.NewDNS("example.com"), - Port: httpPort, Path: "/ok", ExpectedBody: "ok", ExpectedRecords: []core.ValidationRecord{ @@ -1059,7 +1044,6 @@ func TestFetchHTTP(t *testing.T) { { Name: "Redirect to uppercase Public Suffix", Ident: identifier.NewDNS("example.com"), - Port: httpPort, Path: "/redir-uppercase-publicsuffix", ExpectedBody: "ok", ExpectedRecords: []core.ValidationRecord{ @@ -1084,7 +1068,6 @@ func TestFetchHTTP(t *testing.T) { { Name: "Reflected response body containing printf verbs", Ident: identifier.NewDNS("example.com"), - Port: httpPort, Path: "/printf-verbs", ExpectedProblem: &probs.ProblemDetails{ Type: probs.UnauthorizedProblem, From c7ae9322a8fc843c1793a1a4900bf50c811ba4a8 Mon Sep 17 00:00:00 2001 From: James Renken Date: Fri, 21 Feb 2025 17:47:12 -0800 Subject: [PATCH 09/27] Fix TestHTTP failure --- va/http_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/va/http_test.go b/va/http_test.go index 48dd8149e52..ee2eec6b405 100644 --- a/va/http_test.go +++ b/va/http_test.go @@ -1254,6 +1254,7 @@ func TestHTTP(t *testing.T) { } test.AssertEquals(t, len(log.GetAllMatching(`\[AUDIT\] `)), 1) + log.Clear() _, err = va.validateHTTP01(ctx, identifier.NewIP(netip.MustParseAddr("127.0.0.1")), expectedToken, expectedKeyAuthorization) if err != nil { t.Errorf("Unexpected failure in HTTP validation for IP: %s", err) From e330358289796bfed800372fa823facb3c0c1ad5 Mon Sep 17 00:00:00 2001 From: James Renken Date: Fri, 21 Feb 2025 20:24:48 -0800 Subject: [PATCH 10/27] Move tlsConfig generation inside getChallengeCert; fix SNI hostname field for IPs --- va/tlsalpn.go | 58 ++++++++++++++++++++++++++++++++-------------- va/tlsalpn_test.go | 2 +- 2 files changed, 42 insertions(+), 18 deletions(-) diff --git a/va/tlsalpn.go b/va/tlsalpn.go index d7e557b5517..6f11b1b09f4 100644 --- a/va/tlsalpn.go +++ b/va/tlsalpn.go @@ -16,6 +16,8 @@ import ( "strconv" "strings" + "github.com/miekg/dns" + "github.com/letsencrypt/boulder/bdns" "github.com/letsencrypt/boulder/core" berrors "github.com/letsencrypt/boulder/errors" @@ -61,7 +63,6 @@ func certAltNames(cert *x509.Certificate) []string { func (va *ValidationAuthorityImpl) tryGetChallengeCert( ctx context.Context, ident identifier.ACMEIdentifier, - tlsConfig *tls.Config, ) (*x509.Certificate, *tls.ConnectionState, core.ValidationRecord, error) { validationRecord := core.ValidationRecord{ DnsName: ident.Value, @@ -98,7 +99,7 @@ func (va *ValidationAuthorityImpl) tryGetChallengeCert( address := net.JoinHostPort(v6[0].String(), validationRecord.Port) validationRecord.AddressUsed = v6[0] - cert, cs, err := va.getChallengeCert(ctx, address, ident, tlsConfig) + cert, cs, err := va.getChallengeCert(ctx, address, ident) // If there is no problem, return immediately if err == nil { @@ -125,27 +126,50 @@ func (va *ValidationAuthorityImpl) tryGetChallengeCert( // talking to the first IPv6 address, try the first IPv4 address validationRecord.AddressUsed = v4[0] address := net.JoinHostPort(v4[0].String(), validationRecord.Port) - cert, cs, err := va.getChallengeCert(ctx, address, ident, tlsConfig) + cert, cs, err := va.getChallengeCert(ctx, address, ident) return cert, cs, validationRecord, err } func (va *ValidationAuthorityImpl) getChallengeCert( ctx context.Context, hostPort string, - identifier identifier.ACMEIdentifier, - config *tls.Config, + ident identifier.ACMEIdentifier, ) (*x509.Certificate, *tls.ConnectionState, error) { - va.log.Info(fmt.Sprintf("%s [%s] Attempting to validate for %s %s", core.ChallengeTypeTLSALPN01, identifier, hostPort, config.ServerName)) + tlsConfig := &tls.Config{ + MinVersion: tls.VersionTLS12, + NextProtos: []string{ACMETLS1Protocol}, + } + switch ident.Type { + case identifier.TypeDNS: + tlsConfig.ServerName = ident.Value + case identifier.TypeIP: + // TODO(#8020): Test this against RFC 8738 Sec. 6. + // https://datatracker.ietf.org/doc/html/rfc8738#section-6 + reverseIP, err := dns.ReverseAddr(ident.Value) + if err != nil { + va.log.Infof("%s Failed to parse IP address %s.", core.ChallengeTypeTLSALPN01, ident.Value) + return nil, nil, fmt.Errorf("Failed to parse IP address") + } + tlsConfig.ServerName = reverseIP + default: + // This should never happen. The calling function should check the + // identifier type. + va.log.Infof("%s Unknown identifier type '%s' for %s.", core.ChallengeTypeTLSALPN01, ident.Type, ident.Value) + return nil, nil, fmt.Errorf("Unknown identifier type: %s", ident.Type) + } + + va.log.Info(fmt.Sprintf("%s [%s] Attempting to validate for %s %s", core.ChallengeTypeTLSALPN01, ident, hostPort, tlsConfig.ServerName)) + // We expect a self-signed challenge certificate, do not verify it here. - config.InsecureSkipVerify = true + tlsConfig.InsecureSkipVerify = true dialCtx, cancel := context.WithTimeout(ctx, va.singleDialTimeout) defer cancel() - dialer := &tls.Dialer{Config: config} + dialer := &tls.Dialer{Config: tlsConfig} conn, err := dialer.DialContext(dialCtx, "tcp", hostPort) if err != nil { - va.log.Infof("%s connection failure for %s. err=[%#v] errStr=[%s]", core.ChallengeTypeTLSALPN01, identifier, err, err) + va.log.Infof("%s connection failure for %s. err=[%#v] errStr=[%s]", core.ChallengeTypeTLSALPN01, ident, err, err) host, _, splitErr := net.SplitHostPort(hostPort) if splitErr == nil && net.ParseIP(host) != nil { // Wrap the validation error and the IP of the remote host in an @@ -161,12 +185,12 @@ func (va *ValidationAuthorityImpl) getChallengeCert( cs := conn.(*tls.Conn).ConnectionState() certs := cs.PeerCertificates if len(certs) == 0 { - va.log.Infof("%s challenge for %s resulted in no certificates", core.ChallengeTypeTLSALPN01, identifier.Value) + va.log.Infof("%s challenge for %s resulted in no certificates", core.ChallengeTypeTLSALPN01, ident.Value) return nil, nil, berrors.UnauthorizedError("No certs presented for %s challenge", core.ChallengeTypeTLSALPN01) } for i, cert := range certs { va.log.AuditInfof("%s challenge for %s received certificate (%d of %d): cert=[%s]", - core.ChallengeTypeTLSALPN01, identifier.Value, i+1, len(certs), hex.EncodeToString(cert.Raw)) + core.ChallengeTypeTLSALPN01, ident.Value, i+1, len(certs), hex.EncodeToString(cert.Raw)) } return certs[0], &cs, nil } @@ -191,7 +215,7 @@ func checkExpectedSAN(cert *x509.Certificate, ident identifier.ACMEIdentifier) e default: // This should never happen. The calling function should check the // identifier type. - return errors.New("unsupported identifier type") + return fmt.Errorf("Unknown identifier type: %s", ident.Type) } for _, ext := range cert.Extensions { @@ -205,6 +229,10 @@ func checkExpectedSAN(cert *x509.Certificate, ident identifier.ACMEIdentifier) e } } + if ident.Type == identifier.TypeIP { + certSAN = []byte(net.IP(certSAN).String()) + } + if !strings.EqualFold(string(certSAN), ident.Value) { return errors.New("identifier does not match expected identifier") } @@ -241,11 +269,7 @@ func (va *ValidationAuthorityImpl) validateTLSALPN01(ctx context.Context, ident return nil, berrors.MalformedError("Identifier type for TLS-ALPN-01 challenge was not DNS or IP") } - cert, cs, tvr, err := va.tryGetChallengeCert(ctx, ident, &tls.Config{ - MinVersion: tls.VersionTLS12, - NextProtos: []string{ACMETLS1Protocol}, - ServerName: ident.Value, - }) + cert, cs, tvr, err := va.tryGetChallengeCert(ctx, ident) // Copy the single validationRecord into the slice that we have to return, and // get a reference to it so we can modify it if we have to. validationRecords := []core.ValidationRecord{tvr} diff --git a/va/tlsalpn_test.go b/va/tlsalpn_test.go index 6fdfdb69f1d..00aa5886d51 100644 --- a/va/tlsalpn_test.go +++ b/va/tlsalpn_test.go @@ -372,7 +372,7 @@ func TestTLSALPN01SuccessDNS(t *testing.T) { } func TestTLSALPN01SuccessIP(t *testing.T) { - cert := testTLSCert(nil, []net.IP{net.ParseIP("127.0.0.1")}, nil) + cert := testTLSCert(nil, []net.IP{net.ParseIP("127.0.0.1")}, []pkix.Extension{testACMEExt}) hs := tlsalpn01SrvWithCert(t, cert, 0) va, _ := setup(hs, "", nil, nil) From 84848b6c26ff47988881e55454c02854c23e00db Mon Sep 17 00:00:00 2001 From: James Renken Date: Fri, 21 Feb 2025 20:38:37 -0800 Subject: [PATCH 11/27] Tiny name & comment updates --- va/http_test.go | 24 ++++++++++++------------ va/tlsalpn.go | 2 ++ 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/va/http_test.go b/va/http_test.go index ee2eec6b405..ecb74ba7f07 100644 --- a/va/http_test.go +++ b/va/http_test.go @@ -211,7 +211,7 @@ func TestExtractRequestTarget(t *testing.T) { Name string Req *http.Request ExpectedError error - ExpectedHost identifier.ACMEIdentifier + ExpectedIdent identifier.ACMEIdentifier ExpectedPort int }{ { @@ -261,40 +261,40 @@ func TestExtractRequestTarget(t *testing.T) { Req: &http.Request{ URL: mustURL("https://10.10.10.10"), }, - ExpectedHost: identifier.NewIP(netip.MustParseAddr("10.10.10.10")), - ExpectedPort: 443, + ExpectedIdent: identifier.NewIP(netip.MustParseAddr("10.10.10.10")), + ExpectedPort: 443, }, { Name: "valid HTTP redirect, explicit port", Req: &http.Request{ URL: mustURL("http://cpu.letsencrypt.org:80"), }, - ExpectedHost: identifier.NewDNS("cpu.letsencrypt.org"), - ExpectedPort: 80, + ExpectedIdent: identifier.NewDNS("cpu.letsencrypt.org"), + ExpectedPort: 80, }, { Name: "valid HTTP redirect, implicit port", Req: &http.Request{ URL: mustURL("http://cpu.letsencrypt.org"), }, - ExpectedHost: identifier.NewDNS("cpu.letsencrypt.org"), - ExpectedPort: 80, + ExpectedIdent: identifier.NewDNS("cpu.letsencrypt.org"), + ExpectedPort: 80, }, { Name: "valid HTTPS redirect, explicit port", Req: &http.Request{ URL: mustURL("https://cpu.letsencrypt.org:443/hello.world"), }, - ExpectedHost: identifier.NewDNS("cpu.letsencrypt.org"), - ExpectedPort: 443, + ExpectedIdent: identifier.NewDNS("cpu.letsencrypt.org"), + ExpectedPort: 443, }, { Name: "valid HTTPS redirect, implicit port", Req: &http.Request{ URL: mustURL("https://cpu.letsencrypt.org/hello.world"), }, - ExpectedHost: identifier.NewDNS("cpu.letsencrypt.org"), - ExpectedPort: 443, + ExpectedIdent: identifier.NewDNS("cpu.letsencrypt.org"), + ExpectedPort: 443, }, } @@ -309,7 +309,7 @@ func TestExtractRequestTarget(t *testing.T) { } else if err == nil && tc.ExpectedError != nil { t.Errorf("Expected err %v, got nil", tc.ExpectedError) } else { - test.AssertEquals(t, host, tc.ExpectedHost) + test.AssertEquals(t, host, tc.ExpectedIdent) test.AssertEquals(t, port, tc.ExpectedPort) } }) diff --git a/va/tlsalpn.go b/va/tlsalpn.go index 6f11b1b09f4..11cd43b762a 100644 --- a/va/tlsalpn.go +++ b/va/tlsalpn.go @@ -82,6 +82,8 @@ func (va *ValidationAuthorityImpl) tryGetChallengeCert( case identifier.TypeIP: validationRecord.AddressesResolved = []net.IP{net.ParseIP(ident.Value)} default: + // This should never happen. The calling function should check the + // identifier type. return nil, nil, validationRecord, fmt.Errorf("Unknown identifier type: %s", ident.Type) } From f76c8acac8df1963a47345457e7b868b8edf4dd0 Mon Sep 17 00:00:00 2001 From: James Renken Date: Fri, 21 Feb 2025 20:39:47 -0800 Subject: [PATCH 12/27] Regenerate PBs with comment --- va/proto/va.pb.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/va/proto/va.pb.go b/va/proto/va.pb.go index d9e660822a9..9b79a58d3db 100644 --- a/va/proto/va.pb.go +++ b/va/proto/va.pb.go @@ -26,6 +26,9 @@ type IsCAAValidRequest struct { sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields + // TODO(#7311): Accept an identifier instead of a domain (purely for + // consistency, because only DNS identifiers support CAA checks). + // // NOTE: Domain may be a name with a wildcard prefix (e.g. `*.example.com`) Domain string `protobuf:"bytes,1,opt,name=domain,proto3" json:"domain,omitempty"` ValidationMethod string `protobuf:"bytes,2,opt,name=validationMethod,proto3" json:"validationMethod,omitempty"` From 9df104642c58080d5fedda81a7cc8c96d4cd4a19 Mon Sep 17 00:00:00 2001 From: James Renken Date: Fri, 21 Feb 2025 23:11:52 -0800 Subject: [PATCH 13/27] Add more test cases --- va/dns_test.go | 10 ++++++++++ va/http.go | 1 - va/http_test.go | 17 +++++++++++++++++ va/tlsalpn_test.go | 12 ++++++++++++ va/va_test.go | 3 --- 5 files changed, 39 insertions(+), 4 deletions(-) diff --git a/va/dns_test.go b/va/dns_test.go index 0c2d5102f3b..f64e2cc997b 100644 --- a/va/dns_test.go +++ b/va/dns_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net" + "net/netip" "testing" "time" @@ -78,6 +79,15 @@ func TestDNSValidationFailure(t *testing.T) { test.AssertEquals(t, prob.Type, probs.UnauthorizedProblem) } +func TestDNSValidationIP(t *testing.T) { + va, _ := setup(nil, "", nil, nil) + + _, err := va.validateDNS01(ctx, identifier.NewIP(netip.MustParseAddr("127.0.0.1")), expectedKeyAuthorization) + prob := detailedError(err) + + test.AssertEquals(t, prob.Type, probs.MalformedProblem) +} + func TestDNSValidationInvalid(t *testing.T) { var notDNS = identifier.ACMEIdentifier{ Type: identifier.IdentifierType("iris"), diff --git a/va/http.go b/va/http.go index 6e13f22ecde..3965784d61d 100644 --- a/va/http.go +++ b/va/http.go @@ -642,7 +642,6 @@ func (va *ValidationAuthorityImpl) processHTTPValidation( } func (va *ValidationAuthorityImpl) validateHTTP01(ctx context.Context, ident identifier.ACMEIdentifier, token string, keyAuthorization string) ([]core.ValidationRecord, error) { - // TODO(#8020): This needs testing. if ident.Type != identifier.TypeDNS && ident.Type != identifier.TypeIP { va.log.Info(fmt.Sprintf("Identifier type for HTTP-01 challenge was not DNS or IP: %s", ident)) return nil, berrors.MalformedError("Identifier type for HTTP-01 challenge was not DNS or IP") diff --git a/va/http_test.go b/va/http_test.go index ecb74ba7f07..6900d8b8e26 100644 --- a/va/http_test.go +++ b/va/http_test.go @@ -1222,6 +1222,23 @@ func TestHTTPBadPort(t *testing.T) { } } +func TestHTTPBadIdentifier(t *testing.T) { + hs := httpSrv(t, expectedToken) + defer hs.Close() + + va, _ := setup(hs, "", nil, nil) + + _, err := va.validateHTTP01(ctx, identifier.ACMEIdentifier{Type: "smime", Value: "dobber@bad.horse"}, expectedToken, expectedKeyAuthorization) + if err == nil { + t.Fatalf("Server accepted a hypothetical S/MIME identifier") + } + prob := detailedError(err) + test.AssertEquals(t, prob.Type, probs.MalformedProblem) + if !strings.Contains(prob.Detail, "Identifier type for HTTP-01 challenge was not DNS or IP") { + t.Errorf("Expected an identifier type error, got %q", prob.Detail) + } +} + func TestHTTPKeyAuthorizationFileMismatch(t *testing.T) { m := http.NewServeMux() hs := httptest.NewUnstartedServer(m) diff --git a/va/tlsalpn_test.go b/va/tlsalpn_test.go index 00aa5886d51..4f9b5ca756b 100644 --- a/va/tlsalpn_test.go +++ b/va/tlsalpn_test.go @@ -562,6 +562,18 @@ func TestTLSALPN01WrongName(t *testing.T) { test.AssertContains(t, err.Error(), "identifier does not match expected identifier") } +func TestTLSALPN01WrongIP(t *testing.T) { + // Create a cert with a different IP address from what we're validating + cert := testTLSCert(nil, []net.IP{net.ParseIP("10.10.10.10")}, []pkix.Extension{testACMEExt}) + hs := tlsalpn01SrvWithCert(t, cert, 0) + + va, _ := setup(hs, "", nil, nil) + + _, err := va.validateTLSALPN01(ctx, identifier.NewIP(netip.MustParseAddr("127.0.0.1")), expectedKeyAuthorization) + test.AssertError(t, err, "validation should have failed") + test.AssertContains(t, err.Error(), "identifier does not match expected identifier") +} + func TestTLSALPN01ExtraNames(t *testing.T) { // Create a cert with two names when we only want to validate one. hs := tlsalpn01SrvWithCert(t, testACMECert([]string{"expected", "extra"}), 0) diff --git a/va/va_test.go b/va/va_test.go index 153f45e131c..a9362275264 100644 --- a/va/va_test.go +++ b/va/va_test.go @@ -602,9 +602,6 @@ func TestPerformValidationValid(t *testing.T) { // TestPerformValidationWildcard tests that the VA properly strips the `*.` // prefix from a wildcard name provided to the PerformValidation function. -// -// TODO(#8020): This or another test should check that an IP address identifier -// with a wildcard is rejected. func TestPerformValidationWildcard(t *testing.T) { t.Parallel() From 0e52b95c8b8455de4c600a52738b0c991b0482ea Mon Sep 17 00:00:00 2001 From: James Renken Date: Sat, 22 Feb 2025 16:38:20 -0800 Subject: [PATCH 14/27] Add test cases & correctly handle bare IPv6 in redirects --- va/http.go | 10 +++++++ va/http_test.go | 69 ++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 72 insertions(+), 7 deletions(-) diff --git a/va/http.go b/va/http.go index 3965784d61d..f3568cf5278 100644 --- a/va/http.go +++ b/va/http.go @@ -314,6 +314,16 @@ func (va *ValidationAuthorityImpl) extractRequestTarget(req *http.Request) (iden return identifier.ACMEIdentifier{}, 0, berrors.ConnectionFailureError("Invalid empty host in redirect target") } + // In URLs, bare IPv6 addresses are always enclosed in square brackets, + // which must be stripped in order for net.ParseIP to recognize the address. + if len(reqHost) > 2 && reqHost[0] == '[' && reqHost[len(reqHost)-1] == ']' { + shortHost := reqHost[1 : len(reqHost)-1] + // Square brackets are only valid syntax for bare IPs, not DNS names. + if net.ParseIP(shortHost) != nil { + reqHost = shortHost + } + } + // Often folks will misconfigure their webserver to send an HTTP redirect // missing a `/' between the FQDN and the path. E.g. in Apache using: // Redirect / https://bad-redirect.org diff --git a/va/http_test.go b/va/http_test.go index 6900d8b8e26..ca53bcfcc67 100644 --- a/va/http_test.go +++ b/va/http_test.go @@ -139,26 +139,36 @@ func TestHTTPValidationTarget(t *testing.T) { ExpectedIPs []string }{ { - Name: "No IPs for host", + Name: "No IPs for DNS identifier", Ident: identifier.NewDNS("always.invalid"), ExpectedError: berrors.DNSError("No valid IP addresses found for always.invalid"), }, { - Name: "Only IPv4 addrs for host", + Name: "Only IPv4 addrs for DNS identifier", Ident: identifier.NewDNS("some.example.com"), ExpectedIPs: []string{"127.0.0.1"}, }, { - Name: "Only IPv6 addrs for host", + Name: "Only IPv6 addrs for DNS identifier", Ident: identifier.NewDNS("ipv6.localhost"), ExpectedIPs: []string{"::1"}, }, { - Name: "Both IPv6 and IPv4 addrs for host", + Name: "Both IPv6 and IPv4 addrs for DNS identifier", Ident: identifier.NewDNS("ipv4.and.ipv6.localhost"), // In this case we expect 1 IPv6 address first, and then 1 IPv4 address ExpectedIPs: []string{"::1", "127.0.0.1"}, }, + { + Name: "IPv4 IP address identifier", + Ident: identifier.NewIP(netip.MustParseAddr("127.0.0.1")), + ExpectedIPs: []string{"127.0.0.1"}, + }, + { + Name: "IPv6 IP address identifier", + Ident: identifier.NewIP(netip.MustParseAddr("::1")), + ExpectedIPs: []string{"::1"}, + }, } const ( @@ -257,12 +267,57 @@ func TestExtractRequestTarget(t *testing.T) { ExpectedError: errors.New("Invalid host in redirect target, must end in IANA registered TLD"), }, { - Name: "bare IP", + Name: "malformed wildcard-ish IPv4 address", + Req: &http.Request{ + URL: mustURL("https://10.10.10.*"), + }, + ExpectedError: errors.New("Invalid host in redirect target, must end in IANA registered TLD"), + }, + { + Name: "malformed too-long IPv6 address", + Req: &http.Request{ + URL: mustURL("https://[a:b:c:d:e:f:b:a:d]"), + }, + ExpectedError: errors.New("Invalid host in redirect target, must end in IANA registered TLD"), + }, + { + Name: "DNS name enclosed in square brackets", Req: &http.Request{ - URL: mustURL("https://10.10.10.10"), + URL: mustURL("https://[bad.horse]"), + }, + ExpectedError: errors.New("Invalid host in redirect target, must end in IANA registered TLD"), + }, + { + Name: "bare IPv4, implicit port", + Req: &http.Request{ + URL: mustURL("http://10.10.10.10"), }, ExpectedIdent: identifier.NewIP(netip.MustParseAddr("10.10.10.10")), - ExpectedPort: 443, + ExpectedPort: 80, + }, + { + Name: "bare IPv4, explicit port", + Req: &http.Request{ + URL: mustURL("http://10.10.10.10:80"), + }, + ExpectedIdent: identifier.NewIP(netip.MustParseAddr("10.10.10.10")), + ExpectedPort: 80, + }, + { + Name: "bare IPv6, implicit port", + Req: &http.Request{ + URL: mustURL("http://[::1]"), + }, + ExpectedIdent: identifier.NewIP(netip.MustParseAddr("::1")), + ExpectedPort: 80, + }, + { + Name: "bare IPv6, explicit port", + Req: &http.Request{ + URL: mustURL("http://[::1]:80"), + }, + ExpectedIdent: identifier.NewIP(netip.MustParseAddr("::1")), + ExpectedPort: 80, }, { Name: "valid HTTP redirect, explicit port", From ad6b6b22a02eab785cbd761cd3616ed8b3d0b3b2 Mon Sep 17 00:00:00 2001 From: James Renken Date: Sat, 22 Feb 2025 20:33:03 -0800 Subject: [PATCH 15/27] Fix RFC 8738 Sec. 3 compliance; add IPv6 httptest ability; fix bare IPv6 in URL construction; add test cases --- identifier/identifier.go | 7 +- va/caa_test.go | 2 +- va/http.go | 17 +++- va/http_test.go | 165 ++++++++++++++++++++++++++++----------- va/tlsalpn.go | 2 - va/tlsalpn_test.go | 83 +++++++++++++++----- 6 files changed, 204 insertions(+), 72 deletions(-) diff --git a/identifier/identifier.go b/identifier/identifier.go index 24453547cde..3754657bf64 100644 --- a/identifier/identifier.go +++ b/identifier/identifier.go @@ -59,7 +59,10 @@ func NewDNS(domain string) ACMEIdentifier { // for a given IP address. func NewIP(ip netip.Addr) ACMEIdentifier { return ACMEIdentifier{ - Type: TypeIP, - Value: ip.StringExpanded(), + Type: TypeIP, + // Section 3 of RFC 8738: The identifier value MUST contain the textual + // form of the address as defined in Section 2.1 of RFC1123 for IPv4 and + // in Section 4 of RFC5952 for IPv6. + Value: ip.String(), } } diff --git a/va/caa_test.go b/va/caa_test.go index c65270bcc1f..246df38d2e5 100644 --- a/va/caa_test.go +++ b/va/caa_test.go @@ -1226,7 +1226,7 @@ func TestMultiCAARechecking(t *testing.T) { } func TestCAAFailure(t *testing.T) { - hs := httpSrv(t, expectedToken) + hs := httpSrv(t, expectedToken, false) defer hs.Close() va, _ := setup(hs, "", nil, caaMockDNS{}) diff --git a/va/http.go b/va/http.go index f3568cf5278..1238d073cbc 100644 --- a/va/http.go +++ b/va/http.go @@ -202,8 +202,6 @@ func (vt *httpValidationTarget) nextIP() error { // port, and path. This involves querying DNS for the IP addresses for the host. // An error is returned if there are no usable IP addresses or if the DNS // lookups fail. -// -// TODO(#8020): This needs testing with IP address identifiers. func (va *ValidationAuthorityImpl) newHTTPValidationTarget( ctx context.Context, ident identifier.ACMEIdentifier, @@ -437,10 +435,23 @@ func (va *ValidationAuthorityImpl) processHTTPValidation( return nil, nil, err } + // When constructing a URL, bare IPv6 addresses must be enclosed in square + // brackets. Otherwise, a colon may be interpreted as a port separator. + host := ident.Value + if ident.Type == identifier.TypeIP { + netipHost, err := netip.ParseAddr(host) + if err != nil { + return nil, nil, fmt.Errorf("couldn't parse IP address from identifier") + } + if netipHost.Is6() { + host = "[" + host + "]" + } + } + // Create an initial GET Request initialURL := url.URL{ Scheme: "http", - Host: ident.Value, + Host: host, Path: path, } initialReq, err := http.NewRequest("GET", initialURL.String(), nil) diff --git a/va/http_test.go b/va/http_test.go index ca53bcfcc67..388e9c0ed0c 100644 --- a/va/http_test.go +++ b/va/http_test.go @@ -542,11 +542,19 @@ func TestSetupHTTPValidation(t *testing.T) { } // A more concise version of httpSrv() that supports http.go tests -func httpTestSrv(t *testing.T) *httptest.Server { +func httpTestSrv(t *testing.T, ipv6 bool) *httptest.Server { t.Helper() mux := http.NewServeMux() server := httptest.NewUnstartedServer(mux) + if ipv6 { + l, err := net.Listen("tcp", "[::1]:0") + if err != nil { + panic(fmt.Sprintf("httptest: failed to listen on a port: %v", err)) + } + server.Listener = l + } + server.Start() httpPort := getPort(server) @@ -611,7 +619,7 @@ func httpTestSrv(t *testing.T) *httptest.Server { }) // A path that always redirects to a URL with a bare IP address - mux.HandleFunc("/redir-bare-ip", func(resp http.ResponseWriter, req *http.Request) { + mux.HandleFunc("/redir-bare-ipv4", func(resp http.ResponseWriter, req *http.Request) { http.Redirect( resp, req, @@ -620,6 +628,15 @@ func httpTestSrv(t *testing.T) *httptest.Server { ) }) + mux.HandleFunc("/redir-bare-ipv6", func(resp http.ResponseWriter, req *http.Request) { + http.Redirect( + resp, + req, + "http://[::1]/ok", + http.StatusMovedPermanently, + ) + }) + mux.HandleFunc("/bad-status-code", func(resp http.ResponseWriter, req *http.Request) { resp.WriteHeader(http.StatusGone) fmt.Fprint(resp, "sorry, I'm gone") @@ -794,16 +811,20 @@ func TestFallbackErr(t *testing.T) { } func TestFetchHTTP(t *testing.T) { - // Create a test server - testSrv := httpTestSrv(t) - defer testSrv.Close() + // Create test servers + testSrvIPv4 := httpTestSrv(t, false) + defer testSrvIPv4.Close() + testSrvIPv6 := httpTestSrv(t, true) + defer testSrvIPv6.Close() - // Setup a VA. By providing the testSrv to setup the VA will use the testSrv's + // Setup VAs. By providing the testSrv to setup the VA will use the testSrv's // randomly assigned port as its HTTP port. - va, _ := setup(testSrv, "", nil, nil) + vaIPv4, _ := setup(testSrvIPv4, "", nil, nil) + vaIPv6, _ := setup(testSrvIPv6, "", nil, nil) // We need to know the randomly assigned HTTP port for testcases as well - httpPort := getPort(testSrv) + httpPortIPv4 := getPort(testSrvIPv4) + httpPortIPv6 := getPort(testSrvIPv6) // For the looped test case we expect one validation record per redirect // until boulder detects that a url has been used twice indicating a @@ -817,12 +838,12 @@ func TestFetchHTTP(t *testing.T) { // The first request will not have a port # in the URL. url := "http://example.com/loop" if i != 0 { - url = fmt.Sprintf("http://example.com:%d/loop", httpPort) + url = fmt.Sprintf("http://example.com:%d/loop", httpPortIPv4) } expectedLoopRecords = append(expectedLoopRecords, core.ValidationRecord{ DnsName: "example.com", - Port: strconv.Itoa(httpPort), + Port: strconv.Itoa(httpPortIPv4), URL: url, AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), @@ -838,12 +859,12 @@ func TestFetchHTTP(t *testing.T) { // The first request will not have a port # in the URL. url := "http://example.com/max-redirect/0" if i != 0 { - url = fmt.Sprintf("http://example.com:%d/max-redirect/%d", httpPort, i) + url = fmt.Sprintf("http://example.com:%d/max-redirect/%d", httpPortIPv4, i) } expectedTooManyRedirRecords = append(expectedTooManyRedirRecords, core.ValidationRecord{ DnsName: "example.com", - Port: strconv.Itoa(httpPort), + Port: strconv.Itoa(httpPortIPv4), URL: url, AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), @@ -858,6 +879,7 @@ func TestFetchHTTP(t *testing.T) { testCases := []struct { Name string + IPv6 bool Ident identifier.ACMEIdentifier Path string ExpectedBody string @@ -884,7 +906,7 @@ func TestFetchHTTP(t *testing.T) { ExpectedRecords: []core.ValidationRecord{ { DnsName: "example.com", - Port: strconv.Itoa(httpPort), + Port: strconv.Itoa(httpPortIPv4), URL: "http://example.com/timeout", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), @@ -897,7 +919,7 @@ func TestFetchHTTP(t *testing.T) { Ident: identifier.NewDNS("example.com"), Path: "/loop", ExpectedProblem: probs.Connection(fmt.Sprintf( - "127.0.0.1: Fetching http://example.com:%d/loop: Redirect loop detected", httpPort)), + "127.0.0.1: Fetching http://example.com:%d/loop: Redirect loop detected", httpPortIPv4)), ExpectedRecords: expectedLoopRecords, }, { @@ -905,7 +927,7 @@ func TestFetchHTTP(t *testing.T) { Ident: identifier.NewDNS("example.com"), Path: "/max-redirect/0", ExpectedProblem: probs.Connection(fmt.Sprintf( - "127.0.0.1: Fetching http://example.com:%d/max-redirect/12: Too many redirects", httpPort)), + "127.0.0.1: Fetching http://example.com:%d/max-redirect/12: Too many redirects", httpPortIPv4)), ExpectedRecords: expectedTooManyRedirRecords, }, { @@ -919,7 +941,7 @@ func TestFetchHTTP(t *testing.T) { ExpectedRecords: []core.ValidationRecord{ { DnsName: "example.com", - Port: strconv.Itoa(httpPort), + Port: strconv.Itoa(httpPortIPv4), URL: "http://example.com/redir-bad-proto", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), @@ -933,11 +955,11 @@ func TestFetchHTTP(t *testing.T) { Path: "/redir-bad-port", ExpectedProblem: probs.Connection(fmt.Sprintf( "127.0.0.1: Fetching https://example.com:1987: Invalid port in redirect target. "+ - "Only ports %d and 443 are supported, not 1987", httpPort)), + "Only ports %d and 443 are supported, not 1987", httpPortIPv4)), ExpectedRecords: []core.ValidationRecord{ { DnsName: "example.com", - Port: strconv.Itoa(httpPort), + Port: strconv.Itoa(httpPortIPv4), URL: "http://example.com/redir-bad-port", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), @@ -946,27 +968,50 @@ func TestFetchHTTP(t *testing.T) { }, }, { - Name: "Redirect to bare IP address", + Name: "Redirect to bare IPv4 address", Ident: identifier.NewDNS("example.com"), - Path: "/redir-bare-ip", + Path: "/redir-bare-ipv4", ExpectedBody: "ok", ExpectedRecords: []core.ValidationRecord{ { DnsName: "example.com", - Port: strconv.Itoa(httpPort), - URL: "http://example.com/redir-bare-ip", + Port: strconv.Itoa(httpPortIPv4), + URL: "http://example.com/redir-bare-ipv4", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), ResolverAddrs: []string{"MockClient"}, }, { DnsName: "127.0.0.1", - Port: strconv.Itoa(httpPort), + Port: strconv.Itoa(httpPortIPv4), URL: "http://127.0.0.1/ok", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), }, }, + }, { + Name: "Redirect to bare IPv6 address", + IPv6: true, + Ident: identifier.NewDNS("ipv6.localhost"), + Path: "/redir-bare-ipv6", + ExpectedBody: "ok", + ExpectedRecords: []core.ValidationRecord{ + { + DnsName: "ipv6.localhost", + Port: strconv.Itoa(httpPortIPv6), + URL: "http://ipv6.localhost/redir-bare-ipv6", + AddressesResolved: []net.IP{net.ParseIP("::1")}, + AddressUsed: net.ParseIP("::1"), + ResolverAddrs: []string{"MockClient"}, + }, + { + DnsName: "::1", + Port: strconv.Itoa(httpPortIPv6), + URL: "http://[::1]/ok", + AddressesResolved: []net.IP{net.ParseIP("::1")}, + AddressUsed: net.ParseIP("::1"), + }, + }, }, { Name: "Redirect to long path", @@ -977,7 +1022,7 @@ func TestFetchHTTP(t *testing.T) { ExpectedRecords: []core.ValidationRecord{ { DnsName: "example.com", - Port: strconv.Itoa(httpPort), + Port: strconv.Itoa(httpPortIPv4), URL: "http://example.com/redir-path-too-long", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), @@ -994,7 +1039,7 @@ func TestFetchHTTP(t *testing.T) { ExpectedRecords: []core.ValidationRecord{ { DnsName: "example.com", - Port: strconv.Itoa(httpPort), + Port: strconv.Itoa(httpPortIPv4), URL: "http://example.com/bad-status-code", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), @@ -1011,7 +1056,7 @@ func TestFetchHTTP(t *testing.T) { ExpectedRecords: []core.ValidationRecord{ { DnsName: "example.com", - Port: strconv.Itoa(httpPort), + Port: strconv.Itoa(httpPortIPv4), URL: "http://example.com/303-see-other", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), @@ -1029,7 +1074,7 @@ func TestFetchHTTP(t *testing.T) { ExpectedRecords: []core.ValidationRecord{ { DnsName: "example.com", - Port: strconv.Itoa(httpPort), + Port: strconv.Itoa(httpPortIPv4), URL: "http://example.com/resp-too-big", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), @@ -1046,7 +1091,7 @@ func TestFetchHTTP(t *testing.T) { ExpectedRecords: []core.ValidationRecord{ { DnsName: "ipv6.localhost", - Port: strconv.Itoa(httpPort), + Port: strconv.Itoa(httpPortIPv4), URL: "http://ipv6.localhost/ok", AddressesResolved: []net.IP{net.ParseIP("::1")}, AddressUsed: net.ParseIP("::1"), @@ -1062,7 +1107,7 @@ func TestFetchHTTP(t *testing.T) { ExpectedRecords: []core.ValidationRecord{ { DnsName: "ipv4.and.ipv6.localhost", - Port: strconv.Itoa(httpPort), + Port: strconv.Itoa(httpPortIPv4), URL: "http://ipv4.and.ipv6.localhost/ok", AddressesResolved: []net.IP{net.ParseIP("::1"), net.ParseIP("127.0.0.1")}, // The first validation record should have used the IPv6 addr @@ -1071,7 +1116,7 @@ func TestFetchHTTP(t *testing.T) { }, { DnsName: "ipv4.and.ipv6.localhost", - Port: strconv.Itoa(httpPort), + Port: strconv.Itoa(httpPortIPv4), URL: "http://ipv4.and.ipv6.localhost/ok", AddressesResolved: []net.IP{net.ParseIP("::1"), net.ParseIP("127.0.0.1")}, // The second validation record should have used the IPv4 addr as a fallback @@ -1088,7 +1133,7 @@ func TestFetchHTTP(t *testing.T) { ExpectedRecords: []core.ValidationRecord{ { DnsName: "example.com", - Port: strconv.Itoa(httpPort), + Port: strconv.Itoa(httpPortIPv4), URL: "http://example.com/ok", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), @@ -1104,7 +1149,7 @@ func TestFetchHTTP(t *testing.T) { ExpectedRecords: []core.ValidationRecord{ { DnsName: "example.com", - Port: strconv.Itoa(httpPort), + Port: strconv.Itoa(httpPortIPv4), URL: "http://example.com/redir-uppercase-publicsuffix", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), @@ -1112,7 +1157,7 @@ func TestFetchHTTP(t *testing.T) { }, { DnsName: "example.com", - Port: strconv.Itoa(httpPort), + Port: strconv.Itoa(httpPortIPv4), URL: "http://example.com/ok", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), @@ -1133,7 +1178,7 @@ func TestFetchHTTP(t *testing.T) { ExpectedRecords: []core.ValidationRecord{ { DnsName: "example.com", - Port: strconv.Itoa(httpPort), + Port: strconv.Itoa(httpPortIPv4), URL: "http://example.com/printf-verbs", AddressesResolved: []net.IP{net.ParseIP("127.0.0.1")}, AddressUsed: net.ParseIP("127.0.0.1"), @@ -1147,7 +1192,14 @@ func TestFetchHTTP(t *testing.T) { t.Run(tc.Name, func(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*500) defer cancel() - body, records, err := va.processHTTPValidation(ctx, tc.Ident, tc.Path) + var body []byte + var records []core.ValidationRecord + var err error + if tc.IPv6 { + body, records, err = vaIPv6.processHTTPValidation(ctx, tc.Ident, tc.Path) + } else { + body, records, err = vaIPv4.processHTTPValidation(ctx, tc.Ident, tc.Path) + } if tc.ExpectedProblem == nil { test.AssertNotError(t, err, "expected nil prob") } else { @@ -1180,7 +1232,7 @@ const pathLooper = "looper" const pathValid = "valid" const rejectUserAgent = "rejectMe" -func httpSrv(t *testing.T, token string) *httptest.Server { +func httpSrv(t *testing.T, token string, ipv6 bool) *httptest.Server { m := http.NewServeMux() server := httptest.NewUnstartedServer(m) @@ -1250,12 +1302,20 @@ func httpSrv(t *testing.T, token string) *httptest.Server { } }) + if ipv6 { + l, err := net.Listen("tcp", "[::1]:0") + if err != nil { + panic(fmt.Sprintf("httptest: failed to listen on a port: %v", err)) + } + server.Listener = l + } + server.Start() return server } func TestHTTPBadPort(t *testing.T) { - hs := httpSrv(t, expectedToken) + hs := httpSrv(t, expectedToken, false) defer hs.Close() va, _ := setup(hs, "", nil, nil) @@ -1278,7 +1338,7 @@ func TestHTTPBadPort(t *testing.T) { } func TestHTTPBadIdentifier(t *testing.T) { - hs := httpSrv(t, expectedToken) + hs := httpSrv(t, expectedToken, false) defer hs.Close() va, _ := setup(hs, "", nil, nil) @@ -1315,7 +1375,7 @@ func TestHTTPKeyAuthorizationFileMismatch(t *testing.T) { } func TestHTTP(t *testing.T) { - hs := httpSrv(t, expectedToken) + hs := httpSrv(t, expectedToken, false) defer hs.Close() va, log := setup(hs, "", nil, nil) @@ -1329,7 +1389,7 @@ func TestHTTP(t *testing.T) { log.Clear() _, err = va.validateHTTP01(ctx, identifier.NewIP(netip.MustParseAddr("127.0.0.1")), expectedToken, expectedKeyAuthorization) if err != nil { - t.Errorf("Unexpected failure in HTTP validation for IP: %s", err) + t.Errorf("Unexpected failure in HTTP validation for IPv4: %s", err) } test.AssertEquals(t, len(log.GetAllMatching(`\[AUDIT\] `)), 1) @@ -1379,8 +1439,21 @@ func TestHTTP(t *testing.T) { test.AssertEquals(t, prob.Type, probs.DNSProblem) } +func TestHTTPIPv6(t *testing.T) { + hs := httpSrv(t, expectedToken, true) + defer hs.Close() + + va, log := setup(hs, "", nil, nil) + + _, err := va.validateHTTP01(ctx, identifier.NewIP(netip.MustParseAddr("::1")), expectedToken, expectedKeyAuthorization) + if err != nil { + t.Errorf("Unexpected failure in HTTP validation for IPv6: %s", err) + } + test.AssertEquals(t, len(log.GetAllMatching(`\[AUDIT\] `)), 1) +} + func TestHTTPTimeout(t *testing.T) { - hs := httpSrv(t, expectedToken) + hs := httpSrv(t, expectedToken, false) defer hs.Close() va, _ := setup(hs, "", nil, nil) @@ -1409,7 +1482,7 @@ func TestHTTPTimeout(t *testing.T) { } func TestHTTPRedirectLookup(t *testing.T) { - hs := httpSrv(t, expectedToken) + hs := httpSrv(t, expectedToken, false) defer hs.Close() va, log := setup(hs, "", nil, nil) @@ -1471,7 +1544,7 @@ func TestHTTPRedirectLookup(t *testing.T) { } func TestHTTPRedirectLoop(t *testing.T) { - hs := httpSrv(t, expectedToken) + hs := httpSrv(t, expectedToken, false) defer hs.Close() va, _ := setup(hs, "", nil, nil) @@ -1482,7 +1555,7 @@ func TestHTTPRedirectLoop(t *testing.T) { } func TestHTTPRedirectUserAgent(t *testing.T) { - hs := httpSrv(t, expectedToken) + hs := httpSrv(t, expectedToken, false) defer hs.Close() va, _ := setup(hs, "", nil, nil) va.userAgent = rejectUserAgent @@ -1517,7 +1590,7 @@ func getPort(hs *httptest.Server) int { func TestValidateHTTP(t *testing.T) { token := core.NewToken() - hs := httpSrv(t, token) + hs := httpSrv(t, token, false) defer hs.Close() va, _ := setup(hs, "", nil, nil) @@ -1529,7 +1602,7 @@ func TestValidateHTTP(t *testing.T) { func TestLimitedReader(t *testing.T) { token := core.NewToken() - hs := httpSrv(t, "012345\xff67890123456789012345678901234567890123456789012345678901234567890123456789") + hs := httpSrv(t, "012345\xff67890123456789012345678901234567890123456789012345678901234567890123456789", false) va, _ := setup(hs, "", nil, nil) defer hs.Close() diff --git a/va/tlsalpn.go b/va/tlsalpn.go index 11cd43b762a..8352e47c0b7 100644 --- a/va/tlsalpn.go +++ b/va/tlsalpn.go @@ -59,7 +59,6 @@ func certAltNames(cert *x509.Certificate) []string { return names } -// TODO(#8020): Identifiers need testing here. func (va *ValidationAuthorityImpl) tryGetChallengeCert( ctx context.Context, ident identifier.ACMEIdentifier, @@ -265,7 +264,6 @@ func checkAcceptableExtensions(exts []pkix.Extension, requiredOIDs []asn1.Object } func (va *ValidationAuthorityImpl) validateTLSALPN01(ctx context.Context, ident identifier.ACMEIdentifier, keyAuthorization string) ([]core.ValidationRecord, error) { - // TODO(#8020): This needs testing. if ident.Type != identifier.TypeDNS && ident.Type != identifier.TypeIP { va.log.Info(fmt.Sprintf("Identifier type for TLS-ALPN-01 challenge was not DNS or IP: %s", ident)) return nil, berrors.MalformedError("Identifier type for TLS-ALPN-01 challenge was not DNS or IP") diff --git a/va/tlsalpn_test.go b/va/tlsalpn_test.go index 4f9b5ca756b..f099eb0ba6e 100644 --- a/va/tlsalpn_test.go +++ b/va/tlsalpn_test.go @@ -84,7 +84,7 @@ func testACMECert(names []string) *tls.Certificate { // tlsalpn01SrvWithCert creates a test server which will present the given // certificate when asked to do a tls-alpn-01 handshake. -func tlsalpn01SrvWithCert(t *testing.T, acmeCert *tls.Certificate, tlsVersion uint16) *httptest.Server { +func tlsalpn01SrvWithCert(t *testing.T, acmeCert *tls.Certificate, tlsVersion uint16, ipv6 bool) *httptest.Server { t.Helper() tlsConfig := &tls.Config{ @@ -105,6 +105,13 @@ func tlsalpn01SrvWithCert(t *testing.T, acmeCert *tls.Certificate, tlsVersion ui _ = conn.Close() }, } + if ipv6 { + l, err := net.Listen("tcp", "[::1]:0") + if err != nil { + panic(fmt.Sprintf("httptest: failed to listen on a port: %v", err)) + } + hs.Listener = l + } hs.StartTLS() return hs } @@ -113,7 +120,7 @@ func tlsalpn01SrvWithCert(t *testing.T, acmeCert *tls.Certificate, tlsVersion ui // that don't need to customize specific names or extensions in the certificate // served by the TLS server. func testTLSALPN01Srv(t *testing.T) *httptest.Server { - return tlsalpn01SrvWithCert(t, testACMECert([]string{"expected"}), 0) + return tlsalpn01SrvWithCert(t, testACMECert([]string{"expected"}), 0, false) } func slowTLSSrv() *httptest.Server { @@ -240,7 +247,7 @@ func TestTLSALPN01TalkingToHTTP(t *testing.T) { va, _ := setup(hs, "", nil, nil) // Make the server only speak HTTP. - httpOnly := httpSrv(t, "") + httpOnly := httpSrv(t, "", false) va.tlsPort = getPort(httpOnly) _, err := va.validateTLSALPN01(ctx, dnsi("expected"), expectedKeyAuthorization) @@ -371,9 +378,9 @@ func TestTLSALPN01SuccessDNS(t *testing.T) { hs.Close() } -func TestTLSALPN01SuccessIP(t *testing.T) { +func TestTLSALPN01SuccessIPv4(t *testing.T) { cert := testTLSCert(nil, []net.IP{net.ParseIP("127.0.0.1")}, []pkix.Extension{testACMEExt}) - hs := tlsalpn01SrvWithCert(t, cert, 0) + hs := tlsalpn01SrvWithCert(t, cert, 0, false) va, _ := setup(hs, "", nil, nil) @@ -387,6 +394,22 @@ func TestTLSALPN01SuccessIP(t *testing.T) { hs.Close() } +func TestTLSALPN01SuccessIPv6(t *testing.T) { + cert := testTLSCert(nil, []net.IP{net.ParseIP("::1")}, []pkix.Extension{testACMEExt}) + hs := tlsalpn01SrvWithCert(t, cert, 0, true) + + va, _ := setup(hs, "", nil, nil) + + _, err := va.validateTLSALPN01(ctx, identifier.NewIP(netip.MustParseAddr("::1")), expectedKeyAuthorization) + if err != nil { + t.Errorf("Validation failed: %v", err) + } + test.AssertMetricWithLabelsEquals( + t, va.metrics.tlsALPNOIDCounter, prometheus.Labels{"oid": IdPeAcmeIdentifier.String()}, 1) + + hs.Close() +} + func TestTLSALPN01ObsoleteFailure(t *testing.T) { // NOTE: unfortunately another document claimed the OID we were using in // draft-ietf-acme-tls-alpn-01 for their own extension and IANA chose to @@ -398,7 +421,7 @@ func TestTLSALPN01ObsoleteFailure(t *testing.T) { IdPeAcmeIdentifierV1Obsolete := asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 1, 30, 1} cert := testTLSCert([]string{"expected"}, nil, []pkix.Extension{acmeExtension(IdPeAcmeIdentifierV1Obsolete, expectedKeyAuthorization)}) - hs := tlsalpn01SrvWithCert(t, cert, 0) + hs := tlsalpn01SrvWithCert(t, cert, 0, false) va, _ := setup(hs, "", nil, nil) @@ -411,7 +434,7 @@ func TestValidateTLSALPN01BadChallenge(t *testing.T) { badKeyAuthorization := ka("bad token") cert := testTLSCert([]string{"expected"}, nil, []pkix.Extension{acmeExtension(IdPeAcmeIdentifier, badKeyAuthorization)}) - hs := tlsalpn01SrvWithCert(t, cert, 0) + hs := tlsalpn01SrvWithCert(t, cert, 0, false) va, _ := setup(hs, "", nil, nil) @@ -489,7 +512,7 @@ func TestValidateTLSALPN01MalformedExtnValue(t *testing.T) { for _, badExt := range badExtensions { acmeCert := testTLSCert([]string{"expected"}, nil, []pkix.Extension{badExt}) - hs := tlsalpn01SrvWithCert(t, acmeCert, 0) + hs := tlsalpn01SrvWithCert(t, acmeCert, 0, false) va, _ := setup(hs, "", nil, nil) _, err := va.validateTLSALPN01(ctx, dnsi("expected"), expectedKeyAuthorization) @@ -528,7 +551,7 @@ func TestTLSALPN01TLSVersion(t *testing.T) { }, } { // Create a server that only negotiates the given TLS version - hs := tlsalpn01SrvWithCert(t, cert, tc.version) + hs := tlsalpn01SrvWithCert(t, cert, tc.version, false) va, _ := setup(hs, "", nil, nil) @@ -553,7 +576,7 @@ func TestTLSALPN01TLSVersion(t *testing.T) { func TestTLSALPN01WrongName(t *testing.T) { // Create a cert with a different name from what we're validating - hs := tlsalpn01SrvWithCert(t, testACMECert([]string{"incorrect"}), 0) + hs := tlsalpn01SrvWithCert(t, testACMECert([]string{"incorrect"}), 0, false) va, _ := setup(hs, "", nil, nil) @@ -562,10 +585,10 @@ func TestTLSALPN01WrongName(t *testing.T) { test.AssertContains(t, err.Error(), "identifier does not match expected identifier") } -func TestTLSALPN01WrongIP(t *testing.T) { +func TestTLSALPN01WrongIPv4(t *testing.T) { // Create a cert with a different IP address from what we're validating cert := testTLSCert(nil, []net.IP{net.ParseIP("10.10.10.10")}, []pkix.Extension{testACMEExt}) - hs := tlsalpn01SrvWithCert(t, cert, 0) + hs := tlsalpn01SrvWithCert(t, cert, 0, false) va, _ := setup(hs, "", nil, nil) @@ -574,9 +597,21 @@ func TestTLSALPN01WrongIP(t *testing.T) { test.AssertContains(t, err.Error(), "identifier does not match expected identifier") } +func TestTLSALPN01WrongIPv6(t *testing.T) { + // Create a cert with a different IP address from what we're validating + cert := testTLSCert(nil, []net.IP{net.ParseIP("::2")}, []pkix.Extension{testACMEExt}) + hs := tlsalpn01SrvWithCert(t, cert, 0, true) + + va, _ := setup(hs, "", nil, nil) + + _, err := va.validateTLSALPN01(ctx, identifier.NewIP(netip.MustParseAddr("::1")), expectedKeyAuthorization) + test.AssertError(t, err, "validation should have failed") + test.AssertContains(t, err.Error(), "identifier does not match expected identifier") +} + func TestTLSALPN01ExtraNames(t *testing.T) { // Create a cert with two names when we only want to validate one. - hs := tlsalpn01SrvWithCert(t, testACMECert([]string{"expected", "extra"}), 0) + hs := tlsalpn01SrvWithCert(t, testACMECert([]string{"expected", "extra"}), 0, false) va, _ := setup(hs, "", nil, nil) @@ -630,7 +665,7 @@ func TestTLSALPN01NotSelfSigned(t *testing.T) { PrivateKey: eeKey, } - hs := tlsalpn01SrvWithCert(t, acmeCert, 0) + hs := tlsalpn01SrvWithCert(t, acmeCert, 0, false) va, _ := setup(hs, "", nil, nil) @@ -648,7 +683,7 @@ func TestTLSALPN01NotSelfSigned(t *testing.T) { PrivateKey: eeKey, } - hs = tlsalpn01SrvWithCert(t, acmeCert, 0) + hs = tlsalpn01SrvWithCert(t, acmeCert, 0, false) va, _ = setup(hs, "", nil, nil) @@ -687,7 +722,7 @@ func TestTLSALPN01ExtraIdentifiers(t *testing.T) { PrivateKey: key, } - hs := tlsalpn01SrvWithCert(t, acmeCert, tls.VersionTLS12) + hs := tlsalpn01SrvWithCert(t, acmeCert, tls.VersionTLS12, false) va, _ := setup(hs, "", nil, nil) @@ -710,7 +745,7 @@ func TestTLSALPN01ExtraSANs(t *testing.T) { } extensions := []pkix.Extension{testACMEExt, subjectAltName, subjectAltName} - hs := tlsalpn01SrvWithCert(t, testTLSCert([]string{"expected"}, nil, extensions), 0) + hs := tlsalpn01SrvWithCert(t, testTLSCert([]string{"expected"}, nil, extensions), 0, false) va, _ := setup(hs, "", nil, nil) @@ -725,7 +760,7 @@ func TestTLSALPN01ExtraSANs(t *testing.T) { func TestTLSALPN01ExtraAcmeExtensions(t *testing.T) { // Create a cert with multiple SAN extensions extensions := []pkix.Extension{testACMEExt, testACMEExt} - hs := tlsalpn01SrvWithCert(t, testTLSCert([]string{"expected"}, nil, extensions), 0) + hs := tlsalpn01SrvWithCert(t, testTLSCert([]string{"expected"}, nil, extensions), 0, false) va, _ := setup(hs, "", nil, nil) @@ -786,3 +821,15 @@ func TestAcceptableExtensions(t *testing.T) { err = checkAcceptableExtensions(okayWithUnexpectedExt, requireAcmeAndSAN) test.AssertNotError(t, err, "Correct type and number of extensions") } + +func TestTLSALPN01BadIdentifier(t *testing.T) { + hs := httpSrv(t, expectedToken, false) + defer hs.Close() + + va, _ := setup(hs, "", nil, nil) + + _, err := va.validateTLSALPN01(ctx, identifier.ACMEIdentifier{Type: "smime", Value: "dobber@bad.horse"}, expectedKeyAuthorization) + test.AssertError(t, err, "Server accepted a hypothetical S/MIME identifier") + prob := detailedError(err) + test.AssertContains(t, prob.Error(), "Identifier type for TLS-ALPN-01 challenge was not DNS or IP") +} From e0692b81da2746dc6e3c82b9d1ed8df87ba37f8a Mon Sep 17 00:00:00 2001 From: James Renken Date: Sat, 22 Feb 2025 21:56:19 -0800 Subject: [PATCH 16/27] Add backstop test for RFC 8738, Section 6 --- va/tlsalpn.go | 2 -- va/tlsalpn_test.go | 6 ++++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/va/tlsalpn.go b/va/tlsalpn.go index 8352e47c0b7..f41b5a05c33 100644 --- a/va/tlsalpn.go +++ b/va/tlsalpn.go @@ -144,8 +144,6 @@ func (va *ValidationAuthorityImpl) getChallengeCert( case identifier.TypeDNS: tlsConfig.ServerName = ident.Value case identifier.TypeIP: - // TODO(#8020): Test this against RFC 8738 Sec. 6. - // https://datatracker.ietf.org/doc/html/rfc8738#section-6 reverseIP, err := dns.ReverseAddr(ident.Value) if err != nil { va.log.Infof("%s Failed to parse IP address %s.", core.ChallengeTypeTLSALPN01, ident.Value) diff --git a/va/tlsalpn_test.go b/va/tlsalpn_test.go index f099eb0ba6e..7f6cc8c88c7 100644 --- a/va/tlsalpn_test.go +++ b/va/tlsalpn_test.go @@ -11,6 +11,7 @@ import ( "crypto/x509/pkix" "encoding/asn1" "encoding/hex" + "errors" "fmt" "math/big" "net" @@ -91,6 +92,11 @@ func tlsalpn01SrvWithCert(t *testing.T, acmeCert *tls.Certificate, tlsVersion ui Certificates: []tls.Certificate{}, ClientAuth: tls.NoClientCert, GetCertificate: func(clientHello *tls.ClientHelloInfo) (*tls.Certificate, error) { + // This is a backstop test for RFC 8738, Section 6. Go's + // tls.hostnameInSNI already does the right thing. + if net.ParseIP(clientHello.ServerName) != nil { + return nil, errors.New("TLS client used a bare IP address for SNI") + } return acmeCert, nil }, NextProtos: []string{"http/1.1", ACMETLS1Protocol}, From 01979206fa3c1c87fa93d6d15799d1c042b00544 Mon Sep 17 00:00:00 2001 From: James Renken Date: Sat, 22 Feb 2025 22:09:35 -0800 Subject: [PATCH 17/27] Future-proof for IPv7 --- va/http.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/va/http.go b/va/http.go index 1238d073cbc..0156a221eb0 100644 --- a/va/http.go +++ b/va/http.go @@ -443,7 +443,7 @@ func (va *ValidationAuthorityImpl) processHTTPValidation( if err != nil { return nil, nil, fmt.Errorf("couldn't parse IP address from identifier") } - if netipHost.Is6() { + if !netipHost.Is4() { host = "[" + host + "]" } } From b26365f3d8f67e94529afa86060b8fe6d92ea1ba Mon Sep 17 00:00:00 2001 From: James Renken Date: Sat, 22 Feb 2025 22:44:28 -0800 Subject: [PATCH 18/27] Add test cases; remove dnsi test function --- va/caa_test.go | 4 +-- va/dns_test.go | 16 +++++------ va/tlsalpn.go | 1 - va/tlsalpn_test.go | 66 ++++++++++++++++++++++++++++++++-------------- va/va_test.go | 5 ---- 5 files changed, 56 insertions(+), 36 deletions(-) diff --git a/va/caa_test.go b/va/caa_test.go index 246df38d2e5..667c71a0147 100644 --- a/va/caa_test.go +++ b/va/caa_test.go @@ -1231,13 +1231,13 @@ func TestCAAFailure(t *testing.T) { va, _ := setup(hs, "", nil, caaMockDNS{}) - err := va.checkCAA(ctx, dnsi("reserved.com"), &caaParams{1, core.ChallengeTypeHTTP01}) + err := va.checkCAA(ctx, identifier.NewDNS("reserved.com"), &caaParams{1, core.ChallengeTypeHTTP01}) if err == nil { t.Fatalf("Expected CAA rejection for reserved.com, got success") } test.AssertErrorIs(t, err, berrors.CAA) - err = va.checkCAA(ctx, dnsi("example.gonetld"), &caaParams{1, core.ChallengeTypeHTTP01}) + err = va.checkCAA(ctx, identifier.NewDNS("example.gonetld"), &caaParams{1, core.ChallengeTypeHTTP01}) if err == nil { t.Fatalf("Expected CAA rejection for gonetld, got success") } diff --git a/va/dns_test.go b/va/dns_test.go index f64e2cc997b..095ce0ed85f 100644 --- a/va/dns_test.go +++ b/va/dns_test.go @@ -40,7 +40,7 @@ func TestDNSValidationEmpty(t *testing.T) { func TestDNSValidationWrong(t *testing.T) { va, _ := setup(nil, "", nil, nil) - _, err := va.validateDNS01(context.Background(), dnsi("wrong-dns01.com"), expectedKeyAuthorization) + _, err := va.validateDNS01(context.Background(), identifier.NewDNS("wrong-dns01.com"), expectedKeyAuthorization) if err == nil { t.Fatalf("Successful DNS validation with wrong TXT record") } @@ -51,7 +51,7 @@ func TestDNSValidationWrong(t *testing.T) { func TestDNSValidationWrongMany(t *testing.T) { va, _ := setup(nil, "", nil, nil) - _, err := va.validateDNS01(context.Background(), dnsi("wrong-many-dns01.com"), expectedKeyAuthorization) + _, err := va.validateDNS01(context.Background(), identifier.NewDNS("wrong-many-dns01.com"), expectedKeyAuthorization) if err == nil { t.Fatalf("Successful DNS validation with wrong TXT record") } @@ -62,7 +62,7 @@ func TestDNSValidationWrongMany(t *testing.T) { func TestDNSValidationWrongLong(t *testing.T) { va, _ := setup(nil, "", nil, nil) - _, err := va.validateDNS01(context.Background(), dnsi("long-dns01.com"), expectedKeyAuthorization) + _, err := va.validateDNS01(context.Background(), identifier.NewDNS("long-dns01.com"), expectedKeyAuthorization) if err == nil { t.Fatalf("Successful DNS validation with wrong TXT record") } @@ -73,7 +73,7 @@ func TestDNSValidationWrongLong(t *testing.T) { func TestDNSValidationFailure(t *testing.T) { va, _ := setup(nil, "", nil, nil) - _, err := va.validateDNS01(ctx, dnsi("localhost"), expectedKeyAuthorization) + _, err := va.validateDNS01(ctx, identifier.NewDNS("localhost"), expectedKeyAuthorization) prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.UnauthorizedProblem) @@ -105,7 +105,7 @@ func TestDNSValidationInvalid(t *testing.T) { func TestDNSValidationServFail(t *testing.T) { va, _ := setup(nil, "", nil, nil) - _, err := va.validateDNS01(ctx, dnsi("servfail.com"), expectedKeyAuthorization) + _, err := va.validateDNS01(ctx, identifier.NewDNS("servfail.com"), expectedKeyAuthorization) prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.DNSProblem) @@ -125,7 +125,7 @@ func TestDNSValidationNoServer(t *testing.T) { log, nil) - _, err = va.validateDNS01(ctx, dnsi("localhost"), expectedKeyAuthorization) + _, err = va.validateDNS01(ctx, identifier.NewDNS("localhost"), expectedKeyAuthorization) prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.DNSProblem) } @@ -133,7 +133,7 @@ func TestDNSValidationNoServer(t *testing.T) { func TestDNSValidationOK(t *testing.T) { va, _ := setup(nil, "", nil, nil) - _, prob := va.validateDNS01(ctx, dnsi("good-dns01.com"), expectedKeyAuthorization) + _, prob := va.validateDNS01(ctx, identifier.NewDNS("good-dns01.com"), expectedKeyAuthorization) test.Assert(t, prob == nil, "Should be valid.") } @@ -141,7 +141,7 @@ func TestDNSValidationOK(t *testing.T) { func TestDNSValidationNoAuthorityOK(t *testing.T) { va, _ := setup(nil, "", nil, nil) - _, prob := va.validateDNS01(ctx, dnsi("no-authority-dns01.com"), expectedKeyAuthorization) + _, prob := va.validateDNS01(ctx, identifier.NewDNS("no-authority-dns01.com"), expectedKeyAuthorization) test.Assert(t, prob == nil, "Should be valid.") } diff --git a/va/tlsalpn.go b/va/tlsalpn.go index f41b5a05c33..802aaa666a3 100644 --- a/va/tlsalpn.go +++ b/va/tlsalpn.go @@ -194,7 +194,6 @@ func (va *ValidationAuthorityImpl) getChallengeCert( return certs[0], &cs, nil } -// TODO(#8020): Identifiers need testing here. func checkExpectedSAN(cert *x509.Certificate, ident identifier.ACMEIdentifier) error { var asn1Tag int var certSAN []byte diff --git a/va/tlsalpn_test.go b/va/tlsalpn_test.go index 7f6cc8c88c7..5ed5d89274f 100644 --- a/va/tlsalpn_test.go +++ b/va/tlsalpn_test.go @@ -152,7 +152,7 @@ func TestTLSALPNTimeoutAfterConnect(t *testing.T) { defer cancel() started := time.Now() - _, err := va.validateTLSALPN01(ctx, dnsi("slow.server"), expectedKeyAuthorization) + _, err := va.validateTLSALPN01(ctx, identifier.NewDNS("slow.server"), expectedKeyAuthorization) if err == nil { t.Fatalf("Validation should've failed") } @@ -195,7 +195,7 @@ func TestTLSALPN01DialTimeout(t *testing.T) { // that, just retry until we get something other than "Network unreachable". var err error for range 20 { - _, err = va.validateTLSALPN01(ctx, dnsi("unroutable.invalid"), expectedKeyAuthorization) + _, err = va.validateTLSALPN01(ctx, identifier.NewDNS("unroutable.invalid"), expectedKeyAuthorization) if err != nil && strings.Contains(err.Error(), "Network unreachable") { continue } else { @@ -235,7 +235,7 @@ func TestTLSALPN01Refused(t *testing.T) { // Take down validation server and check that validation fails. hs.Close() - _, err := va.validateTLSALPN01(ctx, dnsi("expected"), expectedKeyAuthorization) + _, err := va.validateTLSALPN01(ctx, identifier.NewDNS("expected"), expectedKeyAuthorization) if err == nil { t.Fatalf("Server's down; expected refusal. Where did we connect?") } @@ -256,7 +256,7 @@ func TestTLSALPN01TalkingToHTTP(t *testing.T) { httpOnly := httpSrv(t, "", false) va.tlsPort = getPort(httpOnly) - _, err := va.validateTLSALPN01(ctx, dnsi("expected"), expectedKeyAuthorization) + _, err := va.validateTLSALPN01(ctx, identifier.NewDNS("expected"), expectedKeyAuthorization) test.AssertError(t, err, "TLS-SNI-01 validation passed when talking to a HTTP-only server") prob := detailedError(err) expected := "Server only speaks HTTP, not TLS" @@ -281,7 +281,7 @@ func TestTLSError(t *testing.T) { va, _ := setup(hs, "", nil, nil) - _, err := va.validateTLSALPN01(ctx, dnsi("expected"), expectedKeyAuthorization) + _, err := va.validateTLSALPN01(ctx, identifier.NewDNS("expected"), expectedKeyAuthorization) if err == nil { t.Fatalf("TLS validation should have failed: What cert was used?") } @@ -297,7 +297,7 @@ func TestDNSError(t *testing.T) { va, _ := setup(hs, "", nil, nil) - _, err := va.validateTLSALPN01(ctx, dnsi("always.invalid"), expectedKeyAuthorization) + _, err := va.validateTLSALPN01(ctx, identifier.NewDNS("always.invalid"), expectedKeyAuthorization) if err == nil { t.Fatalf("TLS validation should have failed: what IP was used?") } @@ -374,7 +374,7 @@ func TestTLSALPN01SuccessDNS(t *testing.T) { va, _ := setup(hs, "", nil, nil) - _, err := va.validateTLSALPN01(ctx, dnsi("expected"), expectedKeyAuthorization) + _, err := va.validateTLSALPN01(ctx, identifier.NewDNS("expected"), expectedKeyAuthorization) if err != nil { t.Errorf("Validation failed: %v", err) } @@ -431,7 +431,7 @@ func TestTLSALPN01ObsoleteFailure(t *testing.T) { va, _ := setup(hs, "", nil, nil) - _, err := va.validateTLSALPN01(ctx, dnsi("expected"), expectedKeyAuthorization) + _, err := va.validateTLSALPN01(ctx, identifier.NewDNS("expected"), expectedKeyAuthorization) test.AssertNotNil(t, err, "expected validation to fail") test.AssertContains(t, err.Error(), "Required extension OID 1.3.6.1.5.5.7.1.31 is not present") } @@ -444,7 +444,7 @@ func TestValidateTLSALPN01BadChallenge(t *testing.T) { va, _ := setup(hs, "", nil, nil) - _, err := va.validateTLSALPN01(ctx, dnsi("expected"), expectedKeyAuthorization) + _, err := va.validateTLSALPN01(ctx, identifier.NewDNS("expected"), expectedKeyAuthorization) if err == nil { t.Fatalf("TLS ALPN validation should have failed.") } @@ -465,7 +465,7 @@ func TestValidateTLSALPN01BrokenSrv(t *testing.T) { va, _ := setup(hs, "", nil, nil) - _, err := va.validateTLSALPN01(ctx, dnsi("expected"), expectedKeyAuthorization) + _, err := va.validateTLSALPN01(ctx, identifier.NewDNS("expected"), expectedKeyAuthorization) if err == nil { t.Fatalf("TLS ALPN validation should have failed.") } @@ -488,7 +488,7 @@ func TestValidateTLSALPN01UnawareSrv(t *testing.T) { va, _ := setup(hs, "", nil, nil) - _, err := va.validateTLSALPN01(ctx, dnsi("expected"), expectedKeyAuthorization) + _, err := va.validateTLSALPN01(ctx, identifier.NewDNS("expected"), expectedKeyAuthorization) if err == nil { t.Fatalf("TLS ALPN validation should have failed.") } @@ -521,7 +521,7 @@ func TestValidateTLSALPN01MalformedExtnValue(t *testing.T) { hs := tlsalpn01SrvWithCert(t, acmeCert, 0, false) va, _ := setup(hs, "", nil, nil) - _, err := va.validateTLSALPN01(ctx, dnsi("expected"), expectedKeyAuthorization) + _, err := va.validateTLSALPN01(ctx, identifier.NewDNS("expected"), expectedKeyAuthorization) hs.Close() if err == nil { @@ -561,7 +561,7 @@ func TestTLSALPN01TLSVersion(t *testing.T) { va, _ := setup(hs, "", nil, nil) - _, err := va.validateTLSALPN01(ctx, dnsi("expected"), expectedKeyAuthorization) + _, err := va.validateTLSALPN01(ctx, identifier.NewDNS("expected"), expectedKeyAuthorization) if !tc.expectError { if err != nil { t.Errorf("expected success, got: %v", err) @@ -586,7 +586,7 @@ func TestTLSALPN01WrongName(t *testing.T) { va, _ := setup(hs, "", nil, nil) - _, err := va.validateTLSALPN01(ctx, dnsi("expected"), expectedKeyAuthorization) + _, err := va.validateTLSALPN01(ctx, identifier.NewDNS("expected"), expectedKeyAuthorization) test.AssertError(t, err, "validation should have failed") test.AssertContains(t, err.Error(), "identifier does not match expected identifier") } @@ -621,7 +621,33 @@ func TestTLSALPN01ExtraNames(t *testing.T) { va, _ := setup(hs, "", nil, nil) - _, err := va.validateTLSALPN01(ctx, dnsi("expected"), expectedKeyAuthorization) + _, err := va.validateTLSALPN01(ctx, identifier.NewDNS("expected"), expectedKeyAuthorization) + test.AssertError(t, err, "validation should have failed") + test.AssertContains(t, err.Error(), "wrong number of identifiers") +} + +func TestTLSALPN01WrongIdentType(t *testing.T) { + // Create a cert with an IP address encoded as a name. + hs := tlsalpn01SrvWithCert(t, testACMECert([]string{"127.0.0.1"}), 0, false) + + va, _ := setup(hs, "", nil, nil) + + _, err := va.validateTLSALPN01(ctx, identifier.NewIP(netip.MustParseAddr("127.0.0.1")), expectedKeyAuthorization) + test.AssertError(t, err, "validation should have failed") + test.AssertContains(t, err.Error(), "wrong number of identifiers") +} + +func TestTLSALPN01TooManyIdentTypes(t *testing.T) { + // Create a cert with both a name and an IP address when we only want to validate one. + hs := tlsalpn01SrvWithCert(t, testTLSCert([]string{"expected"}, []net.IP{net.ParseIP("127.0.0.1")}, []pkix.Extension{testACMEExt}), 0, false) + + va, _ := setup(hs, "", nil, nil) + + _, err := va.validateTLSALPN01(ctx, identifier.NewDNS("expected"), expectedKeyAuthorization) + test.AssertError(t, err, "validation should have failed") + test.AssertContains(t, err.Error(), "wrong number of identifiers") + + _, err = va.validateTLSALPN01(ctx, identifier.NewIP(netip.MustParseAddr("127.0.0.1")), expectedKeyAuthorization) test.AssertError(t, err, "validation should have failed") test.AssertContains(t, err.Error(), "wrong number of identifiers") } @@ -675,7 +701,7 @@ func TestTLSALPN01NotSelfSigned(t *testing.T) { va, _ := setup(hs, "", nil, nil) - _, err = va.validateTLSALPN01(ctx, dnsi("expected"), expectedKeyAuthorization) + _, err = va.validateTLSALPN01(ctx, identifier.NewDNS("expected"), expectedKeyAuthorization) test.AssertError(t, err, "validation should have failed") test.AssertContains(t, err.Error(), "not self-signed") @@ -693,7 +719,7 @@ func TestTLSALPN01NotSelfSigned(t *testing.T) { va, _ = setup(hs, "", nil, nil) - _, err = va.validateTLSALPN01(ctx, dnsi("expected"), expectedKeyAuthorization) + _, err = va.validateTLSALPN01(ctx, identifier.NewDNS("expected"), expectedKeyAuthorization) test.AssertError(t, err, "validation should have failed") test.AssertContains(t, err.Error(), "not self-signed") } @@ -732,7 +758,7 @@ func TestTLSALPN01ExtraIdentifiers(t *testing.T) { va, _ := setup(hs, "", nil, nil) - _, err = va.validateTLSALPN01(ctx, dnsi("expected"), expectedKeyAuthorization) + _, err = va.validateTLSALPN01(ctx, identifier.NewDNS("expected"), expectedKeyAuthorization) test.AssertError(t, err, "validation should have failed") test.AssertContains(t, err.Error(), "Received certificate with unexpected identifiers") } @@ -755,7 +781,7 @@ func TestTLSALPN01ExtraSANs(t *testing.T) { va, _ := setup(hs, "", nil, nil) - _, err = va.validateTLSALPN01(ctx, dnsi("expected"), expectedKeyAuthorization) + _, err = va.validateTLSALPN01(ctx, identifier.NewDNS("expected"), expectedKeyAuthorization) test.AssertError(t, err, "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. @@ -770,7 +796,7 @@ func TestTLSALPN01ExtraAcmeExtensions(t *testing.T) { va, _ := setup(hs, "", nil, nil) - _, err := va.validateTLSALPN01(ctx, dnsi("expected"), expectedKeyAuthorization) + _, err := va.validateTLSALPN01(ctx, identifier.NewDNS("expected"), expectedKeyAuthorization) test.AssertError(t, err, "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_test.go b/va/va_test.go index a9362275264..98623bd3d81 100644 --- a/va/va_test.go +++ b/va/va_test.go @@ -70,11 +70,6 @@ var expectedToken = "LoqXcYV8q5ONbJQxbmR7SCTNo3tiAXDfowyjxAjEuX0" var expectedThumbprint = "9jg46WB3rR_AHD-EBXdN7cBkH1WOu0tA3M9fm21mqTI" var expectedKeyAuthorization = ka(expectedToken) -// Return an ACME DNS identifier for the given hostname -func dnsi(hostname string) identifier.ACMEIdentifier { - return identifier.NewDNS(hostname) -} - var ctx context.Context func TestMain(m *testing.M) { From 1131332e74809e89a1e46cc46a0426ee170ff6cb Mon Sep 17 00:00:00 2001 From: James Renken Date: Sun, 23 Feb 2025 21:47:51 -0800 Subject: [PATCH 19/27] Introduce identifier.FromProtoWithDefault --- identifier/identifier.go | 9 +++++++++ va/va.go | 9 +++------ va/vampic.go | 9 +++------ 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/identifier/identifier.go b/identifier/identifier.go index 3754657bf64..03edf263fca 100644 --- a/identifier/identifier.go +++ b/identifier/identifier.go @@ -46,6 +46,15 @@ func FromProto(ident *corepb.Identifier) ACMEIdentifier { } } +// FromProtoWithDefault can be removed after DnsNames are no longer used in +// RPCs. TODO(#7311) +func FromProtoWithDefault(ident *corepb.Identifier, name string) ACMEIdentifier { + if ident == nil { + return NewDNS(name) + } + return FromProto(ident) +} + // NewDNS is a convenience function for creating an ACMEIdentifier with Type // "dns" for a given domain name. func NewDNS(domain string) ACMEIdentifier { diff --git a/va/va.go b/va/va.go index 1bcbb0b1529..e52d424c0c3 100644 --- a/va/va.go +++ b/va/va.go @@ -678,10 +678,7 @@ func (va *ValidationAuthorityImpl) performLocalValidation( // also contains Problems. This method does NOT implement Multi-Perspective // Issuance Corroboration as defined in BRs Sections 3.2.2.9 and 5.4.1. func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *vapb.PerformValidationRequest) (*vapb.ValidationResult, error) { - ident := req.Identifier - if ident == nil { - ident = identifier.NewDNS(req.DnsName).AsProto() - } + ident := identifier.FromProtoWithDefault(req.Identifier, req.DnsName) if core.IsAnyNilOrZero(req, ident, req.Challenge, req.Authz, req.ExpectedKeyAuthorization) { return nil, berrors.InternalServerError("Incomplete validation request") @@ -706,7 +703,7 @@ func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *v logEvent := verificationRequestEvent{ AuthzID: req.Authz.Id, Requester: req.Authz.RegID, - Identifier: identifier.FromProto(ident), + Identifier: ident, Challenge: chall, } defer func() { @@ -739,7 +736,7 @@ func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *v // was successful or not, and cannot themselves fail. records, err := va.performLocalValidation( ctx, - identifier.FromProto(ident), + ident, req.Authz.RegID, chall.Type, chall.Token, diff --git a/va/vampic.go b/va/vampic.go index 79c80607850..27a9628bb9d 100644 --- a/va/vampic.go +++ b/va/vampic.go @@ -219,10 +219,7 @@ type validationLogEvent struct { // implements the DCV portion of Multi-Perspective Issuance Corroboration as // defined in BRs Sections 3.2.2.9 and 5.4.1. func (va *ValidationAuthorityImpl) DoDCV(ctx context.Context, req *vapb.PerformValidationRequest) (*vapb.ValidationResult, error) { - ident := req.Identifier - if ident == nil { - ident = identifier.NewDNS(req.DnsName).AsProto() - } + ident := identifier.FromProtoWithDefault(req.Identifier, req.DnsName) if core.IsAnyNilOrZero(req, ident, req.Challenge, req.Authz, req.ExpectedKeyAuthorization) { return nil, berrors.InternalServerError("Incomplete validation request") @@ -248,7 +245,7 @@ func (va *ValidationAuthorityImpl) DoDCV(ctx context.Context, req *vapb.PerformV logEvent := validationLogEvent{ AuthzID: req.Authz.Id, Requester: req.Authz.RegID, - Identifier: identifier.FromProto(ident), + Identifier: ident, Challenge: chall, } defer func() { @@ -282,7 +279,7 @@ func (va *ValidationAuthorityImpl) DoDCV(ctx context.Context, req *vapb.PerformV // was successful or not, and cannot themselves fail. records, err := va.validateChallenge( ctx, - identifier.FromProto(ident), + ident, chall.Type, chall.Token, req.ExpectedKeyAuthorization, From 0d43377d3dc1182a71993508e596c3e5ee8df39b Mon Sep 17 00:00:00 2001 From: James Renken Date: Sun, 23 Feb 2025 21:51:27 -0800 Subject: [PATCH 20/27] Combine struct composition --- va/tlsalpn.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/va/tlsalpn.go b/va/tlsalpn.go index 802aaa666a3..be3b956925c 100644 --- a/va/tlsalpn.go +++ b/va/tlsalpn.go @@ -139,6 +139,8 @@ func (va *ValidationAuthorityImpl) getChallengeCert( tlsConfig := &tls.Config{ MinVersion: tls.VersionTLS12, NextProtos: []string{ACMETLS1Protocol}, + // We expect a self-signed challenge certificate, do not verify it here. + InsecureSkipVerify: true, } switch ident.Type { case identifier.TypeDNS: @@ -159,9 +161,6 @@ func (va *ValidationAuthorityImpl) getChallengeCert( va.log.Info(fmt.Sprintf("%s [%s] Attempting to validate for %s %s", core.ChallengeTypeTLSALPN01, ident, hostPort, tlsConfig.ServerName)) - // We expect a self-signed challenge certificate, do not verify it here. - tlsConfig.InsecureSkipVerify = true - dialCtx, cancel := context.WithTimeout(ctx, va.singleDialTimeout) defer cancel() From 4d19c25b40ea42e2fb76d0348b109f3744a275f3 Mon Sep 17 00:00:00 2001 From: James Renken Date: Mon, 24 Feb 2025 18:21:32 -0800 Subject: [PATCH 21/27] Address feedback --- bdns/mocks.go | 6 ------ identifier/identifier.go | 14 +++++++----- va/http.go | 11 ++-------- va/http_test.go | 44 ++++++++++++++++++++++++++++++++------ va/proto/va.pb.go | 6 +++--- va/proto/va.proto | 6 +++--- va/tlsalpn.go | 44 ++++++++++++++++++++------------------ va/va.go | 5 ++++- va/va_test.go | 46 +++++++++++++++------------------------- va/vampic.go | 5 ++++- 10 files changed, 103 insertions(+), 84 deletions(-) diff --git a/bdns/mocks.go b/bdns/mocks.go index 2bcd3f44965..36bf2e88d29 100644 --- a/bdns/mocks.go +++ b/bdns/mocks.go @@ -28,12 +28,6 @@ func (mock *MockClient) LookupTXT(_ context.Context, hostname string) ([]string, // expected token + test account jwk thumbprint return []string{"LPsIwTo7o8BoG0-vjCyGQGBWSVIPxI-i_X336eUOQZo"}, ResolverAddrs{"MockClient"}, nil } - if hostname == "_acme-challenge.good-dns02.com" { - // base64(sha256("LoqXcYV8q5ONbJQxbmR7SCTNo3tiAXDfowyjxAjEuX0" - // + "." + "9jg46WB3rR_AHD-EBXdN7cBkH1WOu0tA3M9fm21mqTI")) - // expected token + test account jwk thumbprint - return []string{"LPsIwTo7o8BoG0-vjCyGQGBWSVIPxI-i_X336eUOQZo"}, ResolverAddrs{"MockClient"}, nil - } if hostname == "_acme-challenge.wrong-dns01.com" { return []string{"a"}, ResolverAddrs{"MockClient"}, nil } diff --git a/identifier/identifier.go b/identifier/identifier.go index 03edf263fca..82f88b9176b 100644 --- a/identifier/identifier.go +++ b/identifier/identifier.go @@ -4,6 +4,7 @@ package identifier import ( + "errors" "net/netip" corepb "github.com/letsencrypt/boulder/core/proto" @@ -46,13 +47,16 @@ func FromProto(ident *corepb.Identifier) ACMEIdentifier { } } -// FromProtoWithDefault can be removed after DnsNames are no longer used in -// RPCs. TODO(#7311) -func FromProtoWithDefault(ident *corepb.Identifier, name string) ACMEIdentifier { +// FromProtoOrName can be removed after DnsNames are no longer used in RPCs. +// TODO(#8023) +func FromProtoOrName(ident *corepb.Identifier, name string) (ACMEIdentifier, error) { if ident == nil { - return NewDNS(name) + return NewDNS(name), nil } - return FromProto(ident) + if name == "" { + return FromProto(ident), nil + } + return ACMEIdentifier{}, errors.New("can't use both Identifier and DNSName") } // NewDNS is a convenience function for creating an ACMEIdentifier with Type diff --git a/va/http.go b/va/http.go index 0156a221eb0..1a4d399b499 100644 --- a/va/http.go +++ b/va/http.go @@ -221,7 +221,7 @@ func (va *ValidationAuthorityImpl) newHTTPValidationTarget( case identifier.TypeIP: addrs = []net.IP{net.ParseIP(ident.Value)} default: - return nil, fmt.Errorf("Unknown identifier type: %s", ident.Type) + return nil, fmt.Errorf("unknown identifier type: %s", ident.Type) } target := &httpValidationTarget{ @@ -338,14 +338,7 @@ func (va *ValidationAuthorityImpl) extractRequestTarget(req *http.Request) (iden ) } - // We use net.ParseIP to check whether the host is an IP address; otherwise, - // netip.ParseAddr would happily ingest a hostname, returning a garbage IP - // and no error. - if net.ParseIP(reqHost) != nil { - reqIP, err := netip.ParseAddr(reqHost) - if err != nil { - return identifier.ACMEIdentifier{}, 0, berrors.ConnectionFailureError("Invalid IP address in redirect target %q", reqHost) - } + if reqIP, err := netip.ParseAddr(reqHost); err == nil { return identifier.NewIP(reqIP), reqPort, nil } diff --git a/va/http_test.go b/va/http_test.go index 388e9c0ed0c..ae1b80bfe3f 100644 --- a/va/http_test.go +++ b/va/http_test.go @@ -296,13 +296,29 @@ func TestExtractRequestTarget(t *testing.T) { ExpectedPort: 80, }, { - Name: "bare IPv4, explicit port", + Name: "bare IPv4, explicit valid port", Req: &http.Request{ URL: mustURL("http://10.10.10.10:80"), }, ExpectedIdent: identifier.NewIP(netip.MustParseAddr("10.10.10.10")), ExpectedPort: 80, }, + { + Name: "bare IPv4, explicit invalid port", + Req: &http.Request{ + URL: mustURL("http://10.10.10.10:9999"), + }, + ExpectedError: fmt.Errorf("Invalid port in redirect target. Only ports 80 " + + "and 443 are supported, not 9999"), + }, + { + Name: "bare IPv4, HTTPS", + Req: &http.Request{ + URL: mustURL("https://10.10.10.10"), + }, + ExpectedIdent: identifier.NewIP(netip.MustParseAddr("10.10.10.10")), + ExpectedPort: 443, + }, { Name: "bare IPv6, implicit port", Req: &http.Request{ @@ -312,13 +328,29 @@ func TestExtractRequestTarget(t *testing.T) { ExpectedPort: 80, }, { - Name: "bare IPv6, explicit port", + Name: "bare IPv6, explicit valid port", Req: &http.Request{ URL: mustURL("http://[::1]:80"), }, ExpectedIdent: identifier.NewIP(netip.MustParseAddr("::1")), ExpectedPort: 80, }, + { + Name: "bare IPv6, explicit invalid port", + Req: &http.Request{ + URL: mustURL("http://[::1]:9999"), + }, + ExpectedError: fmt.Errorf("Invalid port in redirect target. Only ports 80 " + + "and 443 are supported, not 9999"), + }, + { + Name: "bare IPv6, HTTPS", + Req: &http.Request{ + URL: mustURL("https://[::1]"), + }, + ExpectedIdent: identifier.NewIP(netip.MustParseAddr("::1")), + ExpectedPort: 443, + }, { Name: "valid HTTP redirect, explicit port", Req: &http.Request{ @@ -432,10 +464,10 @@ func TestHTTPValidationDNSIdMismatchError(t *testing.T) { func TestSetupHTTPValidation(t *testing.T) { va, _ := setup(nil, "", nil, nil) - mustTarget := func(t *testing.T, host identifier.ACMEIdentifier, port int, path string) *httpValidationTarget { + mustTarget := func(t *testing.T, host string, port int, path string) *httpValidationTarget { target, err := va.newHTTPValidationTarget( context.Background(), - host, + identifier.NewDNS(host), port, path, "") @@ -484,7 +516,7 @@ func TestSetupHTTPValidation(t *testing.T) { }, { Name: "HTTP input req", - InputTarget: mustTarget(t, identifier.NewDNS("ipv4.and.ipv6.localhost"), va.httpPort, "/yellow/brick/road"), + InputTarget: mustTarget(t, "ipv4.and.ipv6.localhost", va.httpPort, "/yellow/brick/road"), InputURL: httpInputURL, ExpectedRecord: core.ValidationRecord{ DnsName: "ipv4.and.ipv6.localhost", @@ -502,7 +534,7 @@ func TestSetupHTTPValidation(t *testing.T) { }, { Name: "HTTPS input req", - InputTarget: mustTarget(t, identifier.NewDNS("ipv4.and.ipv6.localhost"), va.httpsPort, "/yellow/brick/road"), + InputTarget: mustTarget(t, "ipv4.and.ipv6.localhost", va.httpsPort, "/yellow/brick/road"), InputURL: httpsInputURL, ExpectedRecord: core.ValidationRecord{ DnsName: "ipv4.and.ipv6.localhost", diff --git a/va/proto/va.pb.go b/va/proto/va.pb.go index 9b79a58d3db..ba4b7b642fe 100644 --- a/va/proto/va.pb.go +++ b/va/proto/va.pb.go @@ -26,8 +26,8 @@ type IsCAAValidRequest struct { sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields - // TODO(#7311): Accept an identifier instead of a domain (purely for - // consistency, because only DNS identifiers support CAA checks). + // TODO: Accept an identifier instead of a domain (purely for consistency, + // because only DNS identifiers support CAA checks). // // NOTE: Domain may be a name with a wildcard prefix (e.g. `*.example.com`) Domain string `protobuf:"bytes,1,opt,name=domain,proto3" json:"domain,omitempty"` @@ -166,7 +166,7 @@ type PerformValidationRequest struct { unknownFields protoimpl.UnknownFields // Next unused field number: 6 - // TODO(#7311): dnsNames are being deprecated in favour of identifiers. + // TODO(#8023): dnsNames are being deprecated in favour of identifiers. DnsName string `protobuf:"bytes,1,opt,name=dnsName,proto3" json:"dnsName,omitempty"` Identifier *proto.Identifier `protobuf:"bytes,5,opt,name=identifier,proto3" json:"identifier,omitempty"` Challenge *proto.Challenge `protobuf:"bytes,2,opt,name=challenge,proto3" json:"challenge,omitempty"` diff --git a/va/proto/va.proto b/va/proto/va.proto index aa70aaea11b..db3e8661737 100644 --- a/va/proto/va.proto +++ b/va/proto/va.proto @@ -16,8 +16,8 @@ service CAA { } message IsCAAValidRequest { - // TODO(#7311): Accept an identifier instead of a domain (purely for - // consistency, because only DNS identifiers support CAA checks). + // TODO: Accept an identifier instead of a domain (purely for consistency, + // because only DNS identifiers support CAA checks). // // NOTE: Domain may be a name with a wildcard prefix (e.g. `*.example.com`) string domain = 1; @@ -35,7 +35,7 @@ message IsCAAValidResponse { message PerformValidationRequest { // Next unused field number: 6 - // TODO(#7311): dnsNames are being deprecated in favour of identifiers. + // TODO(#8023): dnsNames are being deprecated in favour of identifiers. string dnsName = 1; core.Identifier identifier = 5; core.Challenge challenge = 2; diff --git a/va/tlsalpn.go b/va/tlsalpn.go index be3b956925c..69896dbcf37 100644 --- a/va/tlsalpn.go +++ b/va/tlsalpn.go @@ -18,7 +18,6 @@ import ( "github.com/miekg/dns" - "github.com/letsencrypt/boulder/bdns" "github.com/letsencrypt/boulder/core" berrors "github.com/letsencrypt/boulder/errors" "github.com/letsencrypt/boulder/identifier" @@ -64,30 +63,30 @@ func (va *ValidationAuthorityImpl) tryGetChallengeCert( ident identifier.ACMEIdentifier, ) (*x509.Certificate, *tls.ConnectionState, core.ValidationRecord, error) { validationRecord := core.ValidationRecord{ - DnsName: ident.Value, - AddressesResolved: []net.IP{}, - Port: strconv.Itoa(va.tlsPort), - ResolverAddrs: bdns.ResolverAddrs{}, + DnsName: ident.Value, + Port: strconv.Itoa(va.tlsPort), } + var addrs []net.IP switch ident.Type { case identifier.TypeDNS: // Resolve IP addresses for the identifier - addrs, resolvers, err := va.getAddrs(ctx, ident.Value) + dnsAddrs, dnsResolvers, err := va.getAddrs(ctx, ident.Value) if err != nil { return nil, nil, validationRecord, err } - validationRecord.AddressesResolved, validationRecord.ResolverAddrs = addrs, resolvers + addrs, validationRecord.ResolverAddrs = dnsAddrs, dnsResolvers case identifier.TypeIP: - validationRecord.AddressesResolved = []net.IP{net.ParseIP(ident.Value)} + addrs = []net.IP{net.ParseIP(ident.Value)} default: // This should never happen. The calling function should check the // identifier type. - return nil, nil, validationRecord, fmt.Errorf("Unknown identifier type: %s", ident.Type) + return nil, nil, validationRecord, fmt.Errorf("unknown identifier type: %s", ident.Type) } + validationRecord.AddressesResolved = addrs // Split the available addresses into v4 and v6 addresses - v4, v6 := availableAddresses(validationRecord.AddressesResolved) + v4, v6 := availableAddresses(addrs) addresses := append(v4, v6...) // This shouldn't happen, but be defensive about it anyway @@ -136,27 +135,30 @@ func (va *ValidationAuthorityImpl) getChallengeCert( hostPort string, ident identifier.ACMEIdentifier, ) (*x509.Certificate, *tls.ConnectionState, error) { - tlsConfig := &tls.Config{ - MinVersion: tls.VersionTLS12, - NextProtos: []string{ACMETLS1Protocol}, - // We expect a self-signed challenge certificate, do not verify it here. - InsecureSkipVerify: true, - } + var serverName string switch ident.Type { case identifier.TypeDNS: - tlsConfig.ServerName = ident.Value + serverName = ident.Value case identifier.TypeIP: reverseIP, err := dns.ReverseAddr(ident.Value) if err != nil { va.log.Infof("%s Failed to parse IP address %s.", core.ChallengeTypeTLSALPN01, ident.Value) - return nil, nil, fmt.Errorf("Failed to parse IP address") + return nil, nil, fmt.Errorf("failed to parse IP address") } - tlsConfig.ServerName = reverseIP + serverName = reverseIP default: // This should never happen. The calling function should check the // identifier type. va.log.Infof("%s Unknown identifier type '%s' for %s.", core.ChallengeTypeTLSALPN01, ident.Type, ident.Value) - return nil, nil, fmt.Errorf("Unknown identifier type: %s", ident.Type) + return nil, nil, fmt.Errorf("unknown identifier type: %s", ident.Type) + } + + tlsConfig := &tls.Config{ + MinVersion: tls.VersionTLS12, + NextProtos: []string{ACMETLS1Protocol}, + ServerName: serverName, + // We expect a self-signed challenge certificate, do not verify it here. + InsecureSkipVerify: true, } va.log.Info(fmt.Sprintf("%s [%s] Attempting to validate for %s %s", core.ChallengeTypeTLSALPN01, ident, hostPort, tlsConfig.ServerName)) @@ -212,7 +214,7 @@ func checkExpectedSAN(cert *x509.Certificate, ident identifier.ACMEIdentifier) e default: // This should never happen. The calling function should check the // identifier type. - return fmt.Errorf("Unknown identifier type: %s", ident.Type) + return fmt.Errorf("unknown identifier type: %s", ident.Type) } for _, ext := range cert.Extensions { diff --git a/va/va.go b/va/va.go index e52d424c0c3..fd8a42c2607 100644 --- a/va/va.go +++ b/va/va.go @@ -678,7 +678,10 @@ func (va *ValidationAuthorityImpl) performLocalValidation( // also contains Problems. This method does NOT implement Multi-Perspective // Issuance Corroboration as defined in BRs Sections 3.2.2.9 and 5.4.1. func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *vapb.PerformValidationRequest) (*vapb.ValidationResult, error) { - ident := identifier.FromProtoWithDefault(req.Identifier, req.DnsName) + ident, err := identifier.FromProtoOrName(req.Identifier, req.DnsName) + if err != nil { + return nil, err + } if core.IsAnyNilOrZero(req, ident, req.Challenge, req.Authz, req.ExpectedKeyAuthorization) { return nil, berrors.InternalServerError("Incomplete validation request") diff --git a/va/va_test.go b/va/va_test.go index 98623bd3d81..d2729551c78 100644 --- a/va/va_test.go +++ b/va/va_test.go @@ -1254,6 +1254,8 @@ func TestLogRemoteDifferentials(t *testing.T) { // TestPerformValidationDnsName modifies the PerformValidationRequest to test // backward compatibility during the transition to using an Identifier instead // of a DnsName. +// +// TODO(#8023): Remove this after the transition is over. func TestPerformValidationDnsName(t *testing.T) { t.Parallel() @@ -1263,27 +1265,33 @@ func TestPerformValidationDnsName(t *testing.T) { validationFunc validationFuncRunner identDomain string transmogrifier func(*vapb.PerformValidationRequest) + expectErr bool + expectErrString string expectLog string }{ { - name: "Identifier overrides DnsName for PerformValidation", + name: "Both Identifier and DnsName for PerformValidation", validationFuncName: "PerformValidation", validationFunc: runPerformValidation, identDomain: "good-dns01.com", transmogrifier: func(req *vapb.PerformValidationRequest) { req.DnsName = "good-dns02.com" }, - expectLog: `"Identifier":{"type":"dns","value":"good-dns01.com"}`, + expectErr: true, + expectErrString: "can't use both Identifier and DNSName", + expectLog: `"Identifier":{"type":"dns","value":"good-dns01.com"}`, }, { - name: "Identifier overrides DnsName for DoDCV", + name: "Both Identifier and DnsName for DoDCV", validationFuncName: "DoDCV", validationFunc: runDoDCV, identDomain: "good-dns01.com", transmogrifier: func(req *vapb.PerformValidationRequest) { req.DnsName = "good-dns02.com" }, - expectLog: `"Identifier":{"type":"dns","value":"good-dns01.com"}`, + expectErr: true, + expectErrString: "can't use both Identifier and DNSName", + expectLog: `"Identifier":{"type":"dns","value":"good-dns01.com"}`, }, { name: "No Identifier for PerformValidation", @@ -1313,36 +1321,16 @@ func TestPerformValidationDnsName(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - va, mockLog := setup(nil, "", nil, nil) + va, _ := setup(nil, "", nil, nil) // create a challenge with well known token req := createValidationRequest(identifier.NewDNS(tc.identDomain), core.ChallengeTypeDNS01) tc.transmogrifier(req) - res, _ := tc.validationFunc(context.Background(), va, req) - test.Assert(t, res.Problem == nil, fmt.Sprintf("validation failed: %#v", res.Problem)) - if tc.validationFuncName == "PerformValidation" { - test.AssertMetricWithLabelsEquals(t, va.metrics.validationLatency, prometheus.Labels{ - "operation": opDCVAndCAA, - "perspective": va.perspective, - "challenge_type": string(core.ChallengeTypeDNS01), - "problem_type": "", - "result": pass, - }, 1) + res, err := tc.validationFunc(context.Background(), va, req) + if tc.expectErr { + test.AssertDeepEquals(t, err, errors.New(tc.expectErrString)) } else { - test.AssertMetricWithLabelsEquals(t, va.metrics.validationLatency, prometheus.Labels{ - "operation": opDCV, - "perspective": va.perspective, - "challenge_type": string(core.ChallengeTypeDNS01), - "problem_type": "", - "result": pass, - }, 1) - } - resultLog := mockLog.GetAllMatching(`Validation result`) - if len(resultLog) != 1 { - t.Fatalf("Wrong number of matching lines for 'Validation result'") - } - if !strings.Contains(resultLog[0], tc.expectLog) { - t.Error("PerformValidation didn't log correct validation identifier.") + test.AssertNotNil(t, res.GetProblem(), fmt.Sprintf("validation failed: %#v", res.Problem)) } }) } diff --git a/va/vampic.go b/va/vampic.go index 27a9628bb9d..2eb77ab3a01 100644 --- a/va/vampic.go +++ b/va/vampic.go @@ -219,7 +219,10 @@ type validationLogEvent struct { // implements the DCV portion of Multi-Perspective Issuance Corroboration as // defined in BRs Sections 3.2.2.9 and 5.4.1. func (va *ValidationAuthorityImpl) DoDCV(ctx context.Context, req *vapb.PerformValidationRequest) (*vapb.ValidationResult, error) { - ident := identifier.FromProtoWithDefault(req.Identifier, req.DnsName) + ident, err := identifier.FromProtoOrName(req.Identifier, req.DnsName) + if err != nil { + return nil, err + } if core.IsAnyNilOrZero(req, ident, req.Challenge, req.Authz, req.ExpectedKeyAuthorization) { return nil, berrors.InternalServerError("Incomplete validation request") From d05232b83e79feb12c362e3e6632fe700d65352b Mon Sep 17 00:00:00 2001 From: James Renken Date: Mon, 24 Feb 2025 18:34:10 -0800 Subject: [PATCH 22/27] Address feedback: refactor checkExpectedSAN --- va/tlsalpn.go | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/va/tlsalpn.go b/va/tlsalpn.go index 69896dbcf37..5f3eb7f8bec 100644 --- a/va/tlsalpn.go +++ b/va/tlsalpn.go @@ -196,21 +196,33 @@ func (va *ValidationAuthorityImpl) getChallengeCert( } func checkExpectedSAN(cert *x509.Certificate, ident identifier.ACMEIdentifier) error { - var asn1Tag int - var certSAN []byte + var expectedSANBytes []byte + var expectedSANValue string switch ident.Type { case identifier.TypeDNS: if len(cert.DNSNames) != 1 || len(cert.IPAddresses) != 0 { return errors.New("wrong number of identifiers") } - asn1Tag = 2 - certSAN = []byte(cert.DNSNames[0]) + bytes, err := asn1.Marshal([]asn1.RawValue{ + {Tag: 2, Class: 2, Bytes: []byte(cert.DNSNames[0])}, + }) + if err != nil { + return errors.New("composing SAN extension") + } + expectedSANBytes = bytes + expectedSANValue = cert.DNSNames[0] case identifier.TypeIP: if len(cert.IPAddresses) != 1 || len(cert.DNSNames) != 0 { return errors.New("wrong number of identifiers") } - asn1Tag = 7 - certSAN = cert.IPAddresses[0] + bytes, err := asn1.Marshal([]asn1.RawValue{ + {Tag: 7, Class: 2, Bytes: cert.IPAddresses[0]}, + }) + if err != nil { + return errors.New("composing SAN extension") + } + expectedSANBytes = bytes + expectedSANValue = cert.IPAddresses[0].String() default: // This should never happen. The calling function should check the // identifier type. @@ -219,20 +231,13 @@ func checkExpectedSAN(cert *x509.Certificate, ident identifier.ACMEIdentifier) e for _, ext := range cert.Extensions { if IdCeSubjectAltName.Equal(ext.Id) { - expectedSANs, err := asn1.Marshal([]asn1.RawValue{ - {Tag: asn1Tag, Class: 2, Bytes: certSAN}, - }) - if err != nil || !bytes.Equal(expectedSANs, ext.Value) { + if !bytes.Equal(expectedSANBytes, ext.Value) { return errors.New("SAN extension does not match expected bytes") } } } - if ident.Type == identifier.TypeIP { - certSAN = []byte(net.IP(certSAN).String()) - } - - if !strings.EqualFold(string(certSAN), ident.Value) { + if !strings.EqualFold(expectedSANValue, ident.Value) { return errors.New("identifier does not match expected identifier") } From 67b0090136d35bebe771b2f39233d25c21b129a7 Mon Sep 17 00:00:00 2001 From: James Renken Date: Mon, 24 Feb 2025 19:06:49 -0800 Subject: [PATCH 23/27] Address feedback: refactor host/port parsing in extractRequestTarget --- va/http.go | 24 ++++++++---------------- va/http_test.go | 7 ------- 2 files changed, 8 insertions(+), 23 deletions(-) diff --git a/va/http.go b/va/http.go index 1a4d399b499..26974847a00 100644 --- a/va/http.go +++ b/va/http.go @@ -283,21 +283,23 @@ func (va *ValidationAuthorityImpl) extractRequestTarget(req *http.Request) (iden // Try and split an explicit port number from the request URL host. If there is // one we need to make sure its a valid port. If there isn't one we need to // pick the port based on the reqScheme default port. - reqHost := req.URL.Host + reqHost := req.URL.Hostname() var reqPort int - if h, p, err := net.SplitHostPort(reqHost); err == nil { - reqHost = h - reqPort, err = strconv.Atoi(p) + reqPortStr := req.URL.Port() + if reqPortStr != "" { + port, err := strconv.Atoi(reqPortStr) if err != nil { return identifier.ACMEIdentifier{}, 0, err } // The explicit port must match the VA's configured HTTP or HTTPS port. - if reqPort != va.httpPort && reqPort != va.httpsPort { + if port != va.httpPort && port != va.httpsPort { return identifier.ACMEIdentifier{}, 0, berrors.ConnectionFailureError( "Invalid port in redirect target. Only ports %d and %d are supported, not %d", - va.httpPort, va.httpsPort, reqPort) + va.httpPort, va.httpsPort, port) } + + reqPort = port } else if reqScheme == "http" { reqPort = va.httpPort } else if reqScheme == "https" { @@ -312,16 +314,6 @@ func (va *ValidationAuthorityImpl) extractRequestTarget(req *http.Request) (iden return identifier.ACMEIdentifier{}, 0, berrors.ConnectionFailureError("Invalid empty host in redirect target") } - // In URLs, bare IPv6 addresses are always enclosed in square brackets, - // which must be stripped in order for net.ParseIP to recognize the address. - if len(reqHost) > 2 && reqHost[0] == '[' && reqHost[len(reqHost)-1] == ']' { - shortHost := reqHost[1 : len(reqHost)-1] - // Square brackets are only valid syntax for bare IPs, not DNS names. - if net.ParseIP(shortHost) != nil { - reqHost = shortHost - } - } - // Often folks will misconfigure their webserver to send an HTTP redirect // missing a `/' between the FQDN and the path. E.g. in Apache using: // Redirect / https://bad-redirect.org diff --git a/va/http_test.go b/va/http_test.go index ae1b80bfe3f..689d31e9de4 100644 --- a/va/http_test.go +++ b/va/http_test.go @@ -280,13 +280,6 @@ func TestExtractRequestTarget(t *testing.T) { }, ExpectedError: errors.New("Invalid host in redirect target, must end in IANA registered TLD"), }, - { - Name: "DNS name enclosed in square brackets", - Req: &http.Request{ - URL: mustURL("https://[bad.horse]"), - }, - ExpectedError: errors.New("Invalid host in redirect target, must end in IANA registered TLD"), - }, { Name: "bare IPv4, implicit port", Req: &http.Request{ From dffcc997bf2459fa70b9a68fbec25a7d98e508f3 Mon Sep 17 00:00:00 2001 From: James Renken Date: Tue, 25 Feb 2025 10:06:02 -0800 Subject: [PATCH 24/27] Address feedback: clarify FromProtoOrName --- identifier/identifier.go | 11 +++++++---- va/va_test.go | 4 ++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/identifier/identifier.go b/identifier/identifier.go index 82f88b9176b..22955f6dee0 100644 --- a/identifier/identifier.go +++ b/identifier/identifier.go @@ -50,13 +50,16 @@ func FromProto(ident *corepb.Identifier) ACMEIdentifier { // FromProtoOrName can be removed after DnsNames are no longer used in RPCs. // TODO(#8023) func FromProtoOrName(ident *corepb.Identifier, name string) (ACMEIdentifier, error) { - if ident == nil { - return NewDNS(name), nil + if ident != nil && name != "" { + return ACMEIdentifier{}, errors.New("both Identifier and DNSName are set") } - if name == "" { + if ident != nil { return FromProto(ident), nil } - return ACMEIdentifier{}, errors.New("can't use both Identifier and DNSName") + if name != "" { + return NewDNS(name), nil + } + return ACMEIdentifier{}, errors.New("neither Identifier nor DNSName are set") } // NewDNS is a convenience function for creating an ACMEIdentifier with Type diff --git a/va/va_test.go b/va/va_test.go index d2729551c78..7a482f9b6c3 100644 --- a/va/va_test.go +++ b/va/va_test.go @@ -1278,7 +1278,7 @@ func TestPerformValidationDnsName(t *testing.T) { req.DnsName = "good-dns02.com" }, expectErr: true, - expectErrString: "can't use both Identifier and DNSName", + expectErrString: "both Identifier and DNSName are set", expectLog: `"Identifier":{"type":"dns","value":"good-dns01.com"}`, }, { @@ -1290,7 +1290,7 @@ func TestPerformValidationDnsName(t *testing.T) { req.DnsName = "good-dns02.com" }, expectErr: true, - expectErrString: "can't use both Identifier and DNSName", + expectErrString: "both Identifier and DNSName are set", expectLog: `"Identifier":{"type":"dns","value":"good-dns01.com"}`, }, { From 6e382760c8461a2109a4ff52052c0232bc092575 Mon Sep 17 00:00:00 2001 From: James Renken Date: Tue, 25 Feb 2025 14:47:37 -0800 Subject: [PATCH 25/27] Change FromProtoOrName to FromProtoWithDefault, improve call sites --- identifier/identifier.go | 19 ++++++------------- va/va.go | 8 +++++--- va/vampic.go | 8 +++++--- 3 files changed, 16 insertions(+), 19 deletions(-) diff --git a/identifier/identifier.go b/identifier/identifier.go index 22955f6dee0..81c895b45c7 100644 --- a/identifier/identifier.go +++ b/identifier/identifier.go @@ -4,7 +4,6 @@ package identifier import ( - "errors" "net/netip" corepb "github.com/letsencrypt/boulder/core/proto" @@ -47,19 +46,13 @@ func FromProto(ident *corepb.Identifier) ACMEIdentifier { } } -// FromProtoOrName can be removed after DnsNames are no longer used in RPCs. -// TODO(#8023) -func FromProtoOrName(ident *corepb.Identifier, name string) (ACMEIdentifier, error) { - if ident != nil && name != "" { - return ACMEIdentifier{}, errors.New("both Identifier and DNSName are set") +// FromProtoWithDefault can be removed after DnsNames are no longer used in +// RPCs. TODO(#8023) +func FromProtoWithDefault(ident *corepb.Identifier, name string) ACMEIdentifier { + if ident == nil { + return NewDNS(name) } - if ident != nil { - return FromProto(ident), nil - } - if name != "" { - return NewDNS(name), nil - } - return ACMEIdentifier{}, errors.New("neither Identifier nor DNSName are set") + return FromProto(ident) } // NewDNS is a convenience function for creating an ACMEIdentifier with Type diff --git a/va/va.go b/va/va.go index fd8a42c2607..64138e55b4c 100644 --- a/va/va.go +++ b/va/va.go @@ -678,10 +678,12 @@ func (va *ValidationAuthorityImpl) performLocalValidation( // also contains Problems. This method does NOT implement Multi-Perspective // Issuance Corroboration as defined in BRs Sections 3.2.2.9 and 5.4.1. func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *vapb.PerformValidationRequest) (*vapb.ValidationResult, error) { - ident, err := identifier.FromProtoOrName(req.Identifier, req.DnsName) - if err != nil { - return nil, err + // TODO(#8023): Once DnsNames are no longer used in RPCs, use req.Identifier + // directly instead of setting ident. + if req.Identifier != nil && req.DnsName != "" { + return nil, errors.New("both Identifier and DNSName are set") } + ident := identifier.FromProtoWithDefault(req.Identifier, req.DnsName) if core.IsAnyNilOrZero(req, ident, req.Challenge, req.Authz, req.ExpectedKeyAuthorization) { return nil, berrors.InternalServerError("Incomplete validation request") diff --git a/va/vampic.go b/va/vampic.go index 2eb77ab3a01..dd70463a417 100644 --- a/va/vampic.go +++ b/va/vampic.go @@ -219,10 +219,12 @@ type validationLogEvent struct { // implements the DCV portion of Multi-Perspective Issuance Corroboration as // defined in BRs Sections 3.2.2.9 and 5.4.1. func (va *ValidationAuthorityImpl) DoDCV(ctx context.Context, req *vapb.PerformValidationRequest) (*vapb.ValidationResult, error) { - ident, err := identifier.FromProtoOrName(req.Identifier, req.DnsName) - if err != nil { - return nil, err + // TODO(#8023): Once DnsNames are no longer used in RPCs, use req.Identifier + // directly instead of setting ident. + if req.Identifier != nil && req.DnsName != "" { + return nil, errors.New("both Identifier and DNSName are set") } + ident := identifier.FromProtoWithDefault(req.Identifier, req.DnsName) if core.IsAnyNilOrZero(req, ident, req.Challenge, req.Authz, req.ExpectedKeyAuthorization) { return nil, berrors.InternalServerError("Incomplete validation request") From d281836947f43d2dbbd96014a3856e97cde7f2c7 Mon Sep 17 00:00:00 2001 From: James Renken Date: Tue, 25 Feb 2025 14:51:38 -0800 Subject: [PATCH 26/27] Address feedback --- identifier/identifier.go | 6 +++--- va/http.go | 3 ++- va/tlsalpn.go | 16 +++++++--------- va/va.go | 7 ++----- 4 files changed, 14 insertions(+), 18 deletions(-) diff --git a/identifier/identifier.go b/identifier/identifier.go index 81c895b45c7..dc4ae8917d0 100644 --- a/identifier/identifier.go +++ b/identifier/identifier.go @@ -69,9 +69,9 @@ func NewDNS(domain string) ACMEIdentifier { func NewIP(ip netip.Addr) ACMEIdentifier { return ACMEIdentifier{ Type: TypeIP, - // Section 3 of RFC 8738: The identifier value MUST contain the textual - // form of the address as defined in Section 2.1 of RFC1123 for IPv4 and - // in Section 4 of RFC5952 for IPv6. + // RFC 8738, Sec. 3: The identifier value MUST contain the textual form + // of the address as defined in RFC 1123, Sec. 2.1 for IPv4 and in RFC + // 5952, Sec. 4 for IPv6. Value: ip.String(), } } diff --git a/va/http.go b/va/http.go index 26974847a00..ae78586f02c 100644 --- a/va/http.go +++ b/va/http.go @@ -330,7 +330,8 @@ func (va *ValidationAuthorityImpl) extractRequestTarget(req *http.Request) (iden ) } - if reqIP, err := netip.ParseAddr(reqHost); err == nil { + reqIP, err := netip.ParseAddr(reqHost) + if err == nil { return identifier.NewIP(reqIP), reqPort, nil } diff --git a/va/tlsalpn.go b/va/tlsalpn.go index 5f3eb7f8bec..6281629428a 100644 --- a/va/tlsalpn.go +++ b/va/tlsalpn.go @@ -153,20 +153,18 @@ func (va *ValidationAuthorityImpl) getChallengeCert( return nil, nil, fmt.Errorf("unknown identifier type: %s", ident.Type) } - tlsConfig := &tls.Config{ + va.log.Info(fmt.Sprintf("%s [%s] Attempting to validate for %s %s", core.ChallengeTypeTLSALPN01, ident, hostPort, serverName)) + + dialCtx, cancel := context.WithTimeout(ctx, va.singleDialTimeout) + defer cancel() + + dialer := &tls.Dialer{Config: &tls.Config{ MinVersion: tls.VersionTLS12, NextProtos: []string{ACMETLS1Protocol}, ServerName: serverName, // We expect a self-signed challenge certificate, do not verify it here. InsecureSkipVerify: true, - } - - va.log.Info(fmt.Sprintf("%s [%s] Attempting to validate for %s %s", core.ChallengeTypeTLSALPN01, ident, hostPort, tlsConfig.ServerName)) - - dialCtx, cancel := context.WithTimeout(ctx, va.singleDialTimeout) - defer cancel() - - dialer := &tls.Dialer{Config: tlsConfig} + }} conn, err := dialer.DialContext(dialCtx, "tcp", hostPort) if err != nil { va.log.Infof("%s connection failure for %s. err=[%#v] errStr=[%s]", core.ChallengeTypeTLSALPN01, ident, err, err) diff --git a/va/va.go b/va/va.go index 64138e55b4c..c8065beef25 100644 --- a/va/va.go +++ b/va/va.go @@ -422,15 +422,12 @@ func (va *ValidationAuthorityImpl) validateChallenge( token string, keyAuthorization string, ) ([]core.ValidationRecord, error) { - if ident.Type == identifier.TypeDNS { - // Strip a (potential) leading wildcard token from the identifier. - ident.Value = strings.TrimPrefix(ident.Value, "*.") - } - switch kind { case core.ChallengeTypeHTTP01: return va.validateHTTP01(ctx, ident, token, keyAuthorization) case core.ChallengeTypeDNS01: + // Strip a (potential) leading wildcard token from the identifier. + ident.Value = strings.TrimPrefix(ident.Value, "*.") return va.validateDNS01(ctx, ident, keyAuthorization) case core.ChallengeTypeTLSALPN01: return va.validateTLSALPN01(ctx, ident, keyAuthorization) From 1699a1c7ee13a2b3b81174f63cd836e51740d2db Mon Sep 17 00:00:00 2001 From: James Renken Date: Tue, 25 Feb 2025 14:54:46 -0800 Subject: [PATCH 27/27] Address feedback: clean up extractRequestTarget changes & improve comment --- va/http.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/va/http.go b/va/http.go index ae78586f02c..58c3535db09 100644 --- a/va/http.go +++ b/va/http.go @@ -280,26 +280,25 @@ func (va *ValidationAuthorityImpl) extractRequestTarget(req *http.Request) (iden `Only "http" and "https" protocol schemes are supported, not %q`, reqScheme) } - // Try and split an explicit port number from the request URL host. If there is - // one we need to make sure its a valid port. If there isn't one we need to - // pick the port based on the reqScheme default port. + // Try to parse an explicit port number from the request URL host. If there + // is one, we need to make sure its a valid port. If there isn't one we need + // to pick the port based on the reqScheme default port. reqHost := req.URL.Hostname() var reqPort int - reqPortStr := req.URL.Port() - if reqPortStr != "" { - port, err := strconv.Atoi(reqPortStr) + if req.URL.Port() != "" { + parsedPort, err := strconv.Atoi(req.URL.Port()) if err != nil { return identifier.ACMEIdentifier{}, 0, err } // The explicit port must match the VA's configured HTTP or HTTPS port. - if port != va.httpPort && port != va.httpsPort { + if parsedPort != va.httpPort && parsedPort != va.httpsPort { return identifier.ACMEIdentifier{}, 0, berrors.ConnectionFailureError( "Invalid port in redirect target. Only ports %d and %d are supported, not %d", - va.httpPort, va.httpsPort, port) + va.httpPort, va.httpsPort, parsedPort) } - reqPort = port + reqPort = parsedPort } else if reqScheme == "http" { reqPort = va.httpPort } else if reqScheme == "https" {