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

Bug tls certs not created with correct SANs #16959

Merged
merged 16 commits into from
May 22, 2023
Merged

Bug tls certs not created with correct SANs #16959

merged 16 commits into from
May 22, 2023

Conversation

lhaig
Copy link
Contributor

@lhaig lhaig commented Apr 21, 2023

The nomad tls cert command did not create certificates with the correct SANs for them to work with non default domain and region names.
This PR updates the code to support non default domains and regions in the certificates.

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

Hi @lhaig! I've left comments on some areas that need work. Also, we have a bunch of TLS certs used for unit testing in helper/tlsutil/testdata. To really make sure this is all right, let's do the following:

  • Regenerate all the certs in helper/tlsutil/testdata
  • Make sure all the unit tests pass with those new certs
  • Add a README.md to helper/tlsutil/testdata that describes the exact commands used to generate those certs.

command/tls_cert_create.go Outdated Show resolved Hide resolved
command/tls_cert_create.go Outdated Show resolved Hide resolved
command/tls_cert_create.go Outdated Show resolved Hide resolved
command/tls_cert_create_test.go Outdated Show resolved Hide resolved
lhaig added 12 commits May 19, 2023 15:27
Nomad requires there to be a `server.global.nomad` alternative name in the certificate when you enable TLS on a cluster.
Added logic to detect custom domain and region configuration.
Ad tests for custom domains
Refactor record preparation to make things simpler
Fix lint errors
Fix Errors due to crypto/x509 verify being updated to require ServerName
Add a server certificate for testing the server.
Fix recordPreperation that was overwriting additional domains
Improved tests as per suggestion from @tgross
Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

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

Just a couple minor corrections but otherwise looks good to me. Thanks Lance!

command/tls_cert_create.go Outdated Show resolved Hide resolved
website/content/docs/commands/tls/cert-create.mdx Outdated Show resolved Hide resolved
Copy link
Member

@tgross tgross 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 @lhaig! Just a few minor items to pick up and I think we can get this merged.

.changelog/16959.txt Outdated Show resolved Hide resolved
command/agent/http_test.go Outdated Show resolved Hide resolved
command/agent/http_test.go Outdated Show resolved Hide resolved
helper/tlsutil/testdata/README.md Show resolved Hide resolved
helper/tlsutil/testdata/README.md Outdated Show resolved Hide resolved
helper/tlsutil/testdata/README.md Outdated Show resolved Hide resolved
lhaig and others added 2 commits May 19, 2023 21:50
Co-authored-by: Tim Gross <tgross@hashicorp.com>
remove debug code

Co-authored-by: Tim Gross <tgross@hashicorp.com>
lhaig added 2 commits May 20, 2023 08:17
Generated additional certificates for testing TLS misconfiguration.
Removed last cfssl generated certificates.
updated testdata readme with new instructions.
Added the Key usage to the cli certificate. Updated the tests to match
Updated the cert-crete.mdx file
added some notes to the upgrade-specific.mdx file for the deprecation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.5.x backport to 1.5.x release line theme/cli type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants