rgw: Fix evaluation of bucket management permissions

I spent some more time looking through the documentation of how work
is evaluated, and the examples on

<http://docs.aws.amazon.com/AmazonS3/latest/dev/
access-control-auth-workflow-bucket-operation.html>

Have convinced me that the behavior that was requested is more correct
than what we were doing before.

Fixes: http://tracker.ceph.com/issues/21901
Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

diff --git a/src/rgw/rgw_common.cc b/src/rgw/rgw_common.cc
index 216b56051d1b5d35ecbca6f9aa83a6c3bf668393..56a354a3faafe28a155e8e38d92f9f6bde2b62a8 100644 (file)
--- a/src/rgw/rgw_common.cc
+++ b/src/rgw/rgw_common.cc
@@ -25,6 +25,7 @@
 #include "common/Clock.h"
 #include "common/Formatter.h"
 #include "common/perf_counters.h"
+#include "common/convenience.h"
 #include "common/strtol.h"
 #include "include/str_list.h"
 #include "auth/Crypto.h"
@@ -1085,6 +1086,19 @@ bool verify_requester_payer_permission(struct req_state *s)
   return false;
 }
 
+namespace {
+Effect eval_or_pass(const optional<Policy>& policy,
+                          const rgw::IAM::Environment& env,
+                          const rgw::auth::Identity& id,
+                          const uint64_t op,
+                          const ARN& arn) {
+  if (!policy)
+    return Effect::Pass;
+  else
+    return policy->eval(env, id, op, arn);
+}
+}
+
 bool verify_bucket_permission(struct req_state * const s,
                              const rgw_bucket& bucket,
                               RGWAccessControlPolicy * const user_acl,
@@ -1095,15 +1109,14 @@ bool verify_bucket_permission(struct req_state * const s,
   if (!verify_requester_payer_permission(s))
     return false;
 
-  if (bucket_policy) {
-    auto r = bucket_policy->eval(s->env, *s->auth.identity, op, ARN(bucket));
-    if (r == Effect::Allow)
-      // It looks like S3 ACLs only GRANT permissions rather than
-      // denying them, so this should be safe.
-      return true;
-    else if (r == Effect::Deny)
-      return false;
-  }
+  auto r = eval_or_pass(bucket_policy, s->env, *s->auth.identity,
+                       op, ARN(bucket));
+  if (r == Effect::Allow)
+    // It looks like S3 ACLs only GRANT permissions rather than
+    // denying them, so this should be safe.
+    return true;
+  else if (r == Effect::Deny)
+    return false;
 
   const auto perm = op_to_perm(op);
 
@@ -1152,18 +1165,22 @@ bool verify_bucket_permission(struct req_state * const s, const uint64_t op)
                                   op);
 }
 
+// Authorize anyone permitted by the policy and the bucket owner
+// unless explicitly denied by the policy.
+
 int verify_bucket_owner_or_policy(struct req_state* const s,
                                  const uint64_t op)
 {
-  if (s->iam_policy) {
-    if (s->iam_policy->eval(s->env, *s->auth.identity, op,
-                           ARN(s->bucket)) == Effect::Allow) {
-      return 0;
-    }
-  } else if (s->auth.identity->is_owner_of(s->bucket_owner.get_id())) {
+  auto e = eval_or_pass(s->iam_policy,
+                       s->env, *s->auth.identity,
+                       op, ARN(s->bucket));
+  if (e == Effect::Allow ||
+      (e == Effect::Pass &&
+       s->auth.identity->is_owner_of(s->bucket_owner.get_id()))) {
     return 0;
+  } else {
+    return -EACCES;
   }
-  return -EACCES;
 }
 
 
@@ -1200,15 +1217,14 @@ bool verify_object_permission(struct req_state * const s,
   if (!verify_requester_payer_permission(s))
     return false;
 
-  if (bucket_policy) {
-    auto r = bucket_policy->eval(s->env, *s->auth.identity, op, ARN(obj));
-    if (r == Effect::Allow)
-      // It looks like S3 ACLs only GRANT permissions rather than
-      // denying them, so this should be safe.
-      return true;
-    else if (r == Effect::Deny)
-      return false;
-  }
+
+  auto r = eval_or_pass(bucket_policy, s->env, *s->auth.identity, op, ARN(obj));
+  if (r == Effect::Allow)
+    // It looks like S3 ACLs only GRANT permissions rather than
+    // denying them, so this should be safe.
+    return true;
+  else if (r == Effect::Deny)
+    return false;
 
   const auto perm = op_to_perm(op);