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

rgw: belong the anonymous object to the bucket owner #34462

Merged
merged 1 commit into from Dec 1, 2020

Conversation

inspur-wyq
Copy link
Contributor

fixbug: https://tracker.ceph.com/issues/44987

After set one bucket full_control to anyone, we can put a object without signature.
However, after putting a object without signature, I can not set the acl of it using s3cmd with the 403 error.Comparing with the s3 public write bucket, I found out that the anonymous object belong to the bucket owner in s3,while it belong to anonymous user in rgw, which made the 403 error.

Signed-off-by: wangyunqing wangyunqing@inspur.com

@@ -366,6 +366,11 @@ int RGWAccessControlList_S3::create_canned(ACLOwner& owner, ACLOwner& bucket_own
rgw_user bid = bucket_owner.get_id();
string bname = bucket_owner.get_display_name();

if (owner.get_id() == rgw_user(RGW_USER_ANON_ID)) {
owner.set_id(bid);
owner.set_name(bname);
Copy link
Contributor

Choose a reason for hiding this comment

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

for PUT Object requests, create_canned() is getting a reference to s->owner in ACLOwner& owner, and we shouldn't mutate that here. this should either operate on a local copy, or we should change create_canned() to take owner by value instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the owner_grant.set_canon do not change the input. So I think we can just make a if branch between owner_grant.set_canon(bid, bname, RGW_PERM_FULL_CONTROL); and owner_grant.set_canon(owner.get_id(), owner.get_display_name(), RGW_PERM_FULL_CONTROL);

owner_grant.set_canon(bid, bname, RGW_PERM_FULL_CONTROL);
} else {
owner_grant.set_canon(owner.get_id(), owner.get_display_name(), RGW_PERM_FULL_CONTROL);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this is better, yeah. but for the 'bucket-owner-read' and 'bucket-owner-full-control' cases below, we're still making comparisons against the original owner, so we would end up with two separate grants for the bucket owner

maybe it is better for create_canned() to take ACLOwner owner by value and overwrite it, so those 'bucket-owner-' comparisons don't have to change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for less test for last modification. Today when I test for the acl owner, I found out that if we do not change the owner with reference int 'RGWAccessControlList_S3::create_canned', it will make the the acl still belong to that anonymous owner in 'RGWAccessControlPolicy_S3::create_canned'. I think We should not only belong the grant_map to the bucket owner but also should belong the acl to the owner.
So should we check the anonymous in the RGWAccessControlPolicy_S3::create_canned? We can make a local copy with ACLOwner bucket_owner for the input of RGWAccessControlPolicy_S3::create_canned and deeper pass it in RGWAccessControlList_S3::create_canned or we can just pass the bucket_owner as the input value of ACLOwner& owner of RGWAccessControlPolicy_S3::create_canned. Which one do you think is better? Thanks.

Signed-off-by: wangyunqing <wangyunqing@inspur.com>
@stale
Copy link

stale bot commented Jun 20, 2020

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale
Copy link

stale bot commented Nov 22, 2020

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Nov 22, 2020
@ivancich ivancich added wip-eric-testing-1 for ivancich testing and removed stale labels Nov 30, 2020
@ivancich ivancich merged commit 9d973a0 into ceph:master Dec 1, 2020
@ivancich ivancich removed needs-qa wip-eric-testing-1 for ivancich testing labels Dec 1, 2020
@ivancich
Copy link
Member

ivancich commented Dec 1, 2020

@inspur-wyq @cbodley @theanalyst The tracker does not list any backports. Please modify the tracker if backports appropriate. Thanks!

@inspur-wyq inspur-wyq deleted the wip-44987 branch April 25, 2021 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants