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.

Sign up for GitHub

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

Jump to bottom

Issue 7 #8

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Issue 7 #8

wants to merge 3 commits into from

Conversation

Canop
Copy link

@Canop Canop commented Dec 8, 2023

Fix parsing of AWS managed policy ARNs, such as "arn:aws:iam::aws:policy/ReadOnlyAccess".

Fix #7

Canop added 3 commits December 8, 2023 08:59
@Canop
fix AWS managed IAM policy not correctly parsed
5939e30 
See johnstonskj#7
@Canop
better fix of issue-7: not creating an invalid account id
1a9f498
@Canop
change fix to ensure parse/to_string round trip
b65b252 
This isn't completely satisfying as is means the AccoundId
will be "aws" but it's probably the best solution with the
current ResourceName and AccountId structures.
@Canop
Copy link
Author

Canop commented Dec 8, 2023

The downside of this implementation is that it allows an AccountId with value "aws", which isn't really, semantically, an account id.

To fix this, if it looks like it's a problem, I'd suggest replacing the Option<AccountId> in ResourceName with an enum whose variants would be None, Account(AccountId) and Aws.

@copumpkin
Copy link

That doesn’t really seem like a downside. AWS doesn’t require account IDs in that field of ARNs so it seems unnecessary for this library’s model of ARNs to have stricter requirements than the underlying system. Another counterexample is the ARN for CloudFront Origin Access Identities, which uses cloudfront in that part of the ARN instead of an account ID.

@nikp
Copy link

nikp commented Jun 10, 2024 •
edited
Loading

Another example is AWS SDK integrations from SFN - they use an ARN format that has a blank account ID: https://docs.aws.amazon.com/step-functions/latest/dg/supported-services-awssdk.html

e.g.: arn:aws:states:::aws-sdk:bedrock:[apiAction]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ResourceName::from_str panics on AWS-managed IAM policy ARNs
3 participants
@Canop @copumpkin @nikp