Fix issue building urls with IPv6 IPs for ACME http-01 challenges (#28718)

* Fix ACME http-01 challenges for IPv6 IPs

 - We weren't properly encapsulating the IPv6 IP within the url provided
   to the http client with [].

* Add cl

* Cleanup a test println

builtin/logical/pki/acme_authorizations.go

@@ -20,6 +20,7 @@ type ACMEIdentifier struct {
 	Value         string             `json:"value"`
 	OriginalValue string             `json:"original_value"`
 	IsWildcard    bool               `json:"is_wildcard"`
+	IsV6IP        bool               `json:"is_v6_ip"`
 }
 
 func (ai *ACMEIdentifier) MaybeParseWildcard() (bool, string, error) {

builtin/logical/pki/acme_challenge_engine.go

@@ -7,6 +7,7 @@ import (
 	"container/list"
 	"context"
 	"fmt"
+	"strings"
 	"sync"
 	"time"
 
@@ -435,7 +436,9 @@ func (ace *ACMEChallengeEngine) _verifyChallenge(sc *storageContext, id string,
 			return ace._verifyChallengeCleanup(sc, err, id)
 		}
 
-		valid, err = ValidateHTTP01Challenge(authz.Identifier.Value, cv.Token, cv.Thumbprint, config)
+		domain := encodeIdentifierForHTTP01Challenge(authz.Identifier)
+
+		valid, err = ValidateHTTP01Challenge(domain, cv.Token, cv.Thumbprint, config)
 		if err != nil {
 			err = fmt.Errorf("%w: error validating http-01 challenge %v: %v; %v", ErrIncorrectResponse, id, err, ChallengeAttemptFailedMsg)
 			return ace._verifyChallengeRetry(sc, cv, authzPath, authz, challenge, err, id)
@@ -494,6 +497,27 @@ func (ace *ACMEChallengeEngine) _verifyChallenge(sc *storageContext, id string,
 	return ace._verifyChallengeCleanup(sc, nil, id)
 }
 
+func encodeIdentifierForHTTP01Challenge(identifier *ACMEIdentifier) string {
+	if !(identifier.Type == ACMEIPIdentifier && identifier.IsV6IP) {
+		return identifier.Value
+	}
+
+	// If our IPv6 identifier has a zone we need to encode the % to %25 otherwise
+	// the http.Client won't properly use it. RFC6874 specifies how the zone
+	// identifier in the URI should be handled. In section 2:
+	//
+	//    According to URI syntax [RFC3986], "%" is always treated as
+	//    an escape character in a URI, so, according to the established URI
+	//    syntax [RFC3986] any occurrences of literal "%" symbols in a URI MUST
+	//    be percent-encoded and represented in the form "%25". Thus, the
+	//    scoped address fe80::a%en1 would appear in a URI as
+	//    http://[fe80::a%25en1].
+	escapedIPv6 := strings.Replace(identifier.Value, "%", "%25", 1)
+
+	// IPv6 addresses need to be surrounded by [] within URLs
+	return fmt.Sprintf("[%s]", escapedIPv6)
+}
+
 func (ace *ACMEChallengeEngine) _verifyChallengeRetry(sc *storageContext, cv *ChallengeValidation, authzPath string, auth *ACMEAuthorization, challenge *ACMEChallenge, verificationErr error, id string) (bool, time.Time, error) {
 	now := time.Now()
 	path := acmeValidationPrefix + id

builtin/logical/pki/acme_challenge_engine_test.go

@@ -0,0 +1,47 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: BUSL-1.1
+
+package pki
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// Test_encodeIdentifierForHTTP01Challenge validates the encoding behaviors of our identifiers
+// for the HTTP01 challenge. Basically properly encode the identifier for an HTTP request.
+func Test_encodeIdentifierForHTTP01Challenge(t *testing.T) {
+	tests := []struct {
+		name string
+		arg  *ACMEIdentifier
+		want string
+	}{
+		{
+			name: "dns",
+			arg:  &ACMEIdentifier{Type: ACMEDNSIdentifier, Value: "www.dadgarcorp.com"},
+			want: "www.dadgarcorp.com",
+		},
+		{
+			name: "ipv4",
+			arg:  &ACMEIdentifier{Type: ACMEIPIdentifier, Value: "192.168.1.1"},
+			want: "192.168.1.1",
+		},
+		{
+			name: "ipv6",
+			arg:  &ACMEIdentifier{Type: ACMEIPIdentifier, Value: "2001:0db8:0000:0000:0000:0000:0000:0068", IsV6IP: true},
+			want: "[2001:0db8:0000:0000:0000:0000:0000:0068]",
+		},
+		{
+			name: "ipv6-zoned",
+			arg:  &ACMEIdentifier{Type: ACMEIPIdentifier, Value: "fe80::1cc0:3e8c:119f:c2e1%ens18", IsV6IP: true},
+			want: "[fe80::1cc0:3e8c:119f:c2e1%25ens18]",
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equalf(t, tt.want, encodeIdentifierForHTTP01Challenge(tt.arg), "encodeIdentifierForHTTP01Challenge(%v)", tt.arg)
+		})
+	}
+}

builtin/logical/pki/path_acme_order.go

@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"net"
 	"net/http"
+	"net/netip"
 	"sort"
 	"strings"
 	"time"
@@ -960,10 +961,24 @@ func parseOrderIdentifiers(data map[string]interface{}) ([]*ACMEIdentifier, erro
 		switch typeStr {
 		case string(ACMEIPIdentifier):
 			identifier.Type = ACMEIPIdentifier
-			ip := net.ParseIP(valueStr)
-			if ip == nil {
+			ip, err := netip.ParseAddr(valueStr)
+			if err != nil {
 				return nil, fmt.Errorf("value argument (%s) failed validation: failed parsing as IP: %w", valueStr, ErrMalformed)
 			}
+			if ip.Is6() {
+				if len(ip.Zone()) > 0 {
+					// If we are given an identifier with a local zone that doesn't make much sense
+					// as zone's are specific to the sender not us. For now disallow, perhaps in the
+					// future we should simply drop the zone?
+					return nil, fmt.Errorf("value argument (%s) failed validation: IPv6 identifiers with zone information are not allowed: %w", valueStr, ErrMalformed)
+				}
+
+				// We should keep whatever formatting of the IPv6 address that came in according
+				// to RFC8738 Section 2:
+				// An identifier for the IPv6 address 2001:db8::1 would be formatted like so:
+				//   {"type": "ip", "value": "2001:db8::1"}
+				identifier.IsV6IP = true
+			}
 		case string(ACMEDNSIdentifier):
 			identifier.Type = ACMEDNSIdentifier
 
@@ -1010,6 +1025,10 @@ func parseOrderIdentifiers(data map[string]interface{}) ([]*ACMEIdentifier, erro
 		identifiers = append(identifiers, identifier)
 	}
 
+	if len(identifiers) == 0 {
+		return nil, fmt.Errorf("no parsed identifiers were found: %w", ErrMalformed)
+	}
+
 	return identifiers, nil
 }
 

builtin/logical/pki/path_acme_order_test.go

@@ -4,12 +4,15 @@
 package pki
 
 import (
+	"fmt"
 	"net"
+	"strings"
 	"testing"
 
 	"github.com/hashicorp/vault/builtin/logical/pki/issuing"
 	"github.com/hashicorp/vault/sdk/framework"
 	"github.com/hashicorp/vault/sdk/logical"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -96,6 +99,135 @@ func TestACME_ValidateIdentifiersAgainstRole(t *testing.T) {
 	}
 }
 
+// Test_parseOrderIdentifiers validates we convert ACME requests into proper ACMEIdentifiers
+func Test_parseOrderIdentifiers(t *testing.T) {
+	tests := []struct {
+		name    string
+		data    map[string]interface{}
+		want    *ACMEIdentifier
+		wantErr assert.ErrorAssertionFunc
+	}{
+		{
+			name: "ipv4",
+			data: map[string]interface{}{"type": "ip", "value": "192.168.1.1"},
+			want: &ACMEIdentifier{
+				Type:          ACMEIPIdentifier,
+				Value:         "192.168.1.1",
+				OriginalValue: "192.168.1.1",
+				IsWildcard:    false,
+				IsV6IP:        false,
+			},
+			wantErr: assert.NoError,
+		},
+		{
+			name: "ipv6",
+			data: map[string]interface{}{"type": "ip", "value": "2001:0:130F::9C0:876A:130B"},
+			want: &ACMEIdentifier{
+				Type:          ACMEIPIdentifier,
+				Value:         "2001:0:130F::9C0:876A:130B",
+				OriginalValue: "2001:0:130F::9C0:876A:130B",
+				IsWildcard:    false,
+				IsV6IP:        true,
+			},
+			wantErr: assert.NoError,
+		},
+		{
+			name: "ipv4-in-ipv6",
+			data: map[string]interface{}{"type": "ip", "value": "::ffff:192.168.1.1"},
+			want: &ACMEIdentifier{
+				Type:          ACMEIPIdentifier,
+				Value:         "::ffff:192.168.1.1",
+				OriginalValue: "::ffff:192.168.1.1",
+				IsWildcard:    false,
+				IsV6IP:        true,
+			},
+			wantErr: assert.NoError,
+		},
+		{
+			name: "dns",
+			data: map[string]interface{}{"type": "dns", "value": "dadgarcorp.com"},
+			want: &ACMEIdentifier{
+				Type:          ACMEDNSIdentifier,
+				Value:         "dadgarcorp.com",
+				OriginalValue: "dadgarcorp.com",
+				IsWildcard:    false,
+				IsV6IP:        false,
+			},
+			wantErr: assert.NoError,
+		},
+		{
+			name: "wildcard-dns",
+			data: map[string]interface{}{"type": "dns", "value": "*.dadgarcorp.com"},
+			want: &ACMEIdentifier{
+				Type:          ACMEDNSIdentifier,
+				Value:         "dadgarcorp.com",
+				OriginalValue: "*.dadgarcorp.com",
+				IsWildcard:    true,
+				IsV6IP:        false,
+			},
+			wantErr: assert.NoError,
+		},
+		{
+			name:    "ipv6-with-zone", // This is debatable if we should strip or fail
+			data:    map[string]interface{}{"type": "ip", "value": "fe80::1cc0:3e8c:119f:c2e1%ens18"},
+			wantErr: ErrorContains("IPv6 identifiers with zone information are not allowed"),
+		},
+		{
+			name:    "bad-dns-wildcard",
+			data:    map[string]interface{}{"type": "dns", "value": "*192.168.1.1"},
+			wantErr: ErrorContains("invalid wildcard"),
+		},
+		{
+			name:    "ip-in-dns",
+			data:    map[string]interface{}{"type": "dns", "value": "192.168.1.1"},
+			wantErr: ErrorContains("parsed OK as IP address"),
+		},
+		{
+			name:    "empty-identifiers",
+			data:    nil,
+			wantErr: ErrorContains("no parsed identifiers were found"),
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			identifiers := map[string]interface{}{"identifiers": []interface{}{}}
+			if tt.data != nil {
+				identifiers["identifiers"] = append(identifiers["identifiers"].([]interface{}), tt.data)
+			}
+			got, err := parseOrderIdentifiers(identifiers)
+			if !tt.wantErr(t, err, fmt.Sprintf("parseOrderIdentifiers(%v)", tt.data)) {
+				return
+			} else if err != nil {
+				// If we passed the test above and an error was set no point in testing below
+				return
+			}
+
+			require.Len(t, got, 1, "expected a single return value")
+			acmeId := got[0]
+			require.Equal(t, tt.want.Type, acmeId.Type)
+			require.Equal(t, tt.want.Value, acmeId.Value)
+			require.Equal(t, tt.want.OriginalValue, acmeId.OriginalValue)
+			require.Equal(t, tt.want.IsWildcard, acmeId.IsWildcard)
+			require.Equal(t, tt.want.IsV6IP, acmeId.IsV6IP)
+		})
+	}
+}
+
+func ErrorContains(errMsg string) assert.ErrorAssertionFunc {
+	return func(t assert.TestingT, err error, i ...interface{}) bool {
+		if err == nil {
+			return assert.Fail(t, "expected error got none", i...)
+		}
+
+		if !strings.Contains(err.Error(), errMsg) {
+			return assert.Fail(t, fmt.Sprintf("error did not contain '%s':\n%+v", errMsg, err), i...)
+		}
+
+		return true
+	}
+}
+
 func _buildACMEIdentifiers(values ...string) []*ACMEIdentifier {
 	var identifiers []*ACMEIdentifier
 

changelog/28718.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+secrets/pki: Address issue with ACME HTTP-01 challenges failing for IPv6 IPs due to improperly formatted URLs
+```