RGW Bucket Policies #14307

adamemerson · 2017-04-03T22:05:35Z

It is by these words— no not the ones you're reading, but the ones in your policy document. it is by those very words that your buckets shall be guarded and gazed, watched and warded. They shall speak and grant access to some and turn others away in despair.

mattbenjamin · 2017-04-04T18:25:00Z

doc/radosgw/bucketpolicy.rst

+Configuration
+=============
+
+Bucket poliies are managed through standard S3 operations rather than


mattbenjamin · 2017-04-04T18:29:55Z

src/rgw/rgw_op.cc

@@ -3314,6 +3431,9 @@ void RGWPutObj::execute()

 int RGWPostObj::verify_permission()


that's strange...

cbodley · 2017-04-04T19:45:19Z

src/rgw/rgw_op.cc

+    if (s->iam_policy->eval(s->env, *s->auth.identity,
+			    s->object.instance.empty() ?
+			    rgw::IAM::s3ListBucket :
+			    rgw::IAM::s3ListBucketVersions,


this check should be based on the RGWListBucket::list_versions flag. the only problem is that it's assigned later in the request processing (when RGWListBucket_ObjStore_S3::get_params() is called by RGWListBucket::execute()), so you won't have access to it here. you probably want to just check s->info.args.exists("versions")

edit: or, move the call to get_params() into this function, like you did with RGWDeleteMultiObj below

cbodley · 2017-04-04T19:55:07Z

src/rgw/rgw_op.cc

+  try {
+    rgw::IAM::Policy p(s->cct, s->bucket_tenant, policy_text);
+    auto attrs = s->bucket_attrs;
+    attrs[RGW_ATTR_IAM_POLICY];


seems like something is missing here. what are we doing with p?

cbodley · 2017-04-04T20:07:25Z

src/common/ipaddr.cc

+	return cmp_addr6((sockaddr_in6*) addrl,
+			 (sockaddr_in6*) addrr,
+			 prefix_len);
+#endif


cbodley · 2017-04-04T20:21:07Z

src/common/iso_8601.cc

+    // This assumes a contiguous block of numbers in the correct order.
+    uint16_t digit(char c) {
+      if (!(c >= '0' && c <= '9'))
+	throw std::invalid_argument("Not a digit.");


re: CodingStyle, single-line blocks still get enclosed in { }

rzarzynski · 2017-04-12T12:32:17Z

src/rgw/rgw_iam_policy.cc

+}
+
+Effect Statement::eval(const Environment& e,
+		       optional<const rgw::auth::Identity&> ida,


@adamemerson: is there a reason for taking the optional reference here? Although boost::optional is able to do that, some restrictions exist.

Maybe pointer-to-const would be a simpler approach?

Because I wanted to make it explicit in the function signature that the parameter was optional. const pointers /can/ be nulled, but it's less obvious to people reading the code whether the function does something useful with a null value instead of crashing or returning an error.

(If we were using the GSL we could use the not_null template which accomplishes the same thing.)

adamemerson · 2017-05-16T17:42:04Z

@tchaikov I just wanted to get a checkmark before he logged out for the day, if I could.

adamemerson · 2017-05-16T17:46:39Z

@yehudasa If you want the submodule to point to a repository living under the Ceph project, you'll need to get someone who has access to put it there. I think it would be better to leave it pointing to the original repository until we have good reason to move it, since that makes adopting changes easier.

adamemerson · 2017-05-16T17:48:24Z

@yehudasa At least I'm pretty sure I don't have access to create a new repo under the Ceph project.

cbodley

approval pending submodule

tchaikov · 2017-05-16T17:51:22Z

@adamemerson sorry, Adam. i thought you were about to merge. didn't intent to block you anyway.

rzarzynski

@adamemerson: LGTM. Thanks for bringing this feature and sorry for late response.

rzarzynski · 2017-05-16T22:50:44Z

src/rgw/rgw_iam_policy.h

+      }
+
+      return d;
+    } catch (const std::logic_error& e) {


std::logic_error looks very reasonable here.

There are parts of C++14 that are both useful and easy to implement. This is one of them. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

This is a high performance, MIT licensed JSON parsing library. It provides a SAX interface so that I can compile an S3 policy without building up a JSONObject tree in the middle that gets thrown away. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

tchaikov

submodule inclusion lgtm.

tchaikov · 2017-05-17T17:33:47Z

src/common/backport14.h

+};
+
+template<typename T, typename... Args>
+inline typename uniquity<T>::datum make_unique(Args&&... args) {


tchaikov · 2017-05-17T17:46:53Z

src/test/common/test_iso_8601.cc

+TEST(iso_8601, epoch) {
+  const auto epoch = real_clock::from_time_t(0);
+
+  ASSERT_EQ(to_iso_8601(epoch, iso_8601_format::Y), "1970");


nit, the lhs is the expected value, while the rhs is the actual one.

tchaikov · 2017-05-17T17:47:56Z

src/test/common/test_iso_8601.cc

+/*
+ * Ceph - scalable distributed file system
+ *
+ * Copyright (C) 2014 Red Hat <contact@redhat.com>


i also miss 2014 for some reason.

For parsing and unparsing from ceph::real_time. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

Eventually this will allow us to match all authentication information against all specified principals in a policy. Right now it handles users and wildcards. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

So they can be used when calling the function Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

Make three wrapper functions to tidy up the process of making a bufferlist holding a single static buffer. The lack of any decent handling of const in buffer::list makes me wax wroth, but it's a bit much to fix right now. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

This support is currently incomplete but should provide a starting point. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

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

Only hook into object/bucket checks for now. Once we have STS (giving us Roles) or User/Group policies it will make sense to hook into those, too. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

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

Merge pull request #14307 from adamemerson/wip-sts-authorization common: Add make_unique submodule: Tencent's RapidJSON library common: Add ISO-8601 Date Support rgw: Add is_identity to AuthApplier class rgw: Move globbing flags to header buffer: Make the use of static areas more convenient rgw: Add basic support for IAM policies rgw: Build bucket permission and environment in req_state rgw: Verify policies as WELL as ACLs rgw: RESTful bucket policy ops rgw: Write documentation for bucket policies Reviewed-By: Casey Bodley <cbodley@redhat.com> Reviewed-By: Radoslaw Zarzynski <rzarzynski@mirantis.com> Reviewed-By: Kefu Chai <kchai@redhat.com>

tchaikov · 2017-06-15T08:01:48Z

src/rgw/rgw_iam_policy.cc

+    // Statement
+
+  } else if (w->id == TokenID::Sid) {
+    t->sid.emplace(s, l);


@adamemerson is http://people.redhat.com/bhubbard/scan-build-2017-06-07-193016-13960-1/report-5d592f.html#EndPath a false alarm or a valid warning?

adamemerson force-pushed the wip-sts-authorization branch from 0fd393f to 7953684 Compare April 4, 2017 17:37

mattbenjamin changed the title ~~DNM: Bucket Policies~~ DNM: RGW Bucket Policies Apr 4, 2017

mattbenjamin self-assigned this Apr 4, 2017

mattbenjamin reviewed Apr 4, 2017

View reviewed changes

adamemerson force-pushed the wip-sts-authorization branch from 7953684 to caccecd Compare April 4, 2017 18:31

cbodley reviewed Apr 4, 2017

View reviewed changes

src/common/ipaddr.cc Outdated

return cmp_addr6((sockaddr_in6*) addrl,

(sockaddr_in6*) addrr,

prefix_len);

#endif

Copy link

Contributor

cbodley Apr 4, 2017

Choose a reason for hiding this comment

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

TODO?

cbodley reviewed Apr 4, 2017

View reviewed changes

cbodley added feature rgw labels Apr 4, 2017

adamemerson force-pushed the wip-sts-authorization branch from caccecd to 8e59955 Compare April 8, 2017 00:11

adamemerson requested review from yehudasa and rzarzynski April 8, 2017 00:13

adamemerson force-pushed the wip-sts-authorization branch 2 times, most recently from ddb1c95 to f0e1d1a Compare April 10, 2017 21:21

adamemerson changed the title ~~DNM: RGW Bucket Policies~~ RGW Bucket Policies Apr 11, 2017

adamemerson force-pushed the wip-sts-authorization branch from f0e1d1a to 9e7b330 Compare April 11, 2017 22:03

rzarzynski reviewed Apr 12, 2017

View reviewed changes

adamemerson force-pushed the wip-sts-authorization branch 7 times, most recently from 7579716 to 0ab3cf5 Compare May 2, 2017 16:48

adamemerson force-pushed the wip-sts-authorization branch 3 times, most recently from dbc8de6 to cfc3755 Compare May 4, 2017 19:40

cbodley approved these changes May 16, 2017

View reviewed changes

adamemerson force-pushed the wip-sts-authorization branch from f05f9e0 to 93b623d Compare May 16, 2017 17:51

rzarzynski approved these changes May 16, 2017

View reviewed changes

src/rgw/rgw_iam_policy.h

}

return d;

} catch (const std::logic_error& e) {

Copy link

Contributor

rzarzynski May 16, 2017

Choose a reason for hiding this comment

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

std::logic_error looks very reasonable here.

adamemerson added 2 commits May 17, 2017 12:26

common: Add make_unique

64acb16

There are parts of C++14 that are both useful and easy to implement. This is one of them. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

adamemerson force-pushed the wip-sts-authorization branch from 93b623d to fd9ed1f Compare May 17, 2017 16:26

tchaikov approved these changes May 17, 2017

View reviewed changes

tchaikov reviewed May 17, 2017

View reviewed changes

adamemerson force-pushed the wip-sts-authorization branch from fd9ed1f to 8ed7ead Compare May 17, 2017 17:47

tchaikov reviewed May 17, 2017

View reviewed changes

adamemerson force-pushed the wip-sts-authorization branch from 8ed7ead to 100d150 Compare May 17, 2017 17:56

adamemerson added 9 commits May 17, 2017 14:22

common: Add ISO-8601 Date Support

7cd1ccf

For parsing and unparsing from ceph::real_time. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

rgw: Add is_identity to AuthApplier class

56167d1

Eventually this will allow us to match all authentication information against all specified principals in a policy. Right now it handles users and wildcards. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

rgw: Move globbing flags to header

9e8172b

So they can be used when calling the function Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

rgw: Add basic support for IAM policies

69a5eeb

This support is currently incomplete but should provide a starting point. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

rgw: Build bucket permission and environment in req_state

908d9e4

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

rgw: Verify policies as WELL as ACLs

266103f

Only hook into object/bucket checks for now. Once we have STS (giving us Roles) or User/Group policies it will make sense to hook into those, too. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

rgw: RESTful bucket policy ops

b7d0b74

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

rgw: Write documentation for bucket policies

80b285d

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

adamemerson force-pushed the wip-sts-authorization branch from 100d150 to 80b285d Compare May 17, 2017 18:22

adamemerson merged commit 80b285d into ceph:master May 17, 2017

adamemerson deleted the wip-sts-authorization branch May 17, 2017 20:10

tchaikov reviewed Jun 15, 2017

View reviewed changes

cbodley mentioned this pull request Apr 10, 2022

cmake: use rapidjson headers from s3select's submodule #45818

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RGW Bucket Policies #14307

RGW Bucket Policies #14307

adamemerson commented Apr 3, 2017

mattbenjamin Apr 4, 2017

mattbenjamin Apr 4, 2017

cbodley Apr 4, 2017 •

edited

Loading

cbodley Apr 4, 2017

cbodley Apr 4, 2017

cbodley Apr 4, 2017

rzarzynski Apr 12, 2017

adamemerson Apr 12, 2017

adamemerson commented May 16, 2017

adamemerson commented May 16, 2017

adamemerson commented May 16, 2017

cbodley left a comment

tchaikov commented May 16, 2017 •

edited

Loading

rzarzynski left a comment

rzarzynski May 16, 2017

tchaikov left a comment

tchaikov May 17, 2017

tchaikov May 17, 2017

tchaikov May 17, 2017

tchaikov Jun 15, 2017

		@@ -3314,6 +3431,9 @@ void RGWPutObj::execute()

		int RGWPostObj::verify_permission()

RGW Bucket Policies #14307

RGW Bucket Policies #14307

Conversation

adamemerson commented Apr 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbodley Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamemerson commented May 16, 2017

adamemerson commented May 16, 2017

adamemerson commented May 16, 2017

cbodley left a comment

Choose a reason for hiding this comment

tchaikov commented May 16, 2017 • edited Loading

rzarzynski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbodley Apr 4, 2017 •

edited

Loading

tchaikov commented May 16, 2017 •

edited

Loading