Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ecs.Secret.fromSecretsManager generates secret name instead of secret ARN #10519

Closed
kastork opened this issue Sep 24, 2020 · 4 comments · Fixed by #11042
Closed

ecs.Secret.fromSecretsManager generates secret name instead of secret ARN #10519

kastork opened this issue Sep 24, 2020 · 4 comments · Fixed by #11042
Assignees
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container @aws-cdk/aws-secretsmanager Related to AWS Secrets Manager bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@kastork
Copy link

kastork commented Sep 24, 2020

import * as secretsMgr from "@aws-cdk/aws-secretsmanager";

const mySecret = secretsMgr.Secret.fromSecretName(this,"mysecret",`${appSecretBase}/MySecret`);
const mySecretEnv = ecs.Secret.fromSecretsManager(mySecret)
mySecret.grantRead(fargateTaskExecutionRole);
mySecretEnv.grantRead(fargateTaskExecutionRole);

// ...

const container = fargateTaskDefinition.addContainer("appserver", {

// ...

      secrets: {
        MY_SECRET: mySecretEnv,

// ...


 

What did you expect to happen?

Expect the fargate task to launch

What actually happened?

Fargate task failed to launch with the following error:

Fetching secret data from SSM Parameter Store in us-west-2: AccessDeniedException: User: arn:aws:sts::---:assumed-
role/...taskDefExecutionRole9295-1N6FDVCBGB6RK/67766c7d-d908-4b49-b196--:parameter/my path/MySecret 
status code: 400, request id: 2ed367bc-35a9-42ad-b807-e1b2a70ea8f1

Notice, the error is complaining about SSM (parameter store), not Secrets Manager.

Environment

  • CLI Version : 1.64.0
  • Framework Version: 1.64.0
  • Node.js Version: 12.18.3
  • OS : macos 10.15.6
  • Language (Version): typescript 3.9.7

Other

Looking at the generated secrets configuration, I see

# ...
            "Secrets": [
              {
                "Name": "MY_SECRET",
                "ValueFrom": "/my path/MySecret"
              },

This is a simple parameter name, NOT an ARN to a secret in secret manager, which is what the CloudFormation docs indicate should be here.


This is 🐛 Bug Report

@kastork kastork added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 24, 2020
@SomayaB SomayaB added @aws-cdk/aws-ecs Related to Amazon Elastic Container @aws-cdk/aws-secretsmanager Related to AWS Secrets Manager labels Sep 25, 2020
@iwt-janbrauer
Copy link

I have the exact same problem. I (falsely) understood that providing Secret.fromSecretName would do an actual look up of the secret by name and cache it's ARN within the CDK's context. This isn't the case. Can somebody clarify this? @njlynch maybe.

As a workaround we use:

const secret = new custom.AwsCustomResource(this, "GetSecret", {
            onUpdate: {
                service: "SecretsManager",
                action: "listSecrets",
                parameters: {
                    Filters: [
                        {
                            Key: "name",
                            Values: "/my/cool/secret"
                        }],
                    SortOrder: "asc"
                },
                physicalResourceId: custom.PhysicalResourceId.of('MyCoolSecret')
            },
            policy: custom.AwsCustomResourcePolicy.fromSdkCalls({resources: custom.AwsCustomResourcePolicy.ANY_RESOURCE}),
        });

const secretARN = secret.getResponseField("SecretList.0.ARN")

But that is cumbersome. Especially when the secret has been encrypted with customer managed KMS key. You then need to make sure to give the proper permissions to the task execution role:

taskDefinition.addToExecutionRolePolicy(new iam.PolicyStatement({actions: ['kms:Decrypt', 'kms:GenerateDataKey'], resources: ['*']}));

ecs.Secret.fromSecretsManager(
      secretsManager.Secret.fromSecretArn(
                            this,
                            'ECSSecret',
                            secretARN
                        )

@YuryShchanouskiTR
Copy link

YuryShchanouskiTR commented Oct 15, 2020

CDK v1.67.0
Next code did not return ARN from secret name:

const secret = secrets.Secret.fromSecretName(this, "GitHubSecret", "some-github-secret");
const branch = "master";
const codeRepo = new sagemaker.CfnCodeRepository(this, "MedscrnInfraGitHub", {
    gitConfig: {
        repositoryUrl: `https://github.com/somerepo.git`,
        branch: branch,
        secretArn: secret.secretArn
    },
    codeRepositoryName: `some-code-repository`,
});

Error message during cdk deploy process:

1/7 | 1:30:40 PM | CREATE_FAILED | AWS::SageMaker::CodeRepository | SomeGitHub 1 validation error detected: Value 'some-github-secret' at 'gitConfig.secretArn' failed to satisfy constraint: Member must satisfy regular expression pattern: arn:aws[a-z-]:secretsmanager:[a-z0-9-]:[0-9]{12}:secret:.* (Service: AmazonSageMaker; Status Code: 400; Error Code: ValidationException; Request ID: 768cfe92-503f-45f7-a115-0292f267f041)

@njlynch njlynch self-assigned this Oct 15, 2020
@njlynch
Copy link
Contributor

njlynch commented Oct 15, 2020

Darn, most CloudFormation usages of secrets are perfectly happy to take either the secret name or the full ARN; this appears to be one of the exceptions where only the ARN is acceptable.

I wonder if it must be the full ARN, or if the ARN without the suffix is sufficient. If the latter, this might be a workaround:

// Old way
// const mySecret = secretsMgr.Secret.fromSecretName(this,"mysecret",`${appSecretBase}/MySecret`);
// const mySecretEnv = ecs.Secret.fromSecretsManager(mySecret)

// New way
const mySecretArn = Stack.of(this).formatArn({
  service: 'secretsmanager',
  resource: 'secret',
  resourceName: `${appSecretBase}/MySecret`,
  sep: ':',
});
const mySecret = secretsMgr.Secret.fromSecretArn(this, 'mysecret', mySecretArn);
const mySecretEnv = ecs.Secret.fromSecretsManager(mySecret);

The interesting bit is that for many other service integrations, the above absolutely won't work; if you don't have the full ARN with the Secrets Manager-generated suffix, you must just provide the secret name, and not the incomplete ARN. That's why Secret.fromSecretName(...).secretArn returns what it does today.

If that works, then the real solution here is probably going to have to be some way for the Secret to indicate that the secetArn is the secretName, and offering a convenience method to return the incomplete ARN like above.

Can someone confirm if the above work-around works for them?

@njlynch njlynch added effort/medium Medium work item – several days of effort p1 and removed needs-triage This issue or PR still needs to be triaged. labels Oct 20, 2020
njlynch added a commit that referenced this issue Oct 22, 2020
The ability to import and reference a Secret purely by the secret name was
introduced in #10309. One of the original requests was modelled after the
integration with CodeBuild, where either the secret name or the full ARN
-- including the SecretsManager-provided suffix -- were accepted, but not a
"partial" ARN without the suffix. To ease integrations with other services
in this case, the `secretArn` was defined as returning the `secretName` for
these secrets imported by name.

However, other services -- like ECS -- require that an ARN format is provided,
even as a partial ARN. This introduces a dual behavior where sometimes the
secretName works and partial ARN fails, and other times the partial ARN works
and the secretName fails.

This change introduces an option to the `fromSecretName` factory to control this
behavior, so users can set up the secret properly for the service they are
integrating with.

*Disclaimer:* - I don't *love* this, and am very open to feedback on alternative
approaches that would also be backwards compatible.

Related changes -- I improved the suffix-strippiung logic of `parseSecretName`
to only strip a suffix if it's exactly 6 characters long, as all SecretsManager
suffixes are 6 characters. This prevents accidentally stripping the last word
off of a hyphenated secret name like 'github-token'.

fixes #10519
njlynch added a commit that referenced this issue Oct 29, 2020
The ability to import and reference a Secret purely by the secret name was
introduced in #10309. One of the original requests was modelled after the
integration with CodeBuild, where either the secret name or the full ARN
-- including the SecretsManager-provided suffix -- were accepted, but not a
"partial" ARN without the suffix. To ease integrations with other services
in this case, the `secretArn` was defined as returning the `secretName` for
these secrets imported by name.

However, other services -- like ECS -- require that an ARN format is provided,
even as a partial ARN. This introduces a dual behavior where sometimes the
secretName works and partial ARN fails, and other times the partial ARN works
and the secretName fails.

This change deprecates `fromSecretName` and introduces a new, better-behaved
`fromSecretNameV2` that sets the ARN to a "partial" ARN without the Secrets
Manager suffix. It also introduces a `secretFullArn` that is an optional version
of `secretArn` that will be undefined for secrets imported by name.

Related changes
* I improved the suffix-strippiung logic of `parseSecretName` to only strip a
  suffix if it's exactly 6 characters long, as all SecretsManager
  suffixes are 6 characters. This prevents accidentally stripping the last word
  off of a hyphenated secret name like 'github-token'.
* Updated the CodeBuild integration and added CodeBuild tests.

fixes #10519
njlynch added a commit that referenced this issue Oct 29, 2020
The ability to import and reference a Secret purely by the secret name was
introduced in #10309. One of the original requests was modelled after the
integration with CodeBuild, where either the secret name or the full ARN
-- including the SecretsManager-provided suffix -- were accepted, but not a
"partial" ARN without the suffix. To ease integrations with other services
in this case, the `secretArn` was defined as returning the `secretName` for
these secrets imported by name.

However, other services -- like ECS -- require that an ARN format is provided,
even as a partial ARN. This introduces a dual behavior where sometimes the
secretName works and partial ARN fails, and other times the partial ARN works
and the secretName fails.

This change deprecates `fromSecretName` and introduces a new, better-behaved
`fromSecretNameV2` that sets the ARN to a "partial" ARN without the Secrets
Manager suffix. It also introduces a `secretFullArn` that is an optional version
of `secretArn` that will be undefined for secrets imported by name.

Related changes
* I improved the suffix-strippiung logic of `parseSecretName` to only strip a
  suffix if it's exactly 6 characters long, as all SecretsManager
  suffixes are 6 characters. This prevents accidentally stripping the last word
  off of a hyphenated secret name like 'github-token'.
* Updated the CodeBuild integration and added CodeBuild tests.

fixes #10519
@mergify mergify bot closed this as completed in #11042 Oct 30, 2020
mergify bot pushed a commit that referenced this issue Oct 30, 2020
)

The ability to import and reference a Secret purely by the secret name was
introduced in #10309. One of the original requests was modelled after the
integration with CodeBuild, where either the secret name or the full ARN
-- including the SecretsManager-provided suffix -- were accepted, but not a
"partial" ARN without the suffix. To ease integrations with other services
in this case, the `secretArn` was defined as returning the `secretName` for
these secrets imported by name.

However, other services -- like ECS -- require that an ARN format is provided,
even as a partial ARN. This introduces a dual behavior where sometimes the
secretName works and partial ARN fails, and other times the partial ARN works
and the secretName fails.

This change deprecates `fromSecretName` and introduces a new, better-behaved
`fromSecretNameV2` that sets the ARN to a "partial" ARN without the Secrets
Manager suffix. It also introduces a `secretFullArn` that is an optional version
of `secretArn` that will be undefined for secrets imported by name.

Related changes
* I improved the suffix-strippiung logic of `parseSecretName` to only strip a
  suffix if it's exactly 6 characters long, as all SecretsManager
  suffixes are 6 characters. This prevents accidentally stripping the last word
  off of a hyphenated secret name like 'github-token'.
* Updated the CodeBuild integration and added CodeBuild tests.

fixes #10519


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container @aws-cdk/aws-secretsmanager Related to AWS Secrets Manager bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants