etcdmain, pkg: Support peer and client TLS auth based on SAN fields.

[jmillikin-stripe]

Etcd currently supports validating peers based on their TLS certificate's
CN field. The current best practice for creation and validation of TLS
certs is to use the Subject Alternative Name (SAN) fields instead, so that
a certificate might be issued with a unique CN and its logical
identities in the SANs.

This commit extends the peer validation logic to use Go's
(*"crypto/x509".Certificate).ValidateHostname function for name
validation, which allows SANs to be used for peer access control.

In addition, it allows name validation to be enabled on clients as well.
This is used when running Etcd behind an authenticating proxy, or as
an internal component in a larger system (like a Kubernetes master).

(See issue #8262 and PR #8616 for additional context)

[jingyih]

cc @wenjiaswe

[xiang90]

allowed-san?

[jmillikin-stripe]

When checking whether a given hostname is valid for a TLS certificate, it's typical to not mandate which particular field the name comes from. The implementation of (*x509.Certificate).VerifyHostname() uses SAN if any are set, and falls back to CN.

[xiang90]

OK. The implementation checks CN first, then checks parsed SAN, right? Can we make this clear?

[xiang90]

It is hard to infer this behavior from the flag and current help message.

[jmillikin-stripe]

I believe the verification logic is something like this:

func checkName(name string) bool {
  if len(SAN) == 0 {
    return validHostname(CN) && matches(CN, name)
  }
  for _, sanEntry := range SAN {
    if matches(sanEntry, name) {
      return true
    }
  }
  return false
}

The full behavior is described by RFC 6125 section 6.4, which is too long to describe here.

[xiang90]

@jmillikin-stripe

sorry for the long delay. this pr looks good to me. but can we change allowed-name to allowed-san or something more specific?

[wenjiaswe]

@xiang90 If I understood right, x509's logic is, if a SSL Certificate has a Subject Alternative Name (SAN) field, then SSL clients are supposed to ignore the Common Name value and seek a match in the SAN list. As in RFC 6125, Appendix B.2.:

   If a subjectAltName extension of type dNSName is present, that MUST
   be used as the identity.  Otherwise, the (most specific) Common Name
   field in the Subject field of the certificate MUST be used.  Although
   the use of the Common Name is existing practice, it is deprecated and
   Certification Authorities are encouraged to use the dNSName instead.

I think in this PR, only one of --peer-cert-allowed-cn and --peer-cert-allowed-name can be set at a time. If --peer-cert-allowed-cn is set, then CommonName is used and it follows the old behavior, if new --peer-cert-allowed-name is used, then it follows "current best practice" to check SAN and ignore CN if SAN is set, otherwise check CN. @jmillikin-stripe did I understand correctly?

If that's the case, maybe we could use --peer-cert-allowed-hostname instead of --peer-cert-allowed-name to match (*x509.Certificate).VerifyHostname() function name?

[jmillikin-stripe]

@wenjiaswe Yep, that all matches my understanding. I'd be fine with renaming the new flags to -hostname.

[jmillikin-stripe]

Stray debugging prints removed, sorry about that.

[jmillikin-stripe]

The test failure in ["TARGET=linux-amd64-integration-4-cpu"] looks flaky -- can an etcd maintainer re-try that test?

[jingyih]

The test failure in ["TARGET=linux-amd64-integration-4-cpu"] looks flaky -- can an etcd maintainer re-try that test?

=== RUN TestBalancerUnderBlackholeKeepAliveWatch
--- FAIL: TestBalancerUnderBlackholeKeepAliveWatch (5.73s)
black_hole_test.go:83: took too long to receive watch events

This is a known flaky test, do not worry about it.

[jingyih]

The semaphoreci tests timed out after 20m. Can you rebase to master? It was adjusted to 30m.

[xiang90]

LGTM if all failures are known.

[jmillikin-stripe]

The semaphore test is consistently failing with a timeout in the new test. Please don't merge until I can debug it.

[codecov-io]

Codecov Report

Merging #10614 into master will decrease coverage by 1.03%.
The diff coverage is 23.07%.

@@            Coverage Diff             @@
##           master   #10614      +/-   ##
==========================================
- Coverage   63.35%   62.31%   -1.04%     
==========================================
  Files         398      398              
  Lines       37496    37507      +11     
==========================================
- Hits        23754    23371     -383     
- Misses      12144    12555     +411     
+ Partials     1598     1581      -17

Impacted Files	Coverage Δ
etcdmain/config.go	82.88% <100%> (+0.15%)	⬆️
pkg/transport/listener.go	49.54% <9.09%> (-1.64%)	⬇️
clientv3/balancer/grpc1.7-health.go	5.23% <0%> (-53.78%)	⬇️
client/client.go	42.81% <0%> (-41.18%)	⬇️
client/keys.go	72.86% <0%> (-18.6%)	⬇️
auth/store.go	45.83% <0%> (-13.96%)	⬇️
clientv3/namespace/watch.go	87.87% <0%> (-6.07%)	⬇️
clientv3/leasing/cache.go	87.22% <0%> (-4.45%)	⬇️
proxy/grpcproxy/watcher.go	89.79% <0%> (-4.09%)	⬇️
proxy/grpcproxy/register.go	80.55% <0%> (-2.78%)	⬇️
... and 18 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 eb7dd97...95f3138. Read the comment docs.

[jmillikin-stripe]

SemaphoreCI is green now. I've discovered an interesting behavior in the Go TLS library: the error message returned from a failed handshake is not deterministic. When both sides of a TLS handshake distrust the other, the error races between "client certificate authentication failed" and "remote error: tls: bad certificate" depending on which side sends a NACK frame.

The test for CN tends toward the second error, and the new test tends toward the first.

[jmillikin-stripe]

Gentle ping

[jmillikin-stripe]

Thanks!