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

ACL checks are not sufficient for checking access. Access is falsely denied when bucket policy is used. #133

Merged
merged 7 commits into from
Dec 12, 2020
Merged

Conversation

electricsam
Copy link
Contributor

Pull Request Description

Using ACLs to check for object access is not sufficient. The bucket policy can override the ACL. This can be a problem when accessing a public bucket in a different account where ACLs are not configured. In general, AWS recommends using bucket policies over ACLs.

These changes remove the ACL check and simply check if object exists for the given user. If that user cannot read or write to that object, an error will happen at the time of that action.

Acceptance Test

  • Building the code with mvn clean install -Pintegration-tests still works.

Questions

  • Does this pull request break backward compatibility?

    • Yes
    • No
  • Does this pull request require other pull requests to be merged first?

    • Yes, please see #...
    • No
  • Does this require an update of the documentation?

    • Yes, please see [provide details here]
    • No

@carlspring
Copy link
Owner

Hi @electricsam ,

Thank you for your contribution!

Would you mind signing the ICLA, as described in the Legal section of our wiki?

Also, please, feel free to join our chat channel, if you'd like to learn more about the project and/or like to find out what else you could help with.

Kind regards,

Martin

@carlspring
Copy link
Owner

@ptirador , @elerch, @markjschreiber, @pditommaso ,

Could you please review this? :)

@electricsam
Copy link
Contributor Author

electricsam commented Dec 10, 2020 via email

@carlspring
Copy link
Owner

Thanks a lot, @electricsam ! I can confirm I have received it. )

Copy link
Owner

@carlspring carlspring left a comment

Choose a reason for hiding this comment

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

@electricsam ,

Thanks for this pull request! Would you mind applying the cosmetic code style changes I have raised?

Many thanks in advance! :)

Copy link
Contributor

@ptirador ptirador left a comment

Choose a reason for hiding this comment

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

HI @electricsam,

Thanks for this pull request! Would you mind applying the changes that I requested?
Apart from these, LGTM!

CC @carlspring

Co-authored-by: Pablo Tirado <pablotr87@gmail.com>
Copy link
Owner

@carlspring carlspring left a comment

Choose a reason for hiding this comment

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

LGTM! :)

@carlspring
Copy link
Owner

Thank you very much, @electricsam! Congratulations on your first pull request with our project! :)

@carlspring carlspring merged commit 85eaeb2 into carlspring:master Dec 12, 2020
@carlspring carlspring deleted the check-access-bucket-policy branch December 12, 2020 13:08
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.

3 participants