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

fix(core): Prevent volatile physical name generation #2984

Merged
merged 7 commits into from
Jul 2, 2019

Conversation

RomainMuller
Copy link
Contributor

The PhysicalName generation strategy for cross-account/region use-cases
could generate names that are subject to collisions/sniping when the
account and region were not specified; and those names would also have
changed if a stack went from re-locatable to account/region specific,
even if the target environment would not have changed.

The generator will now throw errors in case the account ID or region is
blank or a Token.


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
    • Design: For significant features, design document added to design folder
  • Title and Description
    • Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

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

The PhysicalName generation strategy for cross-account/region use-cases
could generate names that are subject to collisions/sniping when the
account and region were not specified; and those names would also have
changed if a stack went from re-locatable to account/region specific,
even if the target environment would not have changed.

The generator will now throw errors in case the account ID or region is
blank or a Token.
@RomainMuller RomainMuller requested a review from a team as a code owner June 21, 2019 08:13
@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 21, 2019

collisions/sniping

Good point. Given that, a way to add an app-wide salt to the name generation might also be a good idea.

Right now, if I deploy the same app twice, or happen to have the exact same construct tree in a second app (which is very likely, given that people are likely to call their things 'Stack/Pipeline/Bucket'), the names will collide.

@RomainMuller
Copy link
Contributor Author

@rix0rrr - yeah the app-wide salt is also in my mind but I decided to NOT do it just yet because this is a little more complex (the salt would be a secret...).

W/R/T the collisions - the current state of things for position-independent stacks is collision-vulnerable indeed. The update here makes the account ID and region required part of the name generation, so you couldn't get a generated name for a position-independent stack (and those use-cases typically will involve a determined account and region... because they require cross-account/region workflows).

let region: string | undefined = stack.region;
if (Token.isUnresolved(region)) {
region = undefined;
const region: string = stack.region;
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically stack.region/account will never be undefined but that’s ok

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Can we add a test for this?

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 21, 2019

Yeah I wonder how this would even go wrong.

By the way, we could also make unique hard-name generation a context feature, where we generate a name on-demand and store it it context.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 21, 2019

Salts would only need to be private for buckets, btw. For other resources, since there's no global namespace there's no chance of sniping, we'd only need unique values.

But yes, to protect against sniping we'd need unique AND secret values.

@RomainMuller RomainMuller requested a review from skinny85 as a code owner June 24, 2019 10:30
@RomainMuller RomainMuller merged commit af2680c into master Jul 2, 2019
@RomainMuller RomainMuller deleted the rmuller/physical-names branch July 2, 2019 09:34
eladb pushed a commit that referenced this pull request Jul 2, 2019
Follow up on #2984, which introduced a bunch of new account numbers that we need to whitelist.
Kaixiang-AWS pushed a commit to Kaixiang-AWS/aws-cdk that referenced this pull request Jul 3, 2019
The PhysicalName generation strategy for cross-account/region use-cases
could generate names that are subject to collisions/sniping when the
account and region were not specified; and those names would also have
changed if a stack went from re-locatable to account/region specific,
even if the target environment would not have changed.

The generator will now throw errors in case the account ID or region is
blank or a Token.
Kaixiang-AWS pushed a commit to Kaixiang-AWS/aws-cdk that referenced this pull request Jul 3, 2019
Follow up on aws#2984, which introduced a bunch of new account numbers that we need to whitelist.
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
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
Development

Successfully merging this pull request may close these issues.

4 participants