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: sanitize AWS session tags #20

Merged
merged 8 commits into from
Jan 31, 2020
Merged

fix: sanitize AWS session tags #20

merged 8 commits into from
Jan 31, 2020

Conversation

LaurenceGA
Copy link
Contributor

@LaurenceGA LaurenceGA commented Jan 29, 2020

Closes #18

Ensure that alls tags applied to the AWS role session are valid.

For GITHUB_ACTOR, [ and ] must be removed.

For GITHUB_WORKFLOW I've inverted the tag value requirement regex, [\p{L}\p{Z}\p{N}_.:/=+\-@]+, and replaced any characters that don't conform. I've also truncated the value as it has a maximum of 256 chars.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

clareliguori
clareliguori previously approved these changes Jan 29, 2020
@LaurenceGA LaurenceGA marked this pull request as ready for review January 30, 2020 02:54
@LaurenceGA
Copy link
Contributor Author

@clareliguori Thanks for the review!

I've also added another change to sanitise another tag value also.

@LaurenceGA
Copy link
Contributor Author

Do you think _ is the best character for replacement? I picked it sort of arbitrarily. Maybe * would be more conventional and perhaps look nicer?

@mattsb42-aws
Copy link
Member

One thing that worries me here, though I'm not sure if there's a good answer, is collisions.

Back to my original desire to let this provide a useful way to limit what can assume a particular role, there are potentially a lot of values that will sanitize to the same thing. On the other hand, the repo owner controls all of these values, so they can keep them clean if they want to.

Maybe just add something to the readme warning of this?

@LaurenceGA
Copy link
Contributor Author

I've updated the readme with full(?) documentation about assuming a role. Hopefully it's clarifying.

@clareliguori clareliguori self-requested a review January 30, 2020 14:58
@clareliguori clareliguori dismissed their stale review January 30, 2020 14:59

older commit

@mattsb42-aws
Copy link
Member

Other than the points @clareliguori raised, this LGTM. :)

@LaurenceGA
Copy link
Contributor Author

@clareliguori Well spotted, thanks. Updated :)

@clareliguori clareliguori merged commit 4faf8cd into aws-actions:master Jan 31, 2020
@LaurenceGA LaurenceGA deleted the sanitize-session-tags branch January 31, 2020 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid tag when assuming role
3 participants