Skip to content
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

Merged
merged 11 commits into from Apr 16, 2018

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Apr 13, 2018

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:

{
  "hosts": [
    "127.0.0.1"
  ],

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 empty ServerName 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.

gyuho and others added 7 commits April 13, 2018 11:16
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
@roboll
Copy link
Contributor

roboll commented Apr 13, 2018

Thanks @gyuho - I will run this build on Monday and verify.

@codecov-io
Copy link

Codecov Report

Merging #9570 into master will decrease coverage by 0.29%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
pkg/transport/listener.go 60.32% <100%> (-0.22%) ⬇️
integration/cluster.go 82% <80%> (-0.07%) ⬇️
etcdctl/ctlv2/command/backup_command.go 20.27% <0%> (-47.3%) ⬇️
clientv3/txn.go 73.33% <0%> (-26.67%) ⬇️
pkg/transport/timeout_conn.go 60% <0%> (-20%) ⬇️
proxy/grpcproxy/register.go 69.44% <0%> (-11.12%) ⬇️
pkg/tlsutil/tlsutil.go 86.2% <0%> (-6.9%) ⬇️
auth/simple_token.go 87.03% <0%> (-6.49%) ⬇️
clientv3/namespace/watch.go 72.72% <0%> (-6.07%) ⬇️
proxy/grpcproxy/watcher.go 85.71% <0%> (-4.09%) ⬇️
... and 22 more

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 08641dd...0b0a943. Read the comment docs.

@roboll
Copy link
Contributor

roboll commented Apr 15, 2018

👍 Working as expected.

@gyuho gyuho merged commit ff6ff9d into etcd-io:master Apr 16, 2018
@gyuho gyuho deleted the tls branch April 16, 2018 15:02
@roboll
Copy link
Contributor

roboll commented Apr 16, 2018

@gyuho when can we expect a release that incorporates this?

@gyuho
Copy link
Contributor Author

gyuho commented Apr 16, 2018

@roboll Yes, we will release this in next 3.2 and 3.3. Hopefully, by next week. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants