Add support for self-signed CA certificates (#2268)

Co-authored-by: Bassem Dghaidi <568794+Link-@users.noreply.github.com>
Co-authored-by: Nikola Jokic <jokicnikola07@gmail.com>
Co-authored-by: Tingluo Huang <tingluohuang@github.com>

github/actions/actions_server_test.go

@@ -58,12 +58,6 @@ func newActionsServer(t *testing.T, handler http.Handler, options ...actionsServ
 
 type actionsServerOption func(*actionsServer)
 
-func withActionsToken(token string) actionsServerOption {
-	return func(s *actionsServer) {
-		s.token = token
-	}
-}
-
 type actionsServer struct {
 	*httptest.Server
 

github/actions/client.go

@@ -198,6 +198,12 @@ func (c *Client) Identifier() string {
 		)
 	}
 
+	if c.rootCAs != nil {
+		// ignoring because this cert pool is intended not to come from SystemCertPool
+		// nolint:staticcheck
+		identifier += fmt.Sprintf("rootCAs:%q", c.rootCAs.Subjects())
+	}
+
 	return uuid.NewHash(sha256.New(), uuid.NameSpaceOID, []byte(identifier), 6).String()
 }
 

github/actions/client_tls_test.go

@@ -95,8 +95,8 @@ func TestServerWithSelfSignedCertificates(t *testing.T) {
 		cert, err := os.ReadFile(filepath.Join("testdata", "rootCA.crt"))
 		require.NoError(t, err)
 
-		pool, err := actions.RootCAsFromConfigMap(map[string][]byte{"cert": cert})
-		require.NoError(t, err)
+		pool := x509.NewCertPool()
+		require.True(t, pool.AppendCertsFromPEM(cert))
 
 		client, err := actions.NewClient(configURL, auth, actions.WithRootCAs(pool))
 		require.NoError(t, err)
@@ -123,8 +123,8 @@ func TestServerWithSelfSignedCertificates(t *testing.T) {
 		cert, err := os.ReadFile(filepath.Join("testdata", "intermediate.pem"))
 		require.NoError(t, err)
 
-		pool, err := actions.RootCAsFromConfigMap(map[string][]byte{"cert": cert})
-		require.NoError(t, err)
+		pool := x509.NewCertPool()
+		require.True(t, pool.AppendCertsFromPEM(cert))
 
 		client, err := actions.NewClient(configURL, auth, actions.WithRootCAs(pool), actions.WithRetryMax(0))
 		require.NoError(t, err)

github/actions/github_api_request_test.go

@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"github.com/actions/actions-runner-controller/github/actions"
+	"github.com/actions/actions-runner-controller/github/actions/testserver"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -95,9 +96,9 @@ func TestNewActionsServiceRequest(t *testing.T) {
 	t.Run("manages authentication", func(t *testing.T) {
 		t.Run("client is brand new", func(t *testing.T) {
 			token := defaultActionsToken(t)
-			server := newActionsServer(t, nil, withActionsToken(token))
+			server := testserver.New(t, nil, testserver.WithActionsToken(token))
 
-			client, err := actions.NewClient(server.configURLForOrg("my-org"), defaultCreds)
+			client, err := actions.NewClient(server.ConfigURLForOrg("my-org"), defaultCreds)
 			require.NoError(t, err)
 
 			req, err := client.NewActionsServiceRequest(ctx, http.MethodGet, "my-path", nil)
@@ -108,9 +109,9 @@ func TestNewActionsServiceRequest(t *testing.T) {
 
 		t.Run("admin token is about to expire", func(t *testing.T) {
 			newToken := defaultActionsToken(t)
-			server := newActionsServer(t, nil, withActionsToken(newToken))
+			server := testserver.New(t, nil, testserver.WithActionsToken(newToken))
 
-			client, err := actions.NewClient(server.configURLForOrg("my-org"), defaultCreds)
+			client, err := actions.NewClient(server.ConfigURLForOrg("my-org"), defaultCreds)
 			require.NoError(t, err)
 			client.ActionsServiceAdminToken = "expiring-token"
 			client.ActionsServiceAdminTokenExpiresAt = time.Now().Add(59 * time.Second)
@@ -123,9 +124,9 @@ func TestNewActionsServiceRequest(t *testing.T) {
 
 		t.Run("token is currently valid", func(t *testing.T) {
 			tokenThatShouldNotBeFetched := defaultActionsToken(t)
-			server := newActionsServer(t, nil, withActionsToken(tokenThatShouldNotBeFetched))
+			server := testserver.New(t, nil, testserver.WithActionsToken(tokenThatShouldNotBeFetched))
 
-			client, err := actions.NewClient(server.configURLForOrg("my-org"), defaultCreds)
+			client, err := actions.NewClient(server.ConfigURLForOrg("my-org"), defaultCreds)
 			require.NoError(t, err)
 			client.ActionsServiceAdminToken = "healthy-token"
 			client.ActionsServiceAdminTokenExpiresAt = time.Now().Add(1 * time.Hour)
@@ -138,9 +139,9 @@ func TestNewActionsServiceRequest(t *testing.T) {
 	})
 
 	t.Run("builds the right URL including api version", func(t *testing.T) {
-		server := newActionsServer(t, nil)
+		server := testserver.New(t, nil)
 
-		client, err := actions.NewClient(server.configURLForOrg("my-org"), defaultCreds)
+		client, err := actions.NewClient(server.ConfigURLForOrg("my-org"), defaultCreds)
 		require.NoError(t, err)
 
 		req, err := client.NewActionsServiceRequest(ctx, http.MethodGet, "/my/path?name=banana", nil)
@@ -157,9 +158,9 @@ func TestNewActionsServiceRequest(t *testing.T) {
 	})
 
 	t.Run("populates header", func(t *testing.T) {
-		server := newActionsServer(t, nil)
+		server := testserver.New(t, nil)
 
-		client, err := actions.NewClient(server.configURLForOrg("my-org"), defaultCreds, actions.WithUserAgent("my-agent"))
+		client, err := actions.NewClient(server.ConfigURLForOrg("my-org"), defaultCreds, actions.WithUserAgent("my-agent"))
 		require.NoError(t, err)
 
 		req, err := client.NewActionsServiceRequest(ctx, http.MethodGet, "/my/path", nil)

github/actions/identifier_test.go

@@ -1,6 +1,9 @@
 package actions_test
 
 import (
+	"crypto/x509"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/actions/actions-runner-controller/github/actions"
@@ -108,4 +111,48 @@ func TestClient_Identifier(t *testing.T) {
 			})
 		}
 	})
+
+	t.Run("changes in TLS config", func(t *testing.T) {
+		configURL := "https://github.com/org/repo"
+		defaultCreds := &actions.ActionsAuth{
+			Token: "token",
+		}
+
+		noTlS, err := actions.NewClient(configURL, defaultCreds)
+		require.NoError(t, err)
+
+		poolFromCert := func(t *testing.T, path string) *x509.CertPool {
+			t.Helper()
+			f, err := os.ReadFile(path)
+			require.NoError(t, err)
+			pool := x509.NewCertPool()
+			require.True(t, pool.AppendCertsFromPEM(f))
+			return pool
+		}
+
+		root, err := actions.NewClient(
+			configURL,
+			defaultCreds,
+			actions.WithRootCAs(poolFromCert(t, filepath.Join("testdata", "rootCA.crt"))),
+		)
+		require.NoError(t, err)
+
+		chain, err := actions.NewClient(
+			configURL,
+			defaultCreds,
+			actions.WithRootCAs(poolFromCert(t, filepath.Join("testdata", "intermediate.pem"))),
+		)
+		require.NoError(t, err)
+
+		clients := []*actions.Client{
+			noTlS,
+			root,
+			chain,
+		}
+		identifiers := map[string]struct{}{}
+		for _, client := range clients {
+			identifiers[client.Identifier()] = struct{}{}
+		}
+		assert.Len(t, identifiers, len(clients), "all clients should have a unique identifier")
+	})
 }

github/actions/multi_client.go

@@ -2,7 +2,6 @@ package actions
 
 import (
 	"context"
-	"crypto/x509"
 	"fmt"
 	"strconv"
 	"sync"
@@ -84,7 +83,7 @@ func (m *multiClient) GetClientFor(ctx context.Context, githubConfigURL string,
 	}
 
 	cachedClient, has := m.clients[key]
-	if has {
+	if has && cachedClient.rootCAs.Equal(client.rootCAs) {
 		m.logger.Info("using cache client", "githubConfigURL", githubConfigURL, "namespace", namespace)
 		return cachedClient, nil
 	}
@@ -141,19 +140,3 @@ func (m *multiClient) GetClientFromSecret(ctx context.Context, githubConfigURL,
 	auth.AppCreds = &GitHubAppAuth{AppID: parsedAppID, AppInstallationID: parsedAppInstallationID, AppPrivateKey: appPrivateKey}
 	return m.GetClientFor(ctx, githubConfigURL, auth, namespace, options...)
 }
-
-func RootCAsFromConfigMap(configMapData map[string][]byte) (*x509.CertPool, error) {
-	caCertPool, err := x509.SystemCertPool()
-	if err != nil {
-		caCertPool = x509.NewCertPool()
-	}
-
-	for key, certData := range configMapData {
-		ok := caCertPool.AppendCertsFromPEM(certData)
-		if !ok {
-			return nil, fmt.Errorf("no certificates successfully parsed from key %s", key)
-		}
-	}
-
-	return caCertPool, nil
-}