From ff9e804d9a98c9a7e21a65715764e5b1600356a4 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Sun, 12 Nov 2017 22:43:28 -0300 Subject: [PATCH] Refactor SSL intermediate CA certificate check --- cmd/nginx/flags.go | 52 +++++++++++++--------- hack/update-codegen.sh | 2 +- internal/ingress/controller/backend_ssl.go | 46 ++++++++++++++++++- internal/ingress/controller/controller.go | 2 + internal/ingress/controller/nginx.go | 4 ++ internal/ingress/types_equals.go | 37 +++++++++++++++ internal/net/ssl/ssl.go | 38 ++++++++-------- 7 files changed, 139 insertions(+), 42 deletions(-) diff --git a/cmd/nginx/flags.go b/cmd/nginx/flags.go index 2029f4d78c..e58230cc02 100644 --- a/cmd/nginx/flags.go +++ b/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, diff --git a/hack/update-codegen.sh b/hack/update-codegen.sh index dfe7cd546f..559362ac56 100755 --- a/hack/update-codegen.sh +++ b/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})/../../.." diff --git a/internal/ingress/controller/backend_ssl.go b/internal/ingress/controller/backend_ssl.go index 272b24c37b..a4f3f138fa 100644 --- a/internal/ingress/controller/backend_ssl.go +++ b/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. diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index 0aece8cf50..fb96b12fce 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -106,6 +106,8 @@ type Configuration struct { EnableProfiling bool + EnableSSLChainCompletion bool + FakeCertificatePath string FakeCertificateSHA string } diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go index 69e596eb3d..b74c05cb08 100644 --- a/internal/ingress/controller/nginx.go +++ b/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) } diff --git a/internal/ingress/types_equals.go b/internal/ingress/types_equals.go index c9f6c9d91c..c86e33e7a4 100644 --- a/internal/ingress/types_equals.go +++ b/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 +} diff --git a/internal/net/ssl/ssl.go b/internal/net/ssl/ssl.go index 0b9b791ca8..5efc075039 100644 --- a/internal/net/ssl/ssl.go +++ b/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 }