-
Notifications
You must be signed in to change notification settings - Fork 9.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
*: fix TLS reload when cert includes only IPs (no domain names in SAN field) #9570
Conversation
Signed-off-by: Gyuho Lee <[email protected]>
Signed-off-by: Gyuho Lee <[email protected]>
Signed-off-by: Gyuho Lee <[email protected]>
Signed-off-by: Gyuho Lee <[email protected]>
Signed-off-by: Gyuho Lee <[email protected]>
This reverts commit 3d3ba8b.
Signed-off-by: Gyuho Lee <[email protected]>
Signed-off-by: Gyuho Lee <[email protected]>
Signed-off-by: Gyuho Lee <[email protected]>
Signed-off-by: Gyuho Lee <[email protected]>
Thanks @gyuho - I will run this build on Monday and verify. |
Codecov Report
@@ Coverage Diff @@
## master #9570 +/- ##
=========================================
- Coverage 72.45% 72.15% -0.3%
=========================================
Files 368 368
Lines 31363 31371 +8
=========================================
- Hits 22725 22637 -88
- Misses 6992 7089 +97
+ Partials 1646 1645 -1
Continue to review full report at Codecov.
|
👍 Working as expected. |
@gyuho when can we expect a release that incorporates this? |
@roboll Yes, we will release this in next 3.2 and 3.3. Hopefully, by next week. Thanks! |
This fixes TLS reload when certificate SAN field only includes IP addresses but no domain names. For example, a member is set up with CSRs (with
cfssl
) as below:In Go, server calls
(*tls.Config).GetCertificate
for TLS reload if and only if server's(*tls.Config).Certificates
field is not empty, or(*tls.ClientHelloInfo).ServerName
is not empty with a valid SNI from the client. Previously, etcd always populates(*tls.Config).Certificates
on the initial client TLS handshake, as non-empty. Thus, client was always expected to supply a matching SNI in order to pass the TLS verification and to trigger(*tls.Config).GetCertificate
to reload TLS assets.However, a certificate whose SAN field does not include any domain names but only IP addresses would request
*tls.ClientHelloInfo
with an emptyServerName
field, thus failing to trigger the TLS reload on initial TLS handshake; this becomes a problem when expired certificates need to be replaced online.Now,
(*tls.Config).Certificates
is created empty on initial TLS client handshake, first to trigger(*tls.Config).GetCertificate
and populate rest of the certificates on every new TLS connection, even when client SNI is empty (e.g. cert only includes IPs).Fix commit was authored by @roboll #9542, and I am adding related integration test fixes and documentation.