From 9a0fc14fea217da39cd26b4b31584cb8e6bf251e Mon Sep 17 00:00:00 2001 From: Nick Sardo Date: Tue, 10 Apr 2018 10:13:05 -0700 Subject: [PATCH] Change naming of SSL certs --- pkg/loadbalancers/l7.go | 23 +++++---- pkg/loadbalancers/l7s.go | 1 - pkg/loadbalancers/loadbalancers_test.go | 68 ++++++++++++++----------- pkg/utils/namer.go | 45 ++++++++-------- pkg/utils/namer_test.go | 59 ++++++++++++++++----- 5 files changed, 118 insertions(+), 78 deletions(-) diff --git a/pkg/loadbalancers/l7.go b/pkg/loadbalancers/l7.go index 4a94c4e8a3..afebcd1f20 100644 --- a/pkg/loadbalancers/l7.go +++ b/pkg/loadbalancers/l7.go @@ -30,6 +30,7 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "crypto/sha256" + "k8s.io/ingress-gce/pkg/annotations" "k8s.io/ingress-gce/pkg/backends" "k8s.io/ingress-gce/pkg/utils" @@ -109,8 +110,6 @@ type L7 struct { fws *compute.ForwardingRule // ip is the static-ip associated with both GlobalForwardingRules. ip *compute.Address - // prefix to use in ssl cert names - sslCertPrefix string // sslCerts is the list of ssl certs associated with the targetHTTPSProxy. sslCerts []*compute.SslCertificate // oldSSLCerts is the list of certs that used to be hooked up to the @@ -197,7 +196,7 @@ func (l *L7) deleteOldSSLCerts() (err error) { } certsMap := getMapfromCertList(l.sslCerts) for _, cert := range l.oldSSLCerts { - if !l.isSSLCert(cert.Name) && !l.namer.IsLegacySSLCert(l.Name, cert.Name) { + if !l.namer.IsCertUsedForLB(l.Name, cert.Name) && !l.namer.IsLegacySSLCert(l.Name, cert.Name) { // retain cert if it is managed by GCE(non-ingress) continue } @@ -261,11 +260,6 @@ func (l *L7) usePreSharedCert() (bool, error) { return true, nil } -// isSSLCert returns true if name is ingress managed, specifically by this loadbalancer instance -func (l *L7) isSSLCert(name string) bool { - return strings.HasPrefix(name, l.sslCertPrefix) -} - func getMapfromCertList(certs []*compute.SslCertificate) map[string]*compute.SslCertificate { if len(certs) == 0 { return nil @@ -287,7 +281,7 @@ func (l *L7) populateSSLCert() error { return utils.IgnoreHTTPNotFound(err) } for _, c := range certs { - if l.isSSLCert(c.Name) { + if l.namer.IsCertUsedForLB(l.Name, c.Name) { glog.Infof("Populating ssl cert %s for l7 %s", c.Name, l.Name) l.sslCerts = append(l.sslCerts, c) } @@ -331,7 +325,7 @@ func (l *L7) checkSSLCert() error { for _, tlsCert := range l.runtimeInfo.TLS { ingCert := tlsCert.Cert ingKey := tlsCert.Key - newCertName := l.namer.SSLCertName(l.sslCertPrefix, tlsCert.CertHash) + newCertName := l.namer.SSLCertName(l.Name, tlsCert.CertHash) // PrivateKey is write only, so compare certs alone. We're assuming that // no one will change just the key. We can remember the key and compare, @@ -423,7 +417,7 @@ func (l *L7) checkHttpsProxy() (err error) { if !l.compareCerts(proxy.SslCertificates) { glog.Infof("Https proxy %v has the wrong ssl certs, setting %v overwriting %v", - proxy.Name, l.sslCerts, proxy.SslCertificates) + proxy.Name, toCertNames(l.sslCerts), proxy.SslCertificates) if err := l.cloud.SetSslCertificateForTargetHttpsProxy(proxy, l.sslCerts); err != nil { return err } @@ -989,3 +983,10 @@ func GCEResourceName(ingAnnotations map[string]string, resourceName string) stri func GetCertHash(contents string) string { return fmt.Sprintf("%x", sha256.Sum256([]byte(contents)))[:16] } + +func toCertNames(certs []*compute.SslCertificate) (names []string) { + for _, v := range certs { + names = append(names, v.Name) + } + return names +} diff --git a/pkg/loadbalancers/l7s.go b/pkg/loadbalancers/l7s.go index 7906b13824..dcc0139110 100644 --- a/pkg/loadbalancers/l7s.go +++ b/pkg/loadbalancers/l7s.go @@ -76,7 +76,6 @@ func (l *L7s) create(ri *L7RuntimeInfo) (*L7, error) { cloud: l.cloud, glbcDefaultBackend: l.glbcDefaultBackend, namer: l.namer, - sslCertPrefix: l.namer.SSLCertPrefix(ri.Name), }, nil } diff --git a/pkg/loadbalancers/loadbalancers_test.go b/pkg/loadbalancers/loadbalancers_test.go index b84a424e1c..f0290f2e2f 100644 --- a/pkg/loadbalancers/loadbalancers_test.go +++ b/pkg/loadbalancers/loadbalancers_test.go @@ -23,14 +23,15 @@ import ( compute "google.golang.org/api/compute/v1" "k8s.io/apimachinery/pkg/util/sets" + "strconv" + "strings" + "k8s.io/ingress-gce/pkg/annotations" "k8s.io/ingress-gce/pkg/backends" "k8s.io/ingress-gce/pkg/healthchecks" "k8s.io/ingress-gce/pkg/instances" "k8s.io/ingress-gce/pkg/neg" "k8s.io/ingress-gce/pkg/utils" - "strconv" - "strings" ) const ( @@ -65,11 +66,13 @@ func TestCreateHTTPLoadBalancer(t *testing.T) { } f := NewFakeLoadBalancers(lbInfo.Name, namer) pool := newFakeLoadBalancerPool(f, t, namer) + + // Run Sync pool.Sync([]*L7RuntimeInfo{lbInfo}) - l7, err := pool.Get(lbInfo.Name) + l7, err := pool.Get(lbInfo.Name) if err != nil || l7 == nil { - t.Fatalf("Expected l7 not created") + t.Fatalf("Expected l7 not created, err: %v", err) } um, err := f.GetUrlMap(f.UMName()) if err != nil { @@ -129,12 +132,12 @@ func TestCreateHTTPSLoadBalancer(t *testing.T) { // and the proxy is updated to another cert when the provided cert changes func TestCertUpdate(t *testing.T) { namer := utils.NewNamer("uid1", "fw1") - sslCertPrefix := namer.SSLCertPrefix("test") - certName1 := namer.SSLCertName(sslCertPrefix, GetCertHash("cert")) - certName2 := namer.SSLCertName(sslCertPrefix, GetCertHash("cert2")) + lbName := namer.LoadBalancer("test") + certName1 := namer.SSLCertName(lbName, GetCertHash("cert")) + certName2 := namer.SSLCertName(lbName, GetCertHash("cert2")) lbInfo := &L7RuntimeInfo{ - Name: namer.LoadBalancer("test"), + Name: lbName, AllowHTTP: false, TLS: []*TLSCerts{createCert("key", "cert", "name")}, } @@ -144,7 +147,9 @@ func TestCertUpdate(t *testing.T) { // Sync first cert pool.Sync([]*L7RuntimeInfo{lbInfo}) - t.Logf("name=%q", certName1) + + // Verify certs + t.Logf("lbName=%q, name=%q", lbName, certName1) expectCerts := map[string]string{certName1: lbInfo.TLS[0].Cert} verifyCertAndProxyLink(expectCerts, expectCerts, f, t) @@ -158,12 +163,12 @@ func TestCertUpdate(t *testing.T) { // Tests that controller can overwrite existing, unused certificates func TestCertCreationWithCollision(t *testing.T) { namer := utils.NewNamer("uid1", "fw1") - sslCertPrefix := namer.SSLCertPrefix("test") - certName1 := namer.SSLCertName(sslCertPrefix, GetCertHash("cert")) - certName2 := namer.SSLCertName(sslCertPrefix, GetCertHash("cert2")) + lbName := namer.LoadBalancer("test") + certName1 := namer.SSLCertName(lbName, GetCertHash("cert")) + certName2 := namer.SSLCertName(lbName, GetCertHash("cert2")) lbInfo := &L7RuntimeInfo{ - Name: namer.LoadBalancer("test"), + Name: lbName, AllowHTTP: false, TLS: []*TLSCerts{createCert("key", "cert", "name")}, } @@ -202,18 +207,18 @@ func TestCertCreationWithCollision(t *testing.T) { func TestMultipleCertRetentionAfterRestart(t *testing.T) { namer := utils.NewNamer("uid1", "fw1") - sslCertPrefix := namer.SSLCertPrefix("test") cert1 := createCert("key", "cert", "name") cert2 := createCert("key2", "cert2", "name2") cert3 := createCert("key3", "cert3", "name3") - certName1 := namer.SSLCertName(sslCertPrefix, cert1.CertHash) - certName2 := namer.SSLCertName(sslCertPrefix, cert2.CertHash) - certName3 := namer.SSLCertName(sslCertPrefix, cert3.CertHash) + lbName := namer.LoadBalancer("test") + certName1 := namer.SSLCertName(lbName, cert1.CertHash) + certName2 := namer.SSLCertName(lbName, cert2.CertHash) + certName3 := namer.SSLCertName(lbName, cert3.CertHash) expectCerts := map[string]string{} lbInfo := &L7RuntimeInfo{ - Name: namer.LoadBalancer("test"), + Name: lbName, AllowHTTP: false, TLS: []*TLSCerts{cert1}, } @@ -249,14 +254,15 @@ func TestMultipleCertRetentionAfterRestart(t *testing.T) { // are picked up and deleted when upgrading to the new scheme. func TestUpgradeToNewCertNames(t *testing.T) { namer := utils.NewNamer("uid1", "fw1") + lbName := namer.LoadBalancer("test") lbInfo := &L7RuntimeInfo{ - Name: namer.LoadBalancer("test"), + Name: lbName, AllowHTTP: false, } oldCertName := "k8s-ssl-" + lbInfo.Name tlsCert := createCert("key", "cert", "name") lbInfo.TLS = []*TLSCerts{tlsCert} - newCertName := namer.SSLCertName(namer.SSLCertPrefix("test"), tlsCert.CertHash) + newCertName := namer.SSLCertName(lbName, tlsCert.CertHash) f := NewFakeLoadBalancers(lbInfo.Name, namer) pool := newFakeLoadBalancerPool(f, t, namer) @@ -292,16 +298,16 @@ func TestMaxCertsUpload(t *testing.T) { var tlsCerts []*TLSCerts expectCerts := make(map[string]string) namer := utils.NewNamer("uid1", "fw1") - certPrefix := namer.SSLCertPrefix("test") + lbName := namer.LoadBalancer("test") for ix := 0; ix < TargetProxyCertLimit; ix++ { str := strconv.Itoa(ix) tlsCerts = append(tlsCerts, createCert("key-"+str, "cert-"+str, "name-"+str)) - certName := namer.SSLCertName(certPrefix, GetCertHash("cert-"+str)) + certName := namer.SSLCertName(lbName, GetCertHash("cert-"+str)) expectCerts[certName] = "cert-" + str } lbInfo := &L7RuntimeInfo{ - Name: namer.LoadBalancer("test"), + Name: lbName, AllowHTTP: false, TLS: tlsCerts, } @@ -322,18 +328,18 @@ func TestIdenticalHostnameCerts(t *testing.T) { var tlsCerts []*TLSCerts expectCerts := make(map[string]string) namer := utils.NewNamer("uid1", "fw1") - certPrefix := namer.SSLCertPrefix("test") + lbName := namer.LoadBalancer("test") contents := "" for ix := 0; ix < 3; ix++ { str := strconv.Itoa(ix) contents = "cert-" + str + " foo.com" tlsCerts = append(tlsCerts, createCert("key-"+str, contents, "name-"+str)) - certName := namer.SSLCertName(certPrefix, GetCertHash(contents)) + certName := namer.SSLCertName(lbName, GetCertHash(contents)) expectCerts[certName] = contents } lbInfo := &L7RuntimeInfo{ - Name: namer.LoadBalancer("test"), + Name: lbName, AllowHTTP: false, TLS: tlsCerts, } @@ -391,12 +397,12 @@ func TestIdenticalHostnameCertsPreShared(t *testing.T) { // to secret based cert and verifies the pre-shared cert is retained. func TestPreSharedToSecretBasedCertUpdate(t *testing.T) { namer := utils.NewNamer("uid1", "fw1") - sslCertPrefix := namer.SSLCertPrefix("test") - certName1 := namer.SSLCertName(sslCertPrefix, GetCertHash("cert")) - certName2 := namer.SSLCertName(sslCertPrefix, GetCertHash("cert2")) + lbName := namer.LoadBalancer("test") + certName1 := namer.SSLCertName(lbName, GetCertHash("cert")) + certName2 := namer.SSLCertName(lbName, GetCertHash("cert2")) lbInfo := &L7RuntimeInfo{ - Name: namer.LoadBalancer("test"), + Name: lbName, AllowHTTP: false, } @@ -500,7 +506,7 @@ func verifyCertAndProxyLink(expectCerts map[string]string, expectCertsProxy map[ for certName, certValue := range expectCerts { cert, err := f.GetSslCertificate(certName) if err != nil { - t.Fatalf("expected ssl certificate to exist: %v, err: %v", certName, err) + t.Fatalf("expected ssl certificate to exist: %v, err: %v, all certs: %v", certName, err, toCertNames(allCerts)) } if cert.Certificate != certValue { diff --git a/pkg/utils/namer.go b/pkg/utils/namer.go index 2b10943fd3..fc96554fa7 100644 --- a/pkg/utils/namer.go +++ b/pkg/utils/namer.go @@ -25,6 +25,7 @@ import ( "sync" "crypto/sha256" + "github.com/golang/glog" ) @@ -302,35 +303,33 @@ func (n *Namer) TargetProxy(lbName string, protocol NamerProtocol) string { return "invalid" } -// IsLegacySSLCert returns true if certName is an Ingress managed name following the older naming convention. The check -// also ensures that the cert is managed by the specific ingress instance - lbName -func (n *Namer) IsLegacySSLCert(lbName string, name string) bool { - // old style name is of the form k8s-ssl- or k8s-ssl-1-. - legacyPrefixPrimary := truncate(strings.Join([]string{n.prefix, sslCertPrefix, lbName}, "-")) - legacyPrefixSec := truncate(strings.Join([]string{n.prefix, sslCertPrefix, "1", lbName}, "-")) - return strings.HasPrefix(name, legacyPrefixPrimary) || strings.HasPrefix(name, legacyPrefixSec) +// IsCertUsedForLB returns true if the resourceName belongs to this cluster's ingress. +// It checks that the hashed lbName exists and +func (n *Namer) IsCertUsedForLB(lbName, resourceName string) bool { + lbNameHash := n.lbNameToHash(lbName) + prefix := fmt.Sprintf("%s-%s-%s", n.prefix, sslCertPrefix, lbNameHash) + return strings.HasPrefix(resourceName, prefix) && strings.HasSuffix(resourceName, n.UID()) } -func (n *Namer) SSLCertPrefix(lbKey string) string { - // lbKey is of the form namespace/ingressname. Cert prefix will use the 8 byte(16 chars) hash of namespace-ingressname - // followed by the cluster id(also 16 char). We use hash instead of the actual name so that the cert prefix is of a - // fixed length and at the same time unique for a given loadbalancer instance. +func (n *Namer) lbNameToHash(lbName string) string { + ingHash := fmt.Sprintf("%x", sha256.Sum256([]byte(lbName))) + return ingHash[:16] +} - parts := strings.Split(lbKey, clusterNameDelimiter) - scrubbedName := strings.Replace(lbKey, "/", "-", -1) - clusterName := n.UID() - if parts[len(parts)-1] == clusterName { - scrubbedName = strings.TrimSuffix(scrubbedName, clusterNameDelimiter+clusterName) - } - // sha256 is 32 bytes(64 chars) long, truncating to first 8 bytes still results in low probability of collision - namespaceHash := fmt.Sprintf("%x", sha256.Sum256([]byte(scrubbedName))) - clusterPrefix := namespaceHash[:16] + clusterNameDelimiter + clusterName - return fmt.Sprintf("%s-%s-%s", n.prefix, sslCertPrefix, clusterPrefix) +// IsLegacySSLCert returns true if certName is an Ingress managed name following the older naming convention. The check +// also ensures that the cert is managed by the specific ingress instance - lbName +func (n *Namer) IsLegacySSLCert(lbName string, resourceName string) bool { + // old style name is of the form k8s-ssl- or k8s-ssl-1-. + legacyPrefixPrimary := truncate(fmt.Sprintf("%s-%s-%s", n.prefix, sslCertPrefix, lbName)) + legacyPrefixSec := truncate(fmt.Sprintf("%s-%s-1-%s", n.prefix, sslCertPrefix, lbName)) + return strings.HasPrefix(resourceName, legacyPrefixPrimary) || strings.HasPrefix(resourceName, legacyPrefixSec) } // SSLCertName returns the name of the certificate. -func (n *Namer) SSLCertName(prefix string, secretHash string) string { - return truncate(prefix + "-" + secretHash) +func (n *Namer) SSLCertName(lbName string, secretHash string) string { + lbNameHash := n.lbNameToHash(lbName) + // k8s-ssl-[lbNameHash]-[certhash]--[clusterUID] + return truncate(fmt.Sprintf("%s-%s-%s-%s%s%s", n.prefix, sslCertPrefix, lbNameHash, secretHash, clusterNameDelimiter, n.UID())) } // ForwardingRule returns the name of the forwarding rule prefix. diff --git a/pkg/utils/namer_test.go b/pkg/utils/namer_test.go index c44f8eeaf6..c05fcf1fe2 100644 --- a/pkg/utils/namer_test.go +++ b/pkg/utils/namer_test.go @@ -94,7 +94,6 @@ func TestNamerParseName(t *testing.T) { const uid = "uid1" namer := NewNamer(uid, "fw1") lbName := namer.LoadBalancer("key1") - certPrefix := namer.SSLCertPrefix("key1") secretHash := fmt.Sprintf("%x", sha256.Sum256([]byte("test123")))[:16] for _, tc := range []struct { in string @@ -105,8 +104,8 @@ func TestNamerParseName(t *testing.T) { {namer.InstanceGroup(), &NameComponents{uid, "ig", ""}}, {namer.TargetProxy(lbName, HTTPProtocol), &NameComponents{uid, "tp", ""}}, {namer.TargetProxy(lbName, HTTPSProtocol), &NameComponents{uid, "tps", ""}}, - {namer.SSLCertName(certPrefix, secretHash), &NameComponents{uid, "ssl", ""}}, - {namer.SSLCertName(certPrefix, secretHash), &NameComponents{uid, "ssl", ""}}, + {namer.SSLCertName("default/my-ing", secretHash), &NameComponents{uid, "ssl", ""}}, + {namer.SSLCertName("default/my-ing", secretHash), &NameComponents{uid, "ssl", ""}}, {namer.ForwardingRule(lbName, HTTPProtocol), &NameComponents{uid, "fw", ""}}, {namer.ForwardingRule(lbName, HTTPSProtocol), &NameComponents{uid, "fws", ""}}, {namer.UrlMap(lbName), &NameComponents{uid, "um", ""}}, @@ -125,15 +124,14 @@ func TestNameBelongsToCluster(t *testing.T) { for _, prefix := range []string{defaultPrefix, "mci"} { namer := NewNamerWithPrefix(prefix, uid, "fw1") lbName := namer.LoadBalancer("key1") - certPrefix := namer.SSLCertPrefix("key1") // Positive cases. for _, tc := range []string{ namer.Backend(80), namer.InstanceGroup(), namer.TargetProxy(lbName, HTTPProtocol), namer.TargetProxy(lbName, HTTPSProtocol), - namer.SSLCertName(certPrefix, secretHash), - namer.SSLCertName(certPrefix, secretHash), + namer.SSLCertName("default/my-ing", secretHash), + namer.SSLCertName("default/my-ing", secretHash), namer.ForwardingRule(lbName, HTTPProtocol), namer.ForwardingRule(lbName, HTTPSProtocol), namer.UrlMap(lbName), @@ -269,8 +267,6 @@ func TestNamerFirewallRule(t *testing.T) { func TestNamerLoadBalancer(t *testing.T) { secretHash := fmt.Sprintf("%x", sha256.Sum256([]byte("test123")))[:16] - // namespaceHash is calculated the same way as cert hash - namespaceHash := fmt.Sprintf("%x", sha256.Sum256([]byte("key1")))[:16] for _, tc := range []struct { prefix string @@ -287,7 +283,7 @@ func TestNamerLoadBalancer(t *testing.T) { "key1--uid1", "k8s-tp-key1--uid1", "k8s-tps-key1--uid1", - fmt.Sprintf("k8s-ssl-%s--uid1-%s", namespaceHash, secretHash), + "k8s-ssl-%s-%s--uid1", "k8s-fw-key1--uid1", "k8s-fws-key1--uid1", "k8s-um-key1--uid1", @@ -297,7 +293,7 @@ func TestNamerLoadBalancer(t *testing.T) { "key1--uid1", "mci-tp-key1--uid1", "mci-tps-key1--uid1", - fmt.Sprintf("mci-ssl-%s--uid1-%s", namespaceHash, secretHash), + "mci-ssl-%s-%s--uid1", "mci-fw-key1--uid1", "mci-fws-key1--uid1", "mci-um-key1--uid1", @@ -305,7 +301,10 @@ func TestNamerLoadBalancer(t *testing.T) { } { namer := NewNamerWithPrefix(tc.prefix, "uid1", "fw1") lbName := namer.LoadBalancer("key1") - certPrefix := namer.SSLCertPrefix("key1") + // namespaceHash is calculated the same way as cert hash + namespaceHash := fmt.Sprintf("%x", sha256.Sum256([]byte(lbName)))[:16] + tc.sslCert = fmt.Sprintf(tc.sslCert, namespaceHash, secretHash) + if lbName != tc.lbName { t.Errorf("lbName = %q, want %q", lbName, "key1--uid1") } @@ -318,7 +317,7 @@ func TestNamerLoadBalancer(t *testing.T) { if name != tc.targetHTTPSProxy { t.Errorf("namer.TargetProxy(%q, HTTPSProtocol) = %q, want %q", lbName, name, tc.targetHTTPSProxy) } - name = namer.SSLCertName(certPrefix, secretHash) + name = namer.SSLCertName(lbName, secretHash) if name != tc.sslCert { t.Errorf("namer.SSLCertName(%q, true) = %q, want %q", lbName, name, tc.sslCert) } @@ -337,6 +336,42 @@ func TestNamerLoadBalancer(t *testing.T) { } } +func TestNamerIsCertUsedForLB(t *testing.T) { + cases := map[string]struct { + prefix string + ingName string + secretValue string + }{ + "short ingress name": { + prefix: "k8s", + ingName: "default/my-ingress", + secretValue: "test123321test", + }, + "long ingress name": { + prefix: "k8s", + ingName: "a-very-long-and-useless-namespace-value/a-very-long-and-nondescript-ingress-name", + secretValue: "test123321test", + }, + "long ingress name with mci prefix": { + prefix: "mci", + ingName: "a-very-long-and-useless-namespace-value/a-very-long-and-nondescript-ingress-name", + secretValue: "test123321test", + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + namer := NewNamerWithPrefix(tc.prefix, "cluster-uid", "fw1") + lbName := namer.LoadBalancer(tc.ingName) + secretHash := fmt.Sprintf("%x", sha256.Sum256([]byte(tc.secretValue)))[:16] + + certName := namer.SSLCertName(lbName, secretHash) + if v := namer.IsCertUsedForLB(lbName, certName); !v { + t.Errorf("namer.IsCertUsedForLB(%q, %q) = %v, want %v", lbName, certName, v, true) + } + }) + } +} + func TestNamerNEG(t *testing.T) { longstring := "01234567890123456789012345678901234567890123456789" testCases := []struct {