Merge pull request #2130 from fluxcd/backport-2126-to-release/v1.9.x

[release/v1.9.x] fix: restrict GCS static auth to SA keys

Author: stefanprodan

docs/spec/v1/buckets.md

@@ -750,6 +750,13 @@ Where the (base64 decoded) value of `.data.serviceaccount` looks like this:
 }
 ```
 
+Only service account keys are accepted, i.e. the `type` field must be set to
+`service_account`. Other Google credential configurations, such as
+`external_account` (workload identity federation), are rejected. To authenticate
+without a static key, use Workload Identity as shown in the
+[GCP Controller-Level](#gcp-controller-level-workload-identity-example) and
+[GCP Object-Level](#gcp-object-level-workload-identity-example) examples.
+
 ### Interval
 
 `.spec.interval` is a required field that specifies the interval which the

go.mod

@@ -9,6 +9,7 @@ replace github.com/fluxcd/source-controller/api => ./api
 replace github.com/opencontainers/go-digest => github.com/opencontainers/go-digest v1.0.1-0.20220411205349-bde1400a84be
 
 require (
+	cloud.google.com/go/auth v0.20.0
 	cloud.google.com/go/compute/metadata v0.9.0
 	cloud.google.com/go/storage v1.62.3
 	github.com/AdaLogics/go-fuzz-headers v0.0.0-20240806141605-e8a1dd7889d6
@@ -64,7 +65,6 @@ require (
 	github.com/sirupsen/logrus v1.9.4
 	github.com/spf13/pflag v1.0.10
 	golang.org/x/crypto v0.53.0
-	golang.org/x/oauth2 v0.36.0
 	golang.org/x/sync v0.21.0
 	google.golang.org/api v0.283.0
 	helm.sh/helm/v4 v4.2.2
@@ -80,7 +80,6 @@ require (
 require (
 	cel.dev/expr v0.25.1 // indirect
 	cloud.google.com/go v0.123.0 // indirect
-	cloud.google.com/go/auth v0.20.0 // indirect
 	cloud.google.com/go/auth/oauth2adapt v0.2.8 // indirect
 	cloud.google.com/go/iam v1.11.0 // indirect
 	cloud.google.com/go/monitoring v1.25.0 // indirect
@@ -396,6 +395,7 @@ require (
 	go.yaml.in/yaml/v3 v3.0.4 // indirect
 	golang.org/x/mod v0.36.0 // indirect
 	golang.org/x/net v0.56.0 // indirect
+	golang.org/x/oauth2 v0.36.0 // indirect
 	golang.org/x/sys v0.46.0 // indirect
 	golang.org/x/term v0.44.0 // indirect
 	golang.org/x/text v0.38.0 // indirect

internal/bucket/gcp/gcp.go

@@ -18,6 +18,7 @@ package gcp
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"io"
@@ -26,9 +27,9 @@ import (
 	"os"
 	"path/filepath"
 
+	"cloud.google.com/go/auth/credentials"
 	gcpstorage "cloud.google.com/go/storage"
 	"github.com/go-logr/logr"
-	"golang.org/x/oauth2/google"
 	"google.golang.org/api/iterator"
 	"google.golang.org/api/option"
 	htransport "google.golang.org/api/transport/http"
@@ -112,7 +113,20 @@ func NewClient(ctx context.Context, bucket *sourcev1.Bucket, opts ...Option) (*G
 
 	switch {
 	case o.secret != nil && o.proxyURL == nil:
-		clientOpts = append(clientOpts, option.WithCredentialsJSON(o.secret.Data["serviceaccount"]))
+		if os.Getenv("STORAGE_EMULATOR_HOST") != "" {
+			// The storage client runs unauthenticated when the emulator host is
+			// set and rejects credential options passed alongside it. Validate
+			// the secret, but let the client run unauthenticated.
+			if err := ValidateSecret(o.secret); err != nil {
+				return nil, err
+			}
+		} else {
+			credsOpt, err := staticCredentialsOption(o.secret)
+			if err != nil {
+				return nil, err
+			}
+			clientOpts = append(clientOpts, credsOpt)
+		}
 	case o.secret == nil && o.proxyURL == nil:
 		tokenSource := gcpauth.NewTokenSource(ctx, o.authOpts...)
 		clientOpts = append(clientOpts, option.WithTokenSource(tokenSource))
@@ -142,15 +156,11 @@ func newHTTPClient(ctx context.Context, o *options) (*http.Client, error) {
 	var opts []option.ClientOption
 
 	if o.secret != nil {
-		// Here we can't use option.WithCredentialsJSON() because htransport.NewTransport()
-		// won't know what scopes to use and yield a 400 Bad Request error when retrieving
-		// the OAuth token. Instead we use google.CredentialsFromJSON(), which allows us to
-		// specify the GCS read-only scope.
-		creds, err := google.CredentialsFromJSON(ctx, o.secret.Data["serviceaccount"], gcpstorage.ScopeReadOnly)
+		credsOpt, err := staticCredentialsOption(o.secret)
 		if err != nil {
-			return nil, fmt.Errorf("failed to create Google credentials from secret: %w", err)
+			return nil, err
 		}
-		opts = append(opts, option.WithCredentials(creds))
+		opts = append(opts, credsOpt)
 	} else { // Workload Identity.
 		tokenSource := gcpauth.NewTokenSource(ctx, o.authOpts...)
 		opts = append(opts, option.WithTokenSource(tokenSource))
@@ -163,16 +173,53 @@ func newHTTPClient(ctx context.Context, o *options) (*http.Client, error) {
 	return &http.Client{Transport: transport}, nil
 }
 
+// staticCredentialsOption returns a client option holding type-checked Google
+// credentials built from the service account key in the given Secret,
+// restricted to the GCS read-only scope.
+func staticCredentialsOption(secret *corev1.Secret) (option.ClientOption, error) {
+	key, err := serviceAccountKey(secret)
+	if err != nil {
+		return nil, err
+	}
+	creds, err := credentials.NewCredentialsFromJSON(credentials.ServiceAccount, key, &credentials.DetectOptions{
+		Scopes: []string{gcpstorage.ScopeReadOnly},
+	})
+	if err != nil {
+		return nil, fmt.Errorf("failed to create Google credentials from secret: %w", err)
+	}
+	return option.WithAuthCredentials(creds), nil
+}
+
+// serviceAccountKey returns the service account key held in the 'serviceaccount'
+// field of the given Secret. Only Google service account keys are supported;
+// any other credential configuration type is rejected.
+func serviceAccountKey(secret *corev1.Secret) ([]byte, error) {
+	key, exists := secret.Data["serviceaccount"]
+	if !exists {
+		return nil, fmt.Errorf("invalid '%s' secret data: required fields 'serviceaccount'", secret.Name)
+	}
+	var creds struct {
+		Type string `json:"type"`
+	}
+	// The parser error is not wrapped, as it may quote the malformed input.
+	if err := json.Unmarshal(key, &creds); err != nil {
+		return nil, fmt.Errorf("invalid '%s' secret data: failed to parse 'serviceaccount' as JSON", secret.Name)
+	}
+	if creds.Type != string(credentials.ServiceAccount) {
+		return nil, fmt.Errorf("invalid '%s' secret data: 'serviceaccount' must contain a service account key with 'type' set to '%s'",
+			secret.Name, credentials.ServiceAccount)
+	}
+	return key, nil
+}
+
 // ValidateSecret validates the credential secret. The provided Secret may
 // be nil.
 func ValidateSecret(secret *corev1.Secret) error {
 	if secret == nil {
 		return nil
 	}
-	if _, exists := secret.Data["serviceaccount"]; !exists {
-		return fmt.Errorf("invalid '%s' secret data: required fields 'serviceaccount'", secret.Name)
-	}
-	return nil
+	_, err := serviceAccountKey(secret)
+	return err
 }
 
 // BucketExists returns if an object storage bucket with the provided name

internal/bucket/gcp/gcp_test.go

@@ -66,7 +66,7 @@ var (
 			Namespace: "default",
 		},
 		Data: map[string][]byte{
-			"serviceaccount": []byte("ewogICAgInR5cGUiOiAic2VydmljZV9hY2NvdW50IiwKICAgICJwcm9qZWN0X2lkIjogInBvZGluZm8iLAogICAgInByaXZhdGVfa2V5X2lkIjogIjI4cXdnaDNnZGY1aGozZ2I1ZmozZ3N1NXlmZ2gzNGY0NTMyNDU2OGh5MiIsCiAgICAicHJpdmF0ZV9rZXkiOiAiLS0tLS1CRUdJTiBQUklWQVRFIEtFWS0tLS0tXG5Id2V0aGd5MTIzaHVnZ2hoaGJkY3U2MzU2ZGd5amhzdmd2R0ZESFlnY2RqYnZjZGhic3g2M2Ncbjc2dGd5Y2ZlaHVoVkdURllmdzZ0N3lkZ3lWZ3lkaGV5aHVnZ3ljdWhland5NnQzNWZ0aHl1aGVndmNldGZcblRGVUhHVHlnZ2h1Ymh4ZTY1eWd0NnRneWVkZ3kzMjZodWN5dnN1aGJoY3Zjc2poY3NqaGNzdmdkdEhGQ0dpXG5IY3llNnR5eWczZ2Z5dWhjaGNzYmh5Z2NpamRiaHl5VEY2NnR1aGNldnVoZGNiaHVoaHZmdGN1aGJoM3VoN3Q2eVxuZ2d2ZnRVSGJoNnQ1cmZ0aGh1R1ZSdGZqaGJmY3JkNXI2N3l1aHV2Z0ZUWWpndnRmeWdoYmZjZHJoeWpoYmZjdGZkZnlodmZnXG50Z3ZnZ3RmeWdodmZ0NnR1Z3ZURjVyNjZ0dWpoZ3ZmcnR5aGhnZmN0Nnk3eXRmcjVjdHZnaGJoaHZ0Z2hoanZjdHRmeWNmXG5mZnhmZ2hqYnZnY2d5dDY3dWpiZ3ZjdGZ5aFZDN3VodmdjeWp2aGhqdnl1amNcbmNnZ2hndmdjZmhnZzc2NTQ1NHRjZnRoaGdmdHloaHZ2eXZ2ZmZnZnJ5eXU3N3JlcmVkc3dmdGhoZ2ZjZnR5Y2ZkcnR0ZmhmL1xuLS0tLS1FTkQgUFJJVkFURSBLRVktLS0tLVxuIiwKICAgICJjbGllbnRfZW1haWwiOiAidGVzdEBwb2RpbmZvLmlhbS5nc2VydmljZWFjY291bnQuY29tIiwKICAgICJjbGllbnRfaWQiOiAiMzI2NTc2MzQ2Nzg3NjI1MzY3NDYiLAogICAgImF1dGhfdXJpIjogImh0dHBzOi8vYWNjb3VudHMuZ29vZ2xlLmNvbS9vL29hdXRoMi9hdXRoIiwKICAgICJ0b2tlbl91cmkiOiAiaHR0cHM6Ly9vYXV0aDIuZ29vZ2xlYXBpcy5jb20vdG9rZW4iLAogICAgImF1dGhfcHJvdmlkZXJfeDUwOV9jZXJ0X3VybCI6ICJodHRwczovL3d3dy5nb29nbGVhcGlzLmNvbS9vYXV0aDIvdjEvY2VydHMiLAogICAgImNsaWVudF94NTA5X2NlcnRfdXJsIjogImh0dHBzOi8vd3d3Lmdvb2dsZWFwaXMuY29tL3JvYm90L3YxL21ldGFkYXRhL3g1MDkvdGVzdCU0MHBvZGluZm8uaWFtLmdzZXJ2aWNlYWNjb3VudC5jb20iCn0="),
+			"serviceaccount": []byte(serviceAccountJSON),
 		},
 		Type: "Opaque",
 	}
@@ -80,8 +80,57 @@ var (
 		},
 		Type: "Opaque",
 	}
+	malformedSecret = corev1.Secret{
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "gcp-secret",
+			Namespace: "default",
+		},
+		Data: map[string][]byte{
+			"serviceaccount": []byte("not-json"),
+		},
+		Type: "Opaque",
+	}
+	externalAccountSecret = corev1.Secret{
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "gcp-secret",
+			Namespace: "default",
+		},
+		Data: map[string][]byte{
+			"serviceaccount": []byte(externalAccountJSON),
+		},
+		Type: "Opaque",
+	}
 )
 
+// serviceAccountJSON is a dummy GCP service account key. The private key is a
+// throwaway RSA key generated for this test only; it does not belong to any
+// real service account and cannot be used to retrieve tokens, only to
+// exercise credential construction.
+const serviceAccountJSON = `{
+    "type": "service_account",
+    "project_id": "podinfo",
+    "private_key_id": "28qwgh3gdf5hj3gb5fj3gsu5yfgh34f45324568hy2",
+    "private_key": "-----BEGIN PRIVATE KEY-----\nMIIEvAIBADANBgkqhkiG9w0BAQEFAASCBKYwggSiAgEAAoIBAQC02OoAWFKPVfE2\nTVYmdQKb8WRZo8ciQomAYb7WluGqa6avPu/9z7+q4K7hZtN/9Q2t0EvvBuKXTL8+\nV+AUV2N+oi4nY7T2yq3Ebc66R/c21o9sNvYJog6S8HWtOOd0tvotrvrjFQnfcCX5\nzC48HC9PgUjHAeR8kacYlpKbaEobRI1i9AfN5deuS5tsI0Di8Ag6lGf7G+0gHUpM\nLmUttJCleOltOl1Xwb57/zZCgGwB5VdkaBtkq6KW0xxwhOqF1RiFuGOUf+vRNjfj\nLPx58XSTL0TNtYa3M8koBLMaVznY5du6Pqpq1VBSUo3Yw0Z8DwxErZBICkvwbEsQ\njmQNpK+5AgMBAAECgf8l/OqQ5/yi3z3fI9LU55jrHBzx0QiJycnYWq/ocG+dIyYz\ngz6MsiowwSf7CpgJNaojhX2hCz4A474uxyBRJYotlfB1lbXA1KvSEL7Vom64T8zd\njmEtGApRRosLHmKAKDw+tfxwqhqvNLLFcuTDYg6nqyCYE22x1pnWMGR1AJMqGgFr\nSjfTl2wtSQzT705Cd3oNoItqPdYh4Ky6dxImfiHcj237mFXWy9e5x6l0N7ShJzqS\nH3t8s5wnxjt9jAK7NBqCFSxvYyKSO7MTLBlOLUdM8KuzjsYgKw0e2j+D4LhXKc/N\n1nXY2hUgq4NAOnYoK4HUAkOcN34wLjxTvp6wlwECgYEA7pkh0/QPrGVZ9bjAExq6\nCY9Z+gejUDOHWl4aWQeZA36g3FvlPgHTFTB7hMbmABbVEjCbLrnSFe9aMQsYm0O2\n/4MAUAqXX9bV134YK3kbtYH2qXX/60oGjACcbx0CIzOAO8prF7h0MKol065POaK8\nLGupztnn1fP/cH3GXF/cVLkCgYEAwgmD874uAYrXGPZ25PjH+J+L2IGj7WIa173O\ni+WJe/5Lp/A/fp9zr/ln5x2t5Pg7btysayGz1e8TGfrEBJ8ADzIm9z9ale9XHRz5\nVIBqO+bh3PW+iBs2ocZkfMtXigDCkIBP/lutvzvcFN/fsvGw1DF8PCmN6no6/gC5\nwzNjswECgYEAmbzp4xx7jOWxVXc5rBWokchgfY62WFMbf8rqxzryCSJqnBJKX+3l\nCN44eJGAWcZcfF/9Xdo12BRl1PwFWuYC4BiU9v4cE5DmMPf6suhSRl37ha2WvRDx\nrvwl0CKs4empUt1Wq+4aT9ESlpbWTZjiDu1AeRxHGcEicmVYjuTln2ECgYEAkciC\naiQN/ryoxSmPxJKh88szT7R/TD/0OPlzcKpBdHZns0KPAfyc967kAMHMwAY86RtF\nM6x7qBVafZ9pnKs1aTVeD097KMFM6yO0tGdS6bSbJ98+ipYfosYjA5vnJllR1S2C\nbHHHBbHctZZKRPDP0W1okO8LoAq7vdEfwGgg1QECgYBBuHECqDcz+buKSZRAQqRm\ngqt4hdcu+qRMqleZY4WPNHAZoPna9hU+7EFM+D1qsB6iZxzsUXzKF9gmJfB+Zvcn\nmLQq0YXDeXBt2KWeQSyQLM2bBW0J3aFIpilaP3VRcXUPEF7FstT0DC+SkydbANIf\n5IiZW7E1qU/WBIWMkD4WuA==\n-----END PRIVATE KEY-----\n",
+    "client_email": "test@podinfo.iam.gserviceaccount.com",
+    "client_id": "32657634678762536746",
+    "auth_uri": "https://accounts.google.com/o/oauth2/auth",
+    "token_uri": "https://oauth2.googleapis.com/token",
+    "auth_provider_x509_cert_url": "https://www.googleapis.com/oauth2/v1/certs",
+    "client_x509_cert_url": "https://www.googleapis.com/robot/v1/metadata/x509/test%40podinfo.iam.gserviceaccount.com"
+}`
+
+// externalAccountJSON is a workload identity federation configuration, which
+// is not a supported credential type for the 'serviceaccount' secret field.
+const externalAccountJSON = `{
+    "type": "external_account",
+    "audience": "//iam.googleapis.com/projects/1/locations/global/workloadIdentityPools/p/providers/v",
+    "subject_token_type": "urn:ietf:params:oauth:token-type:jwt",
+    "token_url": "https://sts.googleapis.com/v1/token",
+    "credential_source": {
+        "file": "/var/run/service-account/token"
+    }
+}`
+
 // createTestBucket creates a test bucket for testing purposes
 func createTestBucket() *sourcev1.Bucket {
 	return &sourcev1.Bucket{
@@ -162,14 +211,48 @@ func TestMain(m *testing.M) {
 	os.Exit(run)
 }
 
-func TestNewClientWithSecretErr(t *testing.T) {
-	bucket := createTestBucket()
-	gcpClient, err := NewClient(context.Background(), bucket, WithSecret(secret.DeepCopy()))
-	t.Log(err)
-	g := NewWithT(t)
-	g.Expect(err).To(HaveOccurred())
-	g.Expect(err.Error()).To(Equal("dialing: invalid character 'e' looking for beginning of value"))
-	g.Expect(gcpClient).To(BeNil())
+func TestNewClientWithSecret(t *testing.T) {
+	tests := []struct {
+		name    string
+		secret  *corev1.Secret
+		wantErr string
+	}{
+		{
+			name:   "service account key",
+			secret: secret.DeepCopy(),
+		},
+		{
+			name:    "missing serviceaccount field",
+			secret:  badSecret.DeepCopy(),
+			wantErr: "invalid 'gcp-secret' secret data: required fields 'serviceaccount'",
+		},
+		{
+			name:    "serviceaccount is not JSON",
+			secret:  malformedSecret.DeepCopy(),
+			wantErr: "invalid 'gcp-secret' secret data: failed to parse 'serviceaccount' as JSON",
+		},
+		{
+			name:    "unsupported credential type",
+			secret:  externalAccountSecret.DeepCopy(),
+			wantErr: "invalid 'gcp-secret' secret data: 'serviceaccount' must contain a service account key with 'type' set to 'service_account'",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+			gcpClient, err := NewClient(context.Background(), createTestBucket(), WithSecret(tt.secret))
+			if tt.wantErr != "" {
+				g.Expect(err).To(HaveOccurred())
+				g.Expect(err.Error()).To(Equal(tt.wantErr))
+				g.Expect(gcpClient).To(BeNil())
+				return
+			}
+			g.Expect(err).NotTo(HaveOccurred())
+			g.Expect(gcpClient).NotTo(BeNil())
+			gcpClient.Close(context.Background())
+		})
+	}
 }
 
 func TestNewClientWithProxyErr(t *testing.T) {
@@ -184,9 +267,20 @@ func TestNewClientWithProxyErr(t *testing.T) {
 		gcpClient, err := NewClient(context.Background(), bucket,
 			WithProxyURL(&url.URL{}),
 			WithSecret(secret.DeepCopy()))
+		g.Expect(err).NotTo(HaveOccurred())
+		g.Expect(gcpClient).NotTo(BeNil())
+		gcpClient.Close(context.Background())
+	})
+
+	t.Run("with unsupported credential type", func(t *testing.T) {
+		g := NewWithT(t)
+		bucket := createTestBucket()
+		gcpClient, err := NewClient(context.Background(), bucket,
+			WithProxyURL(&url.URL{}),
+			WithSecret(externalAccountSecret.DeepCopy()))
 		g.Expect(err).To(HaveOccurred())
 		g.Expect(gcpClient).To(BeNil())
-		g.Expect(err.Error()).To(Equal("failed to create Google credentials from secret: invalid character 'e' looking for beginning of value"))
+		g.Expect(err.Error()).To(Equal("invalid 'gcp-secret' secret data: 'serviceaccount' must contain a service account key with 'type' set to 'service_account'"))
 	})
 
 	t.Run("without secret", func(t *testing.T) {
@@ -369,18 +463,32 @@ func TestFGetObjectDirectoryIsFileName(t *testing.T) {
 func TestValidateSecret(t *testing.T) {
 	t.Parallel()
 	testCases := []struct {
-		name   string
-		secret *corev1.Secret
-		error  bool
+		name    string
+		secret  *corev1.Secret
+		wantErr string
 	}{
 		{
-			name:   "valid secret",
+			name:   "nil secret",
+			secret: nil,
+		},
+		{
+			name:   "service account key",
 			secret: secret.DeepCopy(),
 		},
 		{
-			name:   "invalid secret",
-			secret: badSecret.DeepCopy(),
-			error:  true,
+			name:    "missing serviceaccount field",
+			secret:  badSecret.DeepCopy(),
+			wantErr: "invalid 'gcp-secret' secret data: required fields 'serviceaccount'",
+		},
+		{
+			name:    "serviceaccount is not JSON",
+			secret:  malformedSecret.DeepCopy(),
+			wantErr: "invalid 'gcp-secret' secret data: failed to parse 'serviceaccount' as JSON",
+		},
+		{
+			name:    "unsupported credential type",
+			secret:  externalAccountSecret.DeepCopy(),
+			wantErr: "invalid 'gcp-secret' secret data: 'serviceaccount' must contain a service account key with 'type' set to 'service_account'",
 		},
 	}
 	for _, testCase := range testCases {
@@ -389,9 +497,9 @@ func TestValidateSecret(t *testing.T) {
 			t.Parallel()
 			err := ValidateSecret(tt.secret)
 			g := NewWithT(t)
-			if tt.error {
+			if tt.wantErr != "" {
 				g.Expect(err).To(HaveOccurred())
-				g.Expect(err.Error()).To(Equal(fmt.Sprintf("invalid '%v' secret data: required fields 'serviceaccount'", tt.secret.Name)))
+				g.Expect(err.Error()).To(Equal(tt.wantErr))
 			} else {
 				g.Expect(err).NotTo(HaveOccurred())
 			}