From bce085bd3f36e82ea6538579395615f4cd036cbc Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Wed, 16 Oct 2024 14:26:44 -0400 Subject: [PATCH] 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 | 1 + builtin/logical/pki/acme_challenge_engine.go | 26 +++- .../logical/pki/acme_challenge_engine_test.go | 47 +++++++ builtin/logical/pki/path_acme_order.go | 23 ++- builtin/logical/pki/path_acme_order_test.go | 132 ++++++++++++++++++ changelog/28718.txt | 3 + 6 files changed, 229 insertions(+), 3 deletions(-) create mode 100644 builtin/logical/pki/acme_challenge_engine_test.go create mode 100644 changelog/28718.txt diff --git a/builtin/logical/pki/acme_authorizations.go b/builtin/logical/pki/acme_authorizations.go index 64548ffed99e..9a97f858337d 100644 --- a/builtin/logical/pki/acme_authorizations.go +++ b/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) { diff --git a/builtin/logical/pki/acme_challenge_engine.go b/builtin/logical/pki/acme_challenge_engine.go index 525d21546f41..fab922917fda 100644 --- a/builtin/logical/pki/acme_challenge_engine.go +++ b/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 diff --git a/builtin/logical/pki/acme_challenge_engine_test.go b/builtin/logical/pki/acme_challenge_engine_test.go new file mode 100644 index 000000000000..b6095357927d --- /dev/null +++ b/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) + }) + } +} diff --git a/builtin/logical/pki/path_acme_order.go b/builtin/logical/pki/path_acme_order.go index 197b7d09161f..396dd3e76933 100644 --- a/builtin/logical/pki/path_acme_order.go +++ b/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 } diff --git a/builtin/logical/pki/path_acme_order_test.go b/builtin/logical/pki/path_acme_order_test.go index 01472536ce33..a1788dbbfdc2 100644 --- a/builtin/logical/pki/path_acme_order_test.go +++ b/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 diff --git a/changelog/28718.txt b/changelog/28718.txt new file mode 100644 index 000000000000..e32c7b2d593b --- /dev/null +++ b/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 +```