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

Add support customized IPSans and DNSSans for server-tls server cert for vault #1020

Merged
merged 7 commits into from
Feb 8, 2022

Conversation

jmurret
Copy link
Member

@jmurret jmurret commented Feb 4, 2022

Changes proposed in this PR:

  • Add support customized IPSans and DNSSans for server-tls server cert for vault

How I've tested this PR:

  • unit tests
  • manual verification in a consul/vault/k8s environment

How I expect reviewers to test this PR:
manual verification in a consul/vault/k8s environment

Checklist:

  • Tests added
  • CHANGELOG entry added

@hashicorp-cla
Copy link

hashicorp-cla commented Feb 4, 2022

CLA assistant check
All committers have signed the CLA.

@jmurret jmurret changed the title Jm/vault sans ips Add support customized IPSans and DNSSans for server-tls server cert for vault Feb 4, 2022
@jmurret jmurret marked this pull request as ready for review February 7, 2022 13:56
@NicoletaPopoviciu NicoletaPopoviciu requested review from kschoche, a team and t-eckert and removed request for a team February 7, 2022 21:03
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@kschoche kschoche left a comment

Choose a reason for hiding this comment

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

I think this looks great, assuming that you get the manual test working and confirmed I think it's good to merge after.
One comment on my way :
Is there any way you could roll the new templating logic into a helper function? It is really a lot of text to all inline. Thoughts?

{{ "{{" }}- .Data.certificate -{{ "}}" }}
{{ "{{" }}- end -{{ "}}" }}
{{- end -}}

{{- define "consul.serverTLSKeyTemplate" -}}
|
{{ "{{" }}- with secret "{{ .Values.server.serverCert.secretName }}" "{{ printf "common_name=server.%s.%s" .Values.global.datacenter .Values.global.domain }}"
"ttl=1h" "alt_names={{ include "consul.serverTLSAltNames" . }}" "ip_sans=127.0.0.1" -{{ "}}" }}
"ttl=1h" "alt_names={{ include "consul.serverTLSAltNames" . }}{{- if .Values.server.tls -}}{{- if .Values.server.tls.serverAdditionalDNSSANs -}}{{- range $san := .Values.server.tls.serverAdditionalDNSSANs }},{{ $san }} {{- end -}}{{- end -}}{{- end -}}" "ip_sans=127.0.0.1{{- if .Values.server.tls -}}{{- if .Values.server.tls.serverAdditionalIPSANs -}}{{- range $ipsan := .Values.server.tls.serverAdditionalIPSANs }},{{ $ipsan }} {{- end -}}{{- end -}}{{- end -}}" -{{ "}}" }}
Copy link
Contributor

Choose a reason for hiding this comment

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

drive-by comment: these values should come from .Values.global.tls not .Values.server.tls. The goal I think is to re-use existing values:

serverAdditionalDNSSANs: []

Copy link
Member Author

Choose a reason for hiding this comment

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

:face-palm. Thank you!

Comment on lines 1810 to 1811
echo "Expected: ${expected}"
echo "Actual: ${actual}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to keep these here or was it for debugging?

Copy link
Member Author

Choose a reason for hiding this comment

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

ooh. thx!

@jmurret
Copy link
Member Author

jmurret commented Feb 8, 2022

@kschoche great call on the helper function! will add

Copy link
Contributor

@t-eckert t-eckert left a comment

Choose a reason for hiding this comment

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

This looks excellent, John! Great work.

{{ "{{" }}- .Data.private_key -{{ "}}" }}
{{ "{{" }}- end -{{ "}}" }}
{{- end -}}

{{- define "consul.serverTLSAltNames" -}}
{{- $name := include "consul.fullname" . -}}
{{- $ns := .Release.Namespace -}}
{{ printf "localhost,%s-server,*.%s-server,*.%s-server.%s,*.%s-server.%s.svc,*.server.%s.%s" $name $name $name $ns $name $ns (.Values.global.datacenter ) (.Values.global.domain) }}
{{ printf "localhost,%s-server,*.%s-server,*.%s-server.%s,*.%s-server.%s.svc,*.server.%s.%s" $name $name $name $ns $name $ns (.Values.global.datacenter ) (.Values.global.domain) }}{{ include "consul.serverAdditionalDNSSANs" . }}
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks so much better, great work. I totally missed the typos that Iryna pointed out bc it was all one giant line of text and my brain wasnt up to the task. Much better!

Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

Looks great!!

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>
@jmurret
Copy link
Member Author

jmurret commented Feb 8, 2022

Screen Shot 2022-02-08 at 4 42 05 PM

Verified DNS SANs and IP SANs flowing through to servercert. Going to merge.

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

Successfully merging this pull request may close these issues.

5 participants