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 <gsamfira@cloudbasesolutions.com>

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.

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(),
+		Addr: cfg.APIServer.BindAddress(),
-		TLSConfig: tlsCfg,
 		// 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 {
+		if cfg.APIServer.UseTLS {
-			log.Printf("Listening: %+v", err)
+			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)
+			}
 		}
 	}()
 

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"`
+	CRT string `toml:"certificate" json:"certificate"`
-	Key    string `toml:"key" json:"key"`
+	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
 }
 
 // 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)

config/config_test.go

@@ -39,9 +39,8 @@ func getDefaultSectionConfig(configDir string) Default {
 
 func getDefaultTLSConfig() TLSConfig {
 	return TLSConfig{
-		CRT:    "../testdata/certs/srv-pub.pem",
+		CRT: "../testdata/certs/srv-pub.pem",
-		Key:    "../testdata/certs/srv-key.pem",
+		Key: "../testdata/certs/srv-key.pem",
-		CACert: "../testdata/certs/ca-pub.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 {

testdata/certs/ca-pub.pem

@@ -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-----

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-----
+

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.