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

feat(secretsmanager): import secrets by name #10309

Merged
merged 4 commits into from
Sep 14, 2020
Merged

Conversation

njlynch
Copy link
Contributor

@njlynch njlynch commented Sep 11, 2020

Adds the ability to import secrets by name, including without the SecretsManager
assigned suffix. As long as a secret with the same name has been created in each
region with the same name, this allows for the same fromSecretName usage in
stacks across regions.

Oddly enough, most CloudFormation templates that take references to secrets
accept either the full-form ARN, including the suffix or just the base secret
name (not in ARN format). The one place where a full ARN format is needed is in
IAM policy statements, where the wildcard is necessary to account for the
suffix.

Tested this manually against an existing secret with a CodeBuild project; per
the CloudFormation docs, this should work equally well with other
SecretsManager-integrated services.

fixes #7444
fixes #7949
fixes #7994


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@njlynch njlynch requested a review from a team September 11, 2020 11:13
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Sep 11, 2020
Adds the ability to import secrets by name, including without the SecretsManager
assigned suffix. As long as a secret with the same name has been created in each
region with the same name, this allows for the same `fromSecretName` usage in
stacks across regions.

Oddly enough, most CloudFormation templates that take references to secrets
accept either the full-form ARN, including the suffix or just the base secret
name (not in ARN format). The one place where a full ARN format is needed is in
IAM policy statements, where the wildcard is necessary to account for the
suffix.

Tested this manually against an existing secret with a CodeBuild project; per
the CloudFormation docs, this should work equally well with other
SecretsManager-integrated services.

fixes #7444
fixes #7949
fixes #7994
@njlynch njlynch self-assigned this Sep 11, 2020
@rix0rrr rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Sep 11, 2020
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved provided small reconsiderations

packages/@aws-cdk/aws-secretsmanager/lib/secret.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-secretsmanager/lib/secret.ts Outdated Show resolved Hide resolved
@njlynch njlynch removed the pr/do-not-merge This PR should not be merged at this time. label Sep 14, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 0ac4aa7
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Sep 14, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@njlynch njlynch merged commit a8e8ed3 into master Sep 14, 2020
@njlynch njlynch deleted the njlynch/secrets-by-name branch September 14, 2020 10:04
njlynch added a commit that referenced this pull request Sep 17, 2020
In #10309, secretName was added to SecretAttributes, but given the ARN is always
required, it's fairly redundant. Removing to reduce public API surface area.

Not a breaking change, as #10309 has not yet been released.
mergify bot pushed a commit that referenced this pull request Sep 17, 2020
In #10309, secretName was added to SecretAttributes, but given the ARN is always
required, it's fairly redundant. Removing to reduce public API surface area.

Not a breaking change, as #10309 has not yet been released.

----

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

rrrix commented Sep 25, 2020

Uhg. This totally broke my production stack.

I'm importing a secret using from another stack by ARN, like this:

    const dbSecret = secretsmanager.Secret.fromSecretArn(this, 'dbSecret', props.dbSecretArn);

I get this error:

/Users/me/project/cdk/node_modules/@aws-cdk/aws-secretsmanager/lib/secret.ts:605
  throw new Error('invalid ARN format; no secret name provided');
        ^
Error: invalid ARN format; no secret name provided
    at parseSecretName (/Users/me/project/cdk/node_modules/@aws-cdk/aws-secretsmanager/lib/secret.ts:605:9)
    at new Import (/Users/me/project/cdk/node_modules/@aws-cdk/aws-secretsmanager/lib/secret.ts:282:36)
    at Function.fromSecretAttributes (/Users/me/project/cdk/node_modules/@aws-cdk/aws-secretsmanager/lib/secret.ts:286:12)
    at Function.fromSecretArn (/Users/me/project/cdk/node_modules/@aws-cdk/aws-secretsmanager/lib/secret.ts:245:19)
    at new APIService (/Users/me/project/cdk/src/services/api.ts:77:44)
    at main (/Users/me/project/cdk/src/index.ts:190:22)

@njlynch What's supposed to happen if the secretArn passed here is a Token (e.g. doesn't exist yet?)

/** Returns the secret name if defined, otherwise attempts to parse it from the ARN. */
export function parseSecretName(construct: IConstruct, secretArn: string, secretName?: string) {
  if (secretName) { return secretName; }
  const resourceName = Stack.of(construct).parseArn(secretArn).resourceName;
  if (resourceName) {
    // Secret resource names are in the format `${secretName}-${SecretsManager suffix}`
    const secretNameFromArn = resourceName.substr(0, resourceName.lastIndexOf('-'));
    if (secretNameFromArn) { return secretNameFromArn; }
  }
  throw new Error('invalid ARN format; no secret name provided');
}
$ cdk --version
1.64.0 (build 9510201)

@Cloudrage
Copy link

Same here since update in 1.64.0 :

invalid ARN format; no secret name provided
Subprocess exited with error 1

njlynch added a commit that referenced this pull request Sep 28, 2020
The feature to support importing secrets by name (#10309) failed to handle
scenarios where the secret ARN is a token, due to parsing the ARN to retrieve
the secret name.

fixes #10520
njlynch added a commit that referenced this pull request Sep 28, 2020
The feature to support importing secrets by name (#10309) failed to handle
scenarios where the secret ARN is a token, due to parsing the ARN to retrieve
the secret name.

fixes #10520
mergify bot pushed a commit that referenced this pull request Sep 28, 2020
The feature to support importing secrets by name (#10309) failed to handle
scenarios where the secret ARN is a token, due to parsing the ARN to retrieve
the secret name.

fixes #10520


----

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

This also breaks our production stack.

@Cloudrage
Copy link

To prevent that you have to set a fix version of CDK.
When a new version came available, update it but test on your dev/stg environments before updating your prd.

I'm using pipelines in most cases and only with a valid/fixed CDK version; in that case you don't have any surprise.
With the actual SM pb, I've seen it in my DEV environment and came back to the previous version (this version did not contain any expected fixed on my side). Or just the SM package version.

njlynch added a commit that referenced this pull request 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'.
njlynch added a commit that referenced this pull request 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 pull request 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 pull request 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 bot pushed a commit that referenced this pull request 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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
7 participants