Dont decode S3 Bucket policies #57
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
March 4, 2019 17:14
I'm not quite sure how to ensure the errors get raised appropriately
They're already decoded, as opposed to IAM policies
mtibben
reviewed
Mar 4, 2019
iamy/policy_test.go
Outdated
| @@ -126,3 +126,10 @@ Actual: %#v`, nt.description, nt.input, nt.expected, result) | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| func TestNewPolicyDocumentFromJson(t *testing.T) { | |||
| _, err := NewPolicyDocumentFromJson("{\"Version\":\"2012-10-17\",\"Id\":\"AllowPublicRead\",\"Statement\":[{\"Sid\":\"PublicReadBucketObjects\",\"Effect\":\"Allow\",\"Principal\":\"*\",\"Action\":\"s3:GetObject\",\"Resource\":\"arn:aws:s3:::example.com/*\",\"Condition\":{\"StringEquals\":{\"aws:Referer\":\"%zz\"}}}]}") | |||
There was a problem hiding this comment.
Use backticks for the string here for readability?
mtibben
reviewed
Mar 4, 2019
iamy/aws.go
Outdated
| @@ -120,8 +120,11 @@ func (a *AwsFetcher) fetchIamData() error { | |||
| }), | |||
| }, | |||
| func(resp *iam.GetAccountAuthorizationDetailsOutput, lastPage bool) bool { | |||
| // There's a bug here. This doesn't set the variable outside this function scope | |||
| // So IAMY just swallows any errors returned from populateIamData() | |||
There was a problem hiding this comment.
hmm I'm not sure I understand... can you fix or produce a testcase?
There was a problem hiding this comment.
I tried to fix it, but I'm not entirely sure how. I'll pull this out into a seperate WIP PR and see if I can write a failing test.
mtibben
reviewed
Mar 4, 2019
iamy/policy.go
Outdated
| log.Printf("Error unmarshalling JSON %s %s", err, jsonString) | ||
| return nil, err | ||
| } | ||
| //log.Printf("Doc %s\nJSON: %s", doc, jsonString) |
mtibben
reviewed
Mar 4, 2019
iamy/policy.go
Outdated
| return nil, err | ||
| } | ||
| //log.Printf("Doc %s\nJSON: %s", doc, jsonString) |
added 3 commits
March 5, 2019 10:55
Remove commented out code
Will fix in another PR
|
Thanks for the review @mtibben I've addressed your feedback and given this a whirl in one of our accounts. Let me know your thoughts |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes an issue that was introduced in #13.
I can't really figure out why we would want to decode the Policy, it should never be encoded in the first place as far as I can tell.S3 bucket policies are not URL encoded, while other policies are.
If the bucket policy contains invalid encoding, such as
%followed by non-hexadecimal characters, it creates an error such as: