tls: generate server TLS cert from CA #2401

dnephin · 2022-06-24T18:53:22Z

Summary

The first commit moves some functions to tls.go, so best viewed by individual commit.

With the changes in this PR, a user has 3 options for configuring TLS:

set tls.ACME=true, to use ACME to create a publicly trusted certificate
set tls.ca and tls.caPrivateKey, which will be used to generate a TLS ceritifcate for any hostname the client requests.
set tls.certificate and tls.privateKey to use that certificate for TLS

We plan on setting a CA as part of the helm deploy, so the default for any user that uses our helm charts will be to specify nothing and use option 2.

ACME is no longer the default option. It has to be explicitly enabled, which is probably better because currently every single TLS request could make an ACME API request, even when it should never be used.

Resolves #2067
And addresses parts 1 and 2 of #2362

TODO:

fix some CLI tests that are failing due to missing CA option
add test case for generating certs from a CA
update helm to generate the CA as part of deploy

dnephin · 2022-06-24T18:53:53Z

internal/certs/certs.go

@@ -63,64 +57,6 @@ func GenerateCertificate(hosts []string, caCert *x509.Certificate, caKey crypto.
 	return PEMEncodeCertificate(certBytes), keyBytes, nil
 }

-func SelfSignedOrLetsEncryptCert(manager *autocert.Manager) func(hello *tls.ClientHelloInfo) (*tls.Certificate, error) {


I moved this into internal/server/tls.go and renamed it to getCertificate. That's the only place it is used.

dnephin · 2022-06-24T18:54:07Z

internal/certs/certs.go

@@ -146,39 +77,3 @@ func PEMEncodeCertificate(raw []byte) []byte {
 func pemEncodePrivateKey(raw []byte) []byte {
 	return pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: raw})
 }
-
-func newCA() (*x509.Certificate, *rsa.PrivateKey, error) {


No longer used, we'll rely on helm or the user to provide the CA.

What implications does this have on the initial deploy (ex: quickstart)?

Right, good question. I added another TODO to the list in the description. We need to generate the CA in helm as part of the deploy. That should make it invisible to the user. They won't have to do anything new, but helm will take care of creating the secret which contains the CA and CA private key.

I've pushed a commit which does this (generates the CA in helm and stores it in a secret)

The other implication is that the server can no longer be served outside of a helm install, e.g. docker, compose, or local build, unless the CA is manually created and configured. It'll be nice to keep this to support those use cases but not critical

Ya, that's true. We could keep the ability to generate a CA if none is provided for development and demo purposes. I think in practice no real production use case should have the server generate a CA on first install. The operator really needs to save that CA and private key somewhere, and they aren't going to know it exists or where to find it if it was generated on first start.

I'll see about re-adding this for development.

One of the disadvantages to generating the CA by default is that if the user accidentally mis-configured either ca or caPrivateKey. For example, maybe they set only one of the two, or they set primaryKey instead of caPrimaryKey. With the changes in this PR they will get a nice error and know how to fix it.

If we always generate things for them by default then it won't fail, and they'll have a harder time debugging.

I think it might be better to require these are set ahead of starting the server. For dev maybe we could add a --dev flag and do some setup in the CLI before starting the server.

Either Helm or infra can exit error if only one of ca or caPrivateKey is supplied since both are required. It'll reduce the impact of the scenario you're suggesting. Either way, it can be addressed in a future change

dnephin · 2022-06-24T18:54:27Z

internal/server/server.go

@@ -299,63 +299,6 @@ type routine struct {
 	stop func()
 }

-func tlsConfigFromOptions(


Moved to internal/server/tls.go

dnephin · 2022-06-24T18:55:15Z

internal/server/tls.go

+}
+
+func getCertificate(cache autocert.Cache, opts TLSOptions) func(hello *tls.ClientHelloInfo) (*tls.Certificate, error) {
+	var lock sync.RWMutex


This lock should fix #2067

dnephin · 2022-06-28T17:52:22Z

internal/server/tls.go

-			if err := manager.Cache.Put(ctx, serverName+".crt", certBytes); err != nil {
-				return nil, err
-			}
+		hosts := []string{"127.0.0.1", "::1", serverName}


I changed this to always include localhost in the list of valid IP SANs since it's common for a cert to be used on localhost as well.

dnephin

This PR should be ready for review now

dnephin · 2022-06-29T22:07:53Z

internal/server/tls.go

@@ -26,21 +23,47 @@ func tlsConfigFromOptions(
 	tlsCacheDir string,
 	opts TLSOptions,
 ) (*tls.Config, error) {
-	// TODO: print CA fingerprint when the client can trust that fingerprint
+	// TODO: how can we test this?


Not really sure how to test the ACME integration, but it wasn't tested before and it's still not documented as a supported feature, so probably not a requirement for this PR.

dnephin · 2022-06-30T21:45:58Z

helm/charts/infra/templates/server/secret.yaml

+{{- get $secret.data "ca.key" | b64dec | nindent 4 }}
+
+{{- else }}
+{{- $ca := genCA "Infra Server" 3650 }}


This is the name people will see in the Subject line of the certificate. Should we use "Infra" , or "Infra Server" ?

"Infra Server" seems fine to me 👍

These function will be modified in the next commit. Moving them now so that changes are more obvious.

helm/charts/infra/templates/server/deployment.yaml

helm/charts/infra/templates/server/secret.yaml

helm/charts/infra/values.yaml

internal/server/tls.go

mxyng · 2022-07-01T00:25:14Z

helm/charts/infra/templates/server/configmap.yaml

@@ -36,6 +36,12 @@ data:
 {{- end }}
 {{- end }}

+{{- if (not .Values.server.config.tls) }}


Don't need parenthesis around the not

helm/charts/infra/values.yaml

mxyng · 2022-07-04T18:46:14Z

internal/certs/certs.go

@@ -146,39 +77,3 @@ func PEMEncodeCertificate(raw []byte) []byte {
 func pemEncodePrivateKey(raw []byte) []byte {
 	return pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: raw})
 }
-
-func newCA() (*x509.Certificate, *rsa.PrivateKey, error) {


The other implication is that the server can no longer be served outside of a helm install, e.g. docker, compose, or local build, unless the CA is manually created and configured. It'll be nice to keep this to support those use cases but not critical

ssoroka

looks reasonable. no concerns at this point

With this commit, a user has 3 options for configuring TLS: 1. set tls.ACME=true, to use ACME to create a publicly trusted certificate 2. set tls.ca and tls.caPrivateKey, which will be used to generate a TLS ceritifcate for any hostname the client requests 3. set tls.certificate and tls.privateKey to use that certificate for TLS

The certificate is specified from opts instead of populating the cache.

dnephin commented Jun 24, 2022

View reviewed changes

dnephin force-pushed the dnephin/server-tls-config-2 branch from 83bd672 to 50d1125 Compare June 24, 2022 18:58

dnephin mentioned this pull request Jun 27, 2022

server: accept TLS certificate and key #2398

Merged

dnephin force-pushed the dnephin/server-tls-config-2 branch 3 times, most recently from 871965b to cb26d63 Compare June 27, 2022 21:39

dnephin commented Jun 28, 2022

View reviewed changes

Base automatically changed from dnephin/server-tls-config to main June 28, 2022 18:01

dnephin force-pushed the dnephin/server-tls-config-2 branch from cb26d63 to b7b5ecd Compare June 29, 2022 21:54

dnephin marked this pull request as ready for review June 29, 2022 21:54

dnephin requested review from jmorganca, pdevine, kimskimchi and mxyng as code owners June 29, 2022 21:54

dnephin force-pushed the dnephin/server-tls-config-2 branch from b7b5ecd to e10b58f Compare June 29, 2022 22:14

dnephin commented Jun 29, 2022

View reviewed changes

dnephin mentioned this pull request Jun 29, 2022

improve: include the server TLS CA in the chain #2444

Merged

dnephin commented Jun 30, 2022

View reviewed changes

dnephin mentioned this pull request Jun 30, 2022

helm: refactor to not create a new CA on every connector deploy #2458

Merged

maintain: move server TLS funcs

c7b5a3b

These function will be modified in the next commit. Moving them now so that changes are more obvious.

dnephin force-pushed the dnephin/server-tls-config-2 branch 2 times, most recently from 2a51a80 to 8d63f54 Compare July 4, 2022 15:58

BruceMacD reviewed Jul 4, 2022

View reviewed changes

helm/charts/infra/templates/server/deployment.yaml Show resolved Hide resolved

helm/charts/infra/templates/server/secret.yaml Show resolved Hide resolved

helm/charts/infra/values.yaml Show resolved Hide resolved

internal/server/tls.go Show resolved Hide resolved

mxyng reviewed Jul 4, 2022

View reviewed changes

ssoroka approved these changes Jul 4, 2022

View reviewed changes

dnephin added 3 commits July 4, 2022 17:32

maintain: update CLI tests for new TLS config

3b625ec

The certificate is specified from opts instead of populating the cache.

improve: create infra-server-ca secret

4dc9598

dnephin force-pushed the dnephin/server-tls-config-2 branch from 8d63f54 to 4dc9598 Compare July 4, 2022 21:33

dnephin mentioned this pull request Jul 4, 2022

connector: verify the server TLS #2485

Merged

mxyng approved these changes Jul 5, 2022

View reviewed changes

dnephin merged commit 4c888ca into main Jul 5, 2022

dnephin deleted the dnephin/server-tls-config-2 branch July 5, 2022 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: generate server TLS cert from CA #2401

tls: generate server TLS cert from CA #2401

dnephin commented Jun 24, 2022 •

edited

Loading

dnephin Jun 24, 2022

dnephin Jun 24, 2022

BruceMacD Jun 27, 2022

dnephin Jun 27, 2022

dnephin Jun 29, 2022

mxyng Jul 4, 2022

dnephin Jul 4, 2022

dnephin Jul 4, 2022

mxyng Jul 5, 2022 •

edited

Loading

dnephin Jun 24, 2022

dnephin Jun 24, 2022

dnephin Jun 28, 2022 •

edited

Loading

dnephin left a comment

dnephin Jun 29, 2022

dnephin Jun 30, 2022

BruceMacD Jul 4, 2022

mxyng Jul 1, 2022

mxyng Jul 4, 2022

ssoroka left a comment

tls: generate server TLS cert from CA #2401

tls: generate server TLS cert from CA #2401

Conversation

dnephin commented Jun 24, 2022 • edited Loading

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxyng Jul 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssoroka left a comment

Choose a reason for hiding this comment

dnephin commented Jun 24, 2022 •

edited

Loading

mxyng Jul 5, 2022 •

edited

Loading

dnephin Jun 28, 2022 •

edited

Loading