Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

connect: strip port from DNS SANs for ingress gateway leaf cert #15320

Merged
merged 3 commits into from
Nov 14, 2022

Conversation

kyhavlov
Copy link
Contributor

@kyhavlov kyhavlov commented Nov 9, 2022

Closes #11092 - currently we're passing along whatever's in the Hosts field verbatim to construct the list of SANs. This PR changes that to strip the : and everything after it from each Host string so that it's in the format Vault expects when signing the leaf cert.

@kyhavlov kyhavlov requested a review from a team November 9, 2022 23:12
@kyhavlov kyhavlov force-pushed the ingress-dns-sans-strip-port branch from 85156b9 to fbefbcc Compare November 9, 2022 23:15
@jkirschner-hashicorp
Copy link
Contributor

@kyhavlov : Thanks! A few follow-up questions:

  • Which versions do we intend to attempt to backport this to?
  • Should we intervene where you did (at the ingress gateway point)? Or is there any reason to strip host out from dnsnames at a more general point (like in the CSR code)?
  • Do we need any automated tests for this? (Since there are no unit tests, I assume we manually tested instead?)

@kyhavlov
Copy link
Contributor Author

@jkirschner-hashicorp

  • Probably through 1.12
  • Agreed that it makes more sense in the connect package - I was looking at alternate places it could go and moved it there
  • Added a test in the connect package for it (easier to unit test it there too)

@kyhavlov kyhavlov force-pushed the ingress-dns-sans-strip-port branch from 77fee06 to 331312f Compare November 11, 2022 02:13
Copy link
Contributor

@jkirschner-hashicorp jkirschner-hashicorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kyhavlov: I think I spotted something that can quickly be fixed.

formattedDNSNames := make([]string, 0)
for _, host := range dnsNames {
hostSegments := strings.Split(host, ":")
formattedHost := hostSegments[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we confirm that len(hostSegments) > 0 here before accessing the 0th element, to ensure we don't have an out-of-bounds access?

Copy link
Contributor

@jkirschner-hashicorp jkirschner-hashicorp Nov 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To test this edge case, I guess you could add a host "" to the array in your test

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be okay. strings.Split only returns an empty list when both arguments are empty.

formattedHost could be empty here, but that would only occur if the one of dnsNames had no hostname, like :8080.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We wouldn't want a bad configuration (like ":8080") to cause a panic though, right?

continue
}

if len(hostSegments) >= 1 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check seems to be ineffective in its current position, because we're already assuming that hostSegments has at least 1 element by accessing its 0th element without a guard.

formattedDNSNames := make([]string, 0)
for _, host := range dnsNames {
hostSegments := strings.Split(host, ":")
formattedHost := hostSegments[0]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be okay. strings.Split only returns an empty list when both arguments are empty.

formattedHost could be empty here, but that would only occur if the one of dnsNames had no hostname, like :8080.

@kyhavlov kyhavlov force-pushed the ingress-dns-sans-strip-port branch from 2251d55 to a24eaf3 Compare November 14, 2022 18:01
Copy link
Contributor

@jkirschner-hashicorp jkirschner-hashicorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dnsNames in csr for ingress should not include port
4 participants