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

etcdserver: config flag to skip verification of peer client address #10524

Merged
merged 4 commits into from
Jul 30, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG-3.4.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ See [code changes](https://github.com/etcd-io/etcd/compare/v3.3.0...v3.4.0) and
- Add [`Verify` function to perform corruption check on WAL contents](https://github.com/etcd-io/etcd/pull/10603).
- Improve [heartbeat send failure logging](https://github.com/etcd-io/etcd/pull/10663).
- Support [users with no password](https://github.com/etcd-io/etcd/pull/9817) for reducing security risk introduced by leaked password. The users can only be authenticated with CommonName based auth.
- Add flag `--experimental-peer-skip-client-san-verification` to [skip verification of peer client address](https://github.com/etcd-io/etcd/pull/10524)

### Breaking Changes

Expand Down
9 changes: 9 additions & 0 deletions Documentation/op-guide/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -457,3 +457,12 @@ Follow the instructions when using these flags.
[tuning]: ../tuning.md#time-parameters
[sample-config-file]: ../../etcd.conf.yml.sample
[recovery]: recovery.md#disaster-recovery

### --experimental-peer-skip-client-san-verification
+ Skip verification of SAN field in client certificate for peer connections. This can be helpful e.g. if
cluster members run in different networks behind a NAT.

In this case make sure to use peer certificates based on
a private certificate authority using `--peer-cert-file`, `--peer-key-file`, `--peer-trusted-ca-file`
+ default: false
+ env variable: ETCD_EXPERIMENTAL_PEER_SKIP_CLIENT_SAN_VERIFICATION
1 change: 1 addition & 0 deletions etcdmain/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ func newConfig() *config {
fs.StringVar(&cfg.ec.PeerTLSInfo.AllowedCN, "peer-cert-allowed-cn", "", "Allowed CN for inter peer authentication.")
fs.StringVar(&cfg.ec.PeerTLSInfo.AllowedHostname, "peer-cert-allowed-hostname", "", "Allowed TLS hostname for inter peer authentication.")
fs.Var(flags.NewStringsValue(""), "cipher-suites", "Comma-separated list of supported TLS cipher suites between client/server and peers (empty will be auto-populated by Go).")
fs.BoolVar(&cfg.ec.PeerTLSInfo.SkipClientSANVerify, "experimental-peer-skip-client-san-verification", false, "Skip verification of SAN field in client certificate for peer connections.")

fs.Var(
flags.NewUniqueURLsWithExceptions("*", "*"),
Expand Down
20 changes: 12 additions & 8 deletions pkg/transport/listener.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,16 +56,20 @@ func wrapTLS(scheme string, tlsinfo *TLSInfo, l net.Listener) (net.Listener, err
if scheme != "https" && scheme != "unixs" {
return l, nil
}
if tlsinfo != nil && tlsinfo.SkipClientSANVerify {
return NewTLSListener(l, tlsinfo)
}
return newTLSListener(l, tlsinfo, checkSAN)
}

type TLSInfo struct {
CertFile string
KeyFile string
TrustedCAFile string
ClientCertAuth bool
CRLFile string
InsecureSkipVerify bool
CertFile string
KeyFile string
TrustedCAFile string
ClientCertAuth bool
CRLFile string
InsecureSkipVerify bool
SkipClientSANVerify bool

// ServerName ensures the cert matches the given host in case of discovery / virtual hosting
ServerName string
Expand Down Expand Up @@ -109,7 +113,7 @@ func (info TLSInfo) Empty() bool {
return info.CertFile == "" && info.KeyFile == ""
}

func SelfCert(lg *zap.Logger, dirpath string, hosts []string) (info TLSInfo, err error) {
func SelfCert(lg *zap.Logger, dirpath string, hosts []string, additionalUsages ...x509.ExtKeyUsage) (info TLSInfo, err error) {
if err = os.MkdirAll(dirpath, 0700); err != nil {
return
}
Expand Down Expand Up @@ -145,7 +149,7 @@ func SelfCert(lg *zap.Logger, dirpath string, hosts []string) (info TLSInfo, err
NotAfter: time.Now().Add(365 * (24 * time.Hour)),

KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature,
ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
ExtKeyUsage: append([]x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, additionalUsages...),
BasicConstraintsValid: true,
}

Expand Down
111 changes: 108 additions & 3 deletions pkg/transport/listener_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@ package transport

import (
"crypto/tls"
"crypto/x509"
"errors"
"io/ioutil"
"net"
"net/http"
"os"
"testing"
Expand All @@ -26,12 +28,16 @@ import (
"go.uber.org/zap"
)

func createSelfCert() (*TLSInfo, func(), error) {
func createSelfCert(hosts ...string) (*TLSInfo, func(), error) {
return createSelfCertEx("127.0.0.1")
}

func createSelfCertEx(host string, additionalUsages ...x509.ExtKeyUsage) (*TLSInfo, func(), error) {
d, terr := ioutil.TempDir("", "etcd-test-tls-")
if terr != nil {
return nil, nil, terr
}
info, err := SelfCert(zap.NewExample(), d, []string{"127.0.0.1"})
info, err := SelfCert(zap.NewExample(), d, []string{host + ":0"}, additionalUsages...)
if err != nil {
return nil, nil, err
}
Expand Down Expand Up @@ -72,7 +78,106 @@ func testNewListenerTLSInfoAccept(t *testing.T, tlsInfo TLSInfo) {
}
defer conn.Close()
if _, ok := conn.(*tls.Conn); !ok {
t.Errorf("failed to accept *tls.Conn")
t.Error("failed to accept *tls.Conn")
}
}

// TestNewListenerTLSInfoSkipClientSANVerify tests that if client IP address mismatches
// with specified address in its certificate the connection is still accepted
// if the flag SkipClientSANVerify is set (i.e. checkSAN() is disabled for the client side)
func TestNewListenerTLSInfoSkipClientSANVerify(t *testing.T) {
tests := []struct {
skipClientSANVerify bool
goodClientHost bool
acceptExpected bool
}{
{false, true, true},
{false, false, false},
{true, true, true},
{true, false, true},
}
for _, test := range tests {
testNewListenerTLSInfoClientCheck(t, test.skipClientSANVerify, test.goodClientHost, test.acceptExpected)
}
}

func testNewListenerTLSInfoClientCheck(t *testing.T, skipClientSANVerify, goodClientHost, acceptExpected bool) {
tlsInfo, del, err := createSelfCert()
if err != nil {
t.Fatalf("unable to create cert: %v", err)
}
defer del()

host := "127.0.0.222"
if goodClientHost {
host = "127.0.0.1"
}
clientTLSInfo, del2, err := createSelfCertEx(host, x509.ExtKeyUsageClientAuth)
if err != nil {
t.Fatalf("unable to create cert: %v", err)
}
defer del2()

tlsInfo.SkipClientSANVerify = skipClientSANVerify
tlsInfo.TrustedCAFile = clientTLSInfo.CertFile

rootCAs := x509.NewCertPool()
loaded, err := ioutil.ReadFile(tlsInfo.CertFile)
if err != nil {
t.Fatalf("unexpected missing certfile: %v", err)
}
rootCAs.AppendCertsFromPEM(loaded)

clientCert, err := tls.LoadX509KeyPair(clientTLSInfo.CertFile, clientTLSInfo.KeyFile)
if err != nil {
t.Fatalf("unable to create peer cert: %v", err)
}

tlsConfig := &tls.Config{}
tlsConfig.InsecureSkipVerify = false
tlsConfig.Certificates = []tls.Certificate{clientCert}
tlsConfig.RootCAs = rootCAs

ln, err := NewListener("127.0.0.1:0", "https", tlsInfo)
if err != nil {
t.Fatalf("unexpected NewListener error: %v", err)
}
defer ln.Close()

tr := &http.Transport{TLSClientConfig: tlsConfig}
cli := &http.Client{Transport: tr}
chClientErr := make(chan error)
go func() {
_, err := cli.Get("https://" + ln.Addr().String())
chClientErr <- err
}()

chAcceptErr := make(chan error)
chAcceptConn := make(chan net.Conn)
go func() {
conn, err := ln.Accept()
if err != nil {
chAcceptErr <- err
} else {
chAcceptConn <- conn
}
}()

select {
case <-chClientErr:
if acceptExpected {
t.Errorf("accepted for good client address: skipClientSANVerify=%t, goodClientHost=%t", skipClientSANVerify, goodClientHost)
}
case acceptErr := <-chAcceptErr:
t.Fatalf("unexpected Accept error: %v", acceptErr)
case conn := <-chAcceptConn:
defer conn.Close()
if _, ok := conn.(*tls.Conn); !ok {
t.Errorf("failed to accept *tls.Conn")
MartinWeindel marked this conversation as resolved.
Show resolved Hide resolved
}
if !acceptExpected {
t.Errorf("accepted for bad client address: skipClientSANVerify=%t, goodClientHost=%t", skipClientSANVerify, goodClientHost)
}
}
}

Expand Down