From: Adam C. Emerson Date: Tue, 13 Dec 2022 01:40:33 +0000 (-0500) Subject: rgw: Add `rgw_policy_reject_invalid_principals` and messages X-Git-Tag: v18.1.0~651^2~2 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=0dd2ef42459776111d749e5f11e0da2c25bc3ef1;p=ceph.git rgw: Add `rgw_policy_reject_invalid_principals` and messages Reject policies with invalid principals by default and provide more useful error messages while doing so. (Log them but do *not* reject the policy if it's set to false.) Signed-off-by: Adam C. Emerson --- diff --git a/PendingReleaseNotes b/PendingReleaseNotes index b480ec1d0e8d..e216861ba6fd 100644 --- a/PendingReleaseNotes +++ b/PendingReleaseNotes @@ -1,5 +1,9 @@ >=18.0.0 +* The RGW policy parser now rejects unknown principals by default. If you are + mirroring policies between RGW and AWS, you may wish to set + "rgw policy reject invalid principals" to "false". This affects only newly set + policies, not policies that are already in place. * RGW's default backend for `rgw_enable_ops_log` changed from RADOS to file. The default value of `rgw_ops_log_rados` is now false, and `rgw_ops_log_file_path` defaults to "/var/log/ceph/ops-log-$cluster-$name.log". diff --git a/src/common/options/rgw.yaml.in b/src/common/options/rgw.yaml.in index bd9292c58615..21260402c35b 100644 --- a/src/common/options/rgw.yaml.in +++ b/src/common/options/rgw.yaml.in @@ -3701,3 +3701,14 @@ options: default: tank services: - rgw +- name: rgw_policy_reject_invalid_principals + type: bool + level: basic + desc: Whether to reject policies with invalid principals + long_desc: If true, policies with invalid principals will be + rejected. We don't support Canonical User identifiers or some + other form of policies that Amazon does, so if you are mirroring + policies between RGW and AWS, you may wish to set this to false. + default: true + services: + - rgw diff --git a/src/rgw/rgw_admin.cc b/src/rgw/rgw_admin.cc index 46b6ced3cded..904b4d08770a 100644 --- a/src/rgw/rgw_admin.cc +++ b/src/rgw/rgw_admin.cc @@ -6785,7 +6785,10 @@ int main(int argc, const char **argv) } bufferlist bl = bufferlist::static_from_string(assume_role_doc); try { - const rgw::IAM::Policy p(g_ceph_context, tenant, bl); + const rgw::IAM::Policy p( + g_ceph_context, tenant, bl, + g_ceph_context->_conf.get_val( + "rgw_policy_reject_invalid_principals")); } catch (rgw::IAM::PolicyParseException& e) { cerr << "failed to parse policy: " << e.what() << std::endl; return -EINVAL; @@ -6840,7 +6843,9 @@ int main(int argc, const char **argv) bufferlist bl = bufferlist::static_from_string(assume_role_doc); try { - const rgw::IAM::Policy p(g_ceph_context, tenant, bl); + const rgw::IAM::Policy p(g_ceph_context, tenant, bl, + g_ceph_context->_conf.get_val( + "rgw_policy_reject_invalid_principals")); } catch (rgw::IAM::PolicyParseException& e) { cerr << "failed to parse policy: " << e.what() << std::endl; return -EINVAL; @@ -6898,7 +6903,9 @@ int main(int argc, const char **argv) bl = bufferlist::static_from_string(perm_policy_doc); } try { - const rgw::IAM::Policy p(g_ceph_context, tenant, bl); + const rgw::IAM::Policy p(g_ceph_context, tenant, bl, + g_ceph_context->_conf.get_val( + "rgw_policy_reject_invalid_principals")); } catch (rgw::IAM::PolicyParseException& e) { cerr << "failed to parse perm policy: " << e.what() << std::endl; return -EINVAL; diff --git a/src/rgw/rgw_auth.cc b/src/rgw/rgw_auth.cc index 92813fdd36ec..2c61b8361a2b 100644 --- a/src/rgw/rgw_auth.cc +++ b/src/rgw/rgw_auth.cc @@ -871,7 +871,7 @@ void rgw::auth::RoleApplier::modify_request_state(const DoutPrefixProvider *dpp, for (auto it: role.role_policies) { try { bufferlist bl = bufferlist::static_from_string(it); - const rgw::IAM::Policy p(s->cct, role.tenant, bl); + const rgw::IAM::Policy p(s->cct, role.tenant, bl, false); s->iam_user_policies.push_back(std::move(p)); } catch (rgw::IAM::PolicyParseException& e) { //Control shouldn't reach here as the policy has already been @@ -884,7 +884,7 @@ void rgw::auth::RoleApplier::modify_request_state(const DoutPrefixProvider *dpp, try { string policy = this->token_attrs.token_policy; bufferlist bl = bufferlist::static_from_string(policy); - const rgw::IAM::Policy p(s->cct, role.tenant, bl); + const rgw::IAM::Policy p(s->cct, role.tenant, bl, false); s->session_policies.push_back(std::move(p)); } catch (rgw::IAM::PolicyParseException& e) { //Control shouldn't reach here as the policy has already been diff --git a/src/rgw/rgw_iam_policy.cc b/src/rgw/rgw_iam_policy.cc index f48acd4c2f20..35aeb15fcdc5 100644 --- a/src/rgw/rgw_iam_policy.cc +++ b/src/rgw/rgw_iam_policy.cc @@ -178,6 +178,8 @@ struct ParseState { void annotate(std::string&& a); + boost::optional parse_principal(string&& s, string* errmsg); + ParseState(PolicyParser* pp, const Keyword* w) : pp(pp), w(w) {} @@ -211,6 +213,8 @@ struct PolicyParser : public BaseReaderHandler, PolicyParser> { Policy& policy; uint32_t v = 0; + const bool reject_invalid_principals; + uint32_t seen = 0; std::string annotation{"No error?"}; @@ -313,8 +317,10 @@ struct PolicyParser : public BaseReaderHandler, PolicyParser> { v = 0; } - PolicyParser(CephContext* cct, const string& tenant, Policy& policy) - : cct(cct), tenant(tenant), policy(policy) {} + PolicyParser(CephContext* cct, const string& tenant, Policy& policy, + bool reject_invalid_principals) + : cct(cct), tenant(tenant), policy(policy), + reject_invalid_principals(reject_invalid_principals) {} PolicyParser(const PolicyParser& policy) = delete; bool StartObject() { @@ -457,14 +463,17 @@ bool ParseState::key(const char* s, size_t l) { // I should just rewrite a few helper functions to use iterators, // which will make all of this ever so much nicer. -static boost::optional parse_principal(CephContext* cct, TokenID t, - string&& s) { - if ((t == TokenID::AWS) && (s == "*")) { +boost::optional ParseState::parse_principal(string&& s, + string* errmsg) { + if ((w->id == TokenID::AWS) && (s == "*")) { // Wildcard! return Principal::wildcard(); - } else if (t == TokenID::CanonicalUser) { + } else if (w->id == TokenID::CanonicalUser) { // Do nothing for now. - } else if (t == TokenID::AWS || t == TokenID::Federated) { + if (errmsg) + *errmsg = "RGW does not support canonical users."; + return boost::none; + } else if (w->id == TokenID::AWS || w->id == TokenID::Federated) { // AWS and Federated ARNs if (auto a = ARN::parse(s)) { if (a->resource == "root") { @@ -494,20 +503,31 @@ static boost::optional parse_principal(CephContext* cct, TokenID t, return Principal::assumed_role(std::move(a->account), match[2]); } } - } else { - if (std::none_of(s.begin(), s.end(), + } else if (std::none_of(s.begin(), s.end(), [](const char& c) { return (c == ':') || (c == '/'); })) { - // Since tenants are simply prefixes, there's no really good - // way to see if one exists or not. So we return the thing and - // let them try to match against it. - return Principal::tenant(std::move(s)); - } + // Since tenants are simply prefixes, there's no really good + // way to see if one exists or not. So we return the thing and + // let them try to match against it. + return Principal::tenant(std::move(s)); } - } - - ldout(cct, 0) << "Supplied principal is discarded: " << s << dendl; + if (errmsg) + *errmsg = + fmt::format( + "`{}` is not a supported AWS or Federated ARN. Supported ARNs are " + "forms like: " + "`arn:aws:iam::tenant:root` or a bare tenant name for a tenant, " + "`arn:aws:iam::tenant:role/role-name` for a role, " + "`arn:aws:sts::tenant:assumed-role/role-name/role-session-name` " + "for an assumed role, " + "`arn:aws:iam::tenant:user/user-name` for a user, " + "`arn:aws:iam::tenant:oidc-provider/idp-url` for OIDC.", s); + } + + if (errmsg) + *errmsg = fmt::format("RGW does not support principals of type `{}`.", + w->name); return boost::none; } @@ -634,8 +654,15 @@ bool ParseState::do_string(CephContext* cct, const char* s, size_t l) { auto& pri = pp->s[pp->s.size() - 2].w->id == TokenID::Principal ? t->princ : t->noprinc; - if (auto o = parse_principal(pp->cct, w->id, string(s, l))) { + string errmsg; + if (auto o = parse_principal({s, l}, &errmsg)) { pri.emplace(std::move(*o)); + } else if (pp->reject_invalid_principals) { + annotate(std::move(errmsg)); + return false; + } else { + ldout(cct, 0) << "Ignored principle `" << std::string_view{s, l} << "`: " + << errmsg << dendl; } } else { // Failure @@ -1524,10 +1551,11 @@ ostream& operator <<(ostream& m, const Statement& s) { } Policy::Policy(CephContext* cct, const string& tenant, - const bufferlist& _text) + const bufferlist& _text, + bool reject_invalid_principals) : text(_text.to_str()) { StringStream ss(text.data()); - PolicyParser pp(cct, tenant, *this); + PolicyParser pp(cct, tenant, *this, reject_invalid_principals); auto pr = Reader{}.Parse(ss, pp); if (!pr) { diff --git a/src/rgw/rgw_iam_policy.h b/src/rgw/rgw_iam_policy.h index 5e07bc2ce5fe..564ddd530c0b 100644 --- a/src/rgw/rgw_iam_policy.h +++ b/src/rgw/rgw_iam_policy.h @@ -523,8 +523,14 @@ struct Policy { std::vector statements; + // reject_invalid_principals should be set to + // `cct->_conf.get_val("rgw_policy_reject_invalid_principals")` + // when executing operations that *set* a bucket policy, but should + // be false when reading a stored bucket policy so as not to break + // backwards configuration. Policy(CephContext* cct, const std::string& tenant, - const bufferlist& text); + const bufferlist& text, + bool reject_invalid_principals); Effect eval(const Environment& e, boost::optional ida, diff --git a/src/rgw/rgw_op.cc b/src/rgw/rgw_op.cc index 29552560b27c..aa697e3d40de 100644 --- a/src/rgw/rgw_op.cc +++ b/src/rgw/rgw_op.cc @@ -293,7 +293,7 @@ static boost::optional get_iam_policy_from_attr(CephContext* cct, const string& tenant) { auto i = attrs.find(RGW_ATTR_IAM_POLICY); if (i != attrs.end()) { - return Policy(cct, tenant, i->second); + return Policy(cct, tenant, i->second, false); } else { return none; } @@ -326,7 +326,7 @@ vector get_iam_user_policy_from_attr(CephContext* cct, decode(policy_map, out_bl); for (auto& it : policy_map) { bufferlist bl = bufferlist::static_from_string(it.second); - Policy p(cct, tenant, bl); + Policy p(cct, tenant, bl, false); policies.push_back(std::move(p)); } } @@ -8114,7 +8114,9 @@ void RGWPutBucketPolicy::execute(optional_yield y) } try { - const Policy p(s->cct, s->bucket_tenant, data); + const Policy p( + s->cct, s->bucket_tenant, data, + s->cct->_conf.get_val("rgw_policy_reject_invalid_principals")); rgw::sal::Attrs attrs(s->bucket_attrs); if (s->bucket_access_conf && s->bucket_access_conf->block_public_policy() && diff --git a/src/rgw/rgw_rest_role.cc b/src/rgw/rgw_rest_role.cc index e397910a0fbf..13f476ea9916 100644 --- a/src/rgw/rgw_rest_role.cc +++ b/src/rgw/rgw_rest_role.cc @@ -164,7 +164,9 @@ int RGWCreateRole::get_params() bufferlist bl = bufferlist::static_from_string(trust_policy); try { - const rgw::IAM::Policy p(s->cct, s->user->get_tenant(), bl); + const rgw::IAM::Policy p( + s->cct, s->user->get_tenant(), bl, + s->cct->_conf.get_val("rgw_policy_reject_invalid_principals")); } catch (rgw::IAM::PolicyParseException& e) { ldpp_dout(this, 20) << "failed to parse policy: " << e.what() << dendl; @@ -568,7 +570,9 @@ int RGWPutRolePolicy::get_params() } bufferlist bl = bufferlist::static_from_string(perm_policy); try { - const rgw::IAM::Policy p(s->cct, s->user->get_tenant(), bl); + const rgw::IAM::Policy p( + s->cct, s->user->get_tenant(), bl, + s->cct->_conf.get_val("rgw_policy_reject_invalid_principals")); } catch (rgw::IAM::PolicyParseException& e) { ldpp_dout(this, 20) << "failed to parse policy: " << e.what() << dendl; diff --git a/src/rgw/rgw_rest_sts.cc b/src/rgw/rgw_rest_sts.cc index 5ae85fa9efcb..cc2922ef8c3b 100644 --- a/src/rgw/rgw_rest_sts.cc +++ b/src/rgw/rgw_rest_sts.cc @@ -526,7 +526,7 @@ int RGWREST_STS::verify_permission(optional_yield y) //Parse the policy //TODO - This step should be part of Role Creation try { - const rgw::IAM::Policy p(s->cct, s->user->get_tenant(), bl); + const rgw::IAM::Policy p(s->cct, s->user->get_tenant(), bl, false); if (!s->principal_tags.empty()) { auto res = p.eval(s->env, *s->auth.identity, rgw::IAM::stsTagSession, boost::none); if (res != rgw::IAM::Effect::Allow) { @@ -644,7 +644,9 @@ int RGWSTSAssumeRoleWithWebIdentity::get_params() if (! policy.empty()) { bufferlist bl = bufferlist::static_from_string(policy); try { - const rgw::IAM::Policy p(s->cct, s->user->get_tenant(), bl); + const rgw::IAM::Policy p( + s->cct, s->user->get_tenant(), bl, + s->cct->_conf.get_val("rgw_policy_reject_invalid_principals")); } catch (rgw::IAM::PolicyParseException& e) { ldpp_dout(this, 20) << "failed to parse policy: " << e.what() << "policy" << policy << dendl; @@ -703,7 +705,9 @@ int RGWSTSAssumeRole::get_params() if (! policy.empty()) { bufferlist bl = bufferlist::static_from_string(policy); try { - const rgw::IAM::Policy p(s->cct, s->user->get_tenant(), bl); + const rgw::IAM::Policy p( + s->cct, s->user->get_tenant(), bl, + s->cct->_conf.get_val("rgw_policy_reject_invalid_principals")); } catch (rgw::IAM::PolicyParseException& e) { ldpp_dout(this, 0) << "failed to parse policy: " << e.what() << "policy" << policy << dendl; diff --git a/src/rgw/rgw_rest_user_policy.cc b/src/rgw/rgw_rest_user_policy.cc index 5eee746f5925..ea56ddd94bb7 100644 --- a/src/rgw/rgw_rest_user_policy.cc +++ b/src/rgw/rgw_rest_user_policy.cc @@ -141,7 +141,9 @@ void RGWPutUserPolicy::execute(optional_yield y) } try { - const Policy p(s->cct, s->user->get_tenant(), bl); + const Policy p( + s->cct, s->user->get_tenant(), bl, + s->cct->_conf.get_val("rgw_policy_reject_invalid_principals")); map policies; if (auto it = user->get_attrs().find(RGW_ATTR_USER_POLICY); it != user->get_attrs().end()) { bufferlist out_bl = it->second; diff --git a/src/test/rgw/test_rgw_iam_policy.cc b/src/test/rgw/test_rgw_iam_policy.cc index 3afe6e1447a1..dff1a37e7d83 100644 --- a/src/test/rgw/test_rgw_iam_policy.cc +++ b/src/test/rgw/test_rgw_iam_policy.cc @@ -166,7 +166,8 @@ TEST_F(PolicyTest, Parse1) { boost::optional p; ASSERT_NO_THROW(p = Policy(cct.get(), arbitrary_tenant, - bufferlist::static_from_string(example1))); + bufferlist::static_from_string(example1), + true)); ASSERT_TRUE(p); EXPECT_EQ(p->text, example1); @@ -195,7 +196,7 @@ TEST_F(PolicyTest, Parse1) { TEST_F(PolicyTest, Eval1) { auto p = Policy(cct.get(), arbitrary_tenant, - bufferlist::static_from_string(example1)); + bufferlist::static_from_string(example1), true); Environment e; ARN arn1(Partition::aws, Service::s3, @@ -219,7 +220,8 @@ TEST_F(PolicyTest, Parse2) { boost::optional p; ASSERT_NO_THROW(p = Policy(cct.get(), arbitrary_tenant, - bufferlist::static_from_string(example2))); + bufferlist::static_from_string(example2), + true)); ASSERT_TRUE(p); EXPECT_EQ(p->text, example2); @@ -261,7 +263,7 @@ TEST_F(PolicyTest, Parse2) { TEST_F(PolicyTest, Eval2) { auto p = Policy(cct.get(), arbitrary_tenant, - bufferlist::static_from_string(example2)); + bufferlist::static_from_string(example2), true); Environment e; auto trueacct = FakeIdentity( @@ -302,7 +304,7 @@ TEST_F(PolicyTest, Parse3) { boost::optional p; ASSERT_NO_THROW(p = Policy(cct.get(), arbitrary_tenant, - bufferlist::static_from_string(example3))); + bufferlist::static_from_string(example3), true)); ASSERT_TRUE(p); EXPECT_EQ(p->text, example3); @@ -416,7 +418,7 @@ TEST_F(PolicyTest, Parse3) { TEST_F(PolicyTest, Eval3) { auto p = Policy(cct.get(), arbitrary_tenant, - bufferlist::static_from_string(example3)); + bufferlist::static_from_string(example3), true); Environment em; Environment tr = { { "aws:MultiFactorAuthPresent", "true" } }; Environment fa = { { "aws:MultiFactorAuthPresent", "false" } }; @@ -527,7 +529,7 @@ TEST_F(PolicyTest, Parse4) { boost::optional p; ASSERT_NO_THROW(p = Policy(cct.get(), arbitrary_tenant, - bufferlist::static_from_string(example4))); + bufferlist::static_from_string(example4), true)); ASSERT_TRUE(p); EXPECT_EQ(p->text, example4); @@ -556,7 +558,7 @@ TEST_F(PolicyTest, Parse4) { TEST_F(PolicyTest, Eval4) { auto p = Policy(cct.get(), arbitrary_tenant, - bufferlist::static_from_string(example4)); + bufferlist::static_from_string(example4), true); Environment e; ARN arn1(Partition::aws, Service::iam, @@ -574,7 +576,7 @@ TEST_F(PolicyTest, Parse5) { boost::optional p; ASSERT_NO_THROW(p = Policy(cct.get(), arbitrary_tenant, - bufferlist::static_from_string(example5))); + bufferlist::static_from_string(example5), true)); ASSERT_TRUE(p); EXPECT_EQ(p->text, example5); EXPECT_EQ(p->version, Version::v2012_10_17); @@ -603,7 +605,7 @@ TEST_F(PolicyTest, Parse5) { TEST_F(PolicyTest, Eval5) { auto p = Policy(cct.get(), arbitrary_tenant, - bufferlist::static_from_string(example5)); + bufferlist::static_from_string(example5), true); Environment e; ARN arn1(Partition::aws, Service::iam, @@ -626,7 +628,7 @@ TEST_F(PolicyTest, Parse6) { boost::optional p; ASSERT_NO_THROW(p = Policy(cct.get(), arbitrary_tenant, - bufferlist::static_from_string(example6))); + bufferlist::static_from_string(example6), true)); ASSERT_TRUE(p); EXPECT_EQ(p->text, example6); EXPECT_EQ(p->version, Version::v2012_10_17); @@ -655,7 +657,7 @@ TEST_F(PolicyTest, Parse6) { TEST_F(PolicyTest, Eval6) { auto p = Policy(cct.get(), arbitrary_tenant, - bufferlist::static_from_string(example6)); + bufferlist::static_from_string(example6), true); Environment e; ARN arn1(Partition::aws, Service::iam, @@ -673,7 +675,7 @@ TEST_F(PolicyTest, Parse7) { boost::optional p; ASSERT_NO_THROW(p = Policy(cct.get(), arbitrary_tenant, - bufferlist::static_from_string(example7))); + bufferlist::static_from_string(example7), true)); ASSERT_TRUE(p); EXPECT_EQ(p->text, example7); @@ -705,7 +707,7 @@ TEST_F(PolicyTest, Parse7) { TEST_F(PolicyTest, Eval7) { auto p = Policy(cct.get(), arbitrary_tenant, - bufferlist::static_from_string(example7)); + bufferlist::static_from_string(example7), true); Environment e; auto subacct = FakeIdentity( @@ -950,8 +952,9 @@ TEST_F(IPPolicyTest, IPEnvironment) { TEST_F(IPPolicyTest, ParseIPAddress) { boost::optional p; - ASSERT_NO_THROW(p = Policy(cct.get(), arbitrary_tenant, - bufferlist::static_from_string(ip_address_full_example))); + ASSERT_NO_THROW( + p = Policy(cct.get(), arbitrary_tenant, + bufferlist::static_from_string(ip_address_full_example), true)); ASSERT_TRUE(p); EXPECT_EQ(p->text, ip_address_full_example); @@ -1007,12 +1010,15 @@ TEST_F(IPPolicyTest, ParseIPAddress) { } TEST_F(IPPolicyTest, EvalIPAddress) { - auto allowp = Policy(cct.get(), arbitrary_tenant, - bufferlist::static_from_string(ip_address_allow_example)); - auto denyp = Policy(cct.get(), arbitrary_tenant, - bufferlist::static_from_string(ip_address_deny_example)); - auto fullp = Policy(cct.get(), arbitrary_tenant, - bufferlist::static_from_string(ip_address_full_example)); + auto allowp = + Policy(cct.get(), arbitrary_tenant, + bufferlist::static_from_string(ip_address_allow_example), true); + auto denyp = + Policy(cct.get(), arbitrary_tenant, + bufferlist::static_from_string(ip_address_deny_example), true); + auto fullp = + Policy(cct.get(), arbitrary_tenant, + bufferlist::static_from_string(ip_address_full_example), true); Environment e; Environment allowedIP, blocklistedIP, allowedIPv6, blocklistedIPv6; allowedIP.emplace("aws:SourceIp","192.168.1.2");