fix(iam): policy statement tries to validate tokens #13493
Conversation
|
|
github-actions
bot
added
the
@aws-cdk/aws-iam
hollanddd
force-pushed
the
iam-validate-unsresolved-token
branch
from
f5e5316
to
f1d24f0
Compare
| @@ -64,6 +64,10 @@ export class PolicyStatement { | |||
| constructor(props: PolicyStatementProps = {}) { | |||
| // Validate actions | |||
| for (const action of [...props.actions || [], ...props.notActions || []]) { | |||
| if (cdk.Token.isUnresolved(action)) { | |||
| throw new Error('Cannot add \'Action\' that contains an unresolved token'); | |||
There was a problem hiding this comment.
I think what we want to do is only validate if we detect the value is NOT unresolved.
That way, we'll validate when we can and won't complain when we can't.
Add a test to check that we can pass in an unresolved value in a position we previously couldn't.
There was a problem hiding this comment.
Thank you. I think I have addressed your feedback. As an aside I'm newish to major contributions and wasn't sure how to git the changes up with out forcing (with lease) and that makes me feel bad so.... Any tips in that area would be greatly appreciated.
There was a problem hiding this comment.
:) yeah, I get that.
Here's the knowledge that will set you free: in our repository, you are allowed to not care about having a clean commit history on your PRs!
Because we will always squash-merge every PR into the main branch (collapsing it all into one commit, with the contents of the PR as the commit title & body), it doesn't actually matter whether your branch' commit history is clean or not. All that is going to go away anyway.
So I add commits named "oops" and "review" and "fix test" with wild abandon, and merge (without rebase!) from "master", and it's all good, as long as attention is paid to the PR body.
Two additional notes:
- This happens to be true in our repository--might not be true in others, so I'm not sure how much this knowledge transfers to other projects (in case you're interested).
- In fact, for GitHub (not
git) it's better to never force-push any branches, especially if they have review comments on them. It looks like GitHub remembers comments based on a combination of something like (commit hash, line number), and if the commit disappears because of force pushing, comments become orphaned and disappear from a thread.
hollanddd
force-pushed
the
iam-validate-unsresolved-token
branch
from
f1d24f0
to
ea9e665
Compare
hollanddd
force-pushed
the
iam-validate-unsresolved-token
branch
from
ea9e665
to
266191a
Compare
|
Left as is it's possible to send a malformed |
hollanddd
force-pushed
the
iam-validate-unsresolved-token
branch
from
266191a
to
dcc936e
Compare
| : action; | ||
|
|
||
| if (!/^(\*|[a-zA-Z0-9-]+:[a-zA-Z0-9*]+)$/.test(subj)) { | ||
| throw new Error(`Action '${subj}' is invalid. An action string consists of a service namespace, a colon, and the name of an action. Action names can include wildcards.`); |
There was a problem hiding this comment.
Do we care if the resolved string is exposed in the error message? Should this be action instead of subj?
There was a problem hiding this comment.
We DO care! I like it when error messages tell me the value they saw (vs the value they were expecting). I find it makes it much easier in my head to reason about where that value is coming from.
The point of Tokens is that they CANNOT be validated. But also the worst thing that is going to happen is that the deployment itself is going to fail. So look at it this way:
|
| throw new Error(`Action '${action}' is invalid. An action string consists of a service namespace, a colon, and the name of an action. Action names can include wildcards.`); | ||
|
|
||
| let subj = cdk.Token.isUnresolved(action) | ||
| ? cdk.Tokenization.resolve(action, { scope: this, resolver: new cdk.DefaultTokenResolver(new cdk.StringConcat()) }) |
There was a problem hiding this comment.
I wouldn't resolve it though. The problem is that the value may be something like { Ref: SomeParameter }, which first of all is not a string, and even if it was would not correctly pass validation.
I was thinking more something like:
if (!Token.isUnresolved(action) && !/^(\*|[a-zA-Z0-9-]+:[a-zA-Z0-9*]+)$/.test(subj)) {
throw new Error(`OMG action '${action}' is weeeiiirrddd`);
}
hollanddd
force-pushed
the
iam-validate-unsresolved-token
branch
from
dcc936e
to
fbb9ae3
Compare
rix0rrr
changed the title
|
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). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
Looking for guidance on error messaging and/or docs to update
Fixes #13479
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license