ROX-33569: Rotate TLS session ticket keys on TLS certificate rotation#19384
ROX-33569: Rotate TLS session ticket keys on TLS certificate rotation#19384
Conversation
|
Images are ready for the commit at 74b8748. To use with deploy scripts, first |
| } | ||
|
|
||
| // rotateSessionTicketKeys generates and sets new session ticket keys for the TLS config. | ||
| // Note: Calling SetSessionTicketKeys disables Go's automatic 24-hour session ticket key rotation. |
There was a problem hiding this comment.
Please note that automatic session ticket rotation is effectively disabled at the current implementaion. The cloned config does not automatically updates ticket keys. See PR description for more details
| errNoTLSConfig = errors.New("no TLS config is available") | ||
|
|
||
| // sessionTicketKeyRotator is the function used to rotate session ticket keys. Used for testing. | ||
| sessionTicketKeyRotator = rotateSessionTicketKeys |
There was a problem hiding this comment.
Due to Go's TLS API limitation. There is no easy way to test that SessionTicketKeys are updated. Hence validate that the update function is called at least
|
/retest |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19384 +/- ##
==========================================
- Coverage 49.64% 49.63% -0.02%
==========================================
Files 2698 2701 +3
Lines 203075 203233 +158
==========================================
+ Hits 100822 100879 +57
- Misses 94732 94827 +95
- Partials 7521 7527 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@kurlov: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
vladbologa
left a comment
There was a problem hiding this comment.
LGTM.
I think this PR introduces a subtle side effect that could impact long running existing connections, but I'd say it's acceptable.
Consider this scenario:
- long running Sensor - Central connection (more than 6 months)
- Sensor certificates expired and it didn't reload them (refresh occurs 6 months in advance, but a pod restart is required to load new certs)
- Central updates its TLS info and refreshes its session keys -> re authentication will now fail
So a client with expired certs would be disconnected, whereas before it kept running until the connection is broken and has to be re-established. Basically it surfaces a problem that was silently hiding. I'm not sure how likely this is to happen, but I thought it's worth mentioning.
Description
When Central's default TLS certificate is rotated the certificate file watcher correctly detects the change and loads the new certificate into memory. However, a browser which typically caches TLS sessions continue to see the old certificate because TLS session ticket keys are not rotated along with the certificate.
The problem is that TLS Session ticket keys are static:
Clone()the config, it copies these static keysOriginally I've experienced this issue when rotated certificate for the integration ACSCS. The browser still see the old certificate even after two days. Basically, I had to invalidate browser cache or close all tabs and wait for connection timeout to receive a new certificate.
Refreshing TLS Session ticket keys along with certificate rotation should prevent the above issue.
A sign of another potential issue
By default TLS Session ticket keys should be rotated every 24 hours. However since I saw the old certificate after two days it means the automatical rotation of session ticket keys does not work. If look at crypto/tls
/common.go ticketKeys the cloned config will have static session ticket keys. Thus the keys will be valid indefinitely.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
Added unit test