From f780fc6ec40395ad0941d4e0309d464fe33836b1 Mon Sep 17 00:00:00 2001 From: Radoslaw Zarzynski Date: Fri, 3 Feb 2017 14:41:50 +0100 Subject: [PATCH] rgw: improve handling of illformed Swift's container ACLs. Fixes: http://tracker.ceph.com/issues/18796 Signed-off-by: Radoslaw Zarzynski --- src/rgw/rgw_acl_swift.cc | 195 ++++++++++++++++++++++---------------- src/rgw/rgw_acl_swift.h | 21 ++-- src/rgw/rgw_rest_swift.cc | 14 +-- 3 files changed, 133 insertions(+), 97 deletions(-) diff --git a/src/rgw/rgw_acl_swift.cc b/src/rgw/rgw_acl_swift.cc index 4b3de96a209..78b60124ba6 100644 --- a/src/rgw/rgw_acl_swift.cc +++ b/src/rgw/rgw_acl_swift.cc @@ -5,6 +5,8 @@ #include +#include + #include "common/ceph_json.h" #include "rgw_common.h" #include "rgw_user.h" @@ -43,6 +45,14 @@ static int parse_list(const std::string& uid_list, return 0; } +static bool is_referrer(const std::string& designator) +{ + return designator.compare(".r") == 0 || + designator.compare(".ref") == 0 || + designator.compare(".referer") == 0 || + designator.compare(".referrer") == 0; +} + static bool uid_is_public(const string& uid) { if (uid[0] != '.' || uid[1] != 'r') @@ -58,105 +68,118 @@ static bool uid_is_public(const string& uid) if (after.compare("*") != 0) return false; - return sub.compare(".r") == 0 || - sub.compare(".ref") == 0 || - sub.compare(".referer") == 0 || - sub.compare(".referrer") == 0; -} - -static bool extract_referer_urlspec(const std::string& uid, - std::string& url_spec) -{ - const size_t pos = uid.find(':'); - if (string::npos == pos) { - return false; - } - - const auto sub = uid.substr(0, pos); - url_spec = uid.substr(pos + 1); - - return sub.compare(".r") == 0 || - sub.compare(".ref") == 0 || - sub.compare(".referer") == 0 || - sub.compare(".referrer") == 0; + return is_referrer(sub); } -static bool normalize_referer_urlspec(string& url_spec, bool& is_negative) +static boost::optional referrer_to_grant(std::string url_spec, + const uint32_t perm) { + /* This function takes url_spec as non-ref std::string because of the trim + * operation that is essential to preserve compliance with Swift. It can't + * be easily accomplished with boost::string_ref. */ try { + bool is_negative; + ACLGrant grant; + if ('-' == url_spec[0]) { - is_negative = true; url_spec = url_spec.substr(1); + boost::algorithm::trim(url_spec); + + is_negative = true; } else { is_negative = false; } - if (url_spec != "*" && '*' == url_spec[0]) { - url_spec = url_spec.substr(1); + + /* We're specially handling the .r:* as the S3 API has a similar concept + * and thus we can have a small portion of compatibility here. */ + if (url_spec == "*") { + grant.set_group(ACL_GROUP_ALL_USERS, is_negative ? 0 : perm); + } else { + if ('*' == url_spec[0]) { + url_spec = url_spec.substr(1); + boost::algorithm::trim(url_spec); + } + + if (url_spec.empty() || url_spec == ".") { + return boost::none; + } + + grant.set_referer(url_spec, is_negative ? 0 : perm); } - return !url_spec.empty() && url_spec != "."; + return grant; } catch (std::out_of_range) { - return false; + return boost::none; } } -void RGWAccessControlPolicy_SWIFT::add_grants(RGWRados * const store, - const std::vector& uids, - const uint32_t perm) +static ACLGrant user_to_grant(CephContext* const cct, + RGWRados* const store, + const std::string& uid, + const uint32_t perm) { - for (const auto& uid : uids) { - ldout(cct, 20) << "trying to add grant for ACL uid=" << uid << dendl; - ACLGrant grant; - string url_spec; + rgw_user user(uid); + RGWUserInfo grant_user; + ACLGrant grant; + + if (rgw_get_user_info_by_uid(store, user, grant_user) < 0) { + ldout(cct, 10) << "grant user does not exist: " << uid << dendl; + /* skipping silently */ + grant.set_canon(user, std::string(), perm); + } else { + grant.set_canon(user, grant_user.display_name, perm); + } - if (uid_is_public(uid)) { - grant.set_group(ACL_GROUP_ALL_USERS, perm); - acl.add_grant(&grant); - } else if (extract_referer_urlspec(uid, url_spec)) { - if (0 != (perm & SWIFT_PERM_WRITE)) { - ldout(cct, 10) << "cannot grant write access for referers" << dendl; - continue; - } + return grant; +} - bool is_negative = false; - if (false == normalize_referer_urlspec(url_spec, is_negative)) { - ldout(cct, 10) << "cannot normalize referer: " << url_spec << dendl; - continue; - } else { - ldout(cct, 10) << "normalized referer to url_spec=" << url_spec - << ", is_negative=" << is_negative << dendl; - } +int RGWAccessControlPolicy_SWIFT::add_grants(RGWRados* const store, + const std::vector& uids, + const uint32_t perm) +{ + for (const auto& uid : uids) { + boost::optional grant; + ldout(cct, 20) << "trying to add grant for ACL uid=" << uid << dendl; - if (is_negative) { - /* Forbid access. */ - grant.set_referer(url_spec, 0); - } else { - grant.set_referer(url_spec, perm); + /* Let's check whether the item has a separator potentially indicating + * a special meaning (like an HTTP referral-based grant). */ + const size_t pos = uid.find(':'); + if (std::string::npos == pos) { + /* No, it don't have -- we've got just a regular user identifier. */ + grant = user_to_grant(cct, store, uid, perm); + } else { + /* Yes, *potentially* an HTTP referral. */ + auto designator = uid.substr(0, pos); + auto designatee = uid.substr(pos + 1); + + /* Swift strips whitespaces at both beginning and end. */ + boost::algorithm::trim(designator); + boost::algorithm::trim(designatee); + + if (! boost::algorithm::starts_with(designator, ".")) { + grant = user_to_grant(cct, store, uid, perm); + } else if ((perm & SWIFT_PERM_WRITE) == 0 && is_referrer(designator)) { + /* HTTP referrer-based ACLs aren't acceptable for writes. */ + grant = referrer_to_grant(designatee, perm); } + } - acl.add_grant(&grant); + if (grant) { + acl.add_grant(&*grant); } else { - rgw_user user(uid); - RGWUserInfo grant_user; - - if (rgw_get_user_info_by_uid(store, user, grant_user) < 0) { - ldout(cct, 10) << "grant user does not exist: " << uid << dendl; - /* skipping silently */ - grant.set_canon(user, std::string(), perm); - acl.add_grant(&grant); - } else { - grant.set_canon(user, grant_user.display_name, perm); - acl.add_grant(&grant); - } + return -EINVAL; } } + + return 0; } -bool RGWAccessControlPolicy_SWIFT::create(RGWRados * const store, - const rgw_user& id, - const std::string& name, - const std::string& read_list, - const std::string& write_list) + +int RGWAccessControlPolicy_SWIFT::create(RGWRados* const store, + const rgw_user& id, + const std::string& name, + const std::string& read_list, + const std::string& write_list) { acl.create_default(id, name); owner.set_id(id); @@ -166,23 +189,35 @@ bool RGWAccessControlPolicy_SWIFT::create(RGWRados * const store, std::vector uids; int r = parse_list(read_list, uids); if (r < 0) { - ldout(cct, 0) << "ERROR: parse_list returned r=" << r << dendl; - return false; + ldout(cct, 0) << "ERROR: parse_list for read returned r=" + << r << dendl; + return r; } - add_grants(store, uids, SWIFT_PERM_READ); + r = add_grants(store, uids, SWIFT_PERM_READ); + if (r < 0) { + ldout(cct, 0) << "ERROR: add_grants for read returned r=" + << r << dendl; + return r; + } } if (write_list.size()) { std::vector uids; int r = parse_list(write_list, uids); if (r < 0) { - ldout(cct, 0) << "ERROR: parse_list returned r=" << r << dendl; - return false; + ldout(cct, 0) << "ERROR: parse_list for write returned r=" + << r << dendl; + return r; } - add_grants(store, uids, SWIFT_PERM_WRITE); + r = add_grants(store, uids, SWIFT_PERM_WRITE); + if (r < 0) { + ldout(cct, 0) << "ERROR: add_grants for write returned r=" + << r << dendl; + return r; + } } - return true; + return 0; } void RGWAccessControlPolicy_SWIFT::to_str(string& read, string& write) diff --git a/src/rgw/rgw_acl_swift.h b/src/rgw/rgw_acl_swift.h index d249bebeffe..e4ecd058283 100644 --- a/src/rgw/rgw_acl_swift.h +++ b/src/rgw/rgw_acl_swift.h @@ -13,20 +13,21 @@ class RGWAccessControlPolicy_SWIFT : public RGWAccessControlPolicy { + int add_grants(RGWRados *store, + const std::vector& uids, + uint32_t perm); + public: - explicit RGWAccessControlPolicy_SWIFT(CephContext * const cct) + explicit RGWAccessControlPolicy_SWIFT(CephContext* const cct) : RGWAccessControlPolicy(cct) { } - ~RGWAccessControlPolicy_SWIFT() {} + ~RGWAccessControlPolicy_SWIFT() = default; - void add_grants(RGWRados *store, - const std::vector& uids, - uint32_t perm); - bool create(RGWRados *store, - const rgw_user& id, - const std::string& name, - const std::string& read_list, - const std::string& write_list); + int create(RGWRados *store, + const rgw_user& id, + const std::string& name, + const std::string& read_list, + const std::string& write_list); void to_str(std::string& read, std::string& write); }; diff --git a/src/rgw/rgw_rest_swift.cc b/src/rgw/rgw_rest_swift.cc index 816dd06c870..a0ebba040de 100644 --- a/src/rgw/rgw_rest_swift.cc +++ b/src/rgw/rgw_rest_swift.cc @@ -518,13 +518,13 @@ static int get_swift_container_settings(req_state * const s, if (read_attr || write_attr) { RGWAccessControlPolicy_SWIFT swift_policy(s->cct); - const bool r = swift_policy.create(store, - s->user->user_id, - s->user->display_name, - read_list, - write_list); - if (r != true) { - return -EINVAL; + const auto r = swift_policy.create(store, + s->user->user_id, + s->user->display_name, + read_list, + write_list); + if (r < 0) { + return r; } *policy = swift_policy; -- 2.39.5