VAULT-33008: ipv6: always display RFC-5952 §4 conformant addresses #29228

ryancragun · 2024-12-18T21:25:06Z

Description

USGv60 requires implementing §4.1.1 of the NISTv6-r1 profile1 for
IPv6-Only capabilities. This section requires that whenever Vault
displays IPv6 addresses (including CLI output, Web UI, logs, etc.) that
all IPv6 addresses must conform to RFC-5952 §4 text representation
recommendations2.

These recommendations do not prevent us from accepting RFC-42413 IPv6
addresses, however, whenever these same addresses are displayed they
must conform to the strict RFC-5952 §4 guidelines.

This PR implements handling of IPv6 address conformance in our
vault server routine. We handle conformance normalization for all
server, http_proxy, listener, seal, storage and telemetry
configuration where an input could contain an IPv6 address, whether
configured via an HCL file or via corresponding environment variables.

The approach I've taken is to handle conformance normalization at
parse time to ensure that all log output and subsequent usage
inside of Vaults various subsystems always reference a conformant
address, that way we don't need concern ourselves with conformance
later. This approach ought to be backwards compatible to prior loose
address configuration requirements, with the understanding that
going forward all IPv6 representation will be strict regardless of
what has been configured.

In many cases I've updated our various parser functions to call the
new configutil.NormalizeAddr() to apply conformance normalization.
Others required no changes because they rely on standard library URL
string output, which always displays IPv6 URLs in a conformant way.

Not included in this changes is any other vault exec mode other than
server. Client, operator commands, agent mode, proxy mode, etc. will
be included in subsequent changes if necessary.

Signed-off-by: Ryan Cragun [email protected]

TODO only if you're a HashiCorp employee

Backport Labels: If this fix needs to be backported, use the appropriate backport/ label that matches the desired release branch. Note that in the CE repo, the latest release branch will look like backport/x.x.x, but older release branches will be backport/ent/x.x.x+ent.
- LTS: If this fixes a critical security vulnerability or severity 1 bug, it will also need to be backported to the current LTS versions of Vault. To ensure this, use all available enterprise labels.
ENT Breakage: If this PR either 1) removes a public function OR 2) changes the signature
of a public function, even if that change is in a CE file, double check that
applying the patch for this PR to the ENT repo and running tests doesn't
break any tests. Sometimes ENT only tests rely on public functions in CE
files.
Jira: If this change has an associated Jira, it's referenced either
in the PR description, commit message, or branch name.
RFC: If this change has an associated RFC, please link it in the description.
ENT PR: If this change has an associated ENT PR, please link it in the
description. Also, make sure the changelog is in this PR, not in your ENT PR.

github-actions · 2024-12-18T21:33:16Z

CI Results:
All Go tests succeeded! ✅

github-actions · 2024-12-18T21:33:38Z

Build Results:
Build failed for these jobs: test:failure. Please refer to this workflow to learn more: https://github.com/hashicorp/vault/actions/runs/12714369807

mpalmi · 2025-01-08T21:13:34Z

internalshared/configutil/normalize.go

+// If the addr is a URL, IP Address, or host:port address that includes an IPv6
+// address, the normalized copy will be conformant with RFC-5942 §4
+// See: https://rfc-editor.org/rfc/rfc5952.html
+func NormalizeAddr(u string) string {


I know you're not looking for nits here, but I couldn't help myself! 😆

Everything about this (description, commit message, etc.) mentions ipv6, but the methods and file names do not reflect that. Would it make sense to make these more ipv6-oriented? IMO that'd make it clearer that this is specifically for ipv6 normalization? I realize this detail is also documented in godocs, but it feels right to make the "interface" more self-documenting...

I agree that the name makes more sense to be more specific about what kinds of addresses it's normalizing. What about
NormalizeAddressIfIPV6 or similar?

Maybe I'm missing something but it looks like this function is designed to take any type of address and if it happens to contain an IPv6 address, then that gets normalized to be RFC-5942 compliant, so from that perspective, I agree with how it's currently named. Otherwise, if we name everything it might be normalizing, the name becomes enormous and unwieldy. If we just call out IPv6 and nothing else, then that's slightly misleading, since it's handling other types of addresses too. I like it the way it is 👌

My thought is that it doesn't touch non-IPv6 addresses, so I would prefer that called out, and why I thought maybe NormalizeAddressIfIPV6 would fit better. I wouldn't love NormalizeIPV6Address for the reasons you called out, Josh, it doesn't touch non-IPv6 addresses.

All that being said, this is far from a hill I want to die on, and it's totally not too bad/confusing the way it is now :)

My thought is that it doesn't touch non-IPv6 addresses

It does though. There's a branch in one of the if statements that deals with IPv4 addresses. @ryancragun can correct me if I'm wrong, but my read through of this function suggests that its intent is you can pass any type of address to it, whether it's IPv4, IPv6, host:port pair, whatever, and it will always do the right thing.

@VioletHynes was correct in that the branch that touches IPv4 can just return the input, which I've done, so we don't actually change anything in ipv4 land.

Regarding the function name, my thinking was that we can safely pass any sort of address to it and receive a normalized version it, regardless of the address type or format. At the moment it only really normalizes addresses that are IPv6 but the name leaves it open to changing in the future if necessary.

Obviously my preference is implicit in the naming choice I made but I definitely don't care. Whatever group consensus is is fine with me.

VioletHynes

This looks great! I've got a few comments that I'd consider non-blocking (other than the make fmt) since the tests assure me that this thing works as we expect. However, I think there are probably some clarity improvements we should make before we merge this.

I really appreciate all of the attention to detail on the tests here.

changelog/29228.txt

command/operator_migrate_test.go

VioletHynes · 2025-01-09T15:10:26Z

internalshared/configutil/kms_test.go

+// can be URLs, IP addresses, or host:port addresses, when configured with an
+// an IPv6 address, the normalized to be conformant with RFC-5942 §4
+// See: https://rfc-editor.org/rfc/rfc5952.html
+func TestMergeKMSEnvConfigAddrConformance(t *testing.T) {


I really appreciate all of the tests added for this functionality. Thank you!

VioletHynes · 2025-01-09T15:11:23Z

internalshared/configutil/normalize.go

+// If the addr is a URL, IP Address, or host:port address that includes an IPv6
+// address, the normalized copy will be conformant with RFC-5942 §4
+// See: https://rfc-editor.org/rfc/rfc5952.html
+func NormalizeAddr(u string) string {


I agree that the name makes more sense to be more specific about what kinds of addresses it's normalizing. What about
NormalizeAddressIfIPV6 or similar?

internalshared/configutil/normalize.go

VioletHynes · 2025-01-09T15:17:43Z

internalshared/configutil/normalize.go

+// If the addr is a URL, IP Address, or host:port address that includes an IPv6
+// address, the normalized copy will be conformant with RFC-5942 §4
+// See: https://rfc-editor.org/rfc/rfc5952.html
+func NormalizeAddr(u string) string {


I'd like if we chose different variable name than u here. Maybe addressToNormalize?

I decided to go with address here as it was certainly better than u (a relic of the genesis of the func when it only took URLs) but less typey than addressToNormalize.

internalshared/configutil/normalize.go

raskchanky

Dang, this is a lot. Thanks so much for doing this Ryan! I left a thousand nit comments, mostly with formatting complaints that will likely be fixed by running make fmt but I like the approach you've taken.

command/server.go

command/server/config.go

raskchanky · 2025-01-09T21:09:14Z

command/server/config.go

@@ -934,33 +936,49 @@ func ParseStorage(result *Config, list *ast.ObjectList, name string) error {
 	}

 	m := make(map[string]string)
-	for key, val := range config {
-		valStr, ok := val.(string)
+	for k, v := range config {


Wow, nice job removing the variable shadowing here. That was subtle.

command/server/config.go

internalshared/configutil/kms_test.go

raskchanky · 2025-01-09T21:29:08Z

internalshared/configutil/kms_test.go

-				if err := os.Setenv(envName, envVal); err != nil {
-					t.Errorf("error setting environment vars for test: %s", err)
-				}
+				t.Setenv(envName, envVal)


So much better, thank you 🙌

raskchanky · 2025-01-09T21:37:30Z

internalshared/configutil/normalize.go

+// If the addr is a URL, IP Address, or host:port address that includes an IPv6
+// address, the normalized copy will be conformant with RFC-5942 §4
+// See: https://rfc-editor.org/rfc/rfc5952.html
+func NormalizeAddr(u string) string {


Maybe I'm missing something but it looks like this function is designed to take any type of address and if it happens to contain an IPv6 address, then that gets normalized to be RFC-5942 compliant, so from that perspective, I agree with how it's currently named. Otherwise, if we name everything it might be normalizing, the name becomes enormous and unwieldy. If we just call out IPv6 and nothing else, then that's slightly misleading, since it's handling other types of addresses too. I like it the way it is 👌

raskchanky · 2025-01-09T21:47:52Z

internalshared/configutil/normalize_test.go

+			require.Equal(t, tc.expected, NormalizeAddr(tc.addr))
+		})
+	}
+}


I appreciate how comprehensive this is, thank you 🙌

internalshared/configutil/telemetry.go

USGv6[0] requires implementing §4.1.1 of the NISTv6-r1 profile[1] for IPv6-Only capabilities. This section requires that whenever Vault displays IPv6 addresses (including CLI output, Web UI, logs, etc.) that _all_ IPv6 addresses must conform to RFC-5952 §4 text representation recommendations[2]. These recommendations do not prevent us from accepting RFC-4241 IPv6 addresses, however, whenever these same addresses are displayed they must conform to the strict RFC-5952 §4 guidelines. This PR implements handling of IPv6 address conformance in our `vault server` routine. We handling conformance normalization in all server, http_proxy, listener, seal, storage and telemetry configuration where an input could contain an IPv6 address, whether configued via an HCL file or via corresponding environment variables. The approach I've taken is to handle conformance normalization at parse time to ensure that all output log output and subsequent usage inside of Vaults various subsystems always reference a conformant address, that way we don't need concern ourselves with conformance later. This approach ought to be backwards compatible to prior loose address configuration requirements, with the understanding that goinging forward all IPv6 representation will be strict regardless of what has been configured. In many cases I've updated our various parser functions to call the new `configutil.NormalizeAddr()` to apply conformance normalization. Others required no changes because they rely on standard library URL string output, which always displays IPv6 URLs in a conformant way. Not included in this changes is any other vault exec mode other than server. Client, operator commands, agent mode, proxy mode, etc. will be included in subsequent changes if necessary. [0]: https://www.nist.gov/publications/usgv6-profile [1]: https://www.nist.gov/publications/nist-ipv6-profile [2]: https://www.rfc-editor.org/rfc/rfc5952.html#section-4 [3]: https://www.rfc-editor.org/rfc/rfc4291 Signed-off-by: Ryan Cragun <[email protected]>

Signed-off-by: Ryan Cragun <[email protected]>

Always use the type when verifying storage and seals. We'll also update an older test that didn't use real storage types during migration to use real storage types. Signed-off-by: Ryan Cragun <[email protected]>

Signed-off-by: Ryan Cragun <[email protected]>

ryancragun requested a review from a team as a code owner December 18, 2024 21:25

ryancragun requested a review from angelocordon December 18, 2024 21:25

github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Dec 18, 2024

ryancragun added the pr/no-milestone label Dec 18, 2024

ryancragun force-pushed the ryan/vault-33009 branch 2 times, most recently from f499d57 to 477e93b Compare December 18, 2024 23:10

ryancragun requested review from raskchanky, VioletHynes and mpalmi January 8, 2025 20:27

mpalmi reviewed Jan 8, 2025

View reviewed changes

VioletHynes previously approved these changes Jan 9, 2025

View reviewed changes

raskchanky previously approved these changes Jan 9, 2025

View reviewed changes

ryancragun added 15 commits January 10, 2025 10:26

ipv6: normalize telemetry addresses

1177265

Signed-off-by: Ryan Cragun <[email protected]>

ipv6: normalize storage addresses

3a4fc03

Signed-off-by: Ryan Cragun <[email protected]>

ipv6: normalize service_registration addresses

347c6b5

Signed-off-by: Ryan Cragun <[email protected]>

ipv6: normalize seal addresses

a6d8ec4

Signed-off-by: Ryan Cragun <[email protected]>

ipv6: normalize seal addresses from env vars

baad416

Signed-off-by: Ryan Cragun <[email protected]>

ipv6: normalize storage retry_join and various other run config

ac18f20

Signed-off-by: Ryan Cragun <[email protected]>

ipv6: cleanup pass before review

8944879

Signed-off-by: Ryan Cragun <[email protected]>

add changelog

99fb757

Signed-off-by: Ryan Cragun <[email protected]>

fix copywrite headers

4edef3f

Signed-off-by: Ryan Cragun <[email protected]>

fix tests

c8c87d5

Always use the type when verifying storage and seals. We'll also update an older test that didn't use real storage types during migration to use real storage types. Signed-off-by: Ryan Cragun <[email protected]>

vet: fix vet issues

963ebf6

Signed-off-by: Ryan Cragun <[email protected]>

changelog: reword things post PR feedback

f9ae8c4

Signed-off-by: Ryan Cragun <[email protected]>

fmt

024422a

Signed-off-by: Ryan Cragun <[email protected]>

address more review feedback

3a127c6

Signed-off-by: Ryan Cragun <[email protected]>

ryancragun dismissed raskchanky’s stale review via 3a127c6 January 10, 2025 17:26

ryancragun dismissed VioletHynes’s stale review via 3a127c6 January 10, 2025 17:26

ryancragun force-pushed the ryan/vault-33009 branch from 377db0c to 3a127c6 Compare January 10, 2025 17:26

fmt again for a few missed blank lines

9995321

Signed-off-by: Ryan Cragun <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VAULT-33008: ipv6: always display RFC-5952 §4 conformant addresses #29228

VAULT-33008: ipv6: always display RFC-5952 §4 conformant addresses #29228

ryancragun commented Dec 18, 2024 •

edited

Loading

github-actions bot commented Dec 18, 2024 •

edited

Loading

github-actions bot commented Dec 18, 2024 •

edited

Loading

mpalmi Jan 8, 2025

VioletHynes Jan 9, 2025

raskchanky Jan 9, 2025

VioletHynes Jan 9, 2025

raskchanky Jan 10, 2025

ryancragun Jan 10, 2025 •

edited

Loading

VioletHynes left a comment

VioletHynes Jan 9, 2025

VioletHynes Jan 9, 2025

VioletHynes Jan 9, 2025

ryancragun Jan 10, 2025

raskchanky left a comment

raskchanky Jan 9, 2025

raskchanky Jan 9, 2025

raskchanky Jan 9, 2025

raskchanky Jan 9, 2025

VAULT-33008: ipv6: always display RFC-5952 §4 conformant addresses #29228

Are you sure you want to change the base?

VAULT-33008: ipv6: always display RFC-5952 §4 conformant addresses #29228

Conversation

ryancragun commented Dec 18, 2024 • edited Loading

Description

TODO only if you're a HashiCorp employee

github-actions bot commented Dec 18, 2024 • edited Loading

github-actions bot commented Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryancragun Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

VioletHynes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raskchanky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryancragun commented Dec 18, 2024 •

edited

Loading

github-actions bot commented Dec 18, 2024 •

edited

Loading

github-actions bot commented Dec 18, 2024 •

edited

Loading

ryancragun Jan 10, 2025 •

edited

Loading