Merge pull request #1699 from aledbf/disable-cert-chain-validation

Refactor SSL intermediate CA certificate check

cmd/nginx/flags.go

@@ -124,6 +124,11 @@ func parseFlags() (bool, *controller.Configuration, error) {
 		healthzPort   = flags.Int("healthz-port", 10254, "port for healthz endpoint.")
 
 		annotationsPrefix = flags.String("annotations-prefix", "nginx.ingress.kubernetes.io", `Prefix of the ingress annotations.`)
+
+		enableSSLChainCompletion = flags.Bool("enable-ssl-chain-completion", true,
+			`Defines if the nginx ingress controller should check the secrets for missing intermediate CA certificates.
+		If the certificate contain issues chain issues is not possible to enable OCSP.
+		Default is true.`)
 	)
 
 	flag.Set("logtostderr", "true")
@@ -178,28 +183,33 @@ func parseFlags() (bool, *controller.Configuration, error) {
 		glog.Warningf("%s is DEPRECATED and will be removed in a future version.", disableNodeList)
 	}
 
+	if !*enableSSLChainCompletion {
+		glog.Warningf("Check of SSL certificate chain is disabled (--enable-ssl-chain-completion=false)")
+	}
+
 	config := &controller.Configuration{
-		AnnotationsPrefix:       *annotationsPrefix,
-		APIServerHost:           *apiserverHost,
-		KubeConfigFile:          *kubeConfigFile,
-		UpdateStatus:            *updateStatus,
-		ElectionID:              *electionID,
-		EnableProfiling:         *profiling,
-		EnableSSLPassthrough:    *enableSSLPassthrough,
-		ResyncPeriod:            *resyncPeriod,
-		DefaultService:          *defaultSvc,
-		IngressClass:            *ingressClass,
-		Namespace:               *watchNamespace,
-		ConfigMapName:           *configMap,
-		TCPConfigMapName:        *tcpConfigMapName,
-		UDPConfigMapName:        *udpConfigMapName,
-		DefaultSSLCertificate:   *defSSLCertificate,
-		DefaultHealthzURL:       *defHealthzURL,
-		PublishService:          *publishSvc,
-		ForceNamespaceIsolation: *forceIsolation,
-		UpdateStatusOnShutdown:  *updateStatusOnShutdown,
-		SortBackends:            *sortBackends,
-		UseNodeInternalIP:       *useNodeInternalIP,
+		AnnotationsPrefix:        *annotationsPrefix,
+		APIServerHost:            *apiserverHost,
+		KubeConfigFile:           *kubeConfigFile,
+		UpdateStatus:             *updateStatus,
+		ElectionID:               *electionID,
+		EnableProfiling:          *profiling,
+		EnableSSLPassthrough:     *enableSSLPassthrough,
+		EnableSSLChainCompletion: *enableSSLChainCompletion,
+		ResyncPeriod:             *resyncPeriod,
+		DefaultService:           *defaultSvc,
+		IngressClass:             *ingressClass,
+		Namespace:                *watchNamespace,
+		ConfigMapName:            *configMap,
+		TCPConfigMapName:         *tcpConfigMapName,
+		UDPConfigMapName:         *udpConfigMapName,
+		DefaultSSLCertificate:    *defSSLCertificate,
+		DefaultHealthzURL:        *defHealthzURL,
+		PublishService:           *publishSvc,
+		ForceNamespaceIsolation:  *forceIsolation,
+		UpdateStatusOnShutdown:   *updateStatusOnShutdown,
+		SortBackends:             *sortBackends,
+		UseNodeInternalIP:        *useNodeInternalIP,
 		ListenPorts: &ngx_config.ListenPorts{
 			Default:  *defServerPort,
 			Health:   *healthzPort,

hack/update-codegen.sh

@@ -31,6 +31,6 @@ CODEGEN_PKG=${CODEGEN_PKG:-$(cd ${SCRIPT_ROOT}; ls -d -1 ./vendor/k8s.io/code-ge
 #  --output-base "$(dirname ${BASH_SOURCE})/../../.."
 
 ${CODEGEN_PKG}/generate-groups.sh "deepcopy" \
-  k8s.io/ingress-nginx/pkg k8s.io/ingress-nginx/pkg \
+  k8s.io/ingress-nginx/internal k8s.io/ingress-nginx/internal \
   .:ingress \
   --output-base "$(dirname ${BASH_SOURCE})/../../.."

internal/ingress/controller/backend_ssl.go

@@ -18,10 +18,11 @@ package controller
 
 import (
 	"fmt"
-	"reflect"
+	"io/ioutil"
 	"strings"
 
 	"github.com/golang/glog"
+	"github.com/imdario/mergo"
 
 	apiv1 "k8s.io/api/core/v1"
 	extensions "k8s.io/api/extensions/v1beta1"
@@ -48,7 +49,7 @@ func (ic *NGINXController) syncSecret(key string) {
 	cur, exists := ic.sslCertTracker.Get(key)
 	if exists {
 		s := cur.(*ingress.SSLCert)
-		if reflect.DeepEqual(s, cert) {
+		if s.Equal(cert) {
 			// no need to update
 			return
 		}
@@ -123,6 +124,47 @@ func (ic *NGINXController) getPemCertificate(secretName string) (*ingress.SSLCer
 	return s, nil
 }
 
+func (ic *NGINXController) checkSSLChainIssues() {
+	for _, secretName := range ic.sslCertTracker.ListKeys() {
+		s, _ := ic.sslCertTracker.Get(secretName)
+		secret := s.(*ingress.SSLCert)
+
+		if secret.FullChainPemFileName != "" {
+			// chain already checked
+			continue
+		}
+
+		data, err := ssl.FullChainCert(secret.PemFileName)
+		if err != nil {
+			glog.Errorf("unexpected error generating SSL certificate with full intermediate chain CA certs: %v", err)
+			continue
+		}
+
+		fullChainPemFileName := fmt.Sprintf("%v/%v-%v-full-chain.pem", ingress.DefaultSSLDirectory, secret.Namespace, secret.Name)
+		err = ioutil.WriteFile(fullChainPemFileName, data, 0655)
+		if err != nil {
+			glog.Errorf("unexpected error creating SSL certificate: %v", err)
+			continue
+		}
+
+		dst := &ingress.SSLCert{}
+
+		err = mergo.MergeWithOverwrite(dst, secret)
+		if err != nil {
+			glog.Errorf("unexpected error creating SSL certificate: %v", err)
+			continue
+		}
+
+		dst.FullChainPemFileName = fullChainPemFileName
+
+		glog.Infof("updating local copy of ssl certificate %v with missing intermediate CA certs", secretName)
+		ic.sslCertTracker.Update(secretName, dst)
+		// this update must trigger an update
+		// (like an update event from a change in Ingress)
+		ic.syncQueue.Enqueue(&extensions.Ingress{})
+	}
+}
+
 // checkMissingSecrets verify if one or more ingress rules contains a reference
 // to a secret that is not present in the local secret store.
 // In this case we call syncSecret.

internal/ingress/controller/controller.go

@@ -106,6 +106,8 @@ type Configuration struct {
 
 	EnableProfiling bool
 
+	EnableSSLChainCompletion bool
+
 	FakeCertificatePath string
 	FakeCertificateSHA  string
 }

internal/ingress/controller/nginx.go

@@ -257,6 +257,10 @@ func (n *NGINXController) Start() {
 
 	go n.syncQueue.Run(time.Second, n.stopCh)
 
+	if n.cfg.EnableSSLChainCompletion {
+		go wait.Until(n.checkSSLChainIssues, 60*time.Second, n.stopCh)
+	}
+
 	if n.syncStatus != nil {
 		go n.syncStatus.Run(n.stopCh)
 	}

internal/ingress/types_equals.go

@@ -271,6 +271,9 @@ func (s1 *Server) Equal(s2 *Server) bool {
 	if !(&s1.CertificateAuth).Equal(&s2.CertificateAuth) {
 		return false
 	}
+	if s1.SSLFullChainCertificate != s2.SSLFullChainCertificate {
+		return false
+	}
 	if s1.RedirectFromToWWW != s2.RedirectFromToWWW {
 		return false
 	}
@@ -461,3 +464,37 @@ func (l4b1 *L4Backend) Equal(l4b2 *L4Backend) bool {
 
 	return true
 }
+
+// Equal tests for equality between two L4Backend types
+func (s1 *SSLCert) Equal(s2 *SSLCert) bool {
+	if s1 == s2 {
+		return true
+	}
+	if s1 == nil || s2 == nil {
+		return false
+	}
+	if s1.PemFileName != s2.PemFileName {
+		return false
+	}
+	if s1.PemSHA != s2.PemSHA {
+		return false
+	}
+	if !s1.ExpireTime.Equal(s2.ExpireTime) {
+		return false
+	}
+
+	for _, cn1 := range s1.CN {
+		found := false
+		for _, cn2 := range s2.CN {
+			if cn1 == cn2 {
+				found = true
+				break
+			}
+		}
+		if !found {
+			return false
+		}
+	}
+
+	return true
+}

internal/net/ssl/ssl.go

@@ -50,8 +50,6 @@ var (
 func AddOrUpdateCertAndKey(name string, cert, key, ca []byte) (*ingress.SSLCert, error) {
 	pemName := fmt.Sprintf("%v.pem", name)
 	pemFileName := fmt.Sprintf("%v/%v", ingress.DefaultSSLDirectory, pemName)
-	fullChainPemFileName := fmt.Sprintf("%v/%v-full-chain.pem", ingress.DefaultSSLDirectory, name)
-
 	tempPemFile, err := ioutil.TempFile(ingress.DefaultSSLDirectory, pemName)
 
 	if err != nil {
@@ -180,14 +178,6 @@ func AddOrUpdateCertAndKey(name string, cert, key, ca []byte) (*ingress.SSLCert,
 		ExpireTime:  pemCert.NotAfter,
 	}
 
-	err = fullChainCert(pemFileName, fullChainPemFileName)
-	if err != nil {
-		glog.Errorf("unexpected error generating SSL certificate with full chain: %v", err)
-		return s, nil
-	}
-
-	s.FullChainPemFileName = fullChainPemFileName
-
 	return s, nil
 }
 
@@ -389,32 +379,44 @@ func GetFakeSSLCert() ([]byte, []byte) {
 	return cert, key
 }
 
-func fullChainCert(in, out string) error {
+// FullChainCert checks if a certificate file contains issues in the intermediate CA chain
+// Returns a new certificate with the intermediate certificates.
+// If the certificate does not contains issues with the chain it return an empty byte array
+func FullChainCert(in string) ([]byte, error) {
 	inputFile, err := os.Open(in)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	data, err := ioutil.ReadAll(inputFile)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	cert, err := certUtil.DecodeCertificate(data)
 	if err != nil {
-		return err
+		return nil, err
+	}
+
+	certPool := x509.NewCertPool()
+	certPool.AddCert(cert)
+
+	_, err = cert.Verify(x509.VerifyOptions{
+		Intermediates: certPool,
+	})
+	if err == nil {
+		return nil, nil
 	}
 
 	certs, err := certUtil.FetchCertificateChain(cert)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	certs, err = certUtil.AddRootCA(certs)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
-	data = certUtil.EncodeCertificates(certs)
-	return ioutil.WriteFile(out, data, 0644)
+	return certUtil.EncodeCertificates(certs), nil
 }