From 27a523f1337364bf107854d5d909b7b8e633de77 Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Tue, 31 Jan 2023 13:42:39 +0000 Subject: [PATCH] Fix TLS listener The TLS listener was not being set up correctly. The TLSConfig was changed to include only cert and key. The cert now needs to be a full chain bundle including intermediary CA certificates. The ca_cert config option was removed. Signed-off-by: Gabriel Adrian Samfira --- README.md | 2 +- cmd/garm/main.go | 18 +++--- config/config.go | 51 +++------------- config/config_test.go | 119 +------------------------------------ testdata/certs/ca-pub.pem | 20 ------- testdata/certs/srv-pub.pem | 21 +++++++ testdata/config.toml | 17 ++++-- 7 files changed, 54 insertions(+), 194 deletions(-) delete mode 100644 testdata/certs/ca-pub.pem diff --git a/README.md b/README.md index 36b22a19..f4ba063e 100644 --- a/README.md +++ b/README.md @@ -27,7 +27,7 @@ You should now have both ```garm``` and ```garm-cli``` in your ```$GOPATH/bin``` If you have docker/podman installed, you can also build statically linked binaries by running: ```bash - make + make build-static ``` The ```garm``` and ```garm-cli``` binaries will be built and copied to the ```bin/``` folder in your current working directory. diff --git a/cmd/garm/main.go b/cmd/garm/main.go index 834c3086..37e1fb2f 100644 --- a/cmd/garm/main.go +++ b/cmd/garm/main.go @@ -160,14 +160,8 @@ func main() { methodsOk := handlers.AllowedMethods([]string{"GET", "HEAD", "POST", "PUT", "OPTIONS", "DELETE"}) headersOk := handlers.AllowedHeaders([]string{"X-Requested-With", "Content-Type", "Authorization"}) - tlsCfg, err := cfg.APIServer.APITLSConfig() - if err != nil { - log.Fatalf("failed to get TLS config: %q", err) - } - srv := &http.Server{ - Addr: cfg.APIServer.BindAddress(), - TLSConfig: tlsCfg, + Addr: cfg.APIServer.BindAddress(), // Pass our instance of gorilla/mux in. Handler: handlers.CORS(methodsOk, headersOk, allowedOrigins)(router), } @@ -178,8 +172,14 @@ func main() { } go func() { - if err := srv.Serve(listener); err != http.ErrServerClosed { - log.Printf("Listening: %+v", err) + if cfg.APIServer.UseTLS { + if err := srv.ServeTLS(listener, cfg.APIServer.TLSConfig.CRT, cfg.APIServer.TLSConfig.Key); err != nil { + log.Printf("Listening: %+v", err) + } + } else { + if err := srv.Serve(listener); err != http.ErrServerClosed { + log.Printf("Listening: %+v", err) + } } }() diff --git a/config/config.go b/config/config.go index 390bc076..932973d8 100644 --- a/config/config.go +++ b/config/config.go @@ -433,45 +433,18 @@ func (m *MySQL) ConnectionString() (string, error) { // TLSConfig is the API server TLS config type TLSConfig struct { - CRT string `toml:"certificate" json:"certificate"` - Key string `toml:"key" json:"key"` - CACert string `toml:"ca_certificate" json:"ca-certificate"` -} - -// TLSConfig returns a new TLSConfig suitable for use in the -// API server -func (t *TLSConfig) TLSConfig() (*tls.Config, error) { - // TLS config not present. - if t.CRT == "" || t.Key == "" { - return nil, fmt.Errorf("missing crt or key") - } - - var roots *x509.CertPool - if t.CACert != "" { - caCertPEM, err := os.ReadFile(t.CACert) - if err != nil { - return nil, err - } - roots = x509.NewCertPool() - ok := roots.AppendCertsFromPEM(caCertPEM) - if !ok { - return nil, fmt.Errorf("failed to parse CA cert") - } - } - - cert, err := tls.LoadX509KeyPair(t.CRT, t.Key) - if err != nil { - return nil, err - } - return &tls.Config{ - Certificates: []tls.Certificate{cert}, - ClientCAs: roots, - }, nil + CRT string `toml:"certificate" json:"certificate"` + Key string `toml:"key" json:"key"` } // Validate validates the TLS config func (t *TLSConfig) Validate() error { - if _, err := t.TLSConfig(); err != nil { + if t.CRT == "" || t.Key == "" { + return fmt.Errorf("missing crt or key") + } + + _, err := tls.LoadX509KeyPair(t.CRT, t.Key) + if err != nil { return err } return nil @@ -492,14 +465,6 @@ type APIServer struct { CORSOrigins []string `toml:"cors_origins" json:"cors-origins"` } -func (a *APIServer) APITLSConfig() (*tls.Config, error) { - if !a.UseTLS { - return nil, nil - } - - return a.TLSConfig.TLSConfig() -} - // BindAddress returns a host:port string. func (a *APIServer) BindAddress() string { return fmt.Sprintf("%s:%d", a.Bind, a.Port) diff --git a/config/config_test.go b/config/config_test.go index d8910fc9..1d7fa446 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -39,9 +39,8 @@ func getDefaultSectionConfig(configDir string) Default { func getDefaultTLSConfig() TLSConfig { return TLSConfig{ - CRT: "../testdata/certs/srv-pub.pem", - Key: "../testdata/certs/srv-key.pem", - CACert: "../testdata/certs/ca-pub.pem", + CRT: "../testdata/certs/srv-pub.pem", + Key: "../testdata/certs/srv-key.pem", } } @@ -293,120 +292,6 @@ func TestAPIBindAddress(t *testing.T) { require.Equal(t, cfg.BindAddress(), "0.0.0.0:9998") } -func TestAPITLSconfig(t *testing.T) { - cfg := getDefaultAPIServerConfig() - - err := cfg.Validate() - require.Nil(t, err) - - tlsCfg, err := cfg.APITLSConfig() - require.Nil(t, err) - require.NotNil(t, tlsCfg) - - // Any error in the TLSConfig should return an error here. - cfg.TLSConfig = TLSConfig{} - tlsCfg, err = cfg.APITLSConfig() - require.Nil(t, tlsCfg) - require.NotNil(t, err) - require.EqualError(t, err, "missing crt or key") - - // If TLS is disabled, don't validate TLSconfig. - cfg.UseTLS = false - tlsCfg, err = cfg.APITLSConfig() - require.Nil(t, err) - require.Nil(t, tlsCfg) -} - -func TestTLSConfig(t *testing.T) { - dir, err := os.MkdirTemp("", "garm-config-test") - if err != nil { - t.Fatalf("failed to create temporary directory: %s", err) - } - t.Cleanup(func() { os.RemoveAll(dir) }) - - invalidCert := filepath.Join(dir, "invalid_cert.pem") - err = os.WriteFile(invalidCert, []byte("bogus content"), 0755) - if err != nil { - t.Fatalf("failed to write file: %s", err) - } - - cfg := getDefaultTLSConfig() - - err = cfg.Validate() - require.Nil(t, err) - - tests := []struct { - name string - cfg TLSConfig - errString string - }{ - { - name: "Config is valid", - cfg: cfg, - errString: "", - }, - { - name: "missing crt", - cfg: TLSConfig{ - CRT: "", - Key: cfg.Key, - CACert: cfg.CACert, - }, - errString: "missing crt or key", - }, - { - name: "missing key", - cfg: TLSConfig{ - CRT: cfg.CRT, - Key: "", - CACert: cfg.CACert, - }, - errString: "missing crt or key", - }, - { - name: "invalid CA cert", - cfg: TLSConfig{ - CRT: cfg.CRT, - Key: cfg.Key, - CACert: invalidCert, - }, - errString: "failed to parse CA cert", - }, - { - name: "invalid cert", - cfg: TLSConfig{ - CRT: invalidCert, - Key: cfg.Key, - CACert: cfg.CACert, - }, - errString: "tls: failed to find any PEM data in certificate input", - }, - { - name: "invalid key", - cfg: TLSConfig{ - CRT: cfg.CRT, - Key: invalidCert, - CACert: cfg.CACert, - }, - errString: "tls: failed to find any PEM data in key input", - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - tlsCfg, err := tc.cfg.TLSConfig() - if tc.errString == "" { - require.Nil(t, err) - require.NotNil(t, tlsCfg) - } else { - require.NotNil(t, err) - require.Nil(t, tlsCfg) - require.Regexp(t, tc.errString, err.Error()) - } - }) - } -} - func TestDatabaseConfig(t *testing.T) { dir, err := os.MkdirTemp("", "garm-config-test") if err != nil { diff --git a/testdata/certs/ca-pub.pem b/testdata/certs/ca-pub.pem deleted file mode 100644 index dcfd8819..00000000 --- a/testdata/certs/ca-pub.pem +++ /dev/null @@ -1,20 +0,0 @@ ------BEGIN CERTIFICATE----- -MIIDTDCCAjSgAwIBAgIRAMrfG/ZCst3T0RtRgcnY5h0wDQYJKoZIhvcNAQELBQAw -MDEcMBoGA1UEChMTQ2xvdWRiYXNlIFNvbHV0aW9uczEQMA4GA1UEAxMHUm9vdCBD -QTAeFw0yMjA3MDQxODM3MjdaFw0zMjA3MDExODM3MjdaMDAxHDAaBgNVBAoTE0Ns -b3VkYmFzZSBTb2x1dGlvbnMxEDAOBgNVBAMTB1Jvb3QgQ0EwggEiMA0GCSqGSIb3 -DQEBAQUAA4IBDwAwggEKAoIBAQCooxNCaSfs8UhVmavARonC5S2mFLCSLXrHGPhS -+hjAgqQrF8pI/LvJmsV1LAjYjIAqwE9eDIbuyj8nBuaPJYFoS4McaKZmuo/UYqgx -5K+rDfMdDUFIP0XGTopJFcfKguQQU6Tx+8v5Ubc8C9WjRFDRbmR5ihNzKb1Eb/y1 -OrwmsNtMmgyZFCm6yDMymXFgqTo58zTj9d04uwumVLjSoJxPEqytf9LpKJoeoj7O -tnSq8OMPyhsPu3LgZyvyB65ehb1NChica99Dh5bee4muQAkYGUqbRXgbau6edhsQ -MtXM4mzaXdQIMva3rPfnBfPuhc9WjnKmmmOsWDv510nbl0hVAgMBAAGjYTBfMA4G -A1UdDwEB/wQEAwICBDAdBgNVHSUEFjAUBggrBgEFBQcDAgYIKwYBBQUHAwEwDwYD -VR0TAQH/BAUwAwEB/zAdBgNVHQ4EFgQUoesgOFlL4kKwT1enjyGkxCEUPSowDQYJ -KoZIhvcNAQELBQADggEBAHlWX8iDkTtADKMAbfBMzvsolzQjZALMab2Jdco7QKtO -i3g6k7UWsgAO+cbH8XCM87ty6JuPUFJNibLWHl1A4dSqwbGGNw+wLVLqNzu8dDqC -E1WG3rIFkXFa3z3eZhauBcdp2FCLbuHtD4g/yGHE/LExnIZeHMpbF2MuC0V34PhY -daj2FUYJe+hmiKRajXGYjv6jOTAbylLK8qzF7HmTnLIJ0hahmKnykC2FiwAVpxZC -T0OcNEXjR1FJfSVHJC2OCOZXPftP7ssZmO18j35UMGk/oxkrUz0839rQtT5oZPI6 -UuNSyP3+BcQtdrDUiCOum651ojwvEO4umlQX3zGmo7U= ------END CERTIFICATE----- diff --git a/testdata/certs/srv-pub.pem b/testdata/certs/srv-pub.pem index 24c1712e..82923058 100644 --- a/testdata/certs/srv-pub.pem +++ b/testdata/certs/srv-pub.pem @@ -21,3 +21,24 @@ wCB6NY6X2RriUGoBr1dIJeDWjYMLQqBh2m9+8vCLsQ7A7TzDJfF7JxhHV94qZ4c4 LKaBdKxl4UGr6HuRMPapsoQqVWV+ZjZrPf4LSoj3DenNwzOc/Rm3Nya8xDCCLxqu RgNpSos4SD2WTF1RG2buzmgrsdvd3mWX -----END CERTIFICATE----- +-----BEGIN CERTIFICATE----- +MIIDTDCCAjSgAwIBAgIRAMrfG/ZCst3T0RtRgcnY5h0wDQYJKoZIhvcNAQELBQAw +MDEcMBoGA1UEChMTQ2xvdWRiYXNlIFNvbHV0aW9uczEQMA4GA1UEAxMHUm9vdCBD +QTAeFw0yMjA3MDQxODM3MjdaFw0zMjA3MDExODM3MjdaMDAxHDAaBgNVBAoTE0Ns +b3VkYmFzZSBTb2x1dGlvbnMxEDAOBgNVBAMTB1Jvb3QgQ0EwggEiMA0GCSqGSIb3 +DQEBAQUAA4IBDwAwggEKAoIBAQCooxNCaSfs8UhVmavARonC5S2mFLCSLXrHGPhS ++hjAgqQrF8pI/LvJmsV1LAjYjIAqwE9eDIbuyj8nBuaPJYFoS4McaKZmuo/UYqgx +5K+rDfMdDUFIP0XGTopJFcfKguQQU6Tx+8v5Ubc8C9WjRFDRbmR5ihNzKb1Eb/y1 +OrwmsNtMmgyZFCm6yDMymXFgqTo58zTj9d04uwumVLjSoJxPEqytf9LpKJoeoj7O +tnSq8OMPyhsPu3LgZyvyB65ehb1NChica99Dh5bee4muQAkYGUqbRXgbau6edhsQ +MtXM4mzaXdQIMva3rPfnBfPuhc9WjnKmmmOsWDv510nbl0hVAgMBAAGjYTBfMA4G +A1UdDwEB/wQEAwICBDAdBgNVHSUEFjAUBggrBgEFBQcDAgYIKwYBBQUHAwEwDwYD +VR0TAQH/BAUwAwEB/zAdBgNVHQ4EFgQUoesgOFlL4kKwT1enjyGkxCEUPSowDQYJ +KoZIhvcNAQELBQADggEBAHlWX8iDkTtADKMAbfBMzvsolzQjZALMab2Jdco7QKtO +i3g6k7UWsgAO+cbH8XCM87ty6JuPUFJNibLWHl1A4dSqwbGGNw+wLVLqNzu8dDqC +E1WG3rIFkXFa3z3eZhauBcdp2FCLbuHtD4g/yGHE/LExnIZeHMpbF2MuC0V34PhY +daj2FUYJe+hmiKRajXGYjv6jOTAbylLK8qzF7HmTnLIJ0hahmKnykC2FiwAVpxZC +T0OcNEXjR1FJfSVHJC2OCOZXPftP7ssZmO18j35UMGk/oxkrUz0839rQtT5oZPI6 +UuNSyP3+BcQtdrDUiCOum651ojwvEO4umlQX3zGmo7U= +-----END CERTIFICATE----- + diff --git a/testdata/config.toml b/testdata/config.toml index 45746f27..1182ad4a 100644 --- a/testdata/config.toml +++ b/testdata/config.toml @@ -3,12 +3,18 @@ # This URL is used by instances to send back status messages as they install # the github actions runner. Status messages can be seen by querying the # runner status in garm. +# Note: If you're using a reverse proxy in front of your garm installation, +# this URL needs to point to the address of the reverse proxy. Using TLS is +# highly encouraged. callback_url = "https://garm.example.com/api/v1/callbacks/status" # This URL is used by instances to retrieve information they need to set themselves # up. Access to this URL is granted using the same JWT token used to send back # status updates. Once the instance transitions to "installed" or "failed" state, # access to both the status and metadata endpoints is disabled. +# Note: If you're using a reverse proxy in front of your garm installation, +# this URL needs to point to the address of the reverse proxy. Using TLS is +# highly encouraged. metadata_url = "https://garm.example.com/api/v1/metadata" # This folder is defined here for future use. Right now, we create a SSH @@ -28,7 +34,8 @@ enable = true # Toggle to disable authentication (not recommended) on the metrics endpoint. # If you do disable authentication, I encourage you to put a reverse proxy in front # of garm and limit which systems can access that particular endpoint. Ideally, you -# would enable some kind of authentication using the reverse proxy. +# would enable some kind of authentication using the reverse proxy, if the built-in auth +# is not sufficient for your needs. disable_auth = false [jwt_auth] @@ -57,12 +64,14 @@ time_to_live = "8760h" # A literal of "*" will allow any origin cors_origins = ["*"] [apiserver.tls] - # Path on disk to a x509 certificate. + # Path on disk to a x509 certificate bundle. + # NOTE: if your certificate is signed by an intermediary CA, this file + # must contain the entire certificate bundle needed for clients to validate + # the certificate. This usually means concatenating the certificate and the + # CA bundle you received. certificate = "" # The path on disk to the corresponding private key for the certificate. key = "" - # CA certificate bundle to use. - ca_certificate = "" [database] # Turn on/off debugging for database queries.