feat: support multiple certificates in coder server and helm #4150
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
coadler
approved these changes
Sep 28, 2022
Emyrk
reviewed
Sep 29, 2022
cli/server.go
Outdated
Comment on lines
1078
to
1098
| tlsConfig.GetCertificate = func(hi *tls.ClientHelloInfo) (*tls.Certificate, error) { | ||
| // If there's only one certificate, return it. | ||
| if len(certs) == 1 { | ||
| return &certs[0], nil | ||
| } | ||
|
|
||
| // Expensively check which certificate matches the client hello. | ||
| for _, cert := range certs { | ||
| cert := cert | ||
| if err := hi.SupportsCertificate(&cert); err == nil { | ||
| return &cert, nil | ||
| } | ||
| } | ||
|
|
||
| // Return the first certificate if we have one, or return nil so the | ||
| // server doesn't fail. | ||
| if len(certs) > 0 { | ||
| return &certs[0], nil | ||
| } | ||
| return nil, nil //nolint:nilnil | ||
| } |
There was a problem hiding this comment.
👍 This allows us to hot reload certs later.
| func configureTLS(listener net.Listener, tlsMinVersion, tlsClientAuth, tlsCertFile, tlsKeyFile, tlsClientCAFile string) (net.Listener, error) { | ||
| func loadCertificates(tlsCertFiles, tlsKeyFiles []string) ([]tls.Certificate, error) { | ||
| if len(tlsCertFiles) != len(tlsKeyFiles) { | ||
| return nil, xerrors.New("--tls-cert-file and --tls-key-file must be used the same amount of times") |
There was a problem hiding this comment.
Suggested change
| return nil, xerrors.New("--tls-cert-file and --tls-key-file must be used the same amount of times") | |
| return nil, xerrors.New("the number of --tls-cert-file and --tls-key-file must be the same") |
There was a problem hiding this comment.
I personally think the old message is better, what do you think @kylecarbs
| dials int64 | ||
| ) | ||
| client := codersdk.New(accessURL) | ||
| client.HTTPClient = &http.Client{ |
There was a problem hiding this comment.
Could add the certs to the client. But not a big deal as you verify hostname later 🤷
There was a problem hiding this comment.
I'm going to leave this as-is because we will get better test failure messages if the server somehow returns an unexpected certificate
Emyrk
reviewed
Sep 29, 2022
Emyrk
approved these changes
Sep 29, 2022
kylecarbs
approved these changes
Sep 30, 2022
|
Tested with real helm deployment and found a few bugs in the Helm chart which are now fixed in the latest commit. The coder binary worked flawlessly though. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds multiple certificate support to
coder serverand the Helm chart as it's likely admins will want to be able to use multiple certificates to support the new wildcard access feature.--tls-cert-fileand--tls-key-fileto be array flags instead of single string flags.coder.tls.secretNamesfor loading multiple certificates._helpers.tpl.coder.tls.secretNamein favor ofcoder.tls.secretNames.NOTES.txtthat informs users if they are using the deprecated value and says hi. :)TODO:
coder servermultiple certificate loading functionality.helm templateandhelm install --dry-runto ensure template correctness.