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

rgw: Give useful errors when policies fail to parse #49395

Merged
merged 5 commits into from
Dec 14, 2022
Merged

rgw: Give useful errors when policies fail to parse #49395

merged 5 commits into from
Dec 14, 2022

Conversation

adamemerson
Copy link
Contributor

@adamemerson adamemerson commented Dec 13, 2022 •
edited

This gives useful error messages when policies fail to parse and adds an option that (when true) will reject policies with invalid principals rather than logging them and moving on.

We can now get error messages like:

$ rgw-policy-check -t foo test*.json      
test1.json: At offset 467: `2022-10-17` is not a valid version. Valid versions are `2008-10-17` and `2012-10-17`.
test2.json: At offset 345: Policy owned by tenant `foo` cannot grant access to resource owned by tenant `bar`.
test3.json: At offset 158: `s3:GetObjectVersio` is not a valid action.
test4.json: At offset 336: `arn:aws:s3:::foo:ourdata/*` is not a valid ARN. Resource ARNs should have a format like `arn:aws:s3::tenant:resource' or `arn:aws:s3:::resource`.

Contribution Guidelines

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@adamemerson
rgw: Minor cleanups in rgw_iam_policy.cc
2887155 
Unused using, confusing indentation, bracing.

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
@adamemerson adamemerson requested a review from a team as a code owner December 13, 2022 04:22
@adamemerson adamemerson force-pushed the wip-policy-useful-error-messages branch from 32f01dd to f0a38b1 Compare December 13, 2022 15:51
@mattbenjamin
Copy link
Contributor

oh, this is amazing; @adamemerson is this potentially backportable to our test fix branch?

adamemerson and others added 4 commits December 13, 2022 16:34
@adamemerson
rgw: Provide useful error messages from policy parser
613e010 
It would be much nicer to give people an idea why their policies are
failing rather than just telling them where they're failing.

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
@adamemerson
rgw: Add rgw_policy_reject_invalid_principals and messages
0dd2ef4 
Reject policies with invalid principals by default and provide more
useful error messages while doing so.

(Log them but do *not* reject the policy if it's set to false.)

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
@adamemerson
rgw: Put useful policy error messages in error document
0b0fd44 
This affects the various create/put operations that take a policy document.

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
@mdw-at-linuxbox @adamemerson
rgw: added rgw-policy-check
2886431 
rgw-policy-check - a program to do syntax checking on bucket policy.
This program just reads the policy into memory, so it is not
checking anything except syntax.

Signed-off-by: Marcus Watts <mwatts@redhat.com>

rgw: Fix return value of `rgw-policy-check`

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

rgw: Use ceph initialization in `rgw-policy-check`

Specifically so we can pull in the options from `ceph.conf` and similar.

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
@adamemerson adamemerson force-pushed the wip-policy-useful-error-messages branch from f0a38b1 to 2886431 Compare December 13, 2022 21:49
@github-actions github-actions bot added the tests label Dec 13, 2022
mattbenjamin
mattbenjamin approved these changes Dec 13, 2022
View reviewed changes
Copy link
Contributor

@mattbenjamin mattbenjamin left a comment

Choose a reason for hiding this comment

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

the policy checker, together with human-readable error reporting from the parser, make the foundation for an IAM semantic error checker. I'm really glad we were able to do this

@adamemerson
Copy link
Contributor Author

https://pulpito.ceph.com/aemerson-2022-12-13_23:46:45-rgw-wip-policy-useful-error-messages-distro-default-smithi/

Looks like none of the test failures are policy related, just multisite, tempest, and AMQP.

@adamemerson adamemerson merged commit 1a918d1 into ceph:main Dec 14, 2022
@adamemerson adamemerson deleted the wip-policy-useful-error-messages branch January 6, 2023 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattbenjamin mattbenjamin mattbenjamin approved these changes

@mdw-at-linuxbox mdw-at-linuxbox Awaiting requested review from mdw-at-linuxbox

Assignees
No one assigned
Labels
build/ops common documentation rgw tests
Projects
None yet
Milestone
No milestone
3 participants
@adamemerson @mattbenjamin @mdw-at-linuxbox