Browse Source

fix: restore AWS credential chain resolution for ECRAuthorizationToken generator (#5082)

* restore AWS default credential chain

Signed-off-by: Aditya Menon <amenon@canarytechnologies.com>

* fix: ensure SecretStore status updates persist when validation fails

When validateStore fails, the controller was returning an error immediately
which prevented the deferred status patch from executing. This caused test
failures where status conditions were not being persisted.

Changes:
- Return with RequeueAfter instead of error when validation fails to allow
  status update to complete
- Fix patch creation timing by capturing original state before modifications
- Use ReasonInvalidProviderConfig for ValidationResultError to match test
  expectations

This ensures status conditions are properly set and persisted even when
store validation fails, providing better feedback to users about
configuration issues.

Signed-off-by: Aditya Menorahalli <aditya@canary.ws>
Signed-off-by: Aditya Menon <amenon@canarytechnologies.com>

* add tests for auth

Signed-off-by: Aditya Menon <amenon@canarytechnologies.com>

* add error handling for config.LoadDefaultConfig(ctx)

Signed-off-by: Aditya Menon <amenon@canarytechnologies.com>

* remove whitspace in tests ctx.Background()

Signed-off-by: Aditya Menon <amenon@canarytechnologies.com>

---------

Signed-off-by: Aditya Menon <amenon@canarytechnologies.com>
Signed-off-by: Aditya Menorahalli <aditya@canary.ws>
Aditya Menon 8 months ago
parent
commit
7acab694b6

+ 8 - 2
pkg/controllers/secretstore/common.go

@@ -124,9 +124,15 @@ func validateStore(ctx context.Context, namespace, controllerClass string, store
 	}
 	validationResult, err := cl.Validate()
 	if err != nil && validationResult != esapi.ValidationResultUnknown {
-		cond := NewSecretStoreCondition(esapi.SecretStoreReady, v1.ConditionFalse, esapi.ReasonValidationFailed, fmt.Sprintf(errUnableValidateStore, err))
+		// Use ReasonInvalidProviderConfig for validation errors that indicate
+		// invalid configuration (like empty server URL)
+		reason := esapi.ReasonValidationFailed
+		if validationResult == esapi.ValidationResultError {
+			reason = esapi.ReasonInvalidProviderConfig
+		}
+		cond := NewSecretStoreCondition(esapi.SecretStoreReady, v1.ConditionFalse, reason, fmt.Sprintf(errUnableValidateStore, err))
 		SetExternalSecretCondition(store, *cond, gaugeVecGetter)
-		recorder.Event(store, v1.EventTypeWarning, esapi.ReasonValidationFailed, err.Error())
+		recorder.Event(store, v1.EventTypeWarning, reason, err.Error())
 		return fmt.Errorf(errValidationFailed, err)
 	}
 

+ 4 - 0
pkg/generator/ecr/ecr_test.go

@@ -54,6 +54,7 @@ func TestGenerate(t *testing.T) {
 		{
 			name: "nil spec",
 			args: args{
+				ctx: context.Background(),
 				jsonSpec: nil,
 			},
 			wantErr: true,
@@ -61,6 +62,7 @@ func TestGenerate(t *testing.T) {
 		{
 			name: "invalid json",
 			args: args{
+				ctx: context.Background(),
 				authTokenPrivateFunc: func(gati *ecr.GetAuthorizationTokenInput) (*ecr.GetAuthorizationTokenOutput, error) {
 					return nil, errors.New("boom")
 				},
@@ -73,6 +75,7 @@ func TestGenerate(t *testing.T) {
 		{
 			name: "private ECR full spec",
 			args: args{
+				ctx: context.Background(),
 				namespace: "foobar",
 				kube: clientfake.NewClientBuilder().WithObjects(&v1.Secret{
 					ObjectMeta: metav1.ObjectMeta{
@@ -123,6 +126,7 @@ spec:
 		{
 			name: "public ECR full spec",
 			args: args{
+				ctx: context.Background(),
 				namespace: "foobar",
 				authTokenPublicFunc: func(in *ecrpublic.GetAuthorizationTokenInput) (*ecrpublic.GetAuthorizationTokenOutput, error) {
 					t := time.Unix(5678, 0)

+ 3 - 0
pkg/generator/sts/sts_test.go

@@ -51,6 +51,7 @@ func TestGenerate(t *testing.T) {
 		{
 			name: "nil spec",
 			args: args{
+				ctx: context.Background(),
 				jsonSpec: nil,
 			},
 			wantErr: true,
@@ -58,6 +59,7 @@ func TestGenerate(t *testing.T) {
 		{
 			name: "invalid json",
 			args: args{
+				ctx: context.Background(),
 				tokenFunc: func(ctx context.Context, input *sts.GetSessionTokenInput, optFns ...func(*sts.Options)) (*sts.GetSessionTokenOutput, error) {
 					return nil, errors.New("boom")
 				},
@@ -70,6 +72,7 @@ func TestGenerate(t *testing.T) {
 		{
 			name: "full spec",
 			args: args{
+				ctx: context.Background(),
 				namespace: "foobar",
 				kube: clientfake.NewClientBuilder().WithObjects(&v1.Secret{
 					ObjectMeta: metav1.ObjectMeta{

+ 10 - 7
pkg/provider/aws/auth/auth.go

@@ -195,20 +195,23 @@ func NewGeneratorSession(ctx context.Context, auth esv1.AWSAuth, role, region st
 			return nil, err
 		}
 	}
-	config := aws.NewConfig()
+	awscfg, err := config.LoadDefaultConfig(ctx)
+	if err != nil {
+		return nil, err
+	}
 	if credsProvider != nil {
-		config.Credentials = credsProvider
+		awscfg.Credentials = credsProvider
 	}
 	if region != "" {
-		config.Region = region
+		awscfg.Region = region
 	}
 
 	if role != "" {
-		stsclient := assumeRoler(*config)
-		config.Credentials = stscreds.NewAssumeRoleProvider(stsclient, role)
+		stsclient := assumeRoler(awscfg)
+		awscfg.Credentials = stscreds.NewAssumeRoleProvider(stsclient, role)
 	}
-	log.Info("using aws config", "region", config.Region, "credentials", config.Credentials)
-	return config, nil
+	log.Info("using aws config", "region", awscfg.Region, "credentials", awscfg.Credentials)
+	return &awscfg, nil
 }
 
 // credsFromSecretRef pulls access-key / secret-access-key from a secretRef to

+ 171 - 0
pkg/provider/aws/auth/auth_test.go

@@ -647,3 +647,174 @@ func ErrorContains(out error, want string) bool {
 	}
 	return strings.Contains(out.Error(), want)
 }
+
+func TestNewGeneratorSession_DefaultCredentialChain(t *testing.T) {
+	cfg, err := NewGeneratorSession(context.Background(), esv1.AWSAuth{}, "", "us-east-1", clientfake.NewClientBuilder().Build(), "test-ns", DefaultSTSProvider, DefaultJWTProvider)
+	assert.NoError(t, err)
+	assert.NotNil(t, cfg)
+	assert.Equal(t, "us-east-1", cfg.Region)
+}
+
+func TestNewGeneratorSession_CredentialProviderPriority(t *testing.T) {
+	ctx := context.Background()
+	k8sClient := clientfake.NewClientBuilder().Build()
+
+	assert.NoError(t, k8sClient.Create(ctx, &v1.Secret{
+		ObjectMeta: metav1.ObjectMeta{Name: "aws-creds", Namespace: "test-ns"},
+		Data: map[string][]byte{
+			"access-key": []byte("SECRET_KEY_ID"),
+			"secret-key": []byte("SECRET_ACCESS_KEY"),
+		},
+	}))
+
+	assert.NoError(t, k8sClient.Create(ctx, &v1.ServiceAccount{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        "test-sa",
+			Namespace:   "test-ns",
+			Annotations: map[string]string{roleARNAnnotation: "arn:aws:iam::123456789012:role/test-role"},
+		},
+	}))
+
+	jwtProviderCalled := false
+	cfg, err := NewGeneratorSession(ctx, esv1.AWSAuth{
+		JWTAuth: &esv1.AWSJWTAuth{
+			ServiceAccountRef: &esmeta.ServiceAccountSelector{Name: "test-sa"},
+		},
+		SecretRef: &esv1.AWSAuthSecretRef{
+			AccessKeyID:     esmeta.SecretKeySelector{Name: "aws-creds", Key: "access-key"},
+			SecretAccessKey: esmeta.SecretKeySelector{Name: "aws-creds", Key: "secret-key"},
+		},
+	}, "", "us-east-1", k8sClient, "test-ns", DefaultSTSProvider, func(name, namespace, roleArn string, aud []string, region string) (aws.CredentialsProvider, error) {
+		jwtProviderCalled = true
+		assert.Equal(t, "test-sa", name)
+		assert.Equal(t, "test-ns", namespace)
+		assert.Equal(t, "arn:aws:iam::123456789012:role/test-role", roleArn)
+		return fakesess.CredentialsProvider{
+			RetrieveFunc: func() (aws.Credentials, error) {
+				return aws.Credentials{
+					AccessKeyID:     "JWT_ACCESS_KEY",
+					SecretAccessKey: "JWT_SECRET_KEY",
+					SessionToken:    "JWT_SESSION_TOKEN",
+					Source:          "jwt",
+				}, nil
+			},
+		}, nil
+	})
+
+	assert.NoError(t, err)
+	assert.NotNil(t, cfg)
+	assert.True(t, jwtProviderCalled)
+
+	creds, err := cfg.Credentials.Retrieve(ctx)
+	assert.NoError(t, err)
+	assert.Equal(t, "SECRET_KEY_ID", creds.AccessKeyID)
+	assert.Equal(t, "SECRET_ACCESS_KEY", creds.SecretAccessKey)
+}
+
+func TestNewGeneratorSession_OnlySecretRef(t *testing.T) {
+	ctx := context.Background()
+	k8sClient := clientfake.NewClientBuilder().Build()
+
+	assert.NoError(t, k8sClient.Create(ctx, &v1.Secret{
+		ObjectMeta: metav1.ObjectMeta{Name: "aws-creds", Namespace: "test-ns"},
+		Data: map[string][]byte{
+			"access-key": []byte("SECRET_KEY_ID"),
+			"secret-key": []byte("SECRET_ACCESS_KEY"),
+		},
+	}))
+
+	cfg, err := NewGeneratorSession(ctx, esv1.AWSAuth{
+		SecretRef: &esv1.AWSAuthSecretRef{
+			AccessKeyID:     esmeta.SecretKeySelector{Name: "aws-creds", Key: "access-key"},
+			SecretAccessKey: esmeta.SecretKeySelector{Name: "aws-creds", Key: "secret-key"},
+		},
+	}, "", "us-east-1", k8sClient, "test-ns", DefaultSTSProvider, DefaultJWTProvider)
+
+	assert.NoError(t, err)
+	assert.NotNil(t, cfg)
+
+	creds, err := cfg.Credentials.Retrieve(ctx)
+	assert.NoError(t, err)
+	assert.Equal(t, "SECRET_KEY_ID", creds.AccessKeyID)
+	assert.Equal(t, "SECRET_ACCESS_KEY", creds.SecretAccessKey)
+}
+
+func TestNewGeneratorSession_RegionConfiguration(t *testing.T) {
+	ctx := context.Background()
+	k8sClient := clientfake.NewClientBuilder().Build()
+
+	testCases := []struct {
+		name           string
+		region         string
+		expectedRegion string
+	}{
+		{"region specified", "us-east-1", "us-east-1"},
+		{"empty region uses default", "", ""},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			cfg, err := NewGeneratorSession(ctx, esv1.AWSAuth{}, "", tc.region, k8sClient, "test-ns", DefaultSTSProvider, DefaultJWTProvider)
+			assert.NoError(t, err)
+			assert.NotNil(t, cfg)
+			if tc.expectedRegion != "" {
+				assert.Equal(t, tc.expectedRegion, cfg.Region)
+			}
+		})
+	}
+}
+
+func TestNewGeneratorSession_AssumeRoleWithDefaultCredentials(t *testing.T) {
+	t.Setenv("AWS_ACCESS_KEY_ID", "BASE_ACCESS_KEY")
+	t.Setenv("AWS_SECRET_ACCESS_KEY", "BASE_SECRET_KEY")
+
+	stsProviderCalled := false
+	cfg, err := NewGeneratorSession(context.Background(), esv1.AWSAuth{}, "arn:aws:iam::123456789012:role/assumed-role", "us-east-1", clientfake.NewClientBuilder().Build(), "test-ns", func(cfg aws.Config) STSprovider {
+		stsProviderCalled = true
+		creds, err := cfg.Credentials.Retrieve(context.Background())
+		assert.NoError(t, err)
+		assert.Equal(t, "BASE_ACCESS_KEY", creds.AccessKeyID)
+		return &fakesess.AssumeRoler{
+			AssumeRoleFunc: func(input *sts.AssumeRoleInput) (*sts.AssumeRoleOutput, error) {
+				assert.Equal(t, "arn:aws:iam::123456789012:role/assumed-role", *input.RoleArn)
+				return &sts.AssumeRoleOutput{
+					AssumedRoleUser: &ststypes.AssumedRoleUser{
+						Arn:           aws.String("arn:aws:sts::123456789012:assumed-role/assumed-role/session"),
+						AssumedRoleId: aws.String("AROA123456"),
+					},
+					Credentials: &ststypes.Credentials{
+						AccessKeyId:     aws.String("ASSUMED_ACCESS_KEY"),
+						SecretAccessKey: aws.String("ASSUMED_SECRET_KEY"),
+						SessionToken:    aws.String("ASSUMED_SESSION_TOKEN"),
+						Expiration:      aws.Time(time.Now().Add(time.Hour)),
+					},
+				}, nil
+			},
+		}
+	}, DefaultJWTProvider)
+
+	assert.NoError(t, err)
+	assert.NotNil(t, cfg)
+	assert.True(t, stsProviderCalled)
+
+	creds, err := cfg.Credentials.Retrieve(context.Background())
+	assert.NoError(t, err)
+	assert.Equal(t, "ASSUMED_ACCESS_KEY", creds.AccessKeyID)
+	assert.Equal(t, "ASSUMED_SECRET_KEY", creds.SecretAccessKey)
+	assert.Equal(t, "ASSUMED_SESSION_TOKEN", creds.SessionToken)
+}
+
+func TestNewGeneratorSession_DefaultCredentialChainFallback(t *testing.T) {
+	t.Setenv("AWS_ACCESS_KEY_ID", "ENV_ACCESS_KEY")
+	t.Setenv("AWS_SECRET_ACCESS_KEY", "ENV_SECRET_KEY")
+	t.Setenv("AWS_SESSION_TOKEN", "ENV_SESSION_TOKEN")
+
+	cfg, err := NewGeneratorSession(context.Background(), esv1.AWSAuth{}, "", "us-east-1", clientfake.NewClientBuilder().Build(), "test-ns", DefaultSTSProvider, DefaultJWTProvider)
+	assert.NoError(t, err)
+	assert.NotNil(t, cfg)
+
+	creds, err := cfg.Credentials.Retrieve(context.Background())
+	assert.NoError(t, err)
+	assert.NotEmpty(t, creds.AccessKeyID)
+	assert.NotEmpty(t, creds.SecretAccessKey)
+}