diff --git a/pkg/server/config.go b/pkg/server/config.go index 768b03a12..45b277459 100644 --- a/pkg/server/config.go +++ b/pkg/server/config.go @@ -17,7 +17,6 @@ limitations under the License. package server import ( - "crypto/tls" "fmt" "net" "net/http" @@ -235,9 +234,8 @@ type SecureServingInfo struct { // allowed to be in SNICerts. Cert dynamiccertificates.CertKeyContentProvider - // SNICerts are the TLS certificates by name used for SNI. - // todo: use dynamic certificates - SNICerts map[string]*tls.Certificate + // SNICerts are the TLS certificates used for SNI. + SNICerts []dynamiccertificates.SNICertKeyContentProvider // ClientCA is the certificate bundle for all the signers that you'll recognize for incoming client certificates ClientCA dynamiccertificates.CAContentProvider diff --git a/pkg/server/dynamiccertificates/cert_key.go b/pkg/server/dynamiccertificates/cert_key.go index b4612c1eb..114002b1a 100644 --- a/pkg/server/dynamiccertificates/cert_key.go +++ b/pkg/server/dynamiccertificates/cert_key.go @@ -28,7 +28,14 @@ type CertKeyContentProvider interface { CurrentCertKeyContent() ([]byte, []byte) } -// caBundleContent holds the content for the cert and key +// SNICertKeyContentProvider provides a certificate and matching private key as well as optional explicit names +type SNICertKeyContentProvider interface { + CertKeyContentProvider + // SNINames provides names used for SNI. May return nil. + SNINames() []string +} + +// certKeyContent holds the content for the cert and key type certKeyContent struct { cert []byte key []byte @@ -41,3 +48,27 @@ func (c *certKeyContent) Equal(rhs *certKeyContent) bool { return bytes.Equal(c.key, rhs.key) && bytes.Equal(c.cert, rhs.cert) } + +// sniCertKeyContent holds the content for the cert and key as well as any explicit names +type sniCertKeyContent struct { + certKeyContent + sniNames []string +} + +func (c *sniCertKeyContent) Equal(rhs *sniCertKeyContent) bool { + if c == nil || rhs == nil { + return c == rhs + } + + if len(c.sniNames) != len(rhs.sniNames) { + return false + } + + for i := range c.sniNames { + if c.sniNames[i] != rhs.sniNames[i] { + return false + } + } + + return c.certKeyContent.Equal(&rhs.certKeyContent) +} diff --git a/pkg/server/dynamiccertificates/cert_key_test.go b/pkg/server/dynamiccertificates/cert_key_test.go index cec202234..5c5055705 100644 --- a/pkg/server/dynamiccertificates/cert_key_test.go +++ b/pkg/server/dynamiccertificates/cert_key_test.go @@ -16,7 +16,9 @@ limitations under the License. package dynamiccertificates -import "testing" +import ( + "testing" +) func TestCertKeyContentEquals(t *testing.T) { tests := []struct { @@ -74,3 +76,72 @@ func TestCertKeyContentEquals(t *testing.T) { }) } } + +func TestSNICertKeyContentEquals(t *testing.T) { + tests := []struct { + name string + lhs *sniCertKeyContent + rhs *sniCertKeyContent + expected bool + }{ + { + name: "both nil", + expected: true, + }, + { + name: "lhs nil", + rhs: &sniCertKeyContent{}, + expected: false, + }, + { + name: "rhs nil", + lhs: &sniCertKeyContent{}, + expected: false, + }, + { + name: "same", + lhs: &sniCertKeyContent{certKeyContent: certKeyContent{cert: []byte("foo"), key: []byte("baz")}, sniNames: []string{"a"}}, + rhs: &sniCertKeyContent{certKeyContent: certKeyContent{cert: []byte("foo"), key: []byte("baz")}, sniNames: []string{"a"}}, + expected: true, + }, + { + name: "different cert", + lhs: &sniCertKeyContent{certKeyContent: certKeyContent{cert: []byte("foo"), key: []byte("baz")}, sniNames: []string{"a"}}, + rhs: &sniCertKeyContent{certKeyContent: certKeyContent{cert: []byte("bar"), key: []byte("baz")}, sniNames: []string{"a"}}, + expected: false, + }, + { + name: "different key", + lhs: &sniCertKeyContent{certKeyContent: certKeyContent{cert: []byte("foo"), key: []byte("baz")}, sniNames: []string{"a"}}, + rhs: &sniCertKeyContent{certKeyContent: certKeyContent{cert: []byte("foo"), key: []byte("qux")}, sniNames: []string{"a"}}, + expected: false, + }, + { + name: "different cert and key", + lhs: &sniCertKeyContent{certKeyContent: certKeyContent{cert: []byte("foo"), key: []byte("baz")}, sniNames: []string{"a"}}, + rhs: &sniCertKeyContent{certKeyContent: certKeyContent{cert: []byte("bar"), key: []byte("qux")}, sniNames: []string{"a"}}, + expected: false, + }, + { + name: "different names", + lhs: &sniCertKeyContent{certKeyContent: certKeyContent{cert: []byte("foo"), key: []byte("baz")}, sniNames: []string{"a"}}, + rhs: &sniCertKeyContent{certKeyContent: certKeyContent{cert: []byte("foo"), key: []byte("baz")}, sniNames: []string{"b"}}, + expected: false, + }, + { + name: "extra names", + lhs: &sniCertKeyContent{certKeyContent: certKeyContent{cert: []byte("foo"), key: []byte("baz")}, sniNames: []string{"a"}}, + rhs: &sniCertKeyContent{certKeyContent: certKeyContent{cert: []byte("foo"), key: []byte("baz")}, sniNames: []string{"a", "b"}}, + expected: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + actual := test.lhs.Equal(test.rhs) + if actual != test.expected { + t.Error(actual) + } + }) + } +} diff --git a/pkg/server/dynamiccertificates/client_ca.go b/pkg/server/dynamiccertificates/client_ca.go index fcca009cd..f5c8cad43 100644 --- a/pkg/server/dynamiccertificates/client_ca.go +++ b/pkg/server/dynamiccertificates/client_ca.go @@ -34,6 +34,7 @@ type dynamicCertificateContent struct { // clientCA holds the content for the clientCA bundle clientCA caBundleContent servingCert certKeyContent + sniCerts []sniCertKeyContent } // caBundleContent holds the content for the clientCA bundle. Wrapping the bytes makes the Equals work nicely with the @@ -55,6 +56,16 @@ func (c *dynamicCertificateContent) Equal(rhs *dynamicCertificateContent) bool { return false } + if len(c.sniCerts) != len(rhs.sniCerts) { + return false + } + + for i := range c.sniCerts { + if !c.sniCerts[i].Equal(&rhs.sniCerts[i]) { + return false + } + } + return true } diff --git a/pkg/server/dynamiccertificates/named_certificates.go b/pkg/server/dynamiccertificates/named_certificates.go new file mode 100644 index 000000000..5590dea59 --- /dev/null +++ b/pkg/server/dynamiccertificates/named_certificates.go @@ -0,0 +1,89 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package dynamiccertificates + +import ( + "crypto/tls" + "crypto/x509" + "fmt" + "strings" + + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/klog" +) + +// BuildNamedCertificates returns a map of *tls.Certificate by name. It's +// suitable for use in tls.Config#NamedCertificates. Returns an error if any of the certs +// is invalid. Returns nil if len(certs) == 0 +func (c *DynamicServingCertificateController) BuildNamedCertificates(sniCerts []sniCertKeyContent) (map[string]*tls.Certificate, error) { + nameToCertificate := map[string]*tls.Certificate{} + byNameExplicit := map[string]*tls.Certificate{} + + // Iterate backwards so that earlier certs take precedence in the names map + for i := len(sniCerts) - 1; i >= 0; i-- { + cert, err := tls.X509KeyPair(sniCerts[i].cert, sniCerts[i].key) + if err != nil { + return nil, fmt.Errorf("invalid SNI cert keypair [%d/%q]: %v", i, c.sniCerts[i].Name(), err) + } + + // error is not possible given above call to X509KeyPair + x509Cert, _ := x509.ParseCertificate(cert.Certificate[0]) + + names := sniCerts[i].sniNames + for _, name := range names { + byNameExplicit[name] = &cert + } + + klog.V(2).Infof("loaded SNI cert [%d/%q]: %s", i, c.sniCerts[i].Name(), GetHumanCertDetail(x509Cert)) + if c.eventRecorder != nil { + c.eventRecorder.Eventf(nil, nil, v1.EventTypeWarning, "TLSConfigChanged", "SNICertificateReload", "loaded SNI cert [%d/%q]: %s with explicit names %v", i, c.sniCerts[i].Name(), GetHumanCertDetail(x509Cert), names) + } + + if len(names) == 0 { + names = getCertificateNames(x509Cert) + for _, name := range names { + nameToCertificate[name] = &cert + } + } + } + + // Explicitly set names must override + for k, v := range byNameExplicit { + nameToCertificate[k] = v + } + + return nameToCertificate, nil +} + +// getCertificateNames returns names for an x509.Certificate. The names are +// suitable for use in tls.Config#NamedCertificates. +func getCertificateNames(cert *x509.Certificate) []string { + var names []string + + cn := cert.Subject.CommonName + if cn == "*" || len(validation.IsDNS1123Subdomain(strings.TrimPrefix(cn, "*."))) == 0 { + names = append(names, cn) + } + for _, san := range cert.DNSNames { + names = append(names, san) + } + // intentionally all IPs in the cert are ignored as SNI forbids passing IPs + // to select a cert. Before go 1.6 the tls happily passed IPs as SNI values. + + return names +} diff --git a/pkg/server/dynamiccertificates/named_certificates_test.go b/pkg/server/dynamiccertificates/named_certificates_test.go new file mode 100644 index 000000000..443817cfd --- /dev/null +++ b/pkg/server/dynamiccertificates/named_certificates_test.go @@ -0,0 +1,318 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package dynamiccertificates + +import ( + "bytes" + "crypto/rand" + "crypto/rsa" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "encoding/base64" + "encoding/pem" + "fmt" + "math/big" + "net" + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +type testCertSpec struct { + host string + names, ips []string // in certificate +} + +type namedtestCertSpec struct { + testCertSpec + explicitNames []string // as --tls-sni-cert-key explicit names +} + +func TestBuiltNamedCertificates(t *testing.T) { + tests := []struct { + certs []namedtestCertSpec + explicitNames []string + expected map[string]int // name to certs[*] index + errorString string + }{ + { + // empty certs + expected: map[string]int{}, + }, + { + // only one cert + certs: []namedtestCertSpec{ + { + testCertSpec: testCertSpec{ + host: "test.com", + }, + }, + }, + expected: map[string]int{ + "test.com": 0, + }, + }, + { + // ips are ignored + certs: []namedtestCertSpec{ + { + testCertSpec: testCertSpec{ + host: "test.com", + ips: []string{"1.2.3.4"}, + }, + }, + }, + expected: map[string]int{ + "test.com": 0, + }, + }, + { + // two certs with the same name + certs: []namedtestCertSpec{ + { + testCertSpec: testCertSpec{ + host: "test.com", + }, + }, + { + testCertSpec: testCertSpec{ + host: "test.com", + }, + }, + }, + expected: map[string]int{ + "test.com": 0, + }, + }, + { + // two certs with different names + certs: []namedtestCertSpec{ + { + testCertSpec: testCertSpec{ + host: "test2.com", + }, + }, + { + testCertSpec: testCertSpec{ + host: "test1.com", + }, + }, + }, + expected: map[string]int{ + "test1.com": 1, + "test2.com": 0, + }, + }, + { + // two certs with the same name, explicit trumps + certs: []namedtestCertSpec{ + { + testCertSpec: testCertSpec{ + host: "test.com", + }, + }, + { + testCertSpec: testCertSpec{ + host: "test.com", + }, + explicitNames: []string{"test.com"}, + }, + }, + expected: map[string]int{ + "test.com": 1, + }, + }, + { + // certs with partial overlap; ips are ignored + certs: []namedtestCertSpec{ + { + testCertSpec: testCertSpec{ + host: "a", + names: []string{"a.test.com", "test.com"}, + }, + }, + { + testCertSpec: testCertSpec{ + host: "b", + names: []string{"b.test.com", "test.com"}, + }, + }, + }, + expected: map[string]int{ + "a": 0, "b": 1, + "a.test.com": 0, "b.test.com": 1, + "test.com": 0, + }, + }, + { + // wildcards + certs: []namedtestCertSpec{ + { + testCertSpec: testCertSpec{ + host: "a", + names: []string{"a.test.com", "test.com"}, + }, + explicitNames: []string{"*.test.com", "test.com"}, + }, + { + testCertSpec: testCertSpec{ + host: "b", + names: []string{"b.test.com", "test.com"}, + }, + explicitNames: []string{"dev.test.com", "test.com"}, + }}, + expected: map[string]int{ + "test.com": 0, + "*.test.com": 0, + "dev.test.com": 1, + }, + }, + } + +NextTest: + for i, test := range tests { + var sniCerts []SNICertKeyContentProvider + bySignature := map[string]int{} // index in test.certs by cert signature + for j, c := range test.certs { + certProvider, err := createTestTLSCerts(c.testCertSpec, c.explicitNames) + if err != nil { + t.Errorf("%d - failed to create cert %d: %v", i, j, err) + continue NextTest + } + + sniCerts = append(sniCerts, certProvider) + + sig, err := certSignature(certProvider) + if err != nil { + t.Errorf("%d - failed to get signature for %d: %v", i, j, err) + continue NextTest + } + bySignature[sig] = j + } + + c := DynamicServingCertificateController{sniCerts: sniCerts} + content, err := c.newTLSContent() + assert.NoError(t, err) + + certMap, err := c.BuildNamedCertificates(content.sniCerts) + if err == nil && len(test.errorString) != 0 { + t.Errorf("%d - expected no error, got: %v", i, err) + } else if err != nil && err.Error() != test.errorString { + t.Errorf("%d - expected error %q, got: %v", i, test.errorString, err) + } else { + got := map[string]int{} + for name, cert := range certMap { + x509Certs, err := x509.ParseCertificates(cert.Certificate[0]) + assert.NoError(t, err, "%d - invalid certificate for %q", i, name) + assert.True(t, len(x509Certs) > 0, "%d - expected at least one x509 cert in tls cert for %q", i, name) + got[name] = bySignature[x509CertSignature(x509Certs[0])] + } + + assert.EqualValues(t, test.expected, got, "%d - wrong certificate map", i) + } + } +} + +func parseIPList(ips []string) []net.IP { + var netIPs []net.IP + for _, ip := range ips { + netIPs = append(netIPs, net.ParseIP(ip)) + } + return netIPs +} + +func createTestTLSCerts(spec testCertSpec, names []string) (certProvider SNICertKeyContentProvider, err error) { + certPem, keyPem, err := generateSelfSignedCertKey(spec.host, parseIPList(spec.ips), spec.names) + if err != nil { + return nil, err + } + + return NewStaticSNICertKeyContent("test-cert", certPem, keyPem, names...) +} + +func x509CertSignature(cert *x509.Certificate) string { + return base64.StdEncoding.EncodeToString(cert.Signature) +} + +func certSignature(certProvider CertKeyContentProvider) (string, error) { + currentCert, currentKey := certProvider.CurrentCertKeyContent() + + tlsCert, err := tls.X509KeyPair(currentCert, currentKey) + if err != nil { + return "", err + } + + x509Certs, err := x509.ParseCertificates(tlsCert.Certificate[0]) + if err != nil { + return "", err + } + return x509CertSignature(x509Certs[0]), nil +} + +// generateSelfSignedCertKey creates a self-signed certificate and key for the given host. +// Host may be an IP or a DNS name +// You may also specify additional subject alt names (either ip or dns names) for the certificate +func generateSelfSignedCertKey(host string, alternateIPs []net.IP, alternateDNS []string) ([]byte, []byte, error) { + priv, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + return nil, nil, err + } + + template := x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{ + CommonName: fmt.Sprintf("%s@%d", host, time.Now().Unix()), + }, + NotBefore: time.Unix(0, 0), + NotAfter: time.Now().Add(time.Hour * 24 * 365 * 100), + + KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + BasicConstraintsValid: true, + IsCA: true, + } + + if ip := net.ParseIP(host); ip != nil { + template.IPAddresses = append(template.IPAddresses, ip) + } else { + template.DNSNames = append(template.DNSNames, host) + } + + template.IPAddresses = append(template.IPAddresses, alternateIPs...) + template.DNSNames = append(template.DNSNames, alternateDNS...) + + derBytes, err := x509.CreateCertificate(rand.Reader, &template, &template, &priv.PublicKey, priv) + if err != nil { + return nil, nil, err + } + + // Generate cert + certBuffer := bytes.Buffer{} + if err := pem.Encode(&certBuffer, &pem.Block{Type: "CERTIFICATE", Bytes: derBytes}); err != nil { + return nil, nil, err + } + + // Generate key + keyBuffer := bytes.Buffer{} + if err := pem.Encode(&keyBuffer, &pem.Block{Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(priv)}); err != nil { + return nil, nil, err + } + + return certBuffer.Bytes(), keyBuffer.Bytes(), nil +} diff --git a/pkg/server/dynamiccertificates/static_content.go b/pkg/server/dynamiccertificates/static_content.go index 0c587e0ba..1d4b2208f 100644 --- a/pkg/server/dynamiccertificates/static_content.go +++ b/pkg/server/dynamiccertificates/static_content.go @@ -64,6 +64,11 @@ type staticCertKeyContent struct { key []byte } +type staticSNICertKeyContent struct { + staticCertKeyContent + sniNames []string +} + // NewStaticCertKeyContentFromFiles returns a CertKeyContentProvider based on a filename func NewStaticCertKeyContentFromFiles(certFile, keyFile string) (CertKeyContentProvider, error) { if len(certFile) == 0 { @@ -85,6 +90,27 @@ func NewStaticCertKeyContentFromFiles(certFile, keyFile string) (CertKeyContentP return NewStaticCertKeyContent(fmt.Sprintf("cert: %s, key: %s", certFile, keyFile), certPEMBlock, keyPEMBlock) } +// NewStaticSNICertKeyContentFromFiles returns a SNICertKeyContentProvider based on a filename +func NewStaticSNICertKeyContentFromFiles(certFile, keyFile string, sniNames ...string) (SNICertKeyContentProvider, error) { + if len(certFile) == 0 { + return nil, fmt.Errorf("missing filename for certificate") + } + if len(keyFile) == 0 { + return nil, fmt.Errorf("missing filename for key") + } + + certPEMBlock, err := ioutil.ReadFile(certFile) + if err != nil { + return nil, err + } + keyPEMBlock, err := ioutil.ReadFile(keyFile) + if err != nil { + return nil, err + } + + return NewStaticSNICertKeyContent(fmt.Sprintf("cert: %s, key: %s", certFile, keyFile), certPEMBlock, keyPEMBlock, sniNames...) +} + // NewStaticCertKeyContent returns a CertKeyContentProvider that always returns the same value func NewStaticCertKeyContent(name string, cert, key []byte) (CertKeyContentProvider, error) { // Ensure that the key matches the cert and both are valid @@ -100,6 +126,24 @@ func NewStaticCertKeyContent(name string, cert, key []byte) (CertKeyContentProvi }, nil } +// NewStaticSNICertKeyContent returns a SNICertKeyContentProvider that always returns the same value +func NewStaticSNICertKeyContent(name string, cert, key []byte, sniNames ...string) (SNICertKeyContentProvider, error) { + // Ensure that the key matches the cert and both are valid + _, err := tls.X509KeyPair(cert, key) + if err != nil { + return nil, err + } + + return &staticSNICertKeyContent{ + staticCertKeyContent: staticCertKeyContent{ + name: name, + cert: cert, + key: key, + }, + sniNames: sniNames, + }, nil +} + // Name is just an identifier func (c *staticCertKeyContent) Name() string { return c.name @@ -109,3 +153,7 @@ func (c *staticCertKeyContent) Name() string { func (c *staticCertKeyContent) CurrentCertKeyContent() ([]byte, []byte) { return c.cert, c.key } + +func (c *staticSNICertKeyContent) SNINames() []string { + return c.sniNames +} diff --git a/pkg/server/dynamiccertificates/tlsconfig.go b/pkg/server/dynamiccertificates/tlsconfig.go index 80da5c719..194979743 100644 --- a/pkg/server/dynamiccertificates/tlsconfig.go +++ b/pkg/server/dynamiccertificates/tlsconfig.go @@ -46,6 +46,8 @@ type DynamicServingCertificateController struct { clientCA CAContentProvider // servingCert provides the very latest content of the default serving certificate servingCert CertKeyContentProvider + // sniCerts are a list of CertKeyContentProvider with associated names used for SNI + sniCerts []SNICertKeyContentProvider // currentlyServedContent holds the original bytes that we are serving. This is used to decide if we need to set a // new atomic value. The types used for efficient TLSConfig preclude using the processed value. @@ -63,12 +65,14 @@ func NewDynamicServingCertificateController( baseTLSConfig tls.Config, clientCA CAContentProvider, servingCert CertKeyContentProvider, + sniCerts []SNICertKeyContentProvider, eventRecorder events.EventRecorder, ) *DynamicServingCertificateController { c := &DynamicServingCertificateController{ baseTLSConfig: baseTLSConfig, clientCA: clientCA, servingCert: servingCert, + sniCerts: sniCerts, queue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "DynamicServingCertificateController"), eventRecorder: eventRecorder, @@ -113,6 +117,15 @@ func (c *DynamicServingCertificateController) newTLSContent() (*dynamicCertifica newContent.servingCert = certKeyContent{cert: currServingCert, key: currServingKey} } + for i, sniCert := range c.sniCerts { + currCert, currKey := sniCert.CurrentCertKeyContent() + if len(currCert) == 0 || len(currKey) == 0 { + return nil, fmt.Errorf("not loading an empty SNI certificate from %d/%q", i, sniCert.Name()) + } + + newContent.sniCerts = append(newContent.sniCerts, sniCertKeyContent{certKeyContent: certKeyContent{cert: currCert, key: currKey}, sniNames: sniCert.SNINames()}) + } + return newContent, nil } @@ -168,13 +181,20 @@ func (c *DynamicServingCertificateController) syncCerts() error { } newTLSConfigCopy.Certificates = []tls.Certificate{cert} + } + + if len(newContent.sniCerts) > 0 { + newTLSConfigCopy.NameToCertificate, err = c.BuildNamedCertificates(newContent.sniCerts) + if err != nil { + return fmt.Errorf("unable to build named certificate map: %v", err) + } // append all named certs. Otherwise, the go tls stack will think no SNI processing // is necessary because there is only one cert anyway. - // Moreover, if ServerCert.CertFile/ServerCert.KeyFile are not set, the first SNI + // Moreover, if servingCert is not set, the first SNI // cert will become the default cert. That's what we expect anyway. - for _, c := range newTLSConfigCopy.NameToCertificate { - newTLSConfigCopy.Certificates = append(newTLSConfigCopy.Certificates, *c) + for _, sniCert := range newTLSConfigCopy.NameToCertificate { + newTLSConfigCopy.Certificates = append(newTLSConfigCopy.Certificates, *sniCert) } } diff --git a/pkg/server/dynamiccertificates/tlsconfig_test.go b/pkg/server/dynamiccertificates/tlsconfig_test.go index c4ee8a787..973a4d683 100644 --- a/pkg/server/dynamiccertificates/tlsconfig_test.go +++ b/pkg/server/dynamiccertificates/tlsconfig_test.go @@ -72,8 +72,8 @@ M++C29JwS3Hwbubg6WO3wjFjoEhpCwU6qRYUz3MRp4tHO4kxKXx+oQnUiFnR7vW0 YkNtGc1RUDHwecCTFpJtPb7Yu/E= -----END CERTIFICATE-----`) -func TestNewTLSContent(t *testing.T) { - testCertProvider, err := NewStaticCertKeyContent("test-cert", serverCert, serverKey) +func TestNewStaticCertKeyContent(t *testing.T) { + testCertProvider, err := NewStaticSNICertKeyContent("test-cert", serverCert, serverKey, "foo") if err != nil { t.Error(err) } @@ -82,6 +82,7 @@ func TestNewTLSContent(t *testing.T) { name string clientCA CAContentProvider servingCert CertKeyContentProvider + sniCerts []SNICertKeyContentProvider expected *dynamicCertificateContent expectedErr string @@ -90,9 +91,12 @@ func TestNewTLSContent(t *testing.T) { name: "filled", clientCA: NewStaticCAContent("test-ca", []byte("content-1")), servingCert: testCertProvider, + sniCerts: []SNICertKeyContentProvider{testCertProvider}, expected: &dynamicCertificateContent{ - clientCA: caBundleContent{caBundle: []byte("content-1")}, + clientCA: caBundleContent{caBundle: []byte("content-1")}, + // ignore sni names for serving cert servingCert: certKeyContent{cert: serverCert, key: serverKey}, + sniCerts: []sniCertKeyContent{{certKeyContent: certKeyContent{cert: serverCert, key: serverKey}, sniNames: []string{"foo"}}}, }, }, { @@ -112,6 +116,7 @@ func TestNewTLSContent(t *testing.T) { c := &DynamicServingCertificateController{ clientCA: test.clientCA, servingCert: test.servingCert, + sniCerts: test.sniCerts, } actual, err := c.newTLSContent() if !reflect.DeepEqual(actual, test.expected) { diff --git a/pkg/server/options/serving.go b/pkg/server/options/serving.go index c6797fda8..b20a8b386 100644 --- a/pkg/server/options/serving.go +++ b/pkg/server/options/serving.go @@ -17,7 +17,6 @@ limitations under the License. package options import ( - "crypto/tls" "fmt" "net" "path" @@ -250,21 +249,15 @@ func (s *SecureServingOptions) ApplyTo(config **server.SecureServingInfo) error } // load SNI certs - namedTLSCerts := make([]server.NamedTLSCert, 0, len(s.SNICertKeys)) + namedTLSCerts := make([]dynamiccertificates.SNICertKeyContentProvider, 0, len(s.SNICertKeys)) for _, nck := range s.SNICertKeys { - tlsCert, err := tls.LoadX509KeyPair(nck.CertFile, nck.KeyFile) - namedTLSCerts = append(namedTLSCerts, server.NamedTLSCert{ - TLSCert: tlsCert, - Names: nck.Names, - }) + tlsCert, err := dynamiccertificates.NewStaticSNICertKeyContentFromFiles(nck.CertFile, nck.KeyFile, nck.Names...) + namedTLSCerts = append(namedTLSCerts, tlsCert) if err != nil { return fmt.Errorf("failed to load SNI cert and key: %v", err) } } - c.SNICerts, err = server.GetNamedCertificateMap(namedTLSCerts) - if err != nil { - return err - } + c.SNICerts = namedTLSCerts return nil } diff --git a/pkg/server/options/serving_test.go b/pkg/server/options/serving_test.go index 84d6fe3d4..53a8f601b 100644 --- a/pkg/server/options/serving_test.go +++ b/pkg/server/options/serving_test.go @@ -37,8 +37,6 @@ import ( "testing" "time" - "github.com/stretchr/testify/assert" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/apimachinery/pkg/version" @@ -67,190 +65,6 @@ type NamedTestCertSpec struct { explicitNames []string // as --tls-sni-cert-key explicit names } -func TestGetNamedCertificateMap(t *testing.T) { - tests := []struct { - certs []NamedTestCertSpec - explicitNames []string - expected map[string]int // name to certs[*] index - errorString string - }{ - { - // empty certs - expected: map[string]int{}, - }, - { - // only one cert - certs: []NamedTestCertSpec{ - { - TestCertSpec: TestCertSpec{ - host: "test.com", - }, - }, - }, - expected: map[string]int{ - "test.com": 0, - }, - }, - { - // ips are ignored - certs: []NamedTestCertSpec{ - { - TestCertSpec: TestCertSpec{ - host: "test.com", - ips: []string{"1.2.3.4"}, - }, - }, - }, - expected: map[string]int{ - "test.com": 0, - }, - }, - { - // two certs with the same name - certs: []NamedTestCertSpec{ - { - TestCertSpec: TestCertSpec{ - host: "test.com", - }, - }, - { - TestCertSpec: TestCertSpec{ - host: "test.com", - }, - }, - }, - expected: map[string]int{ - "test.com": 0, - }, - }, - { - // two certs with different names - certs: []NamedTestCertSpec{ - { - TestCertSpec: TestCertSpec{ - host: "test2.com", - }, - }, - { - TestCertSpec: TestCertSpec{ - host: "test1.com", - }, - }, - }, - expected: map[string]int{ - "test1.com": 1, - "test2.com": 0, - }, - }, - { - // two certs with the same name, explicit trumps - certs: []NamedTestCertSpec{ - { - TestCertSpec: TestCertSpec{ - host: "test.com", - }, - }, - { - TestCertSpec: TestCertSpec{ - host: "test.com", - }, - explicitNames: []string{"test.com"}, - }, - }, - expected: map[string]int{ - "test.com": 1, - }, - }, - { - // certs with partial overlap; ips are ignored - certs: []NamedTestCertSpec{ - { - TestCertSpec: TestCertSpec{ - host: "a", - names: []string{"a.test.com", "test.com"}, - }, - }, - { - TestCertSpec: TestCertSpec{ - host: "b", - names: []string{"b.test.com", "test.com"}, - }, - }, - }, - expected: map[string]int{ - "a": 0, "b": 1, - "a.test.com": 0, "b.test.com": 1, - "test.com": 0, - }, - }, - { - // wildcards - certs: []NamedTestCertSpec{ - { - TestCertSpec: TestCertSpec{ - host: "a", - names: []string{"a.test.com", "test.com"}, - }, - explicitNames: []string{"*.test.com", "test.com"}, - }, - { - TestCertSpec: TestCertSpec{ - host: "b", - names: []string{"b.test.com", "test.com"}, - }, - explicitNames: []string{"dev.test.com", "test.com"}, - }}, - expected: map[string]int{ - "test.com": 0, - "*.test.com": 0, - "dev.test.com": 1, - }, - }, - } - -NextTest: - for i, test := range tests { - var namedTLSCerts []server.NamedTLSCert - bySignature := map[string]int{} // index in test.certs by cert signature - for j, c := range test.certs { - cert, err := createTestTLSCerts(c.TestCertSpec) - if err != nil { - t.Errorf("%d - failed to create cert %d: %v", i, j, err) - continue NextTest - } - - namedTLSCerts = append(namedTLSCerts, server.NamedTLSCert{ - TLSCert: cert, - Names: c.explicitNames, - }) - - sig, err := certSignature(cert) - if err != nil { - t.Errorf("%d - failed to get signature for %d: %v", i, j, err) - continue NextTest - } - bySignature[sig] = j - } - - certMap, err := server.GetNamedCertificateMap(namedTLSCerts) - if err == nil && len(test.errorString) != 0 { - t.Errorf("%d - expected no error, got: %v", i, err) - } else if err != nil && err.Error() != test.errorString { - t.Errorf("%d - expected error %q, got: %v", i, test.errorString, err) - } else { - got := map[string]int{} - for name, cert := range certMap { - x509Certs, err := x509.ParseCertificates(cert.Certificate[0]) - assert.NoError(t, err, "%d - invalid certificate for %q", i, name) - assert.True(t, len(x509Certs) > 0, "%d - expected at least one x509 cert in tls cert for %q", i, name) - got[name] = bySignature[x509CertSignature(x509Certs[0])] - } - - assert.EqualValues(t, test.expected, got, "%d - wrong certificate map", i) - } - } -} - func TestServerRunWithSNI(t *testing.T) { tests := map[string]struct { Cert TestCertSpec @@ -574,16 +388,6 @@ func parseIPList(ips []string) []net.IP { return netIPs } -func createTestTLSCerts(spec TestCertSpec) (tlsCert tls.Certificate, err error) { - certPem, keyPem, err := generateSelfSignedCertKey(spec.host, parseIPList(spec.ips), spec.names) - if err != nil { - return tlsCert, err - } - - tlsCert, err = tls.X509KeyPair(certPem, keyPem) - return tlsCert, err -} - func getOrCreateTestCertFiles(certFileName, keyFileName string, spec TestCertSpec) (err error) { if _, err := os.Stat(certFileName); err == nil { if _, err := os.Stat(keyFileName); err == nil { diff --git a/pkg/server/options/serving_with_loopback.go b/pkg/server/options/serving_with_loopback.go index 7f1920642..b19258bf9 100644 --- a/pkg/server/options/serving_with_loopback.go +++ b/pkg/server/options/serving_with_loopback.go @@ -17,12 +17,12 @@ limitations under the License. package options import ( - "crypto/tls" "fmt" "github.com/pborman/uuid" "k8s.io/apiserver/pkg/server" + "k8s.io/apiserver/pkg/server/dynamiccertificates" "k8s.io/client-go/rest" certutil "k8s.io/client-go/util/cert" ) @@ -55,7 +55,7 @@ func (s *SecureServingOptionsWithLoopback) ApplyTo(secureServingInfo **server.Se if err != nil { return fmt.Errorf("failed to generate self-signed certificate for loopback connection: %v", err) } - tlsCert, err := tls.X509KeyPair(certPem, keyPem) + certProvider, err := dynamiccertificates.NewStaticSNICertKeyContent("self-signed loopback", certPem, keyPem, server.LoopbackClientServerNameOverride) if err != nil { return fmt.Errorf("failed to generate self-signed certificate for loopback connection: %v", err) } @@ -71,7 +71,8 @@ func (s *SecureServingOptionsWithLoopback) ApplyTo(secureServingInfo **server.Se default: *loopbackClientConfig = secureLoopbackClientConfig - (*secureServingInfo).SNICerts[server.LoopbackClientServerNameOverride] = &tlsCert + // Write to the front of SNICerts so that this overrides any other certs with the same name + (*secureServingInfo).SNICerts = append([]dynamiccertificates.SNICertKeyContentProvider{certProvider}, (*secureServingInfo).SNICerts...) } return nil diff --git a/pkg/server/secure_serving.go b/pkg/server/secure_serving.go index 68f682c1d..7bbe77e79 100644 --- a/pkg/server/secure_serving.go +++ b/pkg/server/secure_serving.go @@ -19,18 +19,15 @@ package server import ( "context" "crypto/tls" - "crypto/x509" "fmt" "net" "net/http" - "strings" "time" "golang.org/x/net/http2" "k8s.io/klog" utilruntime "k8s.io/apimachinery/pkg/util/runtime" - "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apiserver/pkg/server/dynamiccertificates" ) @@ -41,7 +38,6 @@ const ( // tlsConfig produces the tls.Config to serve with. func (s *SecureServingInfo) tlsConfig(stopCh <-chan struct{}) (*tls.Config, error) { tlsConfig := &tls.Config{ - NameToCertificate: s.SNICerts, // Can't use SSLv3 because of POODLE and BEAST // Can't use TLSv1.0 because of POODLE and BEAST using CBC cipher // Can't use TLSv1.1 because of RC4 cipher usage @@ -62,28 +58,18 @@ func (s *SecureServingInfo) tlsConfig(stopCh <-chan struct{}) (*tls.Config, erro tlsConfig.CipherSuites = s.CipherSuites } - // if s.Cert is not nil, this logic is contained within the dynamic serving controller - if s.Cert == nil { - // append all named certs. Otherwise, the go tls stack will think no SNI processing - // is necessary because there is only one cert anyway. - // Moreover, if ServerCert.CertFile/ServerCert.KeyFile are not set, the first SNI - // cert will become the default cert. That's what we expect anyway. - for _, c := range s.SNICerts { - tlsConfig.Certificates = append(tlsConfig.Certificates, *c) - } - } - if s.ClientCA != nil { // Populate PeerCertificates in requests, but don't reject connections without certificates // This allows certificates to be validated by authenticators, while still allowing other auth types tlsConfig.ClientAuth = tls.RequestClientCert } - if s.ClientCA != nil || s.Cert != nil { + if s.ClientCA != nil || s.Cert != nil || len(s.SNICerts) > 0 { dynamicCertificateController := dynamiccertificates.NewDynamicServingCertificateController( *tlsConfig, s.ClientCA, s.Cert, + s.SNICerts, nil, // TODO see how to plumb an event recorder down in here. For now this results in simply klog messages. ) // runonce to be sure that we have a value. @@ -199,57 +185,6 @@ func RunServer( return stoppedCh, nil } -type NamedTLSCert struct { - TLSCert tls.Certificate - - // Names is a list of domain patterns: fully qualified domain names, possibly prefixed with - // wildcard segments. - Names []string -} - -// GetNamedCertificateMap returns a map of *tls.Certificate by name. It's -// suitable for use in tls.Config#NamedCertificates. Returns an error if any of the certs -// cannot be loaded. Returns nil if len(certs) == 0 -func GetNamedCertificateMap(certs []NamedTLSCert) (map[string]*tls.Certificate, error) { - // register certs with implicit names first, reverse order such that earlier trump over the later - byName := map[string]*tls.Certificate{} - for i := len(certs) - 1; i >= 0; i-- { - if len(certs[i].Names) > 0 { - continue - } - cert := &certs[i].TLSCert - - // read names from certificate common names and DNS names - if len(cert.Certificate) == 0 { - return nil, fmt.Errorf("empty SNI certificate, skipping") - } - x509Cert, err := x509.ParseCertificate(cert.Certificate[0]) - if err != nil { - return nil, fmt.Errorf("parse error for SNI certificate: %v", err) - } - cn := x509Cert.Subject.CommonName - if cn == "*" || len(validation.IsDNS1123Subdomain(strings.TrimPrefix(cn, "*."))) == 0 { - byName[cn] = cert - } - for _, san := range x509Cert.DNSNames { - byName[san] = cert - } - // intentionally all IPs in the cert are ignored as SNI forbids passing IPs - // to select a cert. Before go 1.6 the tls happily passed IPs as SNI values. - } - - // register certs with explicit names last, overwriting every of the implicit ones, - // again in reverse order. - for i := len(certs) - 1; i >= 0; i-- { - namedCert := &certs[i] - for _, name := range namedCert.Names { - byName[name] = &certs[i].TLSCert - } - } - - return byName, nil -} - // tcpKeepAliveListener sets TCP keep-alive timeouts on accepted // connections. It's used by ListenAndServe and ListenAndServeTLS so // dead TCP connections (e.g. closing laptop mid-download) eventually