diff --git a/pkg/genericapiserver/config_selfclient.go b/pkg/genericapiserver/config_selfclient.go index 15eaa16f741..056612e3444 100644 --- a/pkg/genericapiserver/config_selfclient.go +++ b/pkg/genericapiserver/config_selfclient.go @@ -75,24 +75,46 @@ func (s *SecureServingInfo) NewSelfClientConfig(token string) (*restclient.Confi BearerToken: token, } - // find certificate for host: either explicitly given, from the server cert bundle or one of the SNI certs + // find certificate for host: either explicitly given, from the server cert bundle or one of the SNI certs, + // but only return CA:TRUE certificates. var derCA []byte if s.CACert != nil { derCA = s.CACert.Certificate[0] } + if derCA == nil && net.ParseIP(host) == nil { + if cert, found := s.SNICerts[host]; found { + chain, err := parseChain(cert.Certificate) + if err != nil { + return nil, fmt.Errorf("failed to parse SNI certificate for host %q: %v", host, err) + } + + if trustedChain(chain) { + return clientConfig, nil + } + + ca, err := findCA(chain) + if err != nil { + return nil, fmt.Errorf("no CA certificate found in SNI server certificate bundle for host %q: %v", host, err) + } + derCA = ca.Raw + } + } if derCA == nil && s.Cert != nil { - x509Cert, err := x509.ParseCertificate(s.Cert.Certificate[0]) + chain, err := parseChain(s.Cert.Certificate) if err != nil { return nil, fmt.Errorf("failed to parse server certificate: %v", err) } - if (net.ParseIP(host) != nil && certMatchesIP(x509Cert, host)) || certMatchesName(x509Cert, host) { - derCA = s.Cert.Certificate[0] - } - } - if derCA == nil && net.ParseIP(host) == nil { - if cert, found := s.SNICerts[host]; found { - derCA = cert.Certificate[0] + if (net.ParseIP(host) != nil && certMatchesIP(chain[0], host)) || certMatchesName(chain[0], host) { + if trustedChain(chain) { + return clientConfig, nil + } + + ca, err := findCA(chain) + if err != nil { + return nil, fmt.Errorf("no CA certificate found in server certificate bundle: %v", err) + } + derCA = ca.Raw } } if derCA == nil { @@ -107,6 +129,41 @@ func (s *SecureServingInfo) NewSelfClientConfig(token string) (*restclient.Confi return clientConfig, nil } +func trustedChain(chain []*x509.Certificate) bool { + intermediates := x509.NewCertPool() + for _, cert := range chain[1:] { + intermediates.AddCert(cert) + } + _, err := chain[0].Verify(x509.VerifyOptions{ + Intermediates: intermediates, + KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + }) + return err == nil +} + +func parseChain(bss [][]byte) ([]*x509.Certificate, error) { + var result []*x509.Certificate + for _, bs := range bss { + x509Cert, err := x509.ParseCertificate(bs) + if err != nil { + return nil, err + } + result = append(result, x509Cert) + } + + return result, nil +} + +func findCA(chain []*x509.Certificate) (*x509.Certificate, error) { + for _, cert := range chain { + if cert.IsCA { + return cert, nil + } + } + + return nil, fmt.Errorf("no certificate with CA:TRUE found in chain") +} + func (s *ServingInfo) NewSelfClientConfig(token string) (*restclient.Config, error) { if s == nil { return nil, nil diff --git a/pkg/genericapiserver/serve_test.go b/pkg/genericapiserver/serve_test.go index bbdb6041c65..e3fb6bb759f 100644 --- a/pkg/genericapiserver/serve_test.go +++ b/pkg/genericapiserver/serve_test.go @@ -25,13 +25,17 @@ import ( "io/ioutil" "net" "os" + "reflect" + "strconv" "testing" "github.com/stretchr/testify/assert" + "k8s.io/kubernetes/pkg/client/clientset_generated/clientset" "k8s.io/kubernetes/pkg/genericapiserver/options" utilcert "k8s.io/kubernetes/pkg/util/cert" "k8s.io/kubernetes/pkg/util/config" + "k8s.io/kubernetes/pkg/version" ) type TestCertSpec struct { @@ -229,23 +233,26 @@ NextTest: } func TestServerRunWithSNI(t *testing.T) { - tests := []struct { + tests := map[string]struct { Cert TestCertSpec SNICerts []NamedTestCertSpec ExpectedCertIndex int // passed in the client hello info, "localhost" if unset ServerName string + + // optional ip or hostname to pass to NewSelfClientConfig + SelfClientBindAddressOverride string + ExpectSelfClientError bool }{ - { - // only one cert + "only one cert": { Cert: TestCertSpec{ host: "localhost", + ips: []string{"127.0.0.1"}, }, ExpectedCertIndex: -1, }, - { - // cert with multiple alternate names + "cert with multiple alternate names": { Cert: TestCertSpec{ host: "localhost", names: []string{"test.com"}, @@ -254,10 +261,10 @@ func TestServerRunWithSNI(t *testing.T) { ExpectedCertIndex: -1, ServerName: "test.com", }, - { - // one SNI and the default cert with the same name + "one SNI and the default cert with the same name": { Cert: TestCertSpec{ host: "localhost", + ips: []string{"127.0.0.1"}, }, SNICerts: []NamedTestCertSpec{ { @@ -268,10 +275,10 @@ func TestServerRunWithSNI(t *testing.T) { }, ExpectedCertIndex: 0, }, - { - // matching SNI cert + "matching SNI cert": { Cert: TestCertSpec{ host: "localhost", + ips: []string{"127.0.0.1"}, }, SNICerts: []NamedTestCertSpec{ { @@ -283,13 +290,12 @@ func TestServerRunWithSNI(t *testing.T) { ExpectedCertIndex: 0, ServerName: "test.com", }, - { - // matching IP in SNI cert and the server cert. But IPs must not be - // passed via SNI. Hence, the ServerName in the HELLO packet is empty - // and the server should select the non-SNI cert. + "matching IP in SNI cert and the server cert": { + // IPs must not be passed via SNI. Hence, the ServerName in the + // HELLO packet is empty and the server should select the non-SNI cert. Cert: TestCertSpec{ host: "localhost", - ips: []string{"10.0.0.1"}, + ips: []string{"10.0.0.1", "127.0.0.1"}, }, SNICerts: []NamedTestCertSpec{ { @@ -302,10 +308,10 @@ func TestServerRunWithSNI(t *testing.T) { ExpectedCertIndex: -1, ServerName: "10.0.0.1", }, - { - // wildcards + "wildcards": { Cert: TestCertSpec{ host: "localhost", + ips: []string{"127.0.0.1"}, }, SNICerts: []NamedTestCertSpec{ { @@ -318,6 +324,79 @@ func TestServerRunWithSNI(t *testing.T) { ExpectedCertIndex: 0, ServerName: "www.test.com", }, + + "loopback: IP for loopback client on SNI cert": { + Cert: TestCertSpec{ + host: "localhost", + }, + SNICerts: []NamedTestCertSpec{ + { + TestCertSpec: TestCertSpec{ + host: "test.com", + ips: []string{"127.0.0.1"}, + }, + }, + }, + ExpectedCertIndex: -1, + ExpectSelfClientError: true, + }, + "loopback: IP for loopback client on server and SNI cert": { + Cert: TestCertSpec{ + ips: []string{"127.0.0.1"}, + host: "localhost", + }, + SNICerts: []NamedTestCertSpec{ + { + TestCertSpec: TestCertSpec{ + host: "test.com", + ips: []string{"127.0.0.1"}, + }, + }, + }, + ExpectedCertIndex: -1, + }, + "loopback: bind to 0.0.0.0 => loopback uses localhost; localhost on server cert": { + Cert: TestCertSpec{ + host: "localhost", + }, + SNICerts: []NamedTestCertSpec{ + { + TestCertSpec: TestCertSpec{ + host: "test.com", + }, + }, + }, + ExpectedCertIndex: -1, + SelfClientBindAddressOverride: "0.0.0.0", + }, + "loopback: bind to 0.0.0.0 => loopback uses localhost; localhost on SNI cert": { + Cert: TestCertSpec{ + host: "test.com", + }, + SNICerts: []NamedTestCertSpec{ + { + TestCertSpec: TestCertSpec{ + host: "localhost", + }, + }, + }, + ExpectedCertIndex: 0, + SelfClientBindAddressOverride: "0.0.0.0", + }, + "loopback: bind to 0.0.0.0 => loopback uses localhost; localhost on server and SNI cert": { + Cert: TestCertSpec{ + host: "localhost", + }, + SNICerts: []NamedTestCertSpec{ + { + TestCertSpec: TestCertSpec{ + host: "localhost", + }, + }, + }, + ExpectedCertIndex: 0, + SelfClientBindAddressOverride: "0.0.0.0", + }, } tempDir, err := ioutil.TempDir("", "") @@ -327,16 +406,16 @@ func TestServerRunWithSNI(t *testing.T) { defer os.RemoveAll(tempDir) NextTest: - for i, test := range tests { + for title, test := range tests { // create server cert serverCertBundleFile, serverKeyFile, err := createTestCertFiles(tempDir, test.Cert) if err != nil { - t.Errorf("%d - failed to create server cert: %v", i, err) + t.Errorf("%q - failed to create server cert: %v", title, err) continue NextTest } ca, err := caCertFromBundle(serverCertBundleFile) if err != nil { - t.Errorf("%d - failed to extract ca cert from server cert bundle: %v", i, err) + t.Errorf("%q - failed to extract ca cert from server cert bundle: %v", title, err) continue NextTest } caCerts := []*x509.Certificate{ca} @@ -345,7 +424,7 @@ NextTest: var namedCertKeys []config.NamedCertKey serverSig, err := certFileSignature(serverCertBundleFile, serverKeyFile) if err != nil { - t.Errorf("%d - failed to get server cert signature: %v", i, err) + t.Errorf("%q - failed to get server cert signature: %v", title, err) continue NextTest } signatures := map[string]int{ @@ -354,7 +433,7 @@ NextTest: for j, c := range test.SNICerts { certBundleFile, keyFile, err := createTestCertFiles(tempDir, c.TestCertSpec) if err != nil { - t.Errorf("%d - failed to create SNI cert %d: %v", i, j, err) + t.Errorf("%q - failed to create SNI cert %d: %v", title, j, err) continue NextTest } @@ -366,7 +445,7 @@ NextTest: ca, err := caCertFromBundle(certBundleFile) if err != nil { - t.Errorf("%d - failed to extract ca cert from SNI cert %d: %v", i, j, err) + t.Errorf("%q - failed to extract ca cert from SNI cert %d: %v", title, j, err) continue NextTest } caCerts = append(caCerts, ca) @@ -374,7 +453,7 @@ NextTest: // store index in namedCertKeys with the signature as the key sig, err := certFileSignature(certBundleFile, keyFile) if err != nil { - t.Errorf("%d - failed get SNI cert %d signature: %v", i, j, err) + t.Errorf("%q - failed get SNI cert %d signature: %v", title, j, err) continue NextTest } signatures[sig] = j @@ -386,6 +465,9 @@ NextTest: etcdserver, config, _ := setUp(t) defer etcdserver.Terminate(t) + v := version.Get() + config.Version = &v + config.EnableIndex = true _, err = config.ApplySecureServingOptions(&options.SecureServingOptions{ ServingOptions: options.ServingOptions{ @@ -401,14 +483,14 @@ NextTest: SNICertKeys: namedCertKeys, }) if err != nil { - t.Errorf("%d - failed applying the SecureServingOptions: %v", i, err) + t.Errorf("%q - failed applying the SecureServingOptions: %v", title, err) continue NextTest } config.InsecureServingInfo = nil s, err := config.Complete().New() if err != nil { - t.Errorf("%d - failed creating the server: %v", i, err) + t.Errorf("%q - failed creating the server: %v", title, err) continue NextTest } @@ -416,7 +498,7 @@ NextTest: s.SecureServingInfo.BindAddress = "127.0.0.1:0" if err := s.serveSecurely(stopCh); err != nil { - t.Errorf("%d - failed running the server: %v", i, err) + t.Errorf("%q - failed running the server: %v", title, err) continue NextTest } @@ -434,7 +516,7 @@ NextTest: ServerName: test.ServerName, // used for SNI in the client HELLO packet }) if err != nil { - t.Errorf("%d - failed to connect: %v", i, err) + t.Errorf("%q - failed to connect: %v", title, err) continue NextTest } @@ -442,13 +524,44 @@ NextTest: sig := x509CertSignature(conn.ConnectionState().PeerCertificates[0]) gotCertIndex, found := signatures[sig] if !found { - t.Errorf("%d - unknown signature returned from server: %s", i, sig) + t.Errorf("%q - unknown signature returned from server: %s", title, sig) } if gotCertIndex != test.ExpectedCertIndex { - t.Errorf("%d - expected cert index %d, got cert index %d", i, test.ExpectedCertIndex, gotCertIndex) + t.Errorf("%q - expected cert index %d, got cert index %d", title, test.ExpectedCertIndex, gotCertIndex) } conn.Close() + + // check that the loopback client can connect + host := "127.0.0.1" + if len(test.SelfClientBindAddressOverride) != 0 { + host = test.SelfClientBindAddressOverride + } + config.SecureServingInfo.ServingInfo.BindAddress = net.JoinHostPort(host, strconv.Itoa(s.effectiveSecurePort)) + cfg, err := config.SecureServingInfo.NewSelfClientConfig("some-token") + if test.ExpectSelfClientError { + if err == nil { + t.Errorf("%q - expected error creating loopback client config", title) + } + continue NextTest + } + if err != nil { + t.Errorf("%q - failed creating loopback client config: %v", title, err) + continue NextTest + } + client, err := clientset.NewForConfig(cfg) + if err != nil { + t.Errorf("%q - failed to create loopback client: %v", title, err) + continue NextTest + } + got, err := client.ServerVersion() + if err != nil { + t.Errorf("%q - failed to connect with loopback client: %v", title, err) + continue NextTest + } + if expected := &v; !reflect.DeepEqual(got, expected) { + t.Errorf("%q - loopback client didn't get correct version info: expected=%v got=%v", title, expected, got) + } } }