From a9ab81b18fad7ba8d88a5012658025a99dc12e4e Mon Sep 17 00:00:00 2001 From: Alexander Yastrebov Date: Mon, 27 Sep 2021 22:34:46 +0200 Subject: [PATCH] Implements HTTPS graceful shutdown (#1866) Fixes #1865 Signed-off-by: Alexander Yastrebov --- fixtures/gencert.sh | 10 ++ fixtures/test2.crt | 22 +++++ fixtures/test2.key | 28 ++++++ skipper.go | 92 ++++++++++-------- skipper_test.go | 221 +++++++++++++++++++------------------------- 5 files changed, 209 insertions(+), 164 deletions(-) create mode 100755 fixtures/gencert.sh create mode 100644 fixtures/test2.crt create mode 100644 fixtures/test2.key diff --git a/fixtures/gencert.sh b/fixtures/gencert.sh new file mode 100755 index 0000000000..eb04f403c3 --- /dev/null +++ b/fixtures/gencert.sh @@ -0,0 +1,10 @@ +#!/bin/bash + +if [[ $# -eq 0 ]] ; then + echo 'Certificate name required' + exit 1 +fi + +openssl req -x509 -sha256 -nodes -newkey rsa:2048 -keyout "$1.key" -out "$1.crt" \ + -days 3650 \ + -subj "/C=DE/ST=Berlin/O=Zalando SE/OU=Technology/CN=do-not-trust-test-data" diff --git a/fixtures/test2.crt b/fixtures/test2.crt new file mode 100644 index 0000000000..38662ae21b --- /dev/null +++ b/fixtures/test2.crt @@ -0,0 +1,22 @@ +-----BEGIN CERTIFICATE----- +MIIDszCCApugAwIBAgIUfXes2jmw6GNLTEObvIsMrlx4NX0wDQYJKoZIhvcNAQEL +BQAwaTELMAkGA1UEBhMCREUxDzANBgNVBAgMBkJlcmxpbjETMBEGA1UECgwKWmFs +YW5kbyBTRTETMBEGA1UECwwKVGVjaG5vbG9neTEfMB0GA1UEAwwWZG8tbm90LXRy +dXN0LXRlc3QtZGF0YTAeFw0yMTA5MjQxMTQ1MjBaFw0zMTA5MjIxMTQ1MjBaMGkx +CzAJBgNVBAYTAkRFMQ8wDQYDVQQIDAZCZXJsaW4xEzARBgNVBAoMClphbGFuZG8g +U0UxEzARBgNVBAsMClRlY2hub2xvZ3kxHzAdBgNVBAMMFmRvLW5vdC10cnVzdC10 +ZXN0LWRhdGEwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDLvAr/DUEe +2T+TzPjoT/Q32892rNXz3p/M4RopdqJWLEZwzASh8Z7FiLBiGOIsWrGhWluwqQCy +nrpulZ/sQOqi8KYjyhsvlRV68L9wczX7GZScxnMCfvsEAJJD0E9GqwtOal+yCmvx +59/5V7sLD6JzrPZWoUL4qEEXk1CoGCjSHGjHUPvVqX1oZmyzYKh73oiUKQ//DqYR +9zhUaGO01R1QvNISMqFMsdYLW9SsatuRiyriAfTsslbIg3jQ+l/n7Y7YztJvbrbe +gB6cMqrignXskNtlqZnEGbJgiX8Ko7m4Y7ttmOMcawLqhwWxzvKj9iBERL6irxqr +Q15mpV915O4vAgMBAAGjUzBRMB0GA1UdDgQWBBSJv18vO9LQKJsCkTiIZ0vhI2xb +KjAfBgNVHSMEGDAWgBSJv18vO9LQKJsCkTiIZ0vhI2xbKjAPBgNVHRMBAf8EBTAD +AQH/MA0GCSqGSIb3DQEBCwUAA4IBAQC0o9QFpmTe0YC5sfRhJcjmejGggS4IZB5U +KQ6/S+Xk/W4UitQCo7iQot+ZF5HOmaGX6FtpUCSx1JeSWxAkzbl8LK1dCO6YNXJ6 +2EzkPthcf5EJkJ9rpCRL5dAoPSXr1EJut646jlR/hwlxOEWCOr7zYQK136hc+HFA +6No4tshp2l4mT+OfR3wn8Ae1hH+tyuWzXGlBdUlm/2OtsUwHjHKeiv/ZnQ63K+gg +nxdkUvQL07O0Avz9ttz9yZ9jYAi00zjotG7e+buSbarGBTuXXxnlG4+ISclQikJ4 +m/ApwEDTocYpOJgsdn+bysY/F2nntrAQ1Sof2E6N5ww9sR8taGe3 +-----END CERTIFICATE----- diff --git a/fixtures/test2.key b/fixtures/test2.key new file mode 100644 index 0000000000..1ebdaffd58 --- /dev/null +++ b/fixtures/test2.key @@ -0,0 +1,28 @@ +-----BEGIN PRIVATE KEY----- +MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQDLvAr/DUEe2T+T +zPjoT/Q32892rNXz3p/M4RopdqJWLEZwzASh8Z7FiLBiGOIsWrGhWluwqQCynrpu +lZ/sQOqi8KYjyhsvlRV68L9wczX7GZScxnMCfvsEAJJD0E9GqwtOal+yCmvx59/5 +V7sLD6JzrPZWoUL4qEEXk1CoGCjSHGjHUPvVqX1oZmyzYKh73oiUKQ//DqYR9zhU +aGO01R1QvNISMqFMsdYLW9SsatuRiyriAfTsslbIg3jQ+l/n7Y7YztJvbrbegB6c +MqrignXskNtlqZnEGbJgiX8Ko7m4Y7ttmOMcawLqhwWxzvKj9iBERL6irxqrQ15m +pV915O4vAgMBAAECggEATQPKjFuwUD8Dn5WOShNfWHZJWK1BO6zeb45wW1gzSav2 +/NDCt40k3bssIgkSBn5KQ5pqqr9YOi1ygDcjeyWXDP03cLQHztbmhdDYLWP/9enX +meQSudDShtLId8YZEbe60Gu5vQ3ffFSRACq/1BCW8m9ht6HCNUk1Qfo4NTLcy3+x +F8gI65jGTPiZzJ7eHz7576KBcgeArv14DSm+jPjkBiKWzrUb9sV8elaps5j6yIjL +glMimWhlTuajt/9d8QD/qN+uR67OYcWtflHnRI4FSRrwyPfN80tK3a9WjpuBKlFZ +MwLaNU3gCw2x9VW4xVOKyFhuQzj7uHUn+vUCWjr5UQKBgQDm5XXUzRu5dRx6n5sR +P3TrIs8UocKOuaNRgp3+aYjhF578szGP9lLG/he58w7irTYEZxvdwCRSE7HADwQv +GUZmyyVtwSuO4yE0xVOVtf02SRJnn7akAWy/CL4V87RbpFGYDXdNtN/0600VaYxO +UxKjw/K9yeCoAZv9Q+fPXKlO7QKBgQDh4paieu1ZvwNcRX1FFYwtq5YHTelL4AD/ +CNQXZLlIp2i4Z0rcrfaIovb5sh5sJ+WXH5YJ0VIzgNe6tuTKMU6Cc45UF4Hb0ZTx +sVjAqZp+6KnajOHBdkc0w5c0Hv6UpAKPdZUzsKlb5r0kT4nRPB1/Ov3CUAqJi/JQ +FZtYgz1yCwKBgAxXcX/pYrT8BIStaU13tdknqCfzKYIVfBxMPgOuQmm9qHrbXSfT +w8LtK/l9e2s0VPHRTRUCQy677MFWTCP0VuYBr8N5Esn1a/31Gi2jZ6ByMXCmgc2s +YdKoNfjYaOiJFO9qsNjPdTUTKrCdTqmVGSb1v1DTrJVuWJcl/QsBae9VAoGAbLHM +KoNck0MHKu+FSCkGOzPGDd2/1XMFB7QH2vns7rkf+xw5Ode8OiOxFJZRbVoFcKMS +X8cJ9x6YsJAxp9nyHXPdmTl2k4BWW7crLgpu/YKXuULxn1Z7DTjRGZOQjZYeZUn/ +cdAgrshpW3+qobR7vS11znsVlvpwr3i2N/FvL+ECgYEAwcvBK5wtd70YweHZ4Rm8 +P8iUsjeVV9LuBT0/0jL46Q48rGxMnHK+hUxXPCp/wl4BrML/lVsTvPZh+KlYQz4C +yk/f2FB2aC8dCFXcsGswVXC0WQGStogkXf56nr05b8Av0LoQ4/REG0NDSoYaApVF +U1EucR/02DUrc+LE0j44D9o= +-----END PRIVATE KEY----- diff --git a/skipper.go b/skipper.go index a083a1fe22..e032898dd1 100644 --- a/skipper.go +++ b/skipper.go @@ -944,8 +944,35 @@ func initLog(o Options) error { return nil } -func (o *Options) isHTTPS() bool { - return (o.ProxyTLS != nil) || (o.CertPathTLS != "" && o.KeyPathTLS != "") +func (o *Options) tlsConfig() (*tls.Config, error) { + if o.ProxyTLS != nil { + return o.ProxyTLS, nil + } + + if o.CertPathTLS == "" && o.KeyPathTLS == "" { + return nil, nil + } + + crts := strings.Split(o.CertPathTLS, ",") + keys := strings.Split(o.KeyPathTLS, ",") + + if len(crts) != len(keys) { + return nil, fmt.Errorf("number of certificates does not match number of keys") + } + + config := &tls.Config{ + MinVersion: o.TLSMinVersion, + } + + for i := 0; i < len(crts); i++ { + crt, key := crts[i], keys[i] + keypair, err := tls.LoadX509KeyPair(crt, key) + if err != nil { + return nil, fmt.Errorf("failed to load X509 keypair from %s and %s: %w", crt, key, err) + } + config.Certificates = append(config.Certificates, keypair) + } + return config, nil } func listen(o *Options, mtr metrics.Metrics) (net.Listener, error) { @@ -1005,11 +1032,14 @@ func listenAndServeQuit( idleConnsCH chan struct{}, mtr metrics.Metrics, ) error { - // create the access log handler - log.Infof("proxy listener on %v", o.Address) + tlsConfig, err := o.tlsConfig() + if err != nil { + return err + } srv := &http.Server{ Addr: o.Address, + TLSConfig: tlsConfig, Handler: proxy, ReadTimeout: o.ReadTimeoutServer, ReadHeaderTimeout: o.ReadHeaderTimeoutServer, @@ -1025,35 +1055,6 @@ func listenAndServeQuit( } } - if o.isHTTPS() { - if o.ProxyTLS != nil { - srv.TLSConfig = o.ProxyTLS - o.CertPathTLS = "" - o.KeyPathTLS = "" - } else if strings.Index(o.CertPathTLS, ",") > 0 && strings.Index(o.KeyPathTLS, ",") > 0 { - tlsCfg := &tls.Config{ - MinVersion: o.TLSMinVersion, - } - crts := strings.Split(o.CertPathTLS, ",") - keys := strings.Split(o.KeyPathTLS, ",") - if len(crts) != len(keys) { - log.Fatalf("number of certs does not match number of keys") - } - for i, crt := range crts { - kp, err := tls.LoadX509KeyPair(crt, keys[i]) - if err != nil { - log.Fatalf("Failed to load X509 keypair from %s/%s: %v", crt, keys[i], err) - } - tlsCfg.Certificates = append(tlsCfg.Certificates, kp) - } - o.CertPathTLS = "" - o.KeyPathTLS = "" - srv.TLSConfig = tlsCfg - } - return srv.ListenAndServeTLS(o.CertPathTLS, o.KeyPathTLS) - } - log.Infof("TLS settings not found, defaulting to HTTP") - // making idleConnsCH and sigs optional parameters is required to be able to tear down a server // from the tests if idleConnsCH == nil { @@ -1079,14 +1080,25 @@ func listenAndServeQuit( close(idleConnsCH) }() - l, err := listen(o, mtr) - if err != nil { - return err - } + log.Infof("proxy listener on %v", o.Address) - if err := srv.Serve(l); err != nil && err != http.ErrServerClosed { - log.Errorf("Failed to start to ListenAndServe: %v", err) - return err + if srv.TLSConfig != nil { + if err := srv.ListenAndServeTLS("", ""); err != http.ErrServerClosed { + log.Errorf("ListenAndServeTLS failed: %v", err) + return err + } + } else { + log.Infof("TLS settings not found, defaulting to HTTP") + + l, err := listen(o, mtr) + if err != nil { + return err + } + + if err := srv.Serve(l); err != http.ErrServerClosed { + log.Errorf("Serve failed: %v", err) + return err + } } <-idleConnsCH diff --git a/skipper_test.go b/skipper_test.go index a815e7c861..326d2ccfeb 100644 --- a/skipper_test.go +++ b/skipper_test.go @@ -6,8 +6,6 @@ import ( "net" "net/http" "os" - "os/signal" - "sync" "syscall" "testing" "time" @@ -20,6 +18,8 @@ import ( "github.com/zalando/skipper/proxy" "github.com/zalando/skipper/ratelimit" "github.com/zalando/skipper/routing" + + "github.com/stretchr/testify/require" ) const ( @@ -73,66 +73,64 @@ func findAddress() (string, error) { return l.Addr().String(), nil } -func TestOptionsDefaultsToHTTP(t *testing.T) { - o := Options{} - if o.isHTTPS() { - t.FailNow() - } -} - -func TestOptionsWithCertUsesHTTPS(t *testing.T) { - o := Options{CertPathTLS: "foo", KeyPathTLS: "bar"} - if !o.isHTTPS() { - t.FailNow() - } +func TestOptionsTLSConfig(t *testing.T) { + cert, err := tls.LoadX509KeyPair("fixtures/test.crt", "fixtures/test.key") + require.NoError(t, err) + + cert2, err := tls.LoadX509KeyPair("fixtures/test2.crt", "fixtures/test2.key") + require.NoError(t, err) + + // empty + o := &Options{} + c, err := o.tlsConfig() + require.NoError(t, err) + require.Nil(t, c) + + // proxy tls config + o = &Options{ProxyTLS: &tls.Config{}} + c, err = o.tlsConfig() + require.NoError(t, err) + require.Equal(t, &tls.Config{}, c) + + // proxy tls config priority + o = &Options{ProxyTLS: &tls.Config{}, CertPathTLS: "fixtures/test.crt", KeyPathTLS: "fixtures/test.key"} + c, err = o.tlsConfig() + require.NoError(t, err) + require.Equal(t, &tls.Config{}, c) + + // cert key path + o = &Options{TLSMinVersion: tls.VersionTLS12, CertPathTLS: "fixtures/test.crt", KeyPathTLS: "fixtures/test.key"} + c, err = o.tlsConfig() + require.NoError(t, err) + require.Equal(t, uint16(tls.VersionTLS12), c.MinVersion) + require.Equal(t, []tls.Certificate{cert}, c.Certificates) + + // multiple cert key paths + o = &Options{TLSMinVersion: tls.VersionTLS13, CertPathTLS: "fixtures/test.crt,fixtures/test2.crt", KeyPathTLS: "fixtures/test.key,fixtures/test2.key"} + c, err = o.tlsConfig() + require.NoError(t, err) + require.Equal(t, uint16(tls.VersionTLS13), c.MinVersion) + require.Equal(t, []tls.Certificate{cert, cert2}, c.Certificates) } -func TestWithWrongCertPathFails(t *testing.T) { - a, err := findAddress() - if err != nil { - t.Fatal(err) - } - - o := Options{Address: a, - CertPathTLS: "fixtures/notFound.crt", - KeyPathTLS: "fixtures/test.key", - } - - rt := routing.New(routing.Options{ - FilterRegistry: builtin.MakeRegistry(), - DataClients: []routing.DataClient{}}) - defer rt.Close() - - proxy := proxy.New(rt, proxy.OptionsNone) - defer proxy.Close() - - err = listenAndServe(proxy, &o) - if err == nil { - t.Fatal(err) - } -} - -func TestWithWrongKeyPathFails(t *testing.T) { - a, err := findAddress() - if err != nil { - t.Fatal(err) - } - - o := Options{Address: a, - CertPathTLS: "fixtures/test.crt", - KeyPathTLS: "fixtures/notFound.key", - } - - rt := routing.New(routing.Options{ - FilterRegistry: builtin.MakeRegistry(), - DataClients: []routing.DataClient{}}) - defer rt.Close() - - proxy := proxy.New(rt, proxy.OptionsNone) - defer proxy.Close() - err = listenAndServe(proxy, &o) - if err == nil { - t.Fatal(err) +func TestOptionsTLSConfigInvalidPaths(t *testing.T) { + for _, tt := range []struct { + name string + options *Options + }{ + {"missing cert path", &Options{KeyPathTLS: "fixtures/test.key"}}, + {"missing key path", &Options{CertPathTLS: "fixtures/test.crt"}}, + {"wrong cert path", &Options{CertPathTLS: "fixtures/notFound.crt", KeyPathTLS: "fixtures/test.key"}}, + {"wrong key path", &Options{CertPathTLS: "fixtures/test.crt", KeyPathTLS: "fixtures/notFound.key"}}, + {"cert key mismatch", &Options{CertPathTLS: "fixtures/test.crt", KeyPathTLS: "fixtures/test2.key"}}, + {"multiple cert key count mismatch", &Options{CertPathTLS: "fixtures/test.crt,fixtures/test2.crt", KeyPathTLS: "fixtures/test.key"}}, + {"multiple cert key mismatch", &Options{CertPathTLS: "fixtures/test.crt,fixtures/test2.crt", KeyPathTLS: "fixtures/test2.key,fixtures/test.key"}}, + } { + t.Run(tt.name, func(t *testing.T) { + _, err := tt.options.tlsConfig() + t.Logf("tlsConfig error: %v", err) + require.Error(t, err) + }) } } @@ -218,92 +216,67 @@ func TestHTTPServer(t *testing.T) { } func TestHTTPServerShutdown(t *testing.T) { - d := 1 * time.Second + o := &Options{} + testServerShutdown(t, o, "http") +} - o := Options{ - Address: ":19999", - WaitForHealthcheckInterval: d, +func TestHTTPSServerShutdown(t *testing.T) { + o := &Options{ + CertPathTLS: "fixtures/test.crt", + KeyPathTLS: "fixtures/test.key", } + testServerShutdown(t, o, "https") +} + +func testServerShutdown(t *testing.T, o *Options, scheme string) { + const shutdownDelay = 1 * time.Second + + address, err := findAddress() + require.NoError(t, err) + + o.Address, o.WaitForHealthcheckInterval = address, shutdownDelay + testUrl := scheme + "://" + address // simulate a backend that got a request and should be handled correctly dc, err := routestring.New(`r0: * -> latency("3s") -> inlineContent("OK") -> status(200) -> `) - if err != nil { - t.Errorf("Failed to create dataclient: %v", err) - } + require.NoError(t, err) rt := routing.New(routing.Options{ FilterRegistry: builtin.MakeRegistry(), - DataClients: []routing.DataClient{ - dc, - }, + DataClients: []routing.DataClient{dc}, }) defer rt.Close() proxy := proxy.New(rt, proxy.OptionsNone) defer proxy.Close() + + sigs := make(chan os.Signal, 1) go func() { - if errLas := listenAndServe(proxy, &o); errLas != nil { - t.Logf("Failed to liste and serve: %v", errLas) - } + err := listenAndServeQuit(proxy, o, sigs, nil, nil) + require.NoError(t, err) }() - pid := syscall.Getpid() - p, err := os.FindProcess(pid) - if err != nil { - t.Errorf("Failed to find current process: %v", err) - } - - var wg sync.WaitGroup - installSigHandler := make(chan struct{}, 1) - wg.Add(1) - go func() { - defer wg.Done() - sigs := make(chan os.Signal, 1) - signal.Notify(sigs, syscall.SIGTERM) + // initiate shutdown + sigs <- syscall.SIGTERM - installSigHandler <- struct{}{} + time.Sleep(shutdownDelay / 2) - <-sigs + t.Logf("ongoing request passing in before shutdown") + r, err := waitConnGet(testUrl) + require.NoError(t, err) + require.Equal(t, 200, r.StatusCode) - // ongoing requests passing in before shutdown - time.Sleep(d / 2) - r, err2 := waitConnGet("http://" + o.Address) - if r != nil { - defer r.Body.Close() - } - if err2 != nil { - t.Errorf("Cannot connect to the local server for testing: %v ", err2) - } - if r.StatusCode != 200 { - t.Errorf("Status code should be 200, instead got: %d\n", r.StatusCode) - } - body, err2 := io.ReadAll(r.Body) - if err2 != nil { - t.Errorf("Failed to stream response body: %v", err2) - } - if s := string(body); s != "OK" { - t.Errorf("Failed to get the right content: %s", s) - } + defer r.Body.Close() - // requests on closed listener should fail - time.Sleep(d / 2) - r2, err2 := waitConnGet("http://" + o.Address) - if r2 != nil { - defer r2.Body.Close() - } - if err2 == nil { - t.Error("Can connect to a closed server for testing") - } - }() + body, err := io.ReadAll(r.Body) + require.NoError(t, err) + require.Equal(t, "OK", string(body)) - <-installSigHandler - time.Sleep(d / 2) + time.Sleep(shutdownDelay / 2) - if err = p.Signal(syscall.SIGTERM); err != nil { - t.Errorf("Failed to signal process: %v", err) - } - wg.Wait() - time.Sleep(d) + t.Logf("request after shutdown should fail") + _, err = waitConnGet(testUrl) + require.Error(t, err) } type (