From: Casey Bodley <cbodley@redhat.com>
Date: Tue, 2 Jan 2024 22:11:03 +0000 (-0500)
Subject: rgw/auth: Identity::is_identity() takes one Principal
X-Git-Tag: testing/wip-pdonnell-testing-20240416.232051-debug~25^2~111
X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=05c15502e8cd7f8a7d279d562e2c7abadcaaeafa;p=ceph-ci.git

rgw/auth: Identity::is_identity() takes one Principal

take a single Principal instead flat_set<Principal>, and iterate over
calls to is_identity() instead

why?
* it simplifies the logic of each is_identity() function because they
  can use early returns to avoid visiting all of the cases
* Statement::eval_principal() no longer has to allocate a flat_set
  with a single element when the Identity is a role
* rgw::auth::Identity no longer depends on rgw::iam's choice of
  container type

Signed-off-by: Casey Bodley <cbodley@redhat.com>
---

diff --git a/src/rgw/rgw_auth.cc b/src/rgw/rgw_auth.cc
index 5075894e4f4..ab93cf0961d 100644
--- a/src/rgw/rgw_auth.cc
+++ b/src/rgw/rgw_auth.cc
@@ -25,6 +25,31 @@ using namespace std;
 namespace rgw {
 namespace auth {
 
+// match a tenant user principal by userid[:subuser]
+static bool match_user_principal(std::string_view userid,
+                                 std::string_view subuser,
+                                 std::string_view expected)
+{
+  // match user by id
+  if (!expected.starts_with(userid)) {
+    return false;
+  }
+  expected.remove_prefix(userid.size());
+  if (expected.empty()) { // exact match
+    return true;
+  }
+
+  // try to match userid:subuser
+  if (!expected.starts_with(":")) {
+    return false;
+  }
+  expected.remove_prefix(1);
+  if (expected.empty()) {
+    return false;
+  }
+  return (expected == "*" || expected == subuser);
+}
+
 static bool match_owner(const rgw_owner& owner, const rgw_user& uid,
                         std::string_view account_id)
 {
@@ -98,17 +123,14 @@ transform_old_authinfo(CephContext* const cct,
       return match_owner(o, id, account_id);
     }
 
-    bool is_identity(const idset_t& ids) const override {
-      for (auto& p : ids) {
-	if (p.is_wildcard()) {
-	  return true;
-	} else if (p.is_account() && p.get_account() == id.tenant) {
-	  return true;
-	} else if (p.is_user() &&
-		   (p.get_account() == id.tenant) &&
-		   (p.get_id() == id.id)) {
-	  return true;
-	}
+    bool is_identity(const Principal& p) const override {
+      if (p.is_wildcard()) {
+        return true;
+      } else if (p.is_account()) {
+        return p.get_account() == id.tenant;
+      } else if (p.is_user()) {
+        return p.get_account() == id.tenant
+            && p.get_id() == id.id;
       }
       return false;
     }
@@ -548,19 +570,10 @@ void rgw::auth::WebIdentityApplier::modify_request_state(const DoutPrefixProvide
   }
 }
 
-bool rgw::auth::WebIdentityApplier::is_identity(const idset_t& ids) const
+bool rgw::auth::WebIdentityApplier::is_identity(const Principal& p) const
 {
-  if (ids.size() > 1) {
-    return false;
-  }
-
-  for (auto id : ids) {
-    string idp_url = get_idp_url();
-    if (id.is_oidc_provider() && id.get_idp_url() == idp_url) {
-      return true;
-    }
-  }
-    return false;
+  return p.is_oidc_provider()
+      && p.get_idp_url() == get_idp_url();
 }
 
 const std::string rgw::auth::RemoteApplier::AuthInfo::NO_SUBUSER;
@@ -625,25 +638,19 @@ bool rgw::auth::RemoteApplier::is_owner_of(const rgw_owner& o) const
   return info.acct_user == *uid;
 }
 
-bool rgw::auth::RemoteApplier::is_identity(const idset_t& ids) const {
-  for (auto& id : ids) {
-    if (id.is_wildcard()) {
-      return true;
-
-      // We also need to cover cases where rgw_keystone_implicit_tenants
-      // was enabled. */
-    } else if (id.is_account() &&
-	       (info.acct_user.tenant.empty() ?
-		info.acct_user.id :
-		info.acct_user.tenant) == id.get_account()) {
-      return true;
-    } else if (id.is_user() &&
-	       info.acct_user.id == id.get_id() &&
-	       (info.acct_user.tenant.empty() ?
-		info.acct_user.id :
-		info.acct_user.tenant) == id.get_account()) {
-      return true;
-    }
+bool rgw::auth::RemoteApplier::is_identity(const Principal& p) const {
+  // We also need to cover cases where rgw_keystone_implicit_tenants
+  // was enabled.
+  std::string_view tenant = info.acct_user.tenant.empty() ?
+                            info.acct_user.id :
+                            info.acct_user.tenant;
+  if (p.is_wildcard()) {
+    return true;
+  } else if (p.is_account()) {
+    return p.get_account() == tenant;
+  } else if (p.is_user()) {
+    return p.get_id() == info.acct_user.id
+        && p.get_account() == tenant;
   }
   return false;
 }
@@ -836,31 +843,14 @@ bool rgw::auth::LocalApplier::is_owner_of(const rgw_owner& o) const
   return match_owner(o, user_info.user_id, user_info.account_id);
 }
 
-bool rgw::auth::LocalApplier::is_identity(const idset_t& ids) const {
-  for (auto& id : ids) {
-    if (id.is_wildcard()) {
-      return true;
-    } else if (id.is_account() &&
-	       id.get_account() == user_info.user_id.tenant) {
-      return true;
-    } else if (id.is_user() &&
-	       (id.get_account() == user_info.user_id.tenant)) {
-      if (id.get_id() == user_info.user_id.id) {
-        return true;
-      }
-      std::string wildcard_subuser = user_info.user_id.id;
-      wildcard_subuser.append(":*");
-      if (wildcard_subuser == id.get_id()) {
-        return true;
-      } else if (subuser != NO_SUBUSER) {
-        std::string user = user_info.user_id.id;
-        user.append(":");
-        user.append(subuser);
-        if (user == id.get_id()) {
-          return true;
-        }
-      }
-    }
+bool rgw::auth::LocalApplier::is_identity(const Principal& p) const {
+  if (p.is_wildcard()) {
+    return true;
+  } else if (p.is_account()) {
+    return p.get_account() == user_info.user_id.tenant;
+  } else if (p.is_user()) {
+    return p.get_account() == user_info.user_id.tenant
+        && match_user_principal(user_info.user_id.id, subuser, p.get_id());
   }
   return false;
 }
@@ -921,35 +911,25 @@ void rgw::auth::RoleApplier::to_str(std::ostream& out) const {
   out << ")";
 }
 
-bool rgw::auth::RoleApplier::is_identity(const idset_t& ids) const {
-  for (auto& p : ids) {
-    if (p.is_wildcard()) {
-      return true;
-    } else if (p.is_role()) {
-      string name = p.get_id();
-      string tenant = p.get_account();
-      if (name == role.name && tenant == role.tenant) {
-        return true;
-      }
-    } else if (p.is_assumed_role()) {
-      string tenant = p.get_account();
-      string role_session = role.name + "/" + token_attrs.role_session_name; //role/role-session
-      if (role.tenant == tenant && role_session == p.get_role_session()) {
-        return true;
-      }
+bool rgw::auth::RoleApplier::is_identity(const Principal& p) const {
+  if (p.is_wildcard()) {
+    return true;
+  } else if (p.is_role()) {
+    return p.get_id() == role.name
+        && p.get_account() == role.tenant;
+  } else if (p.is_assumed_role()) {
+    string role_session = role.name + "/" + token_attrs.role_session_name; //role/role-session
+    return p.get_account() == role.tenant
+        && p.get_role_session() == role_session;
+  } else {
+    string oidc_id;
+    if (token_attrs.user_id.ns.empty()) {
+      oidc_id = token_attrs.user_id.id;
     } else {
-      string id = p.get_id();
-      string tenant = p.get_account();
-      string oidc_id;
-      if (token_attrs.user_id.ns.empty()) {
-        oidc_id = token_attrs.user_id.id;
-      } else {
-        oidc_id = token_attrs.user_id.ns + "$" + token_attrs.user_id.id;
-      }
-      if (oidc_id == id && token_attrs.user_id.tenant == tenant) {
-        return true;
-      }
+      oidc_id = token_attrs.user_id.ns + "$" + token_attrs.user_id.id;
     }
+    return p.get_id() == oidc_id
+        && p.get_account() == token_attrs.user_id.tenant;
   }
   return false;
 }
diff --git a/src/rgw/rgw_auth.h b/src/rgw/rgw_auth.h
index df5f529e64b..796d501f0e8 100644
--- a/src/rgw/rgw_auth.h
+++ b/src/rgw/rgw_auth.h
@@ -32,7 +32,6 @@ using Exception = std::system_error;
 class Identity {
 public:
   typedef std::map<std::string, int> aclspec_t;
-  using idset_t = boost::container::flat_set<Principal>;
 
   virtual ~Identity() = default;
 
@@ -73,7 +72,7 @@ public:
 
   /* Verify whether a given identity corresponds to an identity in the
      provided set */
-  virtual bool is_identity(const idset_t& ids) const = 0;
+  virtual bool is_identity(const Principal& p) const = 0;
 
   /* Identity Type: RGW/ LDAP/ Keystone */
   virtual uint32_t get_identity_type() const = 0;
@@ -473,7 +472,7 @@ public:
 
   void to_str(std::ostream& out) const override;
 
-  bool is_identity(const idset_t& ids) const override;
+  bool is_identity(const Principal& p) const override;
 
   void load_acct_info(const DoutPrefixProvider* dpp, RGWUserInfo& user_info) const override;
 
@@ -630,7 +629,7 @@ public:
   uint32_t get_perms_from_aclspec(const DoutPrefixProvider* dpp, const aclspec_t& aclspec) const override;
   bool is_admin_of(const rgw_owner& o) const override;
   bool is_owner_of(const rgw_owner& o) const override;
-  bool is_identity(const idset_t& ids) const override;
+  bool is_identity(const Principal& p) const override;
 
   uint32_t get_perm_mask() const override { return info.perm_mask; }
   void to_str(std::ostream& out) const override;
@@ -691,7 +690,7 @@ public:
   uint32_t get_perms_from_aclspec(const DoutPrefixProvider* dpp, const aclspec_t& aclspec) const override;
   bool is_admin_of(const rgw_owner& o) const override;
   bool is_owner_of(const rgw_owner& o) const override;
-  bool is_identity(const idset_t& ids) const override;
+  bool is_identity(const Principal& p) const override;
   uint32_t get_perm_mask() const override {
     if (this->perm_mask == RGW_PERM_INVALID) {
       return get_perm_mask(subuser, user_info);
@@ -761,7 +760,7 @@ public:
     // TODO: handle account roles
     return uid && *uid == token_attrs.user_id;
   }
-  bool is_identity(const idset_t& ids) const override;
+  bool is_identity(const Principal& p) const override;
   uint32_t get_perm_mask() const override {
     return RGW_PERM_NONE; 
   }
diff --git a/src/rgw/rgw_auth_filters.h b/src/rgw/rgw_auth_filters.h
index 39b647c909a..b81bcefd13f 100644
--- a/src/rgw/rgw_auth_filters.h
+++ b/src/rgw/rgw_auth_filters.h
@@ -101,9 +101,8 @@ public:
     return get_decoratee().get_subuser();
   }
 
-  bool is_identity(
-    const boost::container::flat_set<Principal>& ids) const override {
-    return get_decoratee().is_identity(ids);
+  bool is_identity(const Principal& p) const override {
+    return get_decoratee().is_identity(p);
   }
 
   void to_str(std::ostream& out) const override {
diff --git a/src/rgw/rgw_iam_policy.cc b/src/rgw/rgw_iam_policy.cc
index 4a288fd55fa..bfd6d60eb1a 100644
--- a/src/rgw/rgw_iam_policy.cc
+++ b/src/rgw/rgw_iam_policy.cc
@@ -1164,6 +1164,15 @@ Effect Statement::eval(const Environment& e,
   return Effect::Pass;
 }
 
+static bool is_identity(const auth::Identity& ida,
+                        const flat_set<auth::Principal>& princ)
+{
+  return std::any_of(princ.begin(), princ.end(),
+      [&ida] (const auth::Principal& p) {
+        return ida.is_identity(p);
+      });
+}
+
 Effect Statement::eval_principal(const Environment& e,
 		       boost::optional<const rgw::auth::Identity&> ida, boost::optional<PolicyPrincipal&> princ_type) const {
   if (princ_type) {
@@ -1173,15 +1182,13 @@ Effect Statement::eval_principal(const Environment& e,
     if (princ.empty() && noprinc.empty()) {
       return Effect::Deny;
     }
-    if (ida->get_identity_type() != TYPE_ROLE && !princ.empty() && !ida->is_identity(princ)) {
+    if (ida->get_identity_type() != TYPE_ROLE && !princ.empty() && !is_identity(*ida, princ)) {
       return Effect::Deny;
     }
     if (ida->get_identity_type() == TYPE_ROLE && !princ.empty()) {
       bool princ_matched = false;
       for (auto p : princ) { // Check each principal to determine the type of the one that has matched
-        boost::container::flat_set<Principal> id;
-        id.insert(p);
-        if (ida->is_identity(id)) {
+        if (ida->is_identity(p)) {
           if (p.is_assumed_role() || p.is_user()) {
             if (princ_type) *princ_type = PolicyPrincipal::Session;
           } else {
@@ -1193,7 +1200,7 @@ Effect Statement::eval_principal(const Environment& e,
       if (!princ_matched) {
         return Effect::Deny;
       }
-    } else if (!noprinc.empty() && ida->is_identity(noprinc)) {
+    } else if (!noprinc.empty() && is_identity(*ida, noprinc)) {
       return Effect::Deny;
     }
   }
diff --git a/src/test/rgw/test_rgw_iam_policy.cc b/src/test/rgw/test_rgw_iam_policy.cc
index 09dadeaacca..1f03920407b 100644
--- a/src/test/rgw/test_rgw_iam_policy.cc
+++ b/src/test/rgw/test_rgw_iam_policy.cc
@@ -148,11 +148,8 @@ public:
     out << id;
   }
 
-  bool is_identity(const flat_set<Principal>& ids) const override {
-    if (id.is_wildcard() && (!ids.empty())) {
-      return true;
-    }
-    return ids.find(id) != ids.end() || ids.find(Principal::wildcard()) != ids.end();
+  bool is_identity(const Principal& p) const override {
+    return id.is_wildcard() || p.is_wildcard() || p == id;
   }
 
   uint32_t get_identity_type() const override {
diff --git a/src/test/rgw/test_rgw_lua.cc b/src/test/rgw/test_rgw_lua.cc
index a1343e10b15..01686b6e374 100644
--- a/src/test/rgw/test_rgw_lua.cc
+++ b/src/test/rgw/test_rgw_lua.cc
@@ -74,7 +74,7 @@ public:
     return;
   }
 
-  bool is_identity(const flat_set<Principal>& ids) const override {
+  bool is_identity(const Principal& p) const override {
     return false;
   }
 };