From 117e8aa0b244b2c00d36973611c33cfc2bc25715 Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Wed, 23 Feb 2022 11:01:22 +0300 Subject: [PATCH 1/4] Support for TLS config on Workhorse As part of a requirement to use end-to-end encryption, Workhorse needs to be able to run HTTPS server. It allows to specify [tls] config in config.toml. The config must contain paths to certificate and private key. After that, the server will be accessible via HTTPS. Relates to #353010 Changelog: added --- doc/development/workhorse/configuration.md | 23 ++++++++++++++++++++++ workhorse/config.toml.example | 4 ++++ workhorse/config_test.go | 5 +++++ workhorse/internal/config/config.go | 6 ++++++ workhorse/main.go | 17 ++++++++++++++-- 5 files changed, 53 insertions(+), 2 deletions(-) diff --git a/doc/development/workhorse/configuration.md b/doc/development/workhorse/configuration.md index 7f9331e6f1ebdf..62d5859b6ba60b 100644 --- a/doc/development/workhorse/configuration.md +++ b/doc/development/workhorse/configuration.md @@ -128,6 +128,29 @@ relative URL in the `authBackend` setting: gitlab-workhorse -authBackend http://localhost:8080/gitlab ``` +## TLS support + +TLS can be configured to be used for incoming requests. +Paths to the files containing a certificate and matching private key for the server must be provided: + +```toml +[tls] + certificate = "/path/to/certificate" + key = "/path/to/private/key" + min_version = "tls1.2" + max_version = "tls1.3" +``` + +If the certificate is signed by a certificate authority, the `certificate` should be the concatenation +of the server's certificate, any intermediates, and the CA's certificate. + +Additionally, the listen address in the format "host:port" for incoming requests with TLS must be specified: + +```toml +[server] + tls_addr = "localhost:3443" +``` + ## Interaction of authBackend and authSocket The interaction between `authBackend` and `authSocket` can be confusing. diff --git a/workhorse/config.toml.example b/workhorse/config.toml.example index 27dc29ee078eff..bd1bcf144186a4 100644 --- a/workhorse/config.toml.example +++ b/workhorse/config.toml.example @@ -20,3 +20,7 @@ URL = "unix:/home/git/gitlab/redis/redis.socket" [image_resizer] max_scaler_procs = 4 # Recommendation: CPUs / 2 max_filesize = 250000 + +[tls] + certificate = "/path/to/certificate" + key = "/path/to/private/key" diff --git a/workhorse/config_test.go b/workhorse/config_test.go index 658a352a33357d..bc6a025692d67f 100644 --- a/workhorse/config_test.go +++ b/workhorse/config_test.go @@ -39,6 +39,9 @@ password = "redis password" provider = "test provider" [image_resizer] max_scaler_procs = 123 +[tls] +certificate = "/path/to/certificate" +key = "/path/to/private/key" ` _, err = io.WriteString(f, data) require.NoError(t, err) @@ -57,6 +60,8 @@ max_scaler_procs = 123 require.Equal(t, []string{"127.0.0.1/8", "192.168.0.1/8"}, cfg.TrustedCIDRsForXForwardedFor) require.Equal(t, []string{"10.0.0.1/8"}, cfg.TrustedCIDRsForPropagation) require.Equal(t, 60*time.Second, cfg.ShutdownTimeout.Duration) + require.Equal(t, "/path/to/certificate", cfg.Tls.Certificate) + require.Equal(t, "/path/to/private/key", cfg.Tls.Key) } func TestConfigErrorHelp(t *testing.T) { diff --git a/workhorse/internal/config/config.go b/workhorse/internal/config/config.go index 60cfd567f5d0eb..de5e6dc9b9cafa 100644 --- a/workhorse/internal/config/config.go +++ b/workhorse/internal/config/config.go @@ -84,6 +84,11 @@ type ImageResizerConfig struct { MaxFilesize uint64 `toml:"max_filesize"` } +type TlsCertificate struct { + Certificate string `toml:"certificate"` + Key string `toml:"key"` +} + type Config struct { Redis *RedisConfig `toml:"redis"` Backend *url.URL `toml:"-"` @@ -106,6 +111,7 @@ type Config struct { ShutdownTimeout TomlDuration `toml:"shutdown_timeout"` TrustedCIDRsForXForwardedFor []string `toml:"trusted_cidrs_for_x_forwarded_for"` TrustedCIDRsForPropagation []string `toml:"trusted_cidrs_for_propagation"` + Tls *TlsCertificate `toml:"tls"` } var DefaultImageResizerConfig = ImageResizerConfig{ diff --git a/workhorse/main.go b/workhorse/main.go index 123d21596e240d..902eab9b4af0c4 100644 --- a/workhorse/main.go +++ b/workhorse/main.go @@ -2,6 +2,7 @@ package main import ( "context" + "crypto/tls" "flag" "fmt" "io/ioutil" @@ -155,6 +156,7 @@ func buildConfig(arg0 string, args []string) (*bootConfig, *config.Config, error cfg.ShutdownTimeout = cfgFromFile.ShutdownTimeout cfg.TrustedCIDRsForXForwardedFor = cfgFromFile.TrustedCIDRsForXForwardedFor cfg.TrustedCIDRsForPropagation = cfgFromFile.TrustedCIDRsForPropagation + cfg.Tls = cfgFromFile.Tls return boot, cfg, nil } @@ -241,8 +243,19 @@ func run(boot bootConfig, cfg config.Config) error { done := make(chan os.Signal, 1) signal.Notify(done, syscall.SIGINT, syscall.SIGTERM) - server := http.Server{Handler: up} - go func() { finalErrors <- server.Serve(listener) }() + server := http.Server{ + Handler: up, + TLSConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, + }, + } + go func() { + if cfg.Tls != nil { + finalErrors <- server.ServeTLS(listener, cfg.Tls.Certificate, cfg.Tls.Key) + } else { + finalErrors <- server.Serve(listener) + } + }() select { case err := <-finalErrors: -- GitLab From fedb2c8d2668fbcf26da023180a832c9958168f8 Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Mon, 7 Mar 2022 08:33:29 +0300 Subject: [PATCH 2/4] Support running both http and https It's handful to have dual listeners when we deploy in order to avoid interrupting the rollout --- workhorse/config.toml.example | 5 + workhorse/config_test.go | 7 + workhorse/internal/config/config.go | 7 + workhorse/internal/server/server.go | 118 ++++++++++++ workhorse/internal/server/server_test.go | 176 ++++++++++++++++++ .../internal/server/testdata/localhost.crt | 27 +++ .../internal/server/testdata/localhost.key | 28 +++ workhorse/main.go | 33 ++-- 8 files changed, 380 insertions(+), 21 deletions(-) create mode 100644 workhorse/internal/server/server.go create mode 100644 workhorse/internal/server/server_test.go create mode 100644 workhorse/internal/server/testdata/localhost.crt create mode 100644 workhorse/internal/server/testdata/localhost.key diff --git a/workhorse/config.toml.example b/workhorse/config.toml.example index bd1bcf144186a4..acd38a1dd9d9b6 100644 --- a/workhorse/config.toml.example +++ b/workhorse/config.toml.example @@ -21,6 +21,11 @@ URL = "unix:/home/git/gitlab/redis/redis.socket" max_scaler_procs = 4 # Recommendation: CPUs / 2 max_filesize = 250000 +[server] + tls_addr = "127.0.0.1:3443" + [tls] certificate = "/path/to/certificate" key = "/path/to/private/key" + min_version = "tls1.2" + max_version = "tls1.3" diff --git a/workhorse/config_test.go b/workhorse/config_test.go index bc6a025692d67f..d0c77a27615aa1 100644 --- a/workhorse/config_test.go +++ b/workhorse/config_test.go @@ -42,6 +42,10 @@ max_scaler_procs = 123 [tls] certificate = "/path/to/certificate" key = "/path/to/private/key" +min_version = "tls1.1" +max_version = "tls1.2" +[server] +tls_addr = "localhost:3443" ` _, err = io.WriteString(f, data) require.NoError(t, err) @@ -62,6 +66,9 @@ key = "/path/to/private/key" require.Equal(t, 60*time.Second, cfg.ShutdownTimeout.Duration) require.Equal(t, "/path/to/certificate", cfg.Tls.Certificate) require.Equal(t, "/path/to/private/key", cfg.Tls.Key) + require.Equal(t, "tls1.1", cfg.Tls.MinVersion) + require.Equal(t, "tls1.2", cfg.Tls.MaxVersion) + require.Equal(t, "localhost:3443", cfg.Server.TlsAddr) } func TestConfigErrorHelp(t *testing.T) { diff --git a/workhorse/internal/config/config.go b/workhorse/internal/config/config.go index de5e6dc9b9cafa..e15f3a976c98e3 100644 --- a/workhorse/internal/config/config.go +++ b/workhorse/internal/config/config.go @@ -87,6 +87,12 @@ type ImageResizerConfig struct { type TlsCertificate struct { Certificate string `toml:"certificate"` Key string `toml:"key"` + MinVersion string `toml:"min_version"` + MaxVersion string `toml:"max_version"` +} + +type Server struct { + TlsAddr string `toml:"tls_addr"` } type Config struct { @@ -112,6 +118,7 @@ type Config struct { TrustedCIDRsForXForwardedFor []string `toml:"trusted_cidrs_for_x_forwarded_for"` TrustedCIDRsForPropagation []string `toml:"trusted_cidrs_for_propagation"` Tls *TlsCertificate `toml:"tls"` + Server Server `toml:"server"` } var DefaultImageResizerConfig = ImageResizerConfig{ diff --git a/workhorse/internal/server/server.go b/workhorse/internal/server/server.go new file mode 100644 index 00000000000000..604791d7663dc7 --- /dev/null +++ b/workhorse/internal/server/server.go @@ -0,0 +1,118 @@ +package server + +import ( + "context" + "crypto/tls" + "fmt" + "net" + "net/http" + "syscall" + + "gitlab.com/gitlab-org/labkit/log" + + "gitlab.com/gitlab-org/gitlab/workhorse/internal/config" +) + +var tlsVersions = map[string]uint16{ + "": 0, // Default value in tls.Config + "tls1.0": tls.VersionTLS10, + "tls1.1": tls.VersionTLS11, + "tls1.2": tls.VersionTLS12, + "tls1.3": tls.VersionTLS13, +} + +type Server struct { + Handler http.Handler + Network, Addr string + Umask int + Config *config.Config + Errors chan error + + servers []*http.Server +} + +func (s *Server) Run() error { + oldUmask := syscall.Umask(s.Umask) + defer syscall.Umask(oldUmask) + + if err := s.runUpstreamServer(s.Network, s.Addr, nil); err != nil { + return fmt.Errorf("server.Run: running upstream server: %v", err) + } + + if s.Config.Server.TlsAddr != "" { + if err := s.runUpstreamServer("tcp", s.Config.Server.TlsAddr, s.Config.Tls); err != nil { + return fmt.Errorf("server.Run: running tls upstream server: %v", err) + } + } + + return nil +} + +func (s *Server) Close() error { + var resultErr error + for _, server := range s.servers { + if err := server.Close(); err != nil { + resultErr = err + } + } + + return resultErr +} + +func (s *Server) Shutdown(ctx context.Context) error { + errC := make(chan error, len(s.servers)) + for _, server := range s.servers { + server := server // Capture loop variable + go func() { errC <- server.Shutdown(ctx) }() + } + + for range s.servers { + if err := <-errC; err != nil { + return err + } + } + + return nil +} + +func (s *Server) runUpstreamServer(network, addr string, tlsConfig *config.TlsCertificate) error { + listener, err := s.newListener("upstream", network, addr, tlsConfig) + if err != nil { + return err + } + + srv := &http.Server{ + Addr: listener.Addr().String(), + Handler: s.Handler, + } + go func() { + s.Errors <- srv.Serve(listener) + }() + + s.servers = append(s.servers, srv) + + return nil +} + +func (s *Server) newListener(name, network, addr string, tlsConfig *config.TlsCertificate) (net.Listener, error) { + if tlsConfig == nil { + log.WithFields(log.Fields{"address": addr, "network": network}).Infof("Running %v server", name) + + return net.Listen(network, addr) + } + + cert, err := tls.LoadX509KeyPair(tlsConfig.Certificate, tlsConfig.Key) + if err != nil { + return nil, err + } + + config := &tls.Config{ + MinVersion: tlsVersions[tlsConfig.MinVersion], + MaxVersion: tlsVersions[tlsConfig.MaxVersion], + Certificates: []tls.Certificate{cert}, + } + + log.WithFields(log.Fields{"address": addr, "network": network}).Infof("Running %v server with tls", name) + + return tls.Listen(network, addr, config) +} diff --git a/workhorse/internal/server/server_test.go b/workhorse/internal/server/server_test.go new file mode 100644 index 00000000000000..a6e2c672373736 --- /dev/null +++ b/workhorse/internal/server/server_test.go @@ -0,0 +1,176 @@ +package server + +import ( + "context" + "crypto/tls" + "crypto/x509" + "net/http" + "testing" + "time" + + "github.com/stretchr/testify/require" + + "gitlab.com/gitlab-org/gitlab/workhorse/internal/config" +) + +const ( + certFile = "testdata/localhost.crt" + keyFile = "testdata/localhost.key" +) + +func TestRun(t *testing.T) { + srv := defaultServer() + srv.Config = &config.Config{} + + require.NoError(t, srv.Run()) + defer srv.Close() + + require.Len(t, srv.servers, 1) + + resp, err := http.Get("http://" + srv.servers[0].Addr) + require.NoError(t, err) + require.Equal(t, 200, resp.StatusCode) +} + +func TestRun_withTls(t *testing.T) { + srv := defaultServer() + + require.NoError(t, srv.Run()) + defer srv.Close() + + require.Len(t, srv.servers, 2) + + clients := buildClients(t, srv.servers) + for url, client := range clients { + resp, err := client.Get(url) + require.NoError(t, err) + require.Equal(t, 200, resp.StatusCode) + } +} + +func TestShutdown(t *testing.T) { + ready := make(chan bool) + done := make(chan bool) + statusCodes := make(chan int) + + srv := defaultServer() + srv.Handler = http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + ready <- true + <-done + rw.WriteHeader(200) + }) + + require.NoError(t, srv.Run()) + defer srv.Close() + + clients := buildClients(t, srv.servers) + + for url, client := range clients { + go func(url string, client *http.Client) { + resp, err := client.Get(url) + require.NoError(t, err) + statusCodes <- resp.StatusCode + }(url, client) + } + + for range clients { + <-ready + } // initiate requests + + shutdownError := make(chan error) + go func() { + shutdownError <- srv.Shutdown(context.Background()) + }() + + for url, client := range clients { + require.Eventually(t, func() bool { + _, err := client.Get(url) + return err != nil + }, time.Second, 10*time.Millisecond, "server must stop accepting new requests") + } + + for range clients { + done <- true + } // finish requests + + require.NoError(t, <-shutdownError) + require.ElementsMatch(t, []int{200, 200}, []int{<-statusCodes, <-statusCodes}) +} + +func TestShutdown_withTimeout(t *testing.T) { + ready := make(chan bool) + done := make(chan bool) + + srv := defaultServer() + srv.Handler = http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + ready <- true + <-done + rw.WriteHeader(200) + }) + + require.NoError(t, srv.Run()) + defer srv.Close() + + clients := buildClients(t, srv.servers) + + for url, client := range clients { + go func(url string, client *http.Client) { + client.Get(url) + }(url, client) + } + + for range clients { + <-ready + } // initiate requets + + ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond) + defer cancel() + + err := srv.Shutdown(ctx) + require.Error(t, err) + require.EqualError(t, err, "context deadline exceeded") +} + +func defaultServer() Server { + return Server{ + Network: "tcp", + Addr: "127.0.0.1:0", // run a server on a random free port + Config: &config.Config{ + Tls: &config.TlsCertificate{ + Certificate: certFile, + Key: keyFile, + }, + Server: config.Server{ + TlsAddr: "127.0.0.1:0", + }, + }, + Handler: http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(200) + }), + Errors: make(chan error), + } +} + +func buildClients(t *testing.T, servers []*http.Server) map[string]*http.Client { + httpsClient := &http.Client{} + certpool := x509.NewCertPool() + + tlsCertificate, err := tls.LoadX509KeyPair(certFile, keyFile) + require.NoError(t, err) + + certificate, err := x509.ParseCertificate(tlsCertificate.Certificate[0]) + require.NoError(t, err) + + certpool.AddCert(certificate) + httpsClient.Transport = &http.Transport{ + TLSClientConfig: &tls.Config{ + RootCAs: certpool, + }, + } + + httpServer, httpsServer := servers[0], servers[1] + return map[string]*http.Client{ + "http://" + httpServer.Addr: http.DefaultClient, + "https://" + httpsServer.Addr: httpsClient, + } +} diff --git a/workhorse/internal/server/testdata/localhost.crt b/workhorse/internal/server/testdata/localhost.crt new file mode 100644 index 00000000000000..bee60e42e00e75 --- /dev/null +++ b/workhorse/internal/server/testdata/localhost.crt @@ -0,0 +1,27 @@ +-----BEGIN CERTIFICATE----- +MIIEjjCCAvagAwIBAgIQC2au+A/aGQ2Z21O0wVoEwjANBgkqhkiG9w0BAQsFADCB +pTEeMBwGA1UEChMVbWtjZXJ0IGRldmVsb3BtZW50IENBMT0wOwYDVQQLDDRpZ29y +ZHJvemRvdkBJZ29ycy1NYWNCb29rLVByby0yLmxvY2FsIChJZ29yIERyb3pkb3Yp +MUQwQgYDVQQDDDtta2NlcnQgaWdvcmRyb3pkb3ZASWdvcnMtTWFjQm9vay1Qcm8t +Mi5sb2NhbCAoSWdvciBEcm96ZG92KTAeFw0yMjAzMDcwNDMxMjRaFw0yNDA2MDcw +NDMxMjRaMGgxJzAlBgNVBAoTHm1rY2VydCBkZXZlbG9wbWVudCBjZXJ0aWZpY2F0 +ZTE9MDsGA1UECww0aWdvcmRyb3pkb3ZASWdvcnMtTWFjQm9vay1Qcm8tMi5sb2Nh +bCAoSWdvciBEcm96ZG92KTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEB +AMJ8ofGdcnenVRtNGViF4oxPv+CCFA6D2nfsjkJG8kmO6WW7VlbhJYxCMAuyFF1F +b2UI2rrTFL8Aeq1KxeQzdrb3cpCquVH/UQ00G4ply28XVPRdbIyLQvOThMEeLL6v +6gb4edL5oZmo/vWhdQxv0NGt282PAEt+bjnbdl28on8WVzmsw/m0nZ2BVWke+oUM +krfsbyFaZj7aW8w0dNeK25ANy/Ldx55ENRDquphwYHDnpFOQpkHo5nPuoms5j2Sf +GW3u3hgeFhRrFjqDstU3OKdA4AdHntDjl0gHm35w1m8PXiql/3EpkEMMx5ixQAqM +cMZ7VVzy0HIjqsjdJZpzjx8CAwEAAaN2MHQwDgYDVR0PAQH/BAQDAgWgMBMGA1Ud +JQQMMAoGCCsGAQUFBwMBMB8GA1UdIwQYMBaAFKTVZ2JsYLGJOP+UX0AwGO/81Kab +MCwGA1UdEQQlMCOCCWxvY2FsaG9zdIcEfwAAAYcQAAAAAAAAAAAAAAAAAAAAATAN +BgkqhkiG9w0BAQsFAAOCAYEAkGntoogSlhukGqTNbTXN9T/gXLtx9afWlgcBEafF +MYQoJ1DOwXoYCQkMsxE0xWUyLDTpvjzfKkkyQwWzTwcYqRHOKafKYVSvENU5oaDY +c2nk32SfkcF6bqJ50uBlFMEvKFExU1U+YSJhuEH/iqT9sSd52uwmnB0TJhSOc3J/ +1ZapKM2G71ezi8OyizwlwDJAwQ37CqrYS2slVO6Cy8zJ1l/ZsZ+kxRb+ME0LREI0 +J/rFTo9A6iyuXeBQ2jiRUrC6pmmbUQbVSjROx4RSmWoI/58/VnuZBY9P62OAOgUv +pukfAbh3SUjN5++m4Py7WjP/y+L2ILPOFtxTY+CQPWQ5Hbff8iMB4NNfutdU1wSS +CzXT1zWbU12kXod80wkMqWvNb3yU5spqXV6WYhOHiDIyqpPIqp5/i93Ck3Hd6/BQ +DYlNOQsVHdSjWzNw9UubjpatiFqMK4hvJZE0haoLlmfDeZeqWk9oAuuCibLJGPg4 +TQri+lKgi0e76ynUr1zP1xUR +-----END CERTIFICATE----- diff --git a/workhorse/internal/server/testdata/localhost.key b/workhorse/internal/server/testdata/localhost.key new file mode 100644 index 00000000000000..b708582f02e002 --- /dev/null +++ b/workhorse/internal/server/testdata/localhost.key @@ -0,0 +1,28 @@ +-----BEGIN PRIVATE KEY----- +MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQDCfKHxnXJ3p1Ub +TRlYheKMT7/gghQOg9p37I5CRvJJjullu1ZW4SWMQjALshRdRW9lCNq60xS/AHqt +SsXkM3a293KQqrlR/1ENNBuKZctvF1T0XWyMi0Lzk4TBHiy+r+oG+HnS+aGZqP71 +oXUMb9DRrdvNjwBLfm4523ZdvKJ/Flc5rMP5tJ2dgVVpHvqFDJK37G8hWmY+2lvM +NHTXituQDcvy3ceeRDUQ6rqYcGBw56RTkKZB6OZz7qJrOY9knxlt7t4YHhYUaxY6 +g7LVNzinQOAHR57Q45dIB5t+cNZvD14qpf9xKZBDDMeYsUAKjHDGe1Vc8tByI6rI +3SWac48fAgMBAAECggEALuZXNyi8vdYAVAEXp51BsIxavQ0hQQ7S1DCbbagmLU7l +Qb8XZwQMRfKAG5HqD0P7ROYJuRvF2PmIm9l4Nzuh2SV63yAMaJWlOgXizlEV6cg6 +mGMfFhVPI+XjEZ7xM1rAmMW6uwGv0ppKQXmZ/FHKjYXbh4qAi7QFaLZfqOMgXHzf +C4nxf0xMzPP7rBnaxAGBRJWC+/UWxd1MVoHRjink4V/Tdy4zu+cEJ+2wuGawp4nz +dEWYITzXMcBUKmZQHiOm+r58HpWK3mgXpJQBg3WqjR2iNa+ElyoPoGC6zu5Jd8Xg +mMG2jHPFu+2F4UvymgxbKZqKHqcNjO7WMZRtIRiJgQKBgQDZGXUme0S5Bh8/y1us +ltEfy4INFYJAejVxPwv7mRLtySqZLkWAPQTaSGgIk/XMTBYS3Ia9XD6Jl3zwo1qF +R+y3ZkusGmk73o35kBxjc6purDei7CqMzwulbFTsUglDiF9T4X24bv1yK3lP2n8A +Y6kLsscEC1wIEuwV5HFyQ2S9zwKBgQDlVepMrQ84FxQxN474LakwWLSkwo+6jS37 +61VPUqDUQpE4fGM6+F3fG+9YDMgvOVDneZ0MvzoiDRynbzF7K3k3fIBrYYbTRz7J +p23BbTninzhrYTE/xd3LuFCZibCXA7nRa0QmYdXG4nUM2jjsjdR5AG7c/qJQDNun +SXTbfM49sQKBgQCM9Jl6hbiGBTKO4gNAmJ9o7GIhCqEKKg6+23d1QNroZp9w23km +nPeknjRltWN25MPENUiKc/Tqst/dAcLJHHzWSuXA9Vj0FTjLG0VDURsMRmbNMlci +G1/tZNvyoAUBwu5Z8OMGt5F46j8WmL+yygI85TOQLavwVhDQ2gTKcnVbQwKBgQC0 +2VCf0KU8xS5eNYLgARn3jyw89VTkduq5S3aFzBIZ8LiWQ7j4yt0z0NKoq8O9QcSk +FUocwDv2mEJtYwkxKTI46ExY4Zqxx/Aik47AxwKrzIVwYD+3G7DxMtMUkPkZzY1e +MOmYHvS3FuPZE8lp+dqA5S+HxKF44Pria9HkOAJnsQKBgE853d9sR0DlJtEj64yu +FX1rCle/UUODClktPgrwuM+xYutxOiEu6HUWHJI2yvWNk4oNL8Xd0IkR9NlwdatU +E3+WDua+yYAsI9yWYn3+iqp+owNATkEDjWGivt0Onmgttt5kLHzPFCViIcgl32vv +7V/plCsmgrS98xZHRrriTLvz +-----END PRIVATE KEY----- diff --git a/workhorse/main.go b/workhorse/main.go index 902eab9b4af0c4..f4fbed1d8ab5df 100644 --- a/workhorse/main.go +++ b/workhorse/main.go @@ -2,7 +2,6 @@ package main import ( "context" - "crypto/tls" "flag" "fmt" "io/ioutil" @@ -23,6 +22,7 @@ import ( "gitlab.com/gitlab-org/gitlab/workhorse/internal/queueing" "gitlab.com/gitlab-org/gitlab/workhorse/internal/redis" "gitlab.com/gitlab-org/gitlab/workhorse/internal/secret" + "gitlab.com/gitlab-org/gitlab/workhorse/internal/server" "gitlab.com/gitlab-org/gitlab/workhorse/internal/upstream" ) @@ -157,6 +157,7 @@ func buildConfig(arg0 string, args []string) (*bootConfig, *config.Config, error cfg.TrustedCIDRsForXForwardedFor = cfgFromFile.TrustedCIDRsForXForwardedFor cfg.TrustedCIDRsForPropagation = cfgFromFile.TrustedCIDRsForPropagation cfg.Tls = cfgFromFile.Tls + cfg.Server = cfgFromFile.Server return boot, cfg, nil } @@ -179,14 +180,6 @@ func run(boot bootConfig, cfg config.Config) error { } } - // Change the umask only around net.Listen() - oldUmask := syscall.Umask(boot.listenUmask) - listener, err := net.Listen(boot.listenNetwork, boot.listenAddr) - syscall.Umask(oldUmask) - if err != nil { - return fmt.Errorf("main listener: %v", err) - } - finalErrors := make(chan error) // The profiler will only be activated by HTTP requests. HTTP @@ -243,19 +236,17 @@ func run(boot bootConfig, cfg config.Config) error { done := make(chan os.Signal, 1) signal.Notify(done, syscall.SIGINT, syscall.SIGTERM) - server := http.Server{ + srv := &server.Server{ Handler: up, - TLSConfig: &tls.Config{ - MinVersion: tls.VersionTLS12, - }, + Network: boot.listenNetwork, + Addr: boot.listenAddr, + Umask: boot.listenUmask, + Config: &cfg, + Errors: finalErrors, + } + if err := srv.Run(); err != nil { + return fmt.Errorf("running server: %v", err) } - go func() { - if cfg.Tls != nil { - finalErrors <- server.ServeTLS(listener, cfg.Tls.Certificate, cfg.Tls.Key) - } else { - finalErrors <- server.Serve(listener) - } - }() select { case err := <-finalErrors: @@ -267,6 +258,6 @@ func run(boot bootConfig, cfg config.Config) error { defer cancel() redis.Shutdown() - return server.Shutdown(ctx) + return srv.Shutdown(ctx) } } -- GitLab From 8865b8ba66a3cc995ba80b6a7e291bbb5ef9d1c7 Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Fri, 22 Apr 2022 11:26:35 +0400 Subject: [PATCH 3/4] Change config to use [[listeners]] array --- doc/development/workhorse/configuration.md | 14 ++--- workhorse/config.toml.example | 7 ++- workhorse/config_test.go | 21 ++++--- workhorse/internal/config/config.go | 11 ++-- workhorse/internal/server/server.go | 73 +++++++++------------- workhorse/internal/server/server_test.go | 33 ++++------ workhorse/main.go | 15 ++--- 7 files changed, 78 insertions(+), 96 deletions(-) diff --git a/doc/development/workhorse/configuration.md b/doc/development/workhorse/configuration.md index 62d5859b6ba60b..fbf457f4cef0d4 100644 --- a/doc/development/workhorse/configuration.md +++ b/doc/development/workhorse/configuration.md @@ -130,11 +130,14 @@ gitlab-workhorse -authBackend http://localhost:8080/gitlab ## TLS support -TLS can be configured to be used for incoming requests. +A listener with TLS can be configured to be used for incoming requests. Paths to the files containing a certificate and matching private key for the server must be provided: ```toml -[tls] +[[listeners]] +network = "tcp" +addr = "localhost:3443" +[listeners.tls] certificate = "/path/to/certificate" key = "/path/to/private/key" min_version = "tls1.2" @@ -144,13 +147,6 @@ Paths to the files containing a certificate and matching private key for the ser If the certificate is signed by a certificate authority, the `certificate` should be the concatenation of the server's certificate, any intermediates, and the CA's certificate. -Additionally, the listen address in the format "host:port" for incoming requests with TLS must be specified: - -```toml -[server] - tls_addr = "localhost:3443" -``` - ## Interaction of authBackend and authSocket The interaction between `authBackend` and `authSocket` can be confusing. diff --git a/workhorse/config.toml.example b/workhorse/config.toml.example index acd38a1dd9d9b6..1457e20ed88d86 100644 --- a/workhorse/config.toml.example +++ b/workhorse/config.toml.example @@ -21,10 +21,11 @@ URL = "unix:/home/git/gitlab/redis/redis.socket" max_scaler_procs = 4 # Recommendation: CPUs / 2 max_filesize = 250000 -[server] - tls_addr = "127.0.0.1:3443" +[[listeners]] + network = "tcp" + addr = "127.0.0.1:3443" -[tls] +[listeners.tls] certificate = "/path/to/certificate" key = "/path/to/private/key" min_version = "tls1.2" diff --git a/workhorse/config_test.go b/workhorse/config_test.go index d0c77a27615aa1..0c0072322acfc8 100644 --- a/workhorse/config_test.go +++ b/workhorse/config_test.go @@ -39,13 +39,14 @@ password = "redis password" provider = "test provider" [image_resizer] max_scaler_procs = 123 -[tls] +[[listeners]] +network = "tcp" +addr = "localhost:3443" +[listeners.tls] certificate = "/path/to/certificate" key = "/path/to/private/key" min_version = "tls1.1" max_version = "tls1.2" -[server] -tls_addr = "localhost:3443" ` _, err = io.WriteString(f, data) require.NoError(t, err) @@ -64,11 +65,15 @@ tls_addr = "localhost:3443" require.Equal(t, []string{"127.0.0.1/8", "192.168.0.1/8"}, cfg.TrustedCIDRsForXForwardedFor) require.Equal(t, []string{"10.0.0.1/8"}, cfg.TrustedCIDRsForPropagation) require.Equal(t, 60*time.Second, cfg.ShutdownTimeout.Duration) - require.Equal(t, "/path/to/certificate", cfg.Tls.Certificate) - require.Equal(t, "/path/to/private/key", cfg.Tls.Key) - require.Equal(t, "tls1.1", cfg.Tls.MinVersion) - require.Equal(t, "tls1.2", cfg.Tls.MaxVersion) - require.Equal(t, "localhost:3443", cfg.Server.TlsAddr) + + require.Len(t, cfg.Listeners, 1) + listener := cfg.Listeners[0] + require.Equal(t, "/path/to/certificate", listener.Tls.Certificate) + require.Equal(t, "/path/to/private/key", listener.Tls.Key) + require.Equal(t, "tls1.1", listener.Tls.MinVersion) + require.Equal(t, "tls1.2", listener.Tls.MaxVersion) + require.Equal(t, "tcp", listener.Network) + require.Equal(t, "localhost:3443", listener.Addr) } func TestConfigErrorHelp(t *testing.T) { diff --git a/workhorse/internal/config/config.go b/workhorse/internal/config/config.go index e15f3a976c98e3..e83f55f43bf256 100644 --- a/workhorse/internal/config/config.go +++ b/workhorse/internal/config/config.go @@ -84,15 +84,17 @@ type ImageResizerConfig struct { MaxFilesize uint64 `toml:"max_filesize"` } -type TlsCertificate struct { +type TlsConfig struct { Certificate string `toml:"certificate"` Key string `toml:"key"` MinVersion string `toml:"min_version"` MaxVersion string `toml:"max_version"` } -type Server struct { - TlsAddr string `toml:"tls_addr"` +type ListenerConfig struct { + Network string `toml:"network"` + Addr string `toml:"addr"` + Tls *TlsConfig `toml:"tls"` } type Config struct { @@ -117,8 +119,7 @@ type Config struct { ShutdownTimeout TomlDuration `toml:"shutdown_timeout"` TrustedCIDRsForXForwardedFor []string `toml:"trusted_cidrs_for_x_forwarded_for"` TrustedCIDRsForPropagation []string `toml:"trusted_cidrs_for_propagation"` - Tls *TlsCertificate `toml:"tls"` - Server Server `toml:"server"` + Listeners []ListenerConfig `toml:"listeners"` } var DefaultImageResizerConfig = ImageResizerConfig{ diff --git a/workhorse/internal/server/server.go b/workhorse/internal/server/server.go index 604791d7663dc7..e6ad5bff8d3a63 100644 --- a/workhorse/internal/server/server.go +++ b/workhorse/internal/server/server.go @@ -22,11 +22,10 @@ var tlsVersions = map[string]uint16{ } type Server struct { - Handler http.Handler - Network, Addr string - Umask int - Config *config.Config - Errors chan error + Handler http.Handler + Umask int + ListenerConfigs []config.ListenerConfig + Errors chan error servers []*http.Server } @@ -35,52 +34,44 @@ func (s *Server) Run() error { oldUmask := syscall.Umask(s.Umask) defer syscall.Umask(oldUmask) - if err := s.runUpstreamServer(s.Network, s.Addr, nil); err != nil { - return fmt.Errorf("server.Run: running upstream server: %v", err) - } - - if s.Config.Server.TlsAddr != "" { - if err := s.runUpstreamServer("tcp", s.Config.Server.TlsAddr, s.Config.Tls); err != nil { - return fmt.Errorf("server.Run: running tls upstream server: %v", err) + for _, cfg := range s.ListenerConfigs { + listener, err := s.newListener("upstream", cfg) + if err != nil { + return fmt.Errorf("server.Run: failed creating a listener: %v", err) } + + s.runUpstreamServer(listener) } return nil } func (s *Server) Close() error { - var resultErr error - for _, server := range s.servers { - if err := server.Close(); err != nil { - resultErr = err - } - } - - return resultErr + return s.allServers(func(srv *http.Server) error { return srv.Close() }) } func (s *Server) Shutdown(ctx context.Context) error { + return s.allServers(func(srv *http.Server) error { return srv.Shutdown(ctx) }) +} + +func (s *Server) allServers(callback func(*http.Server) error) error { + var resultErr error errC := make(chan error, len(s.servers)) for _, server := range s.servers { server := server // Capture loop variable - go func() { errC <- server.Shutdown(ctx) }() + go func() { errC <- callback(server) }() } for range s.servers { if err := <-errC; err != nil { - return err + resultErr = err } } - return nil + return resultErr } -func (s *Server) runUpstreamServer(network, addr string, tlsConfig *config.TlsCertificate) error { - listener, err := s.newListener("upstream", network, addr, tlsConfig) - if err != nil { - return err - } - +func (s *Server) runUpstreamServer(listener net.Listener) { srv := &http.Server{ Addr: listener.Addr().String(), Handler: s.Handler, @@ -90,29 +81,27 @@ func (s *Server) runUpstreamServer(network, addr string, tlsConfig *config.TlsCe }() s.servers = append(s.servers, srv) - - return nil } -func (s *Server) newListener(name, network, addr string, tlsConfig *config.TlsCertificate) (net.Listener, error) { - if tlsConfig == nil { - log.WithFields(log.Fields{"address": addr, "network": network}).Infof("Running %v server", name) +func (s *Server) newListener(name string, cfg config.ListenerConfig) (net.Listener, error) { + if cfg.Tls == nil { + log.WithFields(log.Fields{"address": cfg.Addr, "network": cfg.Network}).Infof("Running %v server", name) - return net.Listen(network, addr) + return net.Listen(cfg.Network, cfg.Addr) } - cert, err := tls.LoadX509KeyPair(tlsConfig.Certificate, tlsConfig.Key) + cert, err := tls.LoadX509KeyPair(cfg.Tls.Certificate, cfg.Tls.Key) if err != nil { return nil, err } - config := &tls.Config{ - MinVersion: tlsVersions[tlsConfig.MinVersion], - MaxVersion: tlsVersions[tlsConfig.MaxVersion], + log.WithFields(log.Fields{"address": cfg.Addr, "network": cfg.Network}).Infof("Running %v server with tls", name) + + tlsConfig := &tls.Config{ + MinVersion: tlsVersions[cfg.Tls.MinVersion], + MaxVersion: tlsVersions[cfg.Tls.MaxVersion], Certificates: []tls.Certificate{cert}, } - log.WithFields(log.Fields{"address": addr, "network": network}).Infof("Running %v server with tls", name) - - return tls.Listen(network, addr, config) + return tls.Listen(cfg.Network, cfg.Addr, tlsConfig) } diff --git a/workhorse/internal/server/server_test.go b/workhorse/internal/server/server_test.go index a6e2c672373736..c342b4e96a7047 100644 --- a/workhorse/internal/server/server_test.go +++ b/workhorse/internal/server/server_test.go @@ -20,20 +20,6 @@ const ( func TestRun(t *testing.T) { srv := defaultServer() - srv.Config = &config.Config{} - - require.NoError(t, srv.Run()) - defer srv.Close() - - require.Len(t, srv.servers, 1) - - resp, err := http.Get("http://" + srv.servers[0].Addr) - require.NoError(t, err) - require.Equal(t, 200, resp.StatusCode) -} - -func TestRun_withTls(t *testing.T) { - srv := defaultServer() require.NoError(t, srv.Run()) defer srv.Close() @@ -133,15 +119,18 @@ func TestShutdown_withTimeout(t *testing.T) { func defaultServer() Server { return Server{ - Network: "tcp", - Addr: "127.0.0.1:0", // run a server on a random free port - Config: &config.Config{ - Tls: &config.TlsCertificate{ - Certificate: certFile, - Key: keyFile, + ListenerConfigs: []config.ListenerConfig{ + { + Addr: "127.0.0.1:0", + Network: "tcp", }, - Server: config.Server{ - TlsAddr: "127.0.0.1:0", + { + Addr: "127.0.0.1:0", + Network: "tcp", + Tls: &config.TlsConfig{ + Certificate: certFile, + Key: keyFile, + }, }, }, Handler: http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { diff --git a/workhorse/main.go b/workhorse/main.go index f4fbed1d8ab5df..a2156b491e7dfe 100644 --- a/workhorse/main.go +++ b/workhorse/main.go @@ -156,8 +156,7 @@ func buildConfig(arg0 string, args []string) (*bootConfig, *config.Config, error cfg.ShutdownTimeout = cfgFromFile.ShutdownTimeout cfg.TrustedCIDRsForXForwardedFor = cfgFromFile.TrustedCIDRsForXForwardedFor cfg.TrustedCIDRsForPropagation = cfgFromFile.TrustedCIDRsForPropagation - cfg.Tls = cfgFromFile.Tls - cfg.Server = cfgFromFile.Server + cfg.Listeners = cfgFromFile.Listeners return boot, cfg, nil } @@ -236,13 +235,15 @@ func run(boot bootConfig, cfg config.Config) error { done := make(chan os.Signal, 1) signal.Notify(done, syscall.SIGINT, syscall.SIGTERM) - srv := &server.Server{ - Handler: up, + listenerFromBootConfig := config.ListenerConfig{ Network: boot.listenNetwork, Addr: boot.listenAddr, - Umask: boot.listenUmask, - Config: &cfg, - Errors: finalErrors, + } + srv := &server.Server{ + Handler: up, + Umask: boot.listenUmask, + ListenerConfigs: append(cfg.Listeners, listenerFromBootConfig), + Errors: finalErrors, } if err := srv.Run(); err != nil { return fmt.Errorf("running server: %v", err) -- GitLab From 218b0aaf711dd9bac04e0bfc7e499d2939301de0 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Mon, 25 Apr 2022 10:29:06 +0000 Subject: [PATCH 4/4] Clarify the statement about certificates in docs --- doc/development/workhorse/configuration.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/development/workhorse/configuration.md b/doc/development/workhorse/configuration.md index fbf457f4cef0d4..ce80a155489d12 100644 --- a/doc/development/workhorse/configuration.md +++ b/doc/development/workhorse/configuration.md @@ -144,7 +144,7 @@ addr = "localhost:3443" max_version = "tls1.3" ``` -If the certificate is signed by a certificate authority, the `certificate` should be the concatenation +The `certificate` file should contain the concatenation of the server's certificate, any intermediates, and the CA's certificate. ## Interaction of authBackend and authSocket -- GitLab