🌱 Add support for CA/certificate rotation #1062

tmshort · 2024-07-17T21:46:32Z

Fixes #915
Mounted secrets are automatically updated into pods, but...

It doesn't work with subPath mountings
When subPath is not used, then a bunch of directories are mounted
And one of those directories is a symlink, so IsDir() returns false
And a watch is needed to notice the change

So, update the certificate volume patch, which requires a change in how we look for certificates in the CA cert directory.

Add a watch, so when the certs do change, we update the cert pool.
Also look at validity dates of certificates, and error on expired certs.

The default cert-manager certificates have 90 days validities.

Description

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2024-07-17T21:46:50Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`5eea072`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/669ac31547b423000860d4e7
😎 Deploy Preview	https://deploy-preview-1062--olmv1.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.

tmshort · 2024-07-17T21:48:37Z

This also adds a unit-test for an empty cert dir, and for an expired certificate. And also adds an e2e to make sure secret updates and rotations actually work.

codecov · 2024-07-17T21:55:20Z

Codecov Report

Attention: Patch coverage is 71.28713% with 29 lines in your changes missing coverage. Please review.

Project coverage is 72.67%. Comparing base (775613f) to head (5eea072).
Report is 2 commits behind head on main.

Files	Patch %	Lines
internal/httputil/certpoolwatcher.go	68.96%	11 Missing and 7 partials ⚠️
internal/httputil/certutil.go	78.26%	2 Missing and 3 partials ⚠️
internal/catalogmetadata/cache/cache.go	66.66%	1 Missing and 1 partial ⚠️
internal/httputil/httputil.go	60.00%	1 Missing and 1 partial ⚠️
internal/rukpak/source/image_registry.go	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1062      +/-   ##
==========================================
- Coverage   72.85%   72.67%   -0.19%     
==========================================
  Files          31       32       +1     
  Lines        1864     1965     +101     
==========================================
+ Hits         1358     1428      +70     
- Misses        371      388      +17     
- Partials      135      149      +14

Flag	Coverage Δ
e2e	`55.65% <46.53%> (+0.13%)`	⬆️
unit	`45.54% <61.38%> (+0.53%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cmd/manager/main.go

Mounted secrets are automatically updated into pods, but... * It doesn't work with `subPath` mountings * When `subPath` is not used, then a bunch of directories are mounted * And one of those directories is a symlink, so `IsDir()` returns false * And a watch is needed to notice the change So, update the certificate volume patch, which requires a change in how we look for certificates in the CA cert directory. Add a watch, so when the certs do change, we update the cert pool. Also look at validity dates of certificates, and error on expired certs. The default cert-manager certificates have 90 days validities. Signed-off-by: Todd Short <tshort@redhat.com>

cmd/manager/main.go

m1kola

I'm still reviewing.

For these interested, here are some relevant issues in Go and K8s on root CA reloading:

TLDR: it looks like there is no equivalent of GetClientCertificate/GetCertificate, but for root CAs in TLS config in standard library, but it is possible to implement a workaround with VerifyPeerCertificate. This would require setting InsecureSkipVerify to true and re-implementing the standard verification in VerifyPeerCertificate as far as I understand - see this example from Go docs.

Makefile

hack/test/cert-e2e.sh

internal/catalogmetadata/cache/cache.go

internal/httputil/certpoolwatcher.go

hack/test/cert-e2e.sh

tmshort · 2024-07-19T13:30:22Z

TLDR: it looks like there is no equivalent of GetClientCertificate/GetCertificate, but for root CAs in TLS config in standard library, but it is possible to implement a workaround with VerifyPeerCertificate. This would require setting InsecureSkipVerify to true and re-implementing the standard verification in VerifyPeerCertificate as far as I understand - see this example from Go docs.

And we really don't want to do that (set InsecureSkipVerify) by default. And reimplementing the verification code is probably not worth it, when we can just offer up the updated certificate pool on demand.

Signed-off-by: Todd Short <tshort@redhat.com>

m1kola · 2024-07-19T14:27:04Z

And we really don't want to do that (set InsecureSkipVerify) by default. And reimplementing the verification code is probably not worth it, when we can just offer up the updated certificate pool on demand.

Setting InsecureSkipVerify to true and VerifyPeerCertificate means that we take over the responsibility of verifying the certs from standard library. That's not great, but it is not a lot of code (see standard library code, and client example from the docs). It doesn't sounds too terrible to me, so I'm open to consider this.

As far as I understand in this implementation we are re-creating a client each time and will have to establish a connection each we use it which is not optimal.

I don't know how significantly it will affect our performance. One one hand - creating new connections every time is not great, but on the other hand - not sure if we are going to benefit from re-using the connection in these use cases.

Signed-off-by: Todd Short <tshort@redhat.com>

internal/httputil/certpoolwatcher.go

everettraven · 2024-07-22T14:47:15Z

internal/httputil/certpoolwatcher.go

+		for {
+			select {
+			case <-watcher.Events:
+				cpw.drainEvents()


Do you know if any events include only the directory being watched as the event.Name value? If so, I wonder if instead of performing this drainEvents action we could do some event filtering similar to https://github.com/fsnotify/fsnotify/blob/c1467c02fba575afdb5f4201072ab8403bbf00f4/cmd/fsnotify/file.go#L66-L78

I won't block the PR merging on this, but something that could make it so we don't have any "sleep" actions if it is possible

Some filtering might be useful. The only time this path should be updated is when a Secret is updated. The directory is read-only within the pod.

Note: the filter will not work if new files are added, as they will be filtered out. We need to recognize new files, deleted files, updated files, etc.

We need to recognize new files, deleted files, updated files, etc.

If we only react to "directory has been updated" type events wouldn't we catch these events as well?

But it wouldn't catch updates to files within, as that's a change to the file, not the directory.

I thought the reasoning for the drain events operation was because when we receive updates we get mass events on everything when something changed. Maybe I misunderstood, which led me to thinking that if any change happened in the directory (including an individual file), it would trigger an event for the directory as well.

The Watch is on the directory, and that includes the contents. It also depends on how things are mounted. A change to a file within a directory does not necessarily indicate a change to the directory.
The drain is there because the update of a single secret may trigger a number of events (I was seeing 4+, because of how the mounted files were presented), and only one reload of the certs is necessary.
Based on my testing, if there's an update on a file, it only reports the update on that file (i.e. create/update); it doesn't trigger a second update on the directory as well.

I added debug output to the cert watcher test for every event in the unit test:

=== RUN TestCertPoolWatcher certpoolwatcher_test.go:72: Create cert file at "/tmp/cert-pool4285276756/test1.pem" certpoolwatcher_test.go:87: Create cert file at "/tmp/cert-pool4285276756/test2.pem" Event: CREATE "/tmp/cert-pool4285276756/test2.pem" Event: WRITE "/tmp/cert-pool4285276756/test2.pem" --- PASS: TestCertPoolWatcher (1.11s)

So, there's an event for the create of the new PEM, and one for the write, but nothing on the directory itself. There are two events, and that would cause two reloads without the drain mechanism in place.

* Add support for CA/certificate rotation Mounted secrets are automatically updated into pods, but... * It doesn't work with `subPath` mountings * When `subPath` is not used, then a bunch of directories are mounted * And one of those directories is a symlink, so `IsDir()` returns false * And a watch is needed to notice the change So, update the certificate volume patch, which requires a change in how we look for certificates in the CA cert directory. Add a watch, so when the certs do change, we update the cert pool. Also look at validity dates of certificates, and error on expired certs. The default cert-manager certificates have 90 days validities. Signed-off-by: Todd Short <tshort@redhat.com> * fixup! Add support for CA/certificate rotation * fixup! Add support for CA/certificate rotation Signed-off-by: Todd Short <tshort@redhat.com> * fixup! Add support for CA/certificate rotation Signed-off-by: Todd Short <tshort@redhat.com> --------- Signed-off-by: Todd Short <tshort@redhat.com>

tmshort requested a review from a team as a code owner July 17, 2024 21:46

tmshort force-pushed the cert-rotate branch 3 times, most recently from 4450298 to 03c4c85 Compare July 18, 2024 13:21

everettraven reviewed Jul 18, 2024

View reviewed changes

cmd/manager/main.go Outdated Show resolved Hide resolved

tmshort force-pushed the cert-rotate branch 2 times, most recently from 95abebf to ab9e281 Compare July 18, 2024 16:44

tmshort force-pushed the cert-rotate branch from ab9e281 to 7965072 Compare July 18, 2024 20:20

everettraven reviewed Jul 18, 2024

View reviewed changes

cmd/manager/main.go Outdated Show resolved Hide resolved

fixup! Add support for CA/certificate rotation

e52b5b7

tmshort force-pushed the cert-rotate branch from 11f426a to e52b5b7 Compare July 18, 2024 21:46

m1kola reviewed Jul 19, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

hack/test/cert-e2e.sh Outdated Show resolved Hide resolved

internal/catalogmetadata/cache/cache.go Show resolved Hide resolved

m1kola reviewed Jul 19, 2024

View reviewed changes

internal/httputil/certpoolwatcher.go Outdated Show resolved Hide resolved

m1kola reviewed Jul 19, 2024

View reviewed changes

hack/test/cert-e2e.sh Outdated Show resolved Hide resolved

hack/test/cert-e2e.sh Outdated Show resolved Hide resolved

fixup! Add support for CA/certificate rotation

5e7beee

Signed-off-by: Todd Short <tshort@redhat.com>

fixup! Add support for CA/certificate rotation

5eea072

Signed-off-by: Todd Short <tshort@redhat.com>

tmshort force-pushed the cert-rotate branch from 2d53f85 to 5eea072 Compare July 19, 2024 19:48

bentito reviewed Jul 22, 2024

View reviewed changes

internal/httputil/certpoolwatcher.go Show resolved Hide resolved

tmshort mentioned this pull request Jul 22, 2024

Handle CA rotation for Catalogd web server trust #915

Closed

everettraven reviewed Jul 22, 2024

View reviewed changes

internal/httputil/certpoolwatcher.go Show resolved Hide resolved

everettraven reviewed Jul 22, 2024

View reviewed changes

internal/httputil/certpoolwatcher.go Show resolved Hide resolved

everettraven reviewed Jul 22, 2024

View reviewed changes

everettraven approved these changes Jul 22, 2024

View reviewed changes

tmshort added this pull request to the merge queue Jul 22, 2024

Merged via the queue into operator-framework:main with commit e3e6b03 Jul 22, 2024
17 of 19 checks passed

tmshort deleted the cert-rotate branch July 22, 2024 17:53

skattoju mentioned this pull request Sep 9, 2024

🌱 Descriptive Upgrade Constraint Policy constants #1233

Merged

skattoju mentioned this pull request Sep 25, 2024

✨ Remove unpacked status #1309

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Add support for CA/certificate rotation #1062

🌱 Add support for CA/certificate rotation #1062

tmshort commented Jul 17, 2024 •

edited

Loading

netlify bot commented Jul 17, 2024 •

edited

Loading

tmshort commented Jul 17, 2024

codecov bot commented Jul 17, 2024 •

edited

Loading

m1kola left a comment

tmshort commented Jul 19, 2024

m1kola commented Jul 19, 2024

everettraven Jul 22, 2024 •

edited

Loading

tmshort Jul 22, 2024

tmshort Jul 22, 2024

everettraven Jul 22, 2024 •

edited

Loading

tmshort Jul 22, 2024

everettraven Jul 22, 2024

tmshort Jul 22, 2024

tmshort Jul 22, 2024

🌱 Add support for CA/certificate rotation #1062

🌱 Add support for CA/certificate rotation #1062

Conversation

tmshort commented Jul 17, 2024 • edited Loading

Description

Reviewer Checklist

netlify bot commented Jul 17, 2024 • edited Loading

✅ Deploy Preview for olmv1 ready!

tmshort commented Jul 17, 2024

codecov bot commented Jul 17, 2024 • edited Loading

Codecov Report

m1kola left a comment

Choose a reason for hiding this comment

tmshort commented Jul 19, 2024

m1kola commented Jul 19, 2024

everettraven Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

tmshort Jul 22, 2024

Choose a reason for hiding this comment

tmshort Jul 22, 2024

Choose a reason for hiding this comment

everettraven Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

tmshort Jul 22, 2024

Choose a reason for hiding this comment

everettraven Jul 22, 2024

Choose a reason for hiding this comment

tmshort Jul 22, 2024

Choose a reason for hiding this comment

tmshort Jul 22, 2024

Choose a reason for hiding this comment

tmshort commented Jul 17, 2024 •

edited

Loading

netlify bot commented Jul 17, 2024 •

edited

Loading

codecov bot commented Jul 17, 2024 •

edited

Loading

everettraven Jul 22, 2024 •

edited

Loading

everettraven Jul 22, 2024 •

edited

Loading