- Notifications
You must be signed in to change notification settings - Fork928
feat: support multiple certificates in coder server and helm#4150
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
cli/server.go Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| returnnil,xerrors.New("--tls-cert-file and --tls-key-file must beusedthe same amount of times") | |
| returnnil,xerrors.New("the number of--tls-cert-file and --tls-key-file must be the same") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could add the certs to the client. But not a big deal as you verify hostname later 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm going to leave this as-is because we will get better test failure messages if the server somehow returns an unexpected certificate
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Did you manually test the Helm chart with multiple certificates? Just to confirm that it all works.
All the code looks good to me, and I'm impressed with how thorough the tests are 🥳
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. |
Uh oh!
There was an error while loading.Please reload this page.
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.