cmd/contour: hot-reload certificates and key #2198

tsaarni · 2020-02-05T14:00:56Z

This change adds support for certificate rotation for XDS gRPC interface
between Contour and Envoy. It is achieved by lazily loading certificates
and key every time new TLS connection is established by Envoy.

This change addresses only the certificate rotation in Contour (server)
and similar support is needed for Envoy (client) to cover the whole use
case.

Signed-off-by: Tero Saarni [email protected]

tsaarni · 2020-02-05T14:04:36Z

This is just initial stab at #2143, all comments are more than welcome!

At this point I generated two sets of certificates for tests, but I saw #1994 and I wonder would you like to move to certificates that are generated on-the-fly by code?

codecov · 2020-02-05T14:09:51Z

Codecov Report

Merging #2198 into master will increase coverage by 0.55%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2198      +/-   ##
==========================================
+ Coverage   78.16%   78.72%   +0.55%     
==========================================
  Files          57       57              
  Lines        5034     5048      +14     
==========================================
+ Hits         3935     3974      +39     
+ Misses       1013      984      -29     
- Partials       86       90       +4

Impacted Files	Coverage Δ
cmd/contour/contour.go	`4.16% <0.00%> (+1.38%)`	⬆️
cmd/contour/servecontext.go	`94.49% <0.00%> (+26.07%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e31f54...9c78362. Read the comment docs.

stevesloka · 2020-02-07T01:42:58Z

Thanks for this @tsaarni! I want to take a look at this soon. =)

jpeach · 2020-02-07T01:48:02Z

I'd also like to review before merging.

tsaarni · 2020-02-07T08:42:05Z

Here is hands-on instruction how I tested this on a deployed system:

Use openssl s_client to establish TLS connection and to observe the "Not Before" and "Not After" dates in the printed server certificate

kubectl -n projectcontour exec \
  $(kubectl -n projectcontour get pod -l app=envoy -o jsonpath='{.items[0].metadata.name}') -- \
  openssl s_client -connect contour:8001 -CAfile /ca/cacert.pem -cert /certs/tls.crt -key /certs/tls.key \
  | openssl x509 -text -noout

Re-create the server certificate and key for Contour and overwrite the old one with it:

kubectl -n projectcontour create secret tls contourcert \
  --cert=certs/contour.pem --key=certs/contour-key.pem \
  --dry-run -o yaml \
  | kubectl apply -f -

Wait for about 2 minutes for kubelet to update the secrets to the filesystem
Run openssl s_client again to observe that the "Not Before" and "Not After" validity times were updated
Do steps 1-4 again to ensure that the process can be repeated multiple times
Observe that Envoy is still up and in good state by checking pod state and by running traffic
Delete Envoy pod to trigger new TLS connection from client
Observe that the connection was established successfully by checking Envoy pod readiness state, logs and by running traffic

In this sequence I assume that in step 2. the server certificate was re-created with the same old CA i.e. CA was not re-created.

youngnick

Thanks for this. This LGTM, I like the use of testdata for the testing certs.

Also, thanks for the detailed testing-this-PR steps, that saved a lot of time in review.

jpeach

I agree that this looks pretty good. I had a few small nits, but very happy with the tests here.

We should be sure that we are OK with reloading the certificates on every TLS connection, and file a separate issue to make sure that we document this as part of the certificate rotation guide.

jpeach · 2020-02-10T04:49:57Z

cmd/contour/servecontext.go

@@ -221,6 +221,7 @@ func (ctx *serveContext) tlsconfig() *tls.Config {
 	err := ctx.verifyTLSFlags()
 	check(err)

+	// load certificates and key to catch configuration errors early


Nit: Comments should be full sentences with punctuation (here and in other files):

// Load certificates and key to catch configuration errors early.

Sure, I have now rewritten all my comments as full sentences.

cmd/contour/servecontext.go

jpeach · 2020-02-10T05:24:27Z

cmd/contour/servecontext_test.go

+
+	// create temporary directory to store certificates and key for the server
+	configDir, err := ioutil.TempDir("", "contour-testdata-")
+	checkErr(t, err)


In this case, checkErr should call t.Fatalf. It looks like all the other uses in this file should be fatal too, so we can rename it to checkFatalErr or something, and update it.

I renamed checkErr() to checkFatalErr() and changed it to call t.Fatal.

jpeach · 2020-02-10T05:26:30Z

cmd/contour/servecontext_test.go

+			expectedServerCert:   "testdata/2/contourcert.pem",
+			expectError:          false,
+		},
+		"fail to connect with bad client certificate": {


IIUC, this fails because the client and server don't agree on the CA root certificate to expect? Let's update the description.

Yes exactly, I updated the description.

This change adds support for certificate rotation for XDS gRPC interface between Contour and Envoy. It is achieved by lazily loading certificates and key every time new TLS connection is established by Envoy. This change addresses only the certificate rotation in Contour (server) and similar support is needed for Envoy (client) to cover the whole use case. Signed-off-by: Tero Saarni <[email protected]>

jpeach

LGTM

stevesloka added this to the 1.2.0 milestone Feb 6, 2020

stevesloka added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 6, 2020

youngnick approved these changes Feb 10, 2020

View reviewed changes

jpeach suggested changes Feb 10, 2020

View reviewed changes

tsaarni force-pushed the certificate-rotation branch from 5447ae5 to 57a7ef8 Compare February 13, 2020 12:58

jpeach mentioned this pull request Feb 13, 2020

Evaluate caching TLS server contexts #2228

Closed

tsaarni force-pushed the certificate-rotation branch from 57a7ef8 to 9c78362 Compare February 13, 2020 20:45

jpeach mentioned this pull request Feb 13, 2020

Document Contour certificate reloading behavior #2229

Closed

jpeach approved these changes Feb 14, 2020

View reviewed changes

jpeach merged commit 639889f into projectcontour:master Feb 14, 2020

tsaarni mentioned this pull request Feb 18, 2020

Add support for rotation of xDS/gRPC certificates and keys #2143

Closed

7 tasks

stevesloka mentioned this pull request Feb 24, 2020

certs: Rotate certs in 1-year #1856

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/contour: hot-reload certificates and key #2198

cmd/contour: hot-reload certificates and key #2198

tsaarni commented Feb 5, 2020 •

edited

Loading

tsaarni commented Feb 5, 2020

codecov bot commented Feb 5, 2020 •

edited

Loading

stevesloka commented Feb 7, 2020

jpeach commented Feb 7, 2020

tsaarni commented Feb 7, 2020 •

edited

Loading

youngnick left a comment

jpeach left a comment

jpeach Feb 10, 2020

tsaarni Feb 13, 2020

jpeach Feb 10, 2020

tsaarni Feb 13, 2020

jpeach Feb 10, 2020

tsaarni Feb 13, 2020

jpeach left a comment

cmd/contour: hot-reload certificates and key #2198

cmd/contour: hot-reload certificates and key #2198

Conversation

tsaarni commented Feb 5, 2020 • edited Loading

tsaarni commented Feb 5, 2020

codecov bot commented Feb 5, 2020 • edited Loading

Codecov Report

stevesloka commented Feb 7, 2020

jpeach commented Feb 7, 2020

tsaarni commented Feb 7, 2020 • edited Loading

youngnick left a comment

Choose a reason for hiding this comment

jpeach left a comment

Choose a reason for hiding this comment

jpeach Feb 10, 2020

Choose a reason for hiding this comment

tsaarni Feb 13, 2020

Choose a reason for hiding this comment

jpeach Feb 10, 2020

Choose a reason for hiding this comment

tsaarni Feb 13, 2020

Choose a reason for hiding this comment

jpeach Feb 10, 2020

Choose a reason for hiding this comment

tsaarni Feb 13, 2020

Choose a reason for hiding this comment

jpeach left a comment

Choose a reason for hiding this comment

tsaarni commented Feb 5, 2020 •

edited

Loading

codecov bot commented Feb 5, 2020 •

edited

Loading

tsaarni commented Feb 7, 2020 •

edited

Loading