server: Support fsnotify reloading of certs #6415

charlieegan3 · 2023-11-15T18:30:40Z

Fixes #5788

This PR makes the following changes:

Adds a new WithTLSConfig option on the server to support the setting of a cert, key and ca cert at the same time. Previously, it was only possible to set the cert and key, in addition to a polling duration with WithCertificatePaths.
Adds a test for this in TestFSNotifyCertReloading and the interval reloading in TestIntervalCertReloading. These tests are really verbose, and the cert fixture creation might be best if DRYed up some.
Makes it possible to reload the CA certs when the pool file changes on disk. Previously, this was static post init.
Retains the WithCertificate and WithCertPool runtime options as well as the polling behaviour if the interval is set in certLoopPolling

server/certs.go

tjons · 2023-11-17T03:42:09Z

I was interested in this issue so read over your solution and it seems quite well done! one question about a comment

charlieegan3 · 2023-11-20T16:47:18Z

Thanks for the review @tjons! 🙂

runtime/runtime.go

ashutosh-narkar

@charlieegan3 it's a bit difficult to understand the individual changes. Would it be a good idea to create separate PRs unless these changes are related to each other in some way?

charlieegan3 · 2023-11-21T17:31:58Z

Hey @ashutosh-narkar, I guess that if we need to break this down the parts might be:

Adding WithTLSConfig, to support the setting of a ca pool file path on the server that it can then refresh alongside existing polling update of certs and keys.
adding the fsnotify based implementation of the reloading of these files.

They aren't really co-dependent, but I'd rather work to make sure that the changes are easier to understand. I have had a go with some changes to the impl and comments in a621e2b. Hope that helps.

johanfylling

Just a couple of thoughts/questions.

server/certs.go

server/server.go

server/server_test.go

netlify · 2023-11-30T10:12:54Z

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	`054a125`
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/65685fe9a6f08a0008cb31ab
😎 Deploy Preview	https://deploy-preview-6415--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2023-11-30T10:45:55Z

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	`d08398b`
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/657871006490240008aa9395
😎 Deploy Preview	https://deploy-preview-6415--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

charlieegan3

Thanks for the review @johanfylling - think I've got them all now

server/certs.go

server/server.go

server/server_test.go

johanfylling

LGTM 👍

johanfylling · 2023-12-07T16:11:53Z

server/server_test.go

@@ -4852,6 +4863,747 @@ func TestDistributedTracingEnabled(t *testing.T) {
 	}
 }

+func TestCertPoolReloading(t *testing.T) {


I would add another round to this where we change the client CA again; picking up a change nothing->something might be different to picking up a change something->something else.

Thanks for the review, I can get back to this next week and hopefully we can get this in! 🎉

d08398b should do the trick, lmk if that looks good to you.

Reload certs, keys and optionally the CA cert pool when they change on disk. Signed-off-by: Charlie Egan <charlie@styra.com>

Changes to improve readability. Signed-off-by: Charlie Egan <charlie@styra.com>

Signed-off-by: Charlie Egan <charlie@styra.com>

This updates the CA reloading test to check that both switching the CA cert, and placing the CA cert in the first place both behave correctly. Signed-off-by: Charlie Egan <charlie@styra.com>

ashutosh-narkar

LGTM

ashutosh-narkar · 2023-12-12T18:56:59Z

@charlieegan3 please squash your commits and merge. Thanks!

This was missed in open-policy-agent#6415 https://github.com/open-policy-agent/opa/actions/runs/7194437460/job/19595009978?pr=6476 We have a warning of this data race here: ``` ================== WARNING: DATA RACE Write at 0x00c009b562e8 by goroutine 1815: github.com/open-policy-agent/opa/server.(*Server).reloadTLSConfig() /src/server/certs.go:65 +0x608 github.com/open-policy-agent/opa/server.(*Server).getListener.(*Server).certLoopNotify.func2() /src/server/certs.go:174 +0x434 github.com/open-policy-agent/opa/server.TestCertPoolReloading.func1() /src/server/server_test.go:5105 +0x4f github.com/open-policy-agent/opa/server.TestCertPoolReloading.func2() /src/server/server_test.go:5108 +0x41 Previous read at 0x00c009b562e8 by goroutine 1824: github.com/open-policy-agent/opa/server.(*Server).getListenerForHTTPSServer.func1() /src/server/server.go:649 +0x145 crypto/tls.(*Conn).readClientHello() /usr/local/go/src/crypto/tls/handshake_server.go:149 +0x97d crypto/tls.(*Conn).serverHandshake() /usr/local/go/src/crypto/tls/handshake_server.go:42 +0x64 crypto/tls.(*Conn).serverHandshake-fm() <autogenerated>:1 +0x47 crypto/tls.(*Conn).handshakeContext() /usr/local/go/src/crypto/tls/conn.go:1552 +0x615 crypto/tls.(*Conn).HandshakeContext() /usr/local/go/src/crypto/tls/conn.go:1492 +0x16b8 net/http.(*conn).serve() /usr/local/go/src/net/http/server.go:1891 +0x16c0 net/http.(*Server).Serve.func3() /usr/local/go/src/net/http/server.go:3086 +0x4f ``` Signed-off-by: Charlie Egan <charlie@styra.com>

This was missed in #6415 https://github.com/open-policy-agent/opa/actions/runs/7194437460/job/19595009978?pr=6476 We have a warning of this data race here: ``` ================== WARNING: DATA RACE Write at 0x00c009b562e8 by goroutine 1815: github.com/open-policy-agent/opa/server.(*Server).reloadTLSConfig() /src/server/certs.go:65 +0x608 github.com/open-policy-agent/opa/server.(*Server).getListener.(*Server).certLoopNotify.func2() /src/server/certs.go:174 +0x434 github.com/open-policy-agent/opa/server.TestCertPoolReloading.func1() /src/server/server_test.go:5105 +0x4f github.com/open-policy-agent/opa/server.TestCertPoolReloading.func2() /src/server/server_test.go:5108 +0x41 Previous read at 0x00c009b562e8 by goroutine 1824: github.com/open-policy-agent/opa/server.(*Server).getListenerForHTTPSServer.func1() /src/server/server.go:649 +0x145 crypto/tls.(*Conn).readClientHello() /usr/local/go/src/crypto/tls/handshake_server.go:149 +0x97d crypto/tls.(*Conn).serverHandshake() /usr/local/go/src/crypto/tls/handshake_server.go:42 +0x64 crypto/tls.(*Conn).serverHandshake-fm() <autogenerated>:1 +0x47 crypto/tls.(*Conn).handshakeContext() /usr/local/go/src/crypto/tls/conn.go:1552 +0x615 crypto/tls.(*Conn).HandshakeContext() /usr/local/go/src/crypto/tls/conn.go:1492 +0x16b8 net/http.(*conn).serve() /usr/local/go/src/net/http/server.go:1891 +0x16c0 net/http.(*Server).Serve.func3() /usr/local/go/src/net/http/server.go:3086 +0x4f ``` Signed-off-by: Charlie Egan <charlie@styra.com>

szuecs · 2024-01-18T16:19:05Z

server/certs.go

+
+		for evt := range watcher.Events {
+			removalMask := fsnotify.Remove | fsnotify.Rename
+			mask := fsnotify.Create | fsnotify.Write | removalMask


You are aware that this will trigger partial writes:

// The pathname was written to; this does *not* mean the write has finished, // and a write can be followed by more writes. Write

One other thing that I figured out when I did https://pkg.go.dev/github.com/zalando/skipper/secrets#SecretPaths some years ago is that in Kubernetes secret mounts you have symlinks and at least inotify does not work for symlinks as expected. A symlink does not change, if only the content of the targeted file change.

Hey, thanks for this. Since a partial write would result in OPA failing to reload the file (as it'd be invalid in reloadCertificateKeyPair), the main concern here is missing a write, is that correct?

Were you able to mitigate this in skipper without polling? Perhaps we could have OPA fallback to a polling behaviour when the file in question is a symlink?

We do just polling every minute or 30s (something like this). Keep it simple. :)

Fair enough, I'll need to do some digging by the sounds of things!

Yes, no worries I just wanted to hint that it might not work as expected in all environments.

charlieegan3 mentioned this pull request Nov 15, 2023

Support fsnotify based reloading of certificate, key and CA certs #5788

Closed

charlieegan3 force-pushed the fsnotify-reloading-certs branch 8 times, most recently from cba4dea to 849372f Compare November 16, 2023 18:31

tjons reviewed Nov 17, 2023

View reviewed changes

server/certs.go Outdated Show resolved Hide resolved

charlieegan3 force-pushed the fsnotify-reloading-certs branch from 849372f to ccceb16 Compare November 20, 2023 16:46

ashutosh-narkar reviewed Nov 20, 2023

View reviewed changes

runtime/runtime.go Show resolved Hide resolved

ashutosh-narkar reviewed Nov 20, 2023

View reviewed changes

charlieegan3 force-pushed the fsnotify-reloading-certs branch from ccceb16 to a2fbe36 Compare November 21, 2023 17:14

johanfylling reviewed Nov 24, 2023

View reviewed changes

server/certs.go Outdated Show resolved Hide resolved

server/certs.go Outdated Show resolved Hide resolved

server/certs.go Show resolved Hide resolved

server/server.go Show resolved Hide resolved

server/server_test.go Show resolved Hide resolved

charlieegan3 force-pushed the fsnotify-reloading-certs branch 2 times, most recently from 054a125 to 1b45886 Compare November 30, 2023 10:12

charlieegan3 force-pushed the fsnotify-reloading-certs branch from 22529a1 to fa5395c Compare November 30, 2023 11:06

charlieegan3 commented Nov 30, 2023

View reviewed changes

server/certs.go Outdated Show resolved Hide resolved

server/certs.go Show resolved Hide resolved

server/certs.go Outdated Show resolved Hide resolved

server/server.go Show resolved Hide resolved

server/server_test.go Show resolved Hide resolved

charlieegan3 force-pushed the fsnotify-reloading-certs branch 2 times, most recently from d1ada1f to 39f6a8e Compare November 30, 2023 17:48

charlieegan3 force-pushed the fsnotify-reloading-certs branch from 39f6a8e to 1ec66e3 Compare December 7, 2023 15:41

johanfylling previously approved these changes Dec 7, 2023

View reviewed changes

charlieegan3 added 2 commits December 12, 2023 14:25

server: fsnotify based cert reloading

0007b74

Reload certs, keys and optionally the CA cert pool when they change on disk. Signed-off-by: Charlie Egan <charlie@styra.com>

server: Refactor TLS config loading

68fe92d

Changes to improve readability. Signed-off-by: Charlie Egan <charlie@styra.com>

charlieegan3 added 6 commits December 12, 2023 14:25

server: Allow partial TLS config updates

be85e04

Signed-off-by: Charlie Egan <charlie@styra.com>

server: Use GetConfigForClient

5ac58a7

Signed-off-by: Charlie Egan <charlie@styra.com>

server: explain tlsconfig fields

1bdb870

Signed-off-by: Charlie Egan <charlie@styra.com>

Add test for reloading of CA certs

d6db327

Signed-off-by: Charlie Egan <charlie@styra.com>

Allow bad cert or unknown authority error

de09109

Signed-off-by: Charlie Egan <charlie@styra.com>

server: test that ca cert is reloaded

d08398b

This updates the CA reloading test to check that both switching the CA cert, and placing the CA cert in the first place both behave correctly. Signed-off-by: Charlie Egan <charlie@styra.com>

charlieegan3 dismissed johanfylling’s stale review via d08398b December 12, 2023 14:40

charlieegan3 force-pushed the fsnotify-reloading-certs branch from 1ec66e3 to d08398b Compare December 12, 2023 14:40

ashutosh-narkar approved these changes Dec 12, 2023

View reviewed changes

charlieegan3 merged commit a307ec4 into open-policy-agent:main Dec 13, 2023
24 checks passed

charlieegan3 deleted the fsnotify-reloading-certs branch December 13, 2023 08:53

charlieegan3 mentioned this pull request Dec 13, 2023

server: Use tlsConfigMtx when accessing certpool #6477

Merged

szuecs reviewed Jan 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: Support fsnotify reloading of certs #6415

server: Support fsnotify reloading of certs #6415

charlieegan3 commented Nov 15, 2023

tjons commented Nov 17, 2023

charlieegan3 commented Nov 20, 2023

ashutosh-narkar left a comment

charlieegan3 commented Nov 21, 2023

johanfylling left a comment

netlify bot commented Nov 30, 2023

netlify bot commented Nov 30, 2023 •

edited

charlieegan3 left a comment

johanfylling left a comment

johanfylling Dec 7, 2023

charlieegan3 Dec 7, 2023

charlieegan3 Dec 12, 2023

ashutosh-narkar left a comment

ashutosh-narkar commented Dec 12, 2023

szuecs Jan 18, 2024

charlieegan3 Jan 18, 2024

szuecs Jan 18, 2024

charlieegan3 Jan 18, 2024

szuecs Jan 19, 2024

server: Support fsnotify reloading of certs #6415

server: Support fsnotify reloading of certs #6415

Conversation

charlieegan3 commented Nov 15, 2023

tjons commented Nov 17, 2023

charlieegan3 commented Nov 20, 2023

ashutosh-narkar left a comment

Choose a reason for hiding this comment

charlieegan3 commented Nov 21, 2023

johanfylling left a comment

Choose a reason for hiding this comment

netlify bot commented Nov 30, 2023

✅ Deploy Preview for openpolicyagent ready!

netlify bot commented Nov 30, 2023 • edited

✅ Deploy Preview for openpolicyagent ready!

charlieegan3 left a comment

Choose a reason for hiding this comment

johanfylling 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

ashutosh-narkar left a comment

Choose a reason for hiding this comment

ashutosh-narkar commented Dec 12, 2023

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

netlify bot commented Nov 30, 2023 •

edited