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: Added a globbing method for AWS Policies. #12445

Merged
merged 1 commit into from Jan 24, 2017

Conversation

Projects
None yet
4 participants
@pritha-srivastava
Contributor

pritha-srivastava commented Dec 12, 2016

Signed-off-by: Pritha Srivastava prsrivas@redhat.com

@pritha-srivastava pritha-srivastava requested a review from adamemerson Dec 12, 2016

@pritha-srivastava

This comment has been minimized.

Contributor

pritha-srivastava commented Dec 12, 2016

@adamemerson : This is the first draft of the globbing method, please take a look and see if it looks fine.

if (*pattern == *input || *pattern == '?')
return match_internal(pattern + 1, input + 1);
if (*pattern == '*')
return match_internal(pattern + 1, input) || match_internal(pattern, input + 1);

This comment has been minimized.

@cbodley

cbodley Dec 13, 2016

Contributor

with recursion here, it looks like an attacker could pass in a long string of *s to crash the gateway. i wonder what protections other implementations have against this

This comment has been minimized.

@adamemerson

adamemerson Dec 13, 2016

Contributor

Ironically, since these are tailcalls, GCC might even compile it as non-recursive. We can't depend on that.

I for one would be in favor of rewriting all of RadosGW in Scheme, though :)

#define POLICY_STRING 0x08
using namespace boost;

This comment has been minimized.

@adamemerson

adamemerson Dec 20, 2016

Contributor

Boost is rather large. Since we're tracking current, it would probably be better to import just the names we need and not have potential clashes down the road.

}
return 0;
}

This comment has been minimized.

@adamemerson

adamemerson Dec 20, 2016

Contributor

In this function, it would be better ouse boost::string_ref rather than using char* and a NUL terminator. That way we can pass in substrings easily without having to copy and NUL-terminate them.

result.push_back(it);
}
}

This comment has been minimized.

@adamemerson

adamemerson Dec 20, 2016

Contributor

I'd rather avoid this kind of build-up/burn-down strategy. Looking at how this is used below, it seems like we should be able to get what we want by having a function that takes two strings and applies a passed function to each pair of substrings. I think we should be able to signal failure (no match) if one string runs out of tokens before the other.

}
int match(string& pattern, string& input, int flag)
{

This comment has been minimized.

@adamemerson

adamemerson Dec 20, 2016

Contributor

We should not mutate our arguments. (That is, they should either be const std::string& or a boost::string_ref. I'd prefer the latter since we can pass in C-style strings that way without having to copy them.)

input.end(),
input.begin(),
::tolower);
}

This comment has been minimized.

@adamemerson

adamemerson Dec 20, 2016

Contributor

Rather than modifying the string in place like this, I think we might want to make the lower-level function take a 'comparer' as a template parameter. That way it can get inlined and we won't have jump or branch overhead, but we also won't have to modify or copy our input.

This comment has been minimized.

@pritha-srivastava

pritha-srivastava Dec 21, 2016

Contributor

I am not sure I understand what the comparer will do here. Can you please explain a little.

This comment has been minimized.

@adamemerson

adamemerson Dec 21, 2016

Contributor

I was thinking that rather than modify the string, one could have match_internal take a function that compares characters. Something like

template

static int match_internal(string_ref pattern, string_ref input, F&& eq)

And then use it like if (eq(a, b)) instead of if (a == b)

Then when calling match_internal you can either pass a function that does a a case-folding character comparison or a function that does a case sensitive compare.

if (flag & POLICY_ACTION) {
vector<string> services = {"iam", "ec2", "qs", "sns", "s3"};
if (std::find(std::begin(services), std::end(services), result1[0])

This comment has been minimized.

@adamemerson

adamemerson Dec 20, 2016

Contributor

A static, constexpr std::array would be better here, so we don't allocate a vector like this every time we take the branch

if (std::find(std::begin(services), std::end(services), result1[0])
== std::end(services)) {
return 0;
}

This comment has been minimized.

@adamemerson

adamemerson Dec 20, 2016

Contributor

is the idea just to have a pre-sanity check and filter out anything that can't be a service?

This comment has been minimized.

@pritha-srivastava

pritha-srivastava Dec 21, 2016

Contributor

Yes, the document says that in case of ACTIONS, the ARNs should begin with a valid service. Though I am not sure if this list is complete. I'll have to search more.

}
}
return 1;
}

This comment has been minimized.

@adamemerson

adamemerson Dec 20, 2016

Contributor

This is pretty good overall, apart from my allocation nits.

rgw: Added a globbing method for AWS Policies.
Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
@pritha-srivastava

This comment has been minimized.

Contributor

pritha-srivastava commented Jan 18, 2017

@adamemerson : I had incorporated your comments, please take a look and see if this looks fine.

@adamemerson

I approve of these changes. Let them be merged!

@adamemerson adamemerson changed the title from [DNM] rgw: Added a globbing method for AWS Policies. to rgw: Added a globbing method for AWS Policies. Jan 24, 2017

@adamemerson adamemerson merged commit 5ce4036 into ceph:master Jan 24, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jan 25, 2017

hey @adamemerson could you add the "Reviewed-by" line(s) when you merge a PR next time? see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#3-using-reported-by-tested-by-and-reviewed-by .

@adamemerson

This comment has been minimized.

Contributor

adamemerson commented Jan 25, 2017

@tchaikov Sorry about that, I thought the Reviewed-By lines were added by Github's merge button based on the reviewers of the PR.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jan 25, 2017

yeah, we should have a tool for enumerating the approvers and prepare a commit message for merging commit. but i failed to find the github API to do the merge. but there is a API[1] for listing the reviews. @badone we were discussing the tool the other day, IIRC.


[1] https://developer.github.com/v3/pulls/reviews/#list-reviews-on-a-pull-request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment