From 9b223966086148e8946c033d3c761d6cde242322 Mon Sep 17 00:00:00 2001 From: Young Date: Thu, 13 Feb 2020 18:42:31 -0800 Subject: [PATCH] support JSON key and version for secretsmanager secrets specified for roadmap issue #385 https://github.com/aws/containers-roadmap/issues/385 this commit adds the ability for customers to add parameters to the secretsmanager ARN specified in containers. agent will be able to retrieve secret by version or retrieve part of a secret by json key. this commit also fixes a minor issue breaking go vet in an unrelated test. --- .gitignore | 1 + agent/asm/asm.go | 29 ++++ agent/asm/asm_test.go | 78 +++++++++- agent/engine/docker_task_engine_test.go | 4 +- agent/engine/engine_unix_integ_test.go | 2 +- agent/taskresource/asmsecret/asmsecret.go | 82 +++++++++- .../taskresource/asmsecret/asmsecret_test.go | 145 +++++++++++++----- 7 files changed, 289 insertions(+), 52 deletions(-) diff --git a/.gitignore b/.gitignore index 07bb3fe5291..29cc0e1a63d 100644 --- a/.gitignore +++ b/.gitignore @@ -22,3 +22,4 @@ _bin/ /misc/v3-task-endpoint-validator/v3-task-endpoint-validator /misc/elastic-inference-validator/elastic-inference-validator /bin +*.iml diff --git a/agent/asm/asm.go b/agent/asm/asm.go index df5e80d5af6..4bfc22b0536 100644 --- a/agent/asm/asm.go +++ b/agent/asm/asm.go @@ -15,10 +15,12 @@ package asm import ( "encoding/json" + "fmt" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/secretsmanager" "github.com/aws/aws-sdk-go/service/secretsmanager/secretsmanageriface" + "github.com/cihub/seelog" "github.com/docker/docker/api/types" "github.com/pkg/errors" ) @@ -87,6 +89,33 @@ func extractASMValue(out *secretsmanager.GetSecretValueOutput) (types.AuthConfig return dac, nil } +func GetSecretFromASMWithInput(input *secretsmanager.GetSecretValueInput, + client secretsmanageriface.SecretsManagerAPI, jsonKey string) (string, error) { + out, err := client.GetSecretValue(input) + if err != nil { + return "", errors.Wrapf(err, "secret %s", *input.SecretId) + } + + if jsonKey == "" { + return aws.StringValue(out.SecretString), nil + } + + secretMap := make(map[string]interface{}) + jsonErr := json.Unmarshal([]byte(*out.SecretString), &secretMap) + if jsonErr != nil { + seelog.Warnf("Error when treating retrieved secret value with secret id %s as JSON and calling unmarshal.", *input.SecretId) + return "", jsonErr + } + + secretValue, ok := secretMap[jsonKey] + if !ok { + err = errors.New(fmt.Sprintf("Retrieved secret from Secrets Manager did not contain json key %s", jsonKey)) + return "", err + } + + return fmt.Sprintf("%v", secretValue), nil +} + // GetSecretFromASM makes the api call to the AWS Secrets Manager service to // retrieve the secret value func GetSecretFromASM(secretID string, client secretsmanageriface.SecretsManagerAPI) (string, error) { diff --git a/agent/asm/asm_test.go b/agent/asm/asm_test.go index 3b405ad5c98..ba064c1c17f 100644 --- a/agent/asm/asm_test.go +++ b/agent/asm/asm_test.go @@ -22,6 +22,17 @@ import ( "github.com/aws/aws-sdk-go/service/secretsmanager" "github.com/aws/aws-sdk-go/service/secretsmanager/secretsmanageriface" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + versionID = "versionId" + versionStage = "versionStage" + jsonKey = "jsonKey" + valueFrom = "arn:aws:secretsmanager:region:account-id:secret:secretId" + secretValue = "secretValue" + jsonSecretValue = "{\"" + jsonKey + "\": \"" + secretValue + "\",\"some-other-key\": \"secret2\"}" + malformedJsonSecretValue = "{\"" + jsonKey + "\": \"" + secretValue ) type mockGetSecretValue struct { @@ -111,11 +122,70 @@ func TestASMGetAuthConfig(t *testing.T) { } func TestGetSecretFromASM(t *testing.T) { - asmClient := mockGetSecretValue{ + asmClient := createASMInterface(secretValue) + _, err := GetSecretFromASM("secretName", asmClient) + assert.NoError(t, err) +} + +func TestGetSecretFromASMWithJsonKey(t *testing.T) { + asmClient := createASMInterface(jsonSecretValue) + secretValueInput := createSecretValueInput(toPtr(valueFrom), nil, nil) + outSecretValue, _ := GetSecretFromASMWithInput(secretValueInput, asmClient, jsonKey) + assert.Equal(t, secretValue, outSecretValue) +} + +func TestGetSecretFromASMWithMalformedJSON(t *testing.T) { + asmClient := createASMInterface(malformedJsonSecretValue) + secretValueInput := createSecretValueInput(toPtr(valueFrom), nil, nil) + outSecretValue, err := GetSecretFromASMWithInput(secretValueInput, asmClient, jsonKey) + require.Error(t, err) + assert.Equal(t, "", outSecretValue) +} + +func TestGetSecretFromASMWithJSONKeyNotFound(t *testing.T) { + asmClient := createASMInterface(jsonSecretValue) + secretValueInput := createSecretValueInput(toPtr(valueFrom), nil, nil) + nonExistentKey := "nonExistentKey" + _, err := GetSecretFromASMWithInput(secretValueInput, asmClient, nonExistentKey) + assert.Error(t, err) +} + +func TestGetSecretFromASMWithVersionID(t *testing.T) { + asmClient := createASMInterface(secretValue) + secretValueInput := createSecretValueInput(toPtr(valueFrom), toPtr(versionID), nil) + outSecretValue, err := GetSecretFromASMWithInput(secretValueInput, asmClient, "") + require.NoError(t, err) + assert.Equal(t, secretValue, outSecretValue) +} + +func TestGetSecretFromASMWithVersionIDAndStage(t *testing.T) { + asmClient := createASMInterface(secretValue) + secretValueInput := createSecretValueInput(toPtr(valueFrom), toPtr(versionID), toPtr(versionStage)) + outSecretValue, err := GetSecretFromASMWithInput(secretValueInput, asmClient, "") + require.NoError(t, err) + assert.Equal(t, secretValue, outSecretValue) +} + +func toPtr(input string) *string { + if input == "" { + return nil + } + output := input + return &output +} + +func createSecretValueInput(secretID *string, versionID *string, versionStage *string) *secretsmanager.GetSecretValueInput { + return &secretsmanager.GetSecretValueInput{ + SecretId: secretID, + VersionId: versionID, + VersionStage: versionStage, + } +} + +func createASMInterface(secretValue string) mockGetSecretValue { + return mockGetSecretValue{ Resp: secretsmanager.GetSecretValueOutput{ - SecretString: aws.String("secretValue"), + SecretString: aws.String(secretValue), }, } - _, err := GetSecretFromASM("secretName", asmClient) - assert.NoError(t, err) } diff --git a/agent/engine/docker_task_engine_test.go b/agent/engine/docker_task_engine_test.go index d27211a96d4..8bc547cd76a 100644 --- a/agent/engine/docker_task_engine_test.go +++ b/agent/engine/docker_task_engine_test.go @@ -2503,7 +2503,7 @@ func TestTaskSecretsEnvironmentVariables(t *testing.T) { // metadata required for asm secret resource validation asmSecretName := "myASMSecret" - asmSecretValueFrom := "asm/mySecret" + asmSecretValueFrom := "arn:aws:secretsmanager:region:account-id:secret:" + asmSecretName asmSecretRetrievedValue := "myASMSecretValue" asmSecretRegion := "us-west-2" asmSecretKey := asmSecretValueFrom + "_" + asmSecretRegion @@ -2700,7 +2700,7 @@ func TestTaskSecretsEnvironmentVariables(t *testing.T) { }).Return(ssmClientOutput, nil).Times(1) mockASMClient.EXPECT().GetSecretValue(gomock.Any()).Do(func(in *secretsmanager.GetSecretValueInput) { - assert.Equal(t, aws.StringValue(in.SecretId), asmSecretValueFrom) + assert.Equal(t, asmSecretValueFrom, aws.StringValue(in.SecretId)) }).Return(asmClientOutput, nil).Times(1) require.NoError(t, ssmSecretRes.Create()) diff --git a/agent/engine/engine_unix_integ_test.go b/agent/engine/engine_unix_integ_test.go index 8bfb63ebf01..3670ce13ccc 100644 --- a/agent/engine/engine_unix_integ_test.go +++ b/agent/engine/engine_unix_integ_test.go @@ -998,7 +998,7 @@ func TestDockerCfgAuth(t *testing.T) { return nil } data, err := ioutil.ReadFile(path) - t.Log("Reading file:%s", path) + t.Logf("Reading file:%s", path) if err != nil { return err } diff --git a/agent/taskresource/asmsecret/asmsecret.go b/agent/taskresource/asmsecret/asmsecret.go index 02992c85aa8..0e0176bc382 100644 --- a/agent/taskresource/asmsecret/asmsecret.go +++ b/agent/taskresource/asmsecret/asmsecret.go @@ -30,11 +30,17 @@ import ( "github.com/aws/amazon-ecs-agent/agent/credentials" "github.com/aws/amazon-ecs-agent/agent/taskresource" resourcestatus "github.com/aws/amazon-ecs-agent/agent/taskresource/status" + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/arn" + "github.com/aws/aws-sdk-go/service/secretsmanager" ) const ( // ResourceName is the name of the asmsecret resource - ResourceName = "asmsecret" + ResourceName = "asmsecret" + arnDelimiter = ":" + asmARNResourceFormat = "secret:{secretID}" + asmARNResourceWithParametersFormat = "secret:secretID:jsonKey:versionStage:versionID" ) // ASMSecretResource represents secrets as a task resource. @@ -294,8 +300,19 @@ func (secret *ASMSecretResource) retrieveASMSecretValue(apiSecret apicontainer.S asmClient := secret.asmClientCreator.NewASMClient(apiSecret.Region, iamCredentials) seelog.Infof("ASM secret resource: retrieving resource for secret %v in region %s for task: [%s]", apiSecret.ValueFrom, apiSecret.Region, secret.taskARN) - //for asm secret, ValueFrom can be arn or name - secretValue, err := asm.GetSecretFromASM(apiSecret.ValueFrom, asmClient) + input, jsonKey, err := getASMParametersFromInput(apiSecret.ValueFrom) + if err != nil { + errorEvents <- fmt.Errorf("trying to retrieve secret with value %s resulted in error: %v", apiSecret.ValueFrom, err) + return + } + + if input.SecretId == nil { + errorEvents <- fmt.Errorf("could not find a secretsmanager secretID from value %s", apiSecret.ValueFrom) + return + + } + + secretValue, err := asm.GetSecretFromASMWithInput(input, asmClient, jsonKey) if err != nil { errorEvents <- fmt.Errorf("fetching secret data from AWS Secrets Manager in region %s: %v", apiSecret.Region, err) return @@ -309,6 +326,65 @@ func (secret *ASMSecretResource) retrieveASMSecretValue(apiSecret apicontainer.S secret.secretData[secretKey] = secretValue } +func pointerOrNil(in string) *string { + if in == "" { + return nil + } + + return aws.String(in) +} + +// Agent follows what Cloudformation does here with using Dynamic References to specify Template Values +// in the format secret-id:json-key:version-stage:version-id +// the input will always be a full ARN for ASM +func getASMParametersFromInput(valueFrom string) (input *secretsmanager.GetSecretValueInput, jsonKey string, err error) { + arnObj, err := arn.Parse(valueFrom) + if err != nil { + seelog.Warnf("Unable to parse ARN %s when trying to retrieve ASM secret", valueFrom) + return nil, "", err + } + + input = &secretsmanager.GetSecretValueInput{} + + paramValues := strings.Split(arnObj.Resource, arnDelimiter) // arnObj.Resource looks like secret:secretID:... + if len(paramValues) == len(strings.Split(asmARNResourceFormat, arnDelimiter)) { + input.SecretId = &valueFrom + return input, "", nil + } + if len(paramValues) != len(strings.Split(asmARNResourceWithParametersFormat, arnDelimiter)) { + // can't tell what input this is, throw some error + err = errors.New("unexpected ARN format with parameters when trying to retrieve ASM secret") + return nil, "", err + } + + input.SecretId = pointerOrNil(reconstructASMARN(arnObj)) + jsonKey = paramValues[2] + input.VersionStage = pointerOrNil(paramValues[3]) + input.VersionId = pointerOrNil(paramValues[4]) + + return input, jsonKey, nil +} + +// this method is to reconstruct an ASM ARN that has the enhancement parameters +// attached to it. in order to call secretsmanager:GetSecretValue, the entire ARN +// (including the 6 character special identifier tacked on by ASM) is required or +// just the secret name itself is required. +func reconstructASMARN(arnARN arn.ARN) string { + // arn resource should look like secret:secretID:jsonKey:versionStage:versionID + secretIDAndParams := strings.Split(arnARN.Resource, arnDelimiter) + // reconstruct the secret id without the parameters + secretID := fmt.Sprintf("%s%s%s", secretIDAndParams[0], arnDelimiter, secretIDAndParams[1]) + secretIDARN := arn.ARN{ + Partition: arnARN.Partition, + Service: arnARN.Service, + Region: arnARN.Region, + AccountID: arnARN.AccountID, + Resource: secretID, + }.String() + + return secretIDARN +} + // getRequiredSecrets returns the requiredSecrets field of asmsecret task resource func (secret *ASMSecretResource) getRequiredSecrets() map[string]apicontainer.Secret { secret.lock.RLock() diff --git a/agent/taskresource/asmsecret/asmsecret_test.go b/agent/taskresource/asmsecret/asmsecret_test.go index 196343ffbaf..2454612d431 100644 --- a/agent/taskresource/asmsecret/asmsecret_test.go +++ b/agent/taskresource/asmsecret/asmsecret_test.go @@ -43,27 +43,26 @@ const ( region2 = "us-east-1" secretName1 = "db_username_1" secretName2 = "db_username_2" - valueFrom1 = "secret-name" - secretKeyWest1 = "secret-name_us-west-2" - secretKeyEast1 = "secret-name_us-east-1" + secretID = "secretID" + valueFrom1 = "arn:aws:secretsmanager:region:account-id:secret:" + secretID + regionKeyWest = "us-west-2" + regionKeyEast = "us-east-1" + secretCacheJoinChar = "_" secretValue = "secret-value" taskARN = "task1" + valueFromParams = valueFrom1 + ":json-key:version-stage:version-id" + valueFromWrongFormat = valueFrom1 + ":wrong:format" + secretValueJson = "{\"json-key\": \"" + secretValue + "\",\"some-other-key\": \"secret2\"}" ) +var secretKeyWest1 = fmt.Sprintf("%s%s%s", valueFrom1, secretCacheJoinChar, regionKeyWest) +var secretKeyEast1 = fmt.Sprintf("%s%s%s", valueFrom1, secretCacheJoinChar, regionKeyEast) +var secretKeyParams = fmt.Sprintf("%s%s%s", valueFromParams, secretCacheJoinChar, regionKeyWest) + func TestCreateWithMultipleASMCall(t *testing.T) { requiredSecretData := map[string]apicontainer.Secret{ - secretKeyWest1: { - Name: secretName1, - ValueFrom: valueFrom1, - Region: region1, - Provider: "asm", - }, - secretKeyEast1: { - Name: secretName2, - ValueFrom: valueFrom1, - Region: region2, - Provider: "asm", - }, + secretKeyWest1: sampleSecret(secretName1, valueFrom1, region1), + secretKeyEast1: sampleSecret(secretName2, valueFrom1, region2), } ctrl := gomock.NewController(t) @@ -86,7 +85,7 @@ func TestCreateWithMultipleASMCall(t *testing.T) { asmClientCreator.EXPECT().NewASMClient(region1, iamRoleCreds).Return(mockASMClient) asmClientCreator.EXPECT().NewASMClient(region2, iamRoleCreds).Return(mockASMClient) mockASMClient.EXPECT().GetSecretValue(gomock.Any()).Do(func(in *secretsmanager.GetSecretValueInput) { - assert.Equal(t, aws.StringValue(in.SecretId), valueFrom1) + assert.Equal(t, valueFrom1, aws.StringValue(in.SecretId)) }).Return(asmSecretValue, nil).Times(2) asmRes := &ASMSecretResource{ @@ -109,18 +108,8 @@ func TestCreateWithMultipleASMCall(t *testing.T) { func TestCreateReturnMultipleErrors(t *testing.T) { requiredSecretData := map[string]apicontainer.Secret{ - secretKeyWest1: { - Name: secretName1, - ValueFrom: valueFrom1, - Region: region1, - Provider: "asm", - }, - secretKeyEast1: { - Name: secretName2, - ValueFrom: valueFrom1, - Region: region2, - Provider: "asm", - }, + secretKeyWest1: sampleSecret(secretName1, valueFrom1, region1), + secretKeyEast1: sampleSecret(secretName2, valueFrom1, region2), } ctrl := gomock.NewController(t) @@ -141,7 +130,7 @@ func TestCreateReturnMultipleErrors(t *testing.T) { asmClientCreator.EXPECT().NewASMClient(region1, iamRoleCreds).Return(mockASMClient) asmClientCreator.EXPECT().NewASMClient(region2, iamRoleCreds).Return(mockASMClient) mockASMClient.EXPECT().GetSecretValue(gomock.Any()).Do(func(in *secretsmanager.GetSecretValueInput) { - assert.Equal(t, aws.StringValue(in.SecretId), valueFrom1) + assert.Equal(t, valueFrom1, aws.StringValue(in.SecretId)) }).Return(asmSecretValue, errors.New("error response")).Times(2) asmRes := &ASMSecretResource{ @@ -160,12 +149,7 @@ func TestCreateReturnMultipleErrors(t *testing.T) { func TestCreateReturnError(t *testing.T) { requiredSecretData := map[string]apicontainer.Secret{ - secretKeyWest1: { - Name: secretName1, - ValueFrom: valueFrom1, - Region: region1, - Provider: "asm", - }, + secretKeyWest1: sampleSecret(secretName1, valueFrom1, region1), } ctrl := gomock.NewController(t) @@ -186,7 +170,7 @@ func TestCreateReturnError(t *testing.T) { credentialsManager.EXPECT().GetTaskCredentials(executionCredentialsID).Return(creds, true), asmClientCreator.EXPECT().NewASMClient(region1, iamRoleCreds).Return(mockASMClient), mockASMClient.EXPECT().GetSecretValue(gomock.Any()).Do(func(in *secretsmanager.GetSecretValueInput) { - assert.Equal(t, aws.StringValue(in.SecretId), valueFrom1) + assert.Equal(t, valueFrom1, aws.StringValue(in.SecretId)) }).Return(asmSecretValue, errors.New("error response")), ) asmRes := &ASMSecretResource{ @@ -203,12 +187,7 @@ func TestCreateReturnError(t *testing.T) { func TestMarshalUnmarshalJSON(t *testing.T) { requiredSecretData := map[string]apicontainer.Secret{ - secretKeyWest1: { - Name: secretName1, - ValueFrom: valueFrom1, - Region: region1, - Provider: "asm", - }, + secretKeyWest1: sampleSecret(secretName1, valueFrom1, region1), } asmResIn := &ASMSecretResource{ @@ -268,3 +247,85 @@ func TestClearASMSecretValue(t *testing.T) { asmRes.clearASMSecretValue() assert.Equal(t, 0, len(asmRes.secretData)) } + +func TestCreateWithASMParametersWrongFormat(t *testing.T) { + requiredSecretData := map[string]apicontainer.Secret{ + secretKeyWest1: sampleSecret(secretName1, valueFromWrongFormat, region1), + } + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + credentialsManager := mock_credentials.NewMockManager(ctrl) + asmClientCreator := mock_factory.NewMockClientCreator(ctrl) + mockASMClient := mock_secretsmanageriface.NewMockSecretsManagerAPI(ctrl) + + iamRoleCreds := credentials.IAMRoleCredentials{} + creds := credentials.TaskIAMRoleCredentials{ + IAMRoleCredentials: iamRoleCreds, + } + + credentialsManager.EXPECT().GetTaskCredentials(executionCredentialsID).Return(creds, true) + asmClientCreator.EXPECT().NewASMClient(region1, iamRoleCreds).Return(mockASMClient) + + asmRes := &ASMSecretResource{ + executionCredentialsID: executionCredentialsID, + requiredSecrets: requiredSecretData, + credentialsManager: credentialsManager, + asmClientCreator: asmClientCreator, + } + + assert.Error(t, asmRes.Create()) + expectedError := fmt.Sprintf("trying to retrieve secret with value %s resulted in error: "+ + "unexpected ARN format with parameters when trying to retrieve ASM secret", valueFromWrongFormat) + assert.Contains(t, asmRes.GetTerminalReason(), expectedError) +} + +func TestCreateWithASMParametersJSONKeySpecified(t *testing.T) { + requiredSecretData := map[string]apicontainer.Secret{ + secretKeyParams: sampleSecret(secretName1, valueFromParams, region1), + } + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + credentialsManager := mock_credentials.NewMockManager(ctrl) + asmClientCreator := mock_factory.NewMockClientCreator(ctrl) + mockASMClient := mock_secretsmanageriface.NewMockSecretsManagerAPI(ctrl) + + iamRoleCreds := credentials.IAMRoleCredentials{} + creds := credentials.TaskIAMRoleCredentials{ + IAMRoleCredentials: iamRoleCreds, + } + + asmSecretValue := &secretsmanager.GetSecretValueOutput{ + SecretString: aws.String(secretValueJson), + } + + credentialsManager.EXPECT().GetTaskCredentials(executionCredentialsID).Return(creds, true) + asmClientCreator.EXPECT().NewASMClient(region1, iamRoleCreds).Return(mockASMClient) + mockASMClient.EXPECT().GetSecretValue(gomock.Any()).Do(func(in *secretsmanager.GetSecretValueInput) { + assert.Equal(t, valueFrom1, aws.StringValue(in.SecretId)) + }).Return(asmSecretValue, nil) + + asmRes := &ASMSecretResource{ + executionCredentialsID: executionCredentialsID, + requiredSecrets: requiredSecretData, + credentialsManager: credentialsManager, + asmClientCreator: asmClientCreator, + } + require.NoError(t, asmRes.Create()) + + value, ok := asmRes.GetCachedSecretValue(secretKeyParams) + require.True(t, ok) + assert.Equal(t, secretValue, value) +} + +func sampleSecret(secretName string, valueFrom string, region string) apicontainer.Secret { + return apicontainer.Secret{ + Name: secretName, + ValueFrom: valueFrom, + Region: region, + Provider: "asm", + } +}