From 9ffa23beaa9ebb235dd4739fdbf6968993103fbc Mon Sep 17 00:00:00 2001 From: Nick Sardo Date: Fri, 21 Apr 2017 00:58:37 -0700 Subject: [PATCH] Harden ssl cert logic to handle unknown state --- .../gce/loadbalancers/loadbalancers.go | 84 +++++++++++++------ 1 file changed, 57 insertions(+), 27 deletions(-) diff --git a/controllers/gce/loadbalancers/loadbalancers.go b/controllers/gce/loadbalancers/loadbalancers.go index 9972ccc01..10754bc8b 100644 --- a/controllers/gce/loadbalancers/loadbalancers.go +++ b/controllers/gce/loadbalancers/loadbalancers.go @@ -352,25 +352,50 @@ func (l *L7) deleteOldSSLCert() (err error) { return nil } -func (l *L7) checkSSLCert() (err error) { - certName := l.runtimeInfo.TLSName +// Returns the name portion of a link - which is the last section +func getResourceNameFromLink(link string) string { + s := strings.Split(link, "/") + if len(s) == 0 { + return "" + } + return s[len(s)-1] +} +func (l *L7) checkSSLCert() (err error) { + // Handle Pre-Shared cert + preSharedCertName := l.runtimeInfo.TLSName // Use the named GCE cert when it is specified by the annotation. - if certName != "" { + if preSharedCertName != "" { // Ask GCE for the cert, checking for problems and existence. - cert, err := l.cloud.GetSslCertificate(certName) + cert, err := l.cloud.GetSslCertificate(preSharedCertName) if err != nil { return err } if cert == nil { - return fmt.Errorf("cannot find existing sslCertificate %v for %v", certName, l.Name) + return fmt.Errorf("cannot find existing sslCertificate %v for %v", preSharedCertName, l.Name) } - glog.Infof("Using existing sslCertificate %v for %v", certName, l.Name) + glog.V(2).Infof("Using existing sslCertificate %v for %v", preSharedCertName, l.Name) l.sslCert = cert return nil } + // Handle secret-created cert + // Determine what certificate name is being used + var expectedCertName string + if l.sslCert != nil { + expectedCertName = l.sslCert.Name + } else { + // Retrieve the ssl certificate in use by the expected target proxy (if exists) + expectedCertName = getResourceNameFromLink(l.getSslCertInUse()) + } + + // Retrieve known ssl certificate + var usedCert *compute.SslCertificate + if expectedCertName != "" { + usedCert, _ = l.cloud.GetSslCertificate(expectedCertName) + } + // TODO: Currently, GCE only supports a single certificate per static IP // so we don't need to bother with disambiguation. Naming the cert after // the loadbalancer is a simplification. @@ -384,32 +409,28 @@ func (l *L7) checkSSLCert() (err error) { // TODO: Clean this code up into a ring buffer. primaryCertName := l.namer.Truncate(fmt.Sprintf("%v-%v", sslCertPrefix, l.Name)) secondaryCertName := l.namer.Truncate(fmt.Sprintf("%v-%d-%v", sslCertPrefix, 1, l.Name)) - certName = primaryCertName - if l.sslCert != nil { - certName = l.sslCert.Name - } - - // Skip error checking because error-ing out will retry and loop, when we - // should create/update the cert if there is an error or does not exist. - cert, _ := l.cloud.GetSslCertificate(certName) // 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, // but a bug could end up leaking it, which feels worse. - if cert == nil || ingCert != cert.Certificate { - - certChanged := cert != nil && (ingCert != cert.Certificate) - if certChanged { - if certName == primaryCertName { - certName = secondaryCertName - } else { - certName = primaryCertName + if usedCert == nil || ingCert != usedCert.Certificate { + newCertName := primaryCertName + if usedCert != nil && (ingCert != usedCert.Certificate) { + if usedCert.Name == primaryCertName { + newCertName = secondaryCertName } } - glog.Infof("Creating new sslCertificates %v for %v", certName, l.Name) - cert, err = l.cloud.CreateSslCertificate(&compute.SslCertificate{ - Name: certName, + // Perform a delete in case a certificate exists with the exact name + // This certificate should be unused since we check the target proxy's certificate prior + // to this point. Although, it's possible an actor pointed a target proxy to this certificate. + if err = l.cloud.DeleteSslCertificate(newCertName); err != nil && !utils.IsHTTPErrorCode(err, http.StatusNotFound) { + return fmt.Errorf("unable to delete ssl certificate with name %q, expected it to be unused. err: %v", newCertName, err) + } + + glog.Infof("Creating new sslCertificates %v for %v", newCertName, l.Name) + cert, err := l.cloud.CreateSslCertificate(&compute.SslCertificate{ + Name: newCertName, Certificate: ingCert, PrivateKey: ingKey, }) @@ -417,13 +438,22 @@ func (l *L7) checkSSLCert() (err error) { return err } // Save the current cert for cleanup after we update the target proxy. - l.oldSSLCert = l.sslCert + l.oldSSLCert = usedCert + l.sslCert = cert } - l.sslCert = cert return nil } +func (l *L7) getSslCertInUse() string { + proxyName := l.namer.Truncate(fmt.Sprintf("%v-%v", targetHTTPSProxyPrefix, l.Name)) + proxy, _ := l.cloud.GetTargetHttpsProxy(proxyName) + if proxy != nil && len(proxy.SslCertificates) > 0 { + return proxy.SslCertificates[0] + } + return "" +} + func (l *L7) checkHttpsProxy() (err error) { if l.sslCert == nil { glog.V(3).Infof("No SSL certificates for %v, will not create HTTPS proxy.", l.Name)