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: improve handling of illformed Swift's container ACLs. #13248

Merged
merged 1 commit into from Feb 14, 2017

Conversation

rzarzynski
Copy link
Contributor

Fixes: http://tracker.ceph.com/issues/18796
Signed-off-by: Radoslaw Zarzynski rzarzynski@mirantis.com

@rzarzynski
Copy link
Contributor Author

Tempest hasn't found any regression here.

@rzarzynski
Copy link
Contributor Author

@Jing-Scott: could you please take a look?

Copy link
Contributor

@Jing-Scott Jing-Scott left a comment

Choose a reason for hiding this comment

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

lgtm. I've considered this bug before. The SWIF indeed supports the cleaned ACLs.

@@ -64,99 +66,123 @@ static bool uid_is_public(const string& uid)
sub.compare(".referrer") == 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

return is_referrer(sub); may be better, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely.

url_spec = url_spec.substr(1);
boost::algorithm::trim_if(url_spec, boost::is_any_of(" \t\r\n"));
Copy link
Contributor

Choose a reason for hiding this comment

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

the boost::algorithm::trim_if satisfied what I wanted before. very good!

Copy link
Contributor

Choose a reason for hiding this comment

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

@rzarzynski if these strings are only passed as headers, i don't think we should be trying to handle control characters like \t\r\n here. looking at http://docs.openstack.org/developer/swift/overview_acl.html:

Spaces may occur between elements as shown in the following example:

.r : *, .rlistings, 7ec59e87c6584c348b563254aae4c221:*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE.

ldout(cct, 10) << "normalized referer to url_spec=" << url_spec
<< ", is_negative=" << is_negative << dendl;
}
int RGWAccessControlPolicy_SWIFT::add_grants(RGWRados* const store,
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah! This logic looks more clear and more easy to understand!

Copy link
Contributor

@Jing-Scott Jing-Scott left a comment

Choose a reason for hiding this comment

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

Thanks! Look good!

@Jing-Scott
Copy link
Contributor

@cbodley @mattbenjamin
I've no write access. So what do you think?

@rzarzynski
Copy link
Contributor Author

@Jing-Scott: we definitely need to obtain results from Teuthology first. Just scheduled a run together with #13242.

@rzarzynski
Copy link
Contributor Author

The Teuthology run is green except one but unrelated failure.

@cbodley cbodley self-assigned this Feb 6, 2017
Fixes: http://tracker.ceph.com/issues/18796
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
@rzarzynski
Copy link
Contributor Author

@cbodley: I've addressed your comment regarding the whitespace characters by switching from boost::algorithm::trim_if to plain boost::algorithm::trim. Absolutely no other changes here.

@cbodley cbodley merged commit a89b728 into ceph:master Feb 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants