From 32f8cb7ae5899595da53ba047b04468e83d381ba Mon Sep 17 00:00:00 2001 From: Trond Norbye Date: Tue, 16 Nov 2021 15:31:04 +0100 Subject: [PATCH] MB-49148: Add a new frame id to add extra privileges Add a new frame id which allows the calling process to provide an extra privilege to grant the imposed user (note that the authenticated user MUST also hold the privilege in its effective set) Change-Id: Ib22fbcdd2c8ea7848315068019d3570ed3c96a71 Reviewed-on: https://review.couchbase.org/c/kv_engine/+/165850 Tested-by: Build Bot Reviewed-by: Dave Rigby --- daemon/cookie.cc | 13 +++++++++++++ daemon/cookie.h | 9 +++++++++ daemon/mcbp_validators.cc | 17 +++++++++++++++++ docs/BinaryProtocol.md | 8 ++++++++ include/mcbp/protocol/request.h | 1 + protocol/connection/frameinfo.cc | 9 +++++++++ protocol/connection/frameinfo.h | 12 ++++++++++++ protocol/mcbp/dump.cc | 6 ++++++ protocol/mcbp/request.cc | 8 ++++++++ tests/testapp/testapp_external_auth.cc | 14 +++++++++++++- 10 files changed, 96 insertions(+), 1 deletion(-) diff --git a/daemon/cookie.cc b/daemon/cookie.cc index 72ab02c36b..5d67611b28 100644 --- a/daemon/cookie.cc +++ b/daemon/cookie.cc @@ -562,6 +562,7 @@ void Cookie::reset() { euidPrivilegeContext.reset(); frame_copy.reset(); responseStatus = cb::mcbp::Status::COUNT; + euidExtraPrivileges.clear(); } void Cookie::setOpenTracingContext(cb::const_byte_buffer context) { @@ -678,6 +679,12 @@ cb::rbac::PrivilegeAccess Cookie::checkPrivilege( auto ret = checkPrivilege(privilegeContext, privilege, sid, cid); if (ret.success() && euidPrivilegeContext) { + if (std::find(euidExtraPrivileges.begin(), + euidExtraPrivileges.end(), + privilege) != euidExtraPrivileges.end()) { + // the caller explicitly granted the privilege + return cb::rbac::PrivilegeAccessOk; + } return checkPrivilege(*euidPrivilegeContext, privilege, sid, cid); } @@ -751,6 +758,12 @@ cb::rbac::PrivilegeAccess Cookie::testPrivilege( auto ret = testPrivilege(privilegeContext, privilege, sid, cid); if (ret.success() && euidPrivilegeContext) { + if (std::find(euidExtraPrivileges.begin(), + euidExtraPrivileges.end(), + privilege) != euidExtraPrivileges.end()) { + // the caller explicitly granted the privilege + return cb::rbac::PrivilegeAccessOk; + } return testPrivilege(*euidPrivilegeContext, privilege, sid, cid); } diff --git a/daemon/cookie.h b/daemon/cookie.h index 0392f230f6..51e608c3cc 100644 --- a/daemon/cookie.h +++ b/daemon/cookie.h @@ -567,6 +567,10 @@ class Cookie : public CookieIface { return euid; } + void addImposedUserExtraPrivilege(cb::rbac::Privilege privilege) { + euidExtraPrivileges.push_back(privilege); + } + bool isPreserveTtl() const { return preserveTtl; } @@ -758,6 +762,11 @@ class Cookie : public CookieIface { /// is the privilege context for that user (which we'll also test) std::optional euidPrivilegeContext; + /// When impersonating users we may grant the user extra privileges + /// (but the authenticated user must also have the privileges in the + /// effective set) + std::vector euidExtraPrivileges; + /// The response status we sent for this cookie (for a multi-response /// command such as STATS it would be the _last_ status code) cb::mcbp::Status responseStatus = cb::mcbp::Status::COUNT; diff --git a/daemon/mcbp_validators.cc b/daemon/mcbp_validators.cc index d43aba8463..15735e2f3e 100644 --- a/daemon/mcbp_validators.cc +++ b/daemon/mcbp_validators.cc @@ -347,6 +347,23 @@ Status McbpValidator::verify_header(Cookie& cookie, "PreserveTtl should not contain value"); } return status == Status::Success; + case cb::mcbp::request::FrameInfoId::ImpersonateExtraPrivilege: + if (data.empty()) { + cookie.setErrorContext("Privilege name must be set"); + status = Status::Einval; + return false; + } + try { + cookie.addImposedUserExtraPrivilege( + cb::rbac::to_privilege(std::string{ + reinterpret_cast(data.data()), + data.size()})); + } catch (const std::invalid_argument&) { + cookie.setErrorContext("Failed to look up the privilege"); + status = Status ::Einval; + return false; + } + return true; // continue parsing } // switch (id) status = Status::UnknownFrameInfo; return false; diff --git a/docs/BinaryProtocol.md b/docs/BinaryProtocol.md index 8648d8082a..9f98f2bc3f 100644 --- a/docs/BinaryProtocol.md +++ b/docs/BinaryProtocol.md @@ -180,6 +180,14 @@ existing document should be used instead of the TTL provided. If document don't exist the provided TTL should be used. The frame info contains no value (length = 0). +##### ID:6 - Impersonate users extra privilege + +Extra privilege to inject into the impersonated users privilege set ( +the authenticated process must hold the privilege in its effective set +to avoid privilege escalation) + +Multiple privileges may be added by adding multiple frame info entries + ### Response header Byte/ 0 | 1 | 2 | 3 | diff --git a/include/mcbp/protocol/request.h b/include/mcbp/protocol/request.h index 747e56e229..515508b8be 100644 --- a/include/mcbp/protocol/request.h +++ b/include/mcbp/protocol/request.h @@ -37,6 +37,7 @@ enum class FrameInfoId { OpenTracingContext = 3, Impersonate = 4, PreserveTtl = 5, + ImpersonateExtraPrivilege = 6, }; } diff --git a/protocol/connection/frameinfo.cc b/protocol/connection/frameinfo.cc index 121c253ddd..297c68468f 100644 --- a/protocol/connection/frameinfo.cc +++ b/protocol/connection/frameinfo.cc @@ -19,6 +19,8 @@ DurabilityFrameInfo::~DurabilityFrameInfo() = default; DcpStreamIdFrameInfo::~DcpStreamIdFrameInfo() = default; OpenTracingContextFrameInfo::~OpenTracingContextFrameInfo() = default; ImpersonateUserFrameInfo::~ImpersonateUserFrameInfo() = default; +ImpersonateUserExtraPrivilegeFrameInfo:: + ~ImpersonateUserExtraPrivilegeFrameInfo() = default; PreserveTtlFrameInfo::~PreserveTtlFrameInfo() = default; using cb::mcbp::request::FrameInfoId; @@ -116,3 +118,10 @@ std::vector PreserveTtlFrameInfo::encode() const { ret.push_back(uint8_t(FrameInfoId::PreserveTtl) << 0x04U); return ret; } + +std::vector ImpersonateUserExtraPrivilegeFrameInfo::encode() const { + return FrameInfo::encode( + FrameInfoId::ImpersonateExtraPrivilege, + {reinterpret_cast(privilege.data()), + privilege.size()}); +} diff --git a/protocol/connection/frameinfo.h b/protocol/connection/frameinfo.h index 0d6480ac44..9c098d2bbd 100644 --- a/protocol/connection/frameinfo.h +++ b/protocol/connection/frameinfo.h @@ -92,3 +92,15 @@ class PreserveTtlFrameInfo : public FrameInfo { ~PreserveTtlFrameInfo() override; std::vector encode() const override; }; + +class ImpersonateUserExtraPrivilegeFrameInfo : public FrameInfo { +public: + explicit ImpersonateUserExtraPrivilegeFrameInfo(std::string privilege) + : privilege(std::move(privilege)) { + } + ~ImpersonateUserExtraPrivilegeFrameInfo() override; + std::vector encode() const override; + +protected: + const std::string privilege; +}; diff --git a/protocol/mcbp/dump.cc b/protocol/mcbp/dump.cc index e08ab76484..bd4bfa461b 100644 --- a/protocol/mcbp/dump.cc +++ b/protocol/mcbp/dump.cc @@ -252,6 +252,12 @@ class Request : public McbpFrame { reinterpret_cast(payload.data()), payload.size()}; break; + case cb::mcbp::request::FrameInfoId::ImpersonateExtraPrivilege: + ss << " privilege=" + << std::string{ + reinterpret_cast(payload.data()), + payload.size()}; + break; } vector.emplace_back(ss.str()); diff --git a/protocol/mcbp/request.cc b/protocol/mcbp/request.cc index 18b102f595..4e7ece6d1b 100644 --- a/protocol/mcbp/request.cc +++ b/protocol/mcbp/request.cc @@ -363,6 +363,12 @@ nlohmann::json Request::toJSON(bool validated) const { break; case request::FrameInfoId::PreserveTtl: frameid["Preserve TTL"] = true; + break; + case request::FrameInfoId::ImpersonateExtraPrivilege: + frameid["privilege"].push_back(std::string{ + reinterpret_cast(buffer.data()), + buffer.size()}); + break; } return true; @@ -419,6 +425,8 @@ std::string to_string(cb::mcbp::request::FrameInfoId id) { return "Impersonate"; case FrameInfoId::PreserveTtl: return "PreserveTtl"; + case FrameInfoId::ImpersonateExtraPrivilege: + return "ImpersonateExtraPrivilege"; } throw std::invalid_argument("to_string(): Invalid frame id: " + diff --git a/tests/testapp/testapp_external_auth.cc b/tests/testapp/testapp_external_auth.cc index a91115e52c..251eda47fb 100644 --- a/tests/testapp/testapp_external_auth.cc +++ b/tests/testapp/testapp_external_auth.cc @@ -54,7 +54,7 @@ class TestappAuthProvider : public AuthProvider { R"({"satchel" : { "domain" : "external", "buckets": { - "default": ["Read","SimpleStats","Insert","Delete","Upsert"] + "default": ["Read","Insert","Delete","Upsert"] }, "privileges": [] }})"); @@ -366,6 +366,18 @@ TEST_P(ExternalAuthTest, TestImpersonateExternalUser) { // The next time we call the op it should hit it in the cache.. c.execute(cmd); + + // Now that we have the entry in cache, verify that we can + // add privileges as part of impersonate + BinprotGenericCommand stat(cb::mcbp::ClientOpcode::Stat); + stat.addFrameInfo(ImpersonateUserFrameInfo{"^satchel"}); + rsp = c.execute(stat); + EXPECT_EQ(cb::mcbp::Status::Eaccess, rsp.getStatus()); + stat.addFrameInfo( + ImpersonateUserExtraPrivilegeFrameInfo{"SimpleStats"}); + rsp = c.execute(stat); + EXPECT_TRUE(rsp.isSuccess()) + << to_string(rsp.getStatus()) << " " << rsp.getDataString(); }); }