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: check for valid bucket/objects while initializing perms #48491

Closed
wants to merge 1 commit into from
Closed

rgw: check for valid bucket/objects while initializing perms #48491

wants to merge 1 commit into from

Conversation

theanalyst
Copy link
Member

Since we use these objects later on, check that they're valid before proceeding

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

@github-actions github-actions bot added the rgw label Oct 14, 2022
@cbodley
Copy link
Contributor

cbodley commented Oct 14, 2022

@theanalyst this looks like something we'll want to backport, could you please open a tracker issue?

@theanalyst
Copy link
Member Author

@theanalyst this looks like something we'll want to backport, could you please open a tracker issue?

created https://tracker.ceph.com/issues/57877

cbodley
cbodley reviewed Oct 18, 2022
View reviewed changes
src/rgw/rgw_op.cc Outdated
Comment on lines 7961 to 7993
if (op->get_type() != RGW_OP_CREATE_BUCKET &&
rgw::sal::Bucket::empty(s->bucket)) {
return -ERR_NO_SUCH_BUCKET;
Copy link
Contributor

Choose a reason for hiding this comment

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

all of the verify jobs in https://pulpito.ceph.com/cbodley-2022-10-18_16:36:54-rgw-wip-cbodley-testing-distro-default-smithi/ failed during run-reshard.sh:

2022-10-18T17:57:56.641 INFO:tasks.workunit.client.0.smithi037.stderr:botocore.errorfactory.NoSuchBucket: An error occurred (NoSuchBucket) when calling the ListBuckets operation: Unknown

i think we need to filter out RGW_OP_LIST_BUCKETS here too

Copy link
Member Author

Choose a reason for hiding this comment

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

Valid point, I'll rework this!

Copy link
Contributor

Choose a reason for hiding this comment

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

could you please do some testing against some /admin APIs too? there's a new examples/rgw_admin_curl.sh script that helps send these requests

Copy link
Member Author

Choose a reason for hiding this comment

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

This is helpful, I think I need to filter out all ops that use the service endpoint and don't need a bucket at all. I'll try this and update

@theanalyst theanalyst mentioned this pull request Nov 15, 2022
Merged
14 tasks
cbodley
cbodley reviewed Nov 15, 2022
View reviewed changes
src/rgw/rgw_op.cc Outdated
Comment on lines 919 to 921
// TODO: Ideally this should be RGWOp const *, but get_type() isn't
// const marked
static bool rgw_is_service_op(RGWOp* op)
Copy link
Contributor

Choose a reason for hiding this comment

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

how about is_service_op(RGWOpType op)?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure much better actually

@cbodley
Copy link
Contributor

cbodley commented Nov 15, 2022

"ceph API tests" is showing some rgw failures that look related

@theanalyst
Copy link
Member Author

theanalyst commented Nov 16, 2022 •
edited

"ceph API tests" is showing some rgw failures that look related
edit found out these were actually run on the PRs themselves, will look into this
Is there a way to run these tests locally? I tested the few admin apis and swift apis, can check these as well

@cbodley
Copy link
Contributor

cbodley commented Nov 16, 2022

Is there a way to run these tests locally? I tested the few admin apis and swift apis, can check these as well

probably, but i don't know how. if all else fails, you're welcome to push temporary commits and debug through jenkins

@theanalyst
Copy link
Member Author

I changed the approach to only deny on particular object requests for now, since it looks like there are at least a few ops in the admin/roles etc that don't expect a bucket.

@theanalyst
rgw: check for valid bucket/objects while initializing perms
39eb2d3 
Since we use these objects later on, check that they're valid
before proceeding, we also translate a 404 into 403 in case
they ask for invalid bucket names

Signed-off-by: Abhishek Lekshmanan <abhishek.l@cern.ch>
@cbodley
Copy link
Contributor

cbodley commented Nov 23, 2022

can you help me understand how we get into these object ops with empty buckets?

another issue related to these verify_permission() functions is that admin/system users ignore these errors and continue on with null buckets (see #48912)

@theanalyst theanalyst mentioned this pull request Jan 19, 2023
Merged
14 tasks
@cbodley
Copy link
Contributor

cbodley commented Jan 19, 2023

is it still possible to reproduce this one after the merge of #49141?

@theanalyst
Copy link
Member Author

theanalyst commented Jan 23, 2023 •
edited

is it still possible to reproduce this one after the merge of #49141?

Only the swift path, which is what the new one (#49785) fixes, everything else is taken care of now. This PR is no longer needed, the swift part I've put in #49785

@dang
Copy link
Contributor

dang commented Feb 9, 2023

Can we close this one?

@ivancich ivancich added the wip-eric-testing-1 for ivancich testing label Feb 17, 2023
@ivancich
Copy link
Member

Teuthology results:

@ivancich
Copy link
Member

@dang , @cbodley , @theanalyst : I failed to notice discussion was still underway when I ran QA on this. I leave the decision to merge to y'all. I'm adding a DNM until you resolve.

@ivancich ivancich added DNM and removed needs-qa wip-eric-testing-1 for ivancich testing labels Feb 18, 2023
@ivancich
Copy link
Member

@dang , @cbodley , @theanalyst : I failed to notice discussion was still underway when I ran QA on this. I leave the decision to merge to y'all. I'm adding a DNM until you resolve.

I should have added you, @adamemerson , to the above. Sorry!

@theanalyst
Copy link
Member Author

Closing this one, the Swift POST forms PR is already merged

@theanalyst theanalyst closed this Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbodley cbodley cbodley left review comments

@dang dang dang approved these changes

@adamemerson adamemerson adamemerson approved these changes

Assignees
No one assigned
Labels
build/ops DNM rgw
Projects
None yet
Milestone
No milestone
5 participants
@theanalyst @cbodley @dang @ivancich @adamemerson