From 52ecdf0b461f515389606484514b062c99366306 Mon Sep 17 00:00:00 2001 From: Fernando Diaz Date: Sun, 15 Jul 2018 17:24:16 -0500 Subject: [PATCH] Add Better Error Handling for SSLSessionTicketKey Adds more error handling when writing an SSLSessionTicketKey to the config map. Also adds tests and makes the function for modular. Fixes #2756 --- .../nginx-configuration/configmap.md | 3 +- internal/ingress/controller/config/config.go | 2 +- internal/ingress/controller/store/store.go | 41 ++++++++++++++----- .../ingress/controller/store/store_test.go | 39 ++++++++++++++++++ 4 files changed, 72 insertions(+), 13 deletions(-) diff --git a/docs/user-guide/nginx-configuration/configmap.md b/docs/user-guide/nginx-configuration/configmap.md index bd5b4af617..e37c87fae5 100644 --- a/docs/user-guide/nginx-configuration/configmap.md +++ b/docs/user-guide/nginx-configuration/configmap.md @@ -434,8 +434,9 @@ Enables or disables session resumption through [TLS session tickets](http://ngin ## ssl-session-ticket-key Sets the secret key used to encrypt and decrypt TLS session tickets. The value must be a valid base64 string. +To create a ticket: `openssl rand 80 | openssl enc -A -base64` -[TLS session ticket-key](http://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_session_tickets), by default, a randomly generated key is used. To create a ticket: `openssl rand 80 | base64 -w0` +[TLS session ticket-key](http://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_session_tickets), by default, a randomly generated key is used. ## ssl-session-timeout diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go index b6862a5eb0..95538ac506 100644 --- a/internal/ingress/controller/config/config.go +++ b/internal/ingress/controller/config/config.go @@ -303,7 +303,7 @@ type Configuration struct { // Sets the secret key used to encrypt and decrypt TLS session tickets. // http://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_session_tickets // By default, a randomly generated key is used. - // Example: openssl rand 80 | base64 -w0 + // Example: openssl rand 80 | openssl enc -A -base64 SSLSessionTicketKey string `json:"ssl-session-ticket-key,omitempty"` // Time during which a client may reuse the session parameters stored in a cache. diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go index 869fa4ab06..a0a6a6fac2 100644 --- a/internal/ingress/controller/store/store.go +++ b/internal/ingress/controller/store/store.go @@ -521,8 +521,8 @@ func New(checkOCSP bool, if err != nil { glog.Warningf("Unexpected error reading configuration configmap: %v", err) } - store.setConfig(cm) + store.setConfig(cm) return store } @@ -708,6 +708,34 @@ func (s k8sStore) GetAuthCertificate(name string) (*resolver.AuthSSLCert, error) }, nil } +func (s k8sStore) writeSSLSessionTicketKey(cmap *corev1.ConfigMap, fileName string) { + ticketString := ngx_template.ReadConfig(cmap.Data).SSLSessionTicketKey + s.backendConfig.SSLSessionTicketKey = "" + + if ticketString != "" { + ticketBytes := base64.StdEncoding.WithPadding(base64.StdPadding).DecodedLen(len(ticketString)) + + // 81 used instead of 80 because of padding + if !(ticketBytes == 48 || ticketBytes == 81) { + glog.Warningf("ssl-session-ticket-key must contain either 48 or 80 bytes") + } + + decodedTicket, err := base64.StdEncoding.DecodeString(ticketString) + if err != nil { + glog.Errorf("unexpected error decoding ssl-session-ticket-key: %v", err) + return + } + + err = ioutil.WriteFile(fileName, decodedTicket, file.ReadWriteByUser) + if err != nil { + glog.Errorf("unexpected error writing ssl-session-ticket-key to %s: %v", fileName, err) + return + } + + s.backendConfig.SSLSessionTicketKey = ticketString + } +} + // GetDefaultBackend returns the default backend func (s k8sStore) GetDefaultBackend() defaults.Backend { return s.backendConfig.Backend @@ -719,16 +747,7 @@ func (s k8sStore) GetBackendConfiguration() ngx_config.Configuration { func (s *k8sStore) setConfig(cmap *corev1.ConfigMap) { s.backendConfig = ngx_template.ReadConfig(cmap.Data) - - // TODO: this should not be done here - if s.backendConfig.SSLSessionTicketKey != "" { - d, err := base64.StdEncoding.DecodeString(s.backendConfig.SSLSessionTicketKey) - if err != nil { - glog.Warningf("unexpected error decoding key ssl-session-ticket-key: %v", err) - s.backendConfig.SSLSessionTicketKey = "" - } - ioutil.WriteFile("/etc/nginx/tickets.key", d, file.ReadWriteByUser) - } + s.writeSSLSessionTicketKey(cmap, "/etc/nginx/tickets.key") } // Run initiates the synchronization of the informers and the initial diff --git a/internal/ingress/controller/store/store_test.go b/internal/ingress/controller/store/store_test.go index c329e03b8e..9ea2148c5e 100644 --- a/internal/ingress/controller/store/store_test.go +++ b/internal/ingress/controller/store/store_test.go @@ -33,6 +33,9 @@ import ( "k8s.io/client-go/kubernetes" "k8s.io/client-go/tools/cache" + "encoding/base64" + "io/ioutil" + "k8s.io/api/core/v1" "k8s.io/ingress-nginx/internal/file" "k8s.io/ingress-nginx/internal/ingress/annotations/parser" "k8s.io/ingress-nginx/test/e2e/framework" @@ -828,3 +831,39 @@ func TestListIngresses(t *testing.T) { t.Errorf("Expected 3 Ingresses but got %v", s) } } + +func TestWriteSSLSessionTicketKey(t *testing.T) { + tests := []string{ + "9DyULjtYWz520d1rnTLbc4BOmN2nLAVfd3MES/P3IxWuwXkz9Fby0lnOZZUdNEMV", + "9SvN1C9AB5DvNde5fMKoJwAwICpqdjiMyxR+cv6NpAWv22rFd3gKt4wMyGxCm7l9Wh6BQPG0+csyBZSHHr2NOWj52Wx8xCegXf4NsSMBUqA=", + } + + for _, test := range tests { + s := newStore(t) + + cmap := &v1.ConfigMap{ + Data: map[string]string{ + "ssl-session-ticket-key": test, + }, + } + + f, err := ioutil.TempFile("", "ssl-session-ticket-test") + if err != nil { + t.Fatal(err) + } + + s.writeSSLSessionTicketKey(cmap, f.Name()) + + content, err := ioutil.ReadFile(f.Name()) + if err != nil { + t.Fatal(err) + } + encodedContent := base64.StdEncoding.EncodeToString(content) + + f.Close() + + if test != encodedContent { + t.Fatalf("expected %v but returned %s", test, encodedContent) + } + } +}