Feature/use default aws credentials by Strnadj · Pull Request #951 · 0xPolygonID/issuer-node

Strnadj · 2025-11-30T11:46:07Z

No description provided.

ilya-korotya · 2026-03-19T09:14:42Z

+		}
+	}
+
+	return secretsmanager.NewFromConfig(cfg, options...), nil


Possible panic if url == nil, because options[0] will be nil in this case.

ilya-korotya · 2026-03-19T09:16:57Z

+		}
+	}
+
+	return kms.NewFromConfig(cfg, options...), nil


Possible panic if url == nil

ilya-korotya · 2026-03-19T09:18:46Z

-	var options []func(*awskms.Options)
-	if strings.ToLower(awsRegion) == "local" {
-		options = make([]func(*awskms.Options), 1)
+	options := make([]func(*awskms.Options), 1)


Possible panic

ilya-korotya · 2026-03-19T09:20:14Z

 	return nil
 }

+func LoadAWSConfig(ctx context.Context) (aws.Config, error) {


Code duplication. Can we put the LoadAWSConfig to common place?

ilya-korotya · 2026-03-19T09:20:22Z

+	"github.com/aws/aws-sdk-go-v2/service/secretsmanager"
+)
+
+func LoadAWSConfig(ctx context.Context) (aws.Config, error) {


Code duplication. Can we put the LoadAWSConfig to common place?

ilya-korotya · 2026-03-19T10:15:37Z

+	cfg, err := LoadAWSConfig(ctx)
+
+	if err != nil {
+		return nil, fmt.Errorf("unable to load SDK config, %v", err)


Use %w to wrapper the error.

ilya-korotya · 2026-03-19T10:15:47Z

+	cfg, err := LoadAWSConfig(ctx)
+
+	if err != nil {
+		return nil, fmt.Errorf("unable to load SDK config, %v", err)


ilya-korotya · 2026-03-19T10:15:58Z

+	client, err := AwsKms(ctx)
+
+	if err != nil {
+		return nil, fmt.Errorf("failed to create AWS KMS client: %v", err)


ilya-korotya · 2026-03-19T10:16:05Z

-			o.BaseEndpoint = aws.String(conf.URL)
-		}
+	if err != nil {
+		return nil, fmt.Errorf("failed to create AWS Secrets Manager client: %v", err)


ilya-korotya · 2026-03-19T11:14:10Z

These changes can brake local setups for users who use envs: ISSUER_KMS_AWS_ACCESS_KEY, ISSUER_KMS_AWS_SECRET_KEY, ISSUER_KMS_AWS_REGION

AWS_ACCESS_KEY_ID and the others are consumed by AWS lib by default 🤔

ilya-korotya · 2026-03-20T09:59:16Z

@@ -153,32 +147,26 @@ func main() {
 	}

 	if issuerKMSETHProviderToUse == config.AWSSM {


Can we reuse AwsSecretsManager from internal/kms/aws.go?

ilya-korotya · 2026-03-20T09:59:43Z

-		awsconfig.WithCredentialsProvider(credentials.NewStaticCredentialsProvider(awsAccessKey, awsSecretKey, "")),
-	)
+func createEmptyKey(ctx context.Context, privateKeyAlias string) (*string, error) {
+	cfg, err := kms.LoadAWSConfig(ctx)


Can we reuse AwsKMS from internal/kms/aws.go?

ilya-korotya

Don't merge yet.

Copilot

Pull request overview

This PR refactors the KMS AWS integrations to rely on the AWS SDK’s default credential/region resolution (and a shared helper), removing explicit AWS access key/secret/region wiring through the application config.

Changes:

Introduces centralized AWS SDK client creation helpers (AwsSecretsManager, AwsKms, LoadAWSConfig) and switches AWS KMS/Secrets Manager providers to use them.
Removes AWS credential/region fields from KMS/config plumbing and updates KMS/provider constructors accordingly.
Updates multiple tests to use the new constructors (but they now require explicit env setup for LocalStack/CI stability).

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 30 comments.

Show a summary per file

File	Description
internal/kms/aws.go	New helper functions for AWS SDK config + Secrets Manager/KMS clients.
internal/kms/aws_secret_storage_provider.go	Uses shared AWS Secrets Manager client helper.
internal/kms/aws_kms_eth_key_provider.go	Uses shared AWS KMS client helper.
internal/kms/kms.go	Stops passing explicit AWS config into providers; uses new constructors.
internal/config/config.go	Removes AWS access/secret; changes region env tag; leaves AWS URL/region fields present.
internal/config/config_test.go	Removes assertions for removed AWS config fields (but other expectations remain).
cmd/kms_priv_key_importer/main.go	Uses shared AWS client helpers instead of inline AWS SDK config.
internal/kms/*_test.go	Updates provider construction to new AWS constructors (now env-dependent).
Makefile	Falls back to standard AWS_* env vars when ISSUER_KMS_* are unset.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-24T12:48:57Z

 	ls := NewFileStorageManager(tmpFile.Name())

-	awsStorageProvider, err := NewAwsSecretStorageProvider(ctx, AwsSecretStorageProviderConfig{
-		AccessKey: "access_key",
-		SecretKey: "secret_key",
-		Region:    "local",
-		URL:       "http://localhost:4566",
-	})
+	awsStorageProvider, err := NewAwsSecretStorageProvider(ctx)
 	require.NoError(t, err)



This test creates an AWS-backed storage provider without configuring LocalStack endpoint/region/credentials via env vars. Set AWS_REGION, AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY, and ISSUER_KMS_AWS_URL in the test setup (t.Setenv) or mark as integration.

Copilot · 2026-03-24T12:48:58Z

 	ls := NewFileStorageManager(tmpFile.Name())

-	awsStorageProvider, err := NewAwsSecretStorageProvider(ctx, AwsSecretStorageProviderConfig{
-		AccessKey: "access_key",
-		SecretKey: "secret_key",
-		Region:    "local",
-		URL:       "http://localhost:4566",
-	})
+	awsStorageProvider, err := NewAwsSecretStorageProvider(ctx)
 	require.NoError(t, err)



NewAwsSecretStorageProvider(ctx) now relies on default AWS config; without explicit env vars this test is not hermetic and can fail in CI. Configure AWS_REGION + dummy creds + ISSUER_KMS_AWS_URL using t.Setenv before creating the provider.

Copilot · 2026-03-24T12:48:58Z

+	if accessKey != "" && secretKey != "" && region != "" {
+		return config.LoadDefaultConfig(
+			ctx,
+			config.WithCredentialsProvider(
+				credentials.NewStaticCredentialsProvider(accessKey, secretKey, ""),
+			),
+			config.WithRegion(region),
+		)


LoadAWSConfig only applies the deprecated ISSUER_KMS_AWS_ACCESS_KEY/SECRET_KEY when ISSUER_KMS_AWS_REGION is also set. If users set the deprecated access/secret vars but migrate region to AWS_REGION (or shared config), these credentials will be silently ignored and the SDK will fall back to the default chain (possibly using different credentials). Consider using the deprecated static credentials provider whenever access+secret are set, and only override the region when ISSUER_KMS_AWS_REGION is present.

Suggested change

if accessKey != "" && secretKey != "" && region != "" {

return config.LoadDefaultConfig(

ctx,

config.WithCredentialsProvider(

credentials.NewStaticCredentialsProvider(accessKey, secretKey, ""),

),

config.WithRegion(region),

)

if accessKey != "" && secretKey != "" {

opts := []func(*config.LoadOptions){

config.WithCredentialsProvider(

credentials.NewStaticCredentialsProvider(accessKey, secretKey, ""),

),

}

if region != "" {

opts = append(opts, config.WithRegion(region))

}

return config.LoadDefaultConfig(ctx, opts...)

Copilot · 2026-03-24T12:48:58Z

 func Test_getKeyMaterial(t *testing.T) {
 	ctx := context.Background()
-	awsStorageProvider, err := NewAwsSecretStorageProvider(ctx, AwsSecretStorageProviderConfig{
-		AccessKey: "access_key",
-		SecretKey: "secret_key",
-		Region:    "local",
-		URL:       "http://localhost:4566",
-	})
+	awsStorageProvider, err := NewAwsSecretStorageProvider(ctx)
 	require.NoError(t, err)



The provider construction here now relies on external AWS config; without setting LocalStack env vars this test will be non-deterministic and likely fail in CI. Add t.Setenv for AWS_REGION, AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY, ISSUER_KMS_AWS_URL (and optionally AWS_EC2_METADATA_DISABLED) before creating the provider.

Copilot · 2026-03-24T12:48:59Z

@@ -143,12 +123,7 @@ func Test_ListByIdentityInAWSKMS(t *testing.T) {

 func Test_SignInAWSKMS(t *testing.T) {


This test will perform real AWS KMS calls unless LocalStack/region/credentials env vars are set. Add t.Setenv for AWS_REGION, AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY, and ISSUER_KMS_AWS_URL before constructing the provider (or mark these as integration tests) to avoid accidental real-AWS usage and CI flakiness.

Suggested change

func Test_SignInAWSKMS(t *testing.T) {

func Test_SignInAWSKMS(t *testing.T) {

t.Setenv("AWS_REGION", "us-east-1")

t.Setenv("AWS_ACCESS_KEY_ID", "test")

t.Setenv("AWS_SECRET_ACCESS_KEY", "test")

t.Setenv("ISSUER_KMS_AWS_URL", "http://localhost:4566")

add Setenv for all tests taht use aws.

Copilot · 2026-03-24T12:49:04Z

 func Test_LinkToIdentityInAWSKMS(t *testing.T) {
 	ctx := context.Background()
-	awsStorageProvider, err := NewAwsKMSEthKeyProvider(ctx, ethereum, "pbkey", AwKmsEthKeyProviderConfig{
-		AccessKey: "access_key",
-		SecretKey: "secret_key",
-		Region:    "local",
-		URL:       "http://localhost:4566",
-	})
+	awsStorageProvider, err := NewAwsKMSEthKeyProvider(ctx, ethereum, "pbkey")
 	require.NoError(t, err)



This test uses the default AWS credential chain but doesn't configure region/endpoint/credentials for LocalStack. Please set env vars in-test (t.Setenv) or skip when not configured to avoid flaky CI and accidental calls to real AWS.

Copilot · 2026-03-24T12:49:04Z

 	ls := NewFileStorageManager(tmpFile.Name())

-	awsStorageProvider, err := NewAwsSecretStorageProvider(ctx, AwsSecretStorageProviderConfig{
-		AccessKey: "access_key",
-		SecretKey: "secret_key",
-		Region:    "local",
-		URL:       "http://localhost:4566",
-	})
+	awsStorageProvider, err := NewAwsSecretStorageProvider(ctx)
 	require.NoError(t, err)



This test initializes the AWS-backed storage provider with no LocalStack/AWS env setup, which can break CI and/or hit real AWS. Configure AWS_REGION, AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY, ISSUER_KMS_AWS_URL using t.Setenv before creating the provider.

Copilot · 2026-03-24T12:49:04Z

 	ls := NewFileStorageManager(tmpFile.Name())

-	awsStorageProvider, err := NewAwsSecretStorageProvider(ctx, AwsSecretStorageProviderConfig{
-		AccessKey: "access_key",
-		SecretKey: "secret_key",
-		Region:    "local",
-		URL:       "http://localhost:4566",
-	})
+	awsStorageProvider, err := NewAwsSecretStorageProvider(ctx)
 	require.NoError(t, err)


NewAwsSecretStorageProvider(ctx) now depends on the default AWS SDK chain and ISSUER_KMS_AWS_URL. This test doesn't set env vars, so it can fail in CI or hit real AWS. Set AWS_REGION + dummy creds + ISSUER_KMS_AWS_URL via t.Setenv before creating the provider (or gate as integration).

Copilot · 2026-03-24T12:49:04Z

 func Test_searchPrivateKey(t *testing.T) {
 	ctx := context.Background()
-	awsStorageProvider, err := NewAwsSecretStorageProvider(ctx, AwsSecretStorageProviderConfig{
-		AccessKey: "access_key",
-		SecretKey: "secret_key",
-		Region:    "local",
-		URL:       "http://localhost:4566",
-	})
+	awsStorageProvider, err := NewAwsSecretStorageProvider(ctx)
 	require.NoError(t, err)



This test case creates a real AWS Secrets Manager client via the default credential chain but doesn't set the environment expected for LocalStack. As a result, it can call real AWS or fail in CI. Please set AWS_REGION + dummy creds + ISSUER_KMS_AWS_URL in the test setup (or gate behind an integration test flag).

Copilot · 2026-03-24T12:49:05Z

@@ -12,12 +12,7 @@ import (

 func Test_SaveKeyMaterial(t *testing.T) {


These tests now construct the AWS Secrets Manager provider via the default AWS config chain, but they don't set AWS_REGION/AWS credentials or ISSUER_KMS_AWS_URL. In CI this will attempt real AWS (or fail resolving the region) instead of using LocalStack at localhost:4566. Set required env vars in the test (e.g., t.Setenv for AWS_REGION, AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY, ISSUER_KMS_AWS_URL, and optionally AWS_EC2_METADATA_DISABLED) or skip when LocalStack isn't available, so the tests remain deterministic.

Suggested change

func Test_SaveKeyMaterial(t *testing.T) {

func Test_SaveKeyMaterial(t *testing.T) {

t.Setenv("AWS_REGION", "us-east-1")

t.Setenv("AWS_ACCESS_KEY_ID", "test")

t.Setenv("AWS_SECRET_ACCESS_KEY", "test")

t.Setenv("AWS_EC2_METADATA_DISABLED", "true")

t.Setenv("ISSUER_KMS_AWS_URL", "http://localhost:4566")

ilya-korotya · 2026-03-26T10:52:04Z

Yep. I'm totally agree with copilot review. We have to modify our test to use localstack, because by default we use AWS credentials.

Strnadj requested a review from a team as a code owner November 30, 2025 11:46

ilya-korotya requested changes Mar 19, 2026

View reviewed changes

Strnadj force-pushed the feature/use_default_aws_credentials branch from 68ad6b2 to f91e2ea Compare March 19, 2026 16:26

ilya-korotya reviewed Mar 20, 2026

View reviewed changes

ilya-korotya self-requested a review March 24, 2026 11:07

ilya-korotya approved these changes Mar 24, 2026

View reviewed changes

OBrezhniev requested a review from Copilot March 24, 2026 12:38

Copilot started reviewing on behalf of OBrezhniev March 24, 2026 12:39 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

Strnadj force-pushed the feature/use_default_aws_credentials branch from 968b6d6 to b59fb55 Compare March 27, 2026 12:00

AWS configuration from IAM

650cf9a

Strnadj force-pushed the feature/use_default_aws_credentials branch from b59fb55 to 650cf9a Compare March 27, 2026 12:04

		@@ -153,32 +147,26 @@ func main() {
		}

		if issuerKMSETHProviderToUse == config.AWSSM {

		@@ -143,12 +123,7 @@ func Test_ListByIdentityInAWSKMS(t *testing.T) {

		func Test_SignInAWSKMS(t *testing.T) {

-func Test_SignInAWSKMS(t *testing.T) {
+func Test_SignInAWSKMS(t *testing.T) {
+	t.Setenv("AWS_REGION", "us-east-1")
+	t.Setenv("AWS_ACCESS_KEY_ID", "test")
+	t.Setenv("AWS_SECRET_ACCESS_KEY", "test")
+	t.Setenv("ISSUER_KMS_AWS_URL", "http://localhost:4566")

		@@ -12,12 +12,7 @@ import (

		func Test_SaveKeyMaterial(t *testing.T) {

Conversation

Strnadj commented Nov 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilya-korotya left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment