Used `reserve_resources` ACL for static reservations. Review: https://reviews.apache.org/r/64515
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/ae515379 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/ae515379 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/ae515379 Branch: refs/heads/master Commit: ae515379e8447b9f6ac8416f3a69cec57b5d7ba5 Parents: 5d0d352 Author: Jiang Yan Xu <xuj...@apple.com> Authored: Mon Dec 11 16:03:39 2017 -0800 Committer: Jiang Yan Xu <xuj...@apple.com> Committed: Wed Jan 17 11:29:55 2018 -0800 ---------------------------------------------------------------------- include/mesos/authorizer/acls.proto | 5 +- src/master/master.cpp | 75 ++++++++--- src/master/master.hpp | 11 +- src/tests/master_authorization_tests.cpp | 176 +++++++++++++++++++++++++- 4 files changed, 245 insertions(+), 22 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/ae515379/include/mesos/authorizer/acls.proto ---------------------------------------------------------------------- diff --git a/include/mesos/authorizer/acls.proto b/include/mesos/authorizer/acls.proto index d869820..0862743 100644 --- a/include/mesos/authorizer/acls.proto +++ b/include/mesos/authorizer/acls.proto @@ -75,7 +75,10 @@ message ACL { // Specifies which roles a principal can reserve resources for. message ReserveResources { - // Subjects: Framework principal or Operator username. + // Subjects: + // - Framework principal or Operator username in the case of + // dynamic reservation. + // - Agent principal in the case of static reservation. required Entity principals = 1; // Objects: The principal(s) can reserve resources for these roles. http://git-wip-us.apache.org/repos/asf/mesos/blob/ae515379/src/master/master.cpp ---------------------------------------------------------------------- diff --git a/src/master/master.cpp b/src/master/master.cpp index b2e28eb..3af96b1 100644 --- a/src/master/master.cpp +++ b/src/master/master.cpp @@ -3790,28 +3790,53 @@ Future<bool> Master::authorizeDestroyVolume( } -Future<bool> Master::authorizeSlave(const Option<string>& principal) +Future<bool> Master::authorizeSlave( + const SlaveInfo& slaveInfo, + const Option<Principal>& principal) { if (authorizer.isNone()) { return true; } - LOG(INFO) << "Authorizing agent " + list<Future<bool>> authorizations; + + // First authorize whether the agent can register. + LOG(INFO) << "Authorizing agent providing resources " + << "'" << stringify(Resources(slaveInfo.resources())) << "' " << (principal.isSome() - ? "with principal '" + principal.get() + "'" + ? "with principal '" + stringify(principal.get()) + "'" : "without a principal"); authorization::Request request; request.set_action(authorization::REGISTER_AGENT); - if (principal.isSome()) { - request.mutable_subject()->set_value(principal.get()); + Option<authorization::Subject> subject = createSubject(principal); + if (subject.isSome()) { + request.mutable_subject()->CopyFrom(subject.get()); } // No need to set the request's object as it is implicitly set to // ANY by the authorizer. + authorizations.push_back(authorizer.get()->authorized(request)); - return authorizer.get()->authorized(request); + // Next, if static reservations exist, also authorize them. + // + // NOTE: We don't look at dynamic reservations in checkpointed + // resources because they should have gone through authorization + // against the framework / operator's principal when they were + // created. In constrast, static reservations are initiated by the + // agent's principal and authorizing them helps prevent agents from + // advertising reserved resources of arbitrary roles. + if (!Resources(slaveInfo.resources()).reserved().empty()) { + authorizations.push_back( + authorizeReserveResources(slaveInfo.resources(), principal)); + } + + return collect(authorizations) + .then([](const list<bool>& results) + -> Future<bool> { + return std::find(results.begin(), results.end(), false) == results.end(); + }); } @@ -6004,9 +6029,17 @@ void Master::registerSlave( // Note that the principal may be empty if authentication is not // required. Also it is passed along because it may be removed from // `authenticated` while the authorization is pending. - Option<string> principal = authenticated.get(from); + Option<Principal> principal = authenticated.contains(from) + ? Principal(authenticated.at(from)) + : Option<Principal>::none(); + + // Calling the `onAny` continuation below separately so we can move + // `registerSlaveMessage` without it being evaluated before it's used + // by `authorizeSlave`. + Future<bool> authorization = + authorizeSlave(registerSlaveMessage.slave(), principal); - authorizeSlave(principal) + authorization .onAny(defer(self(), &Self::_registerSlave, from, @@ -6019,7 +6052,7 @@ void Master::registerSlave( void Master::_registerSlave( const UPID& pid, RegisterSlaveMessage&& registerSlaveMessage, - const Option<string>& principal, + const Option<Principal>& principal, const Future<bool>& authorized) { CHECK(!authorized.isDiscarded()); @@ -6033,9 +6066,10 @@ void Master::_registerSlave( authorizationError = "Authorization failure: " + authorized.failure(); } else if (!authorized.get()) { authorizationError = - "Not authorized to register as agent " + + "Not authorized to register agent providing resources " + "'" + stringify(Resources(slaveInfo.resources())) + "' " + (principal.isSome() - ? "with principal '" + principal.get() + "'" + ? "with principal '" + stringify(principal.get()) + "'" : "without a principal"); } @@ -6356,9 +6390,17 @@ void Master::reregisterSlave( // Note that the principal may be empty if authentication is not // required. Also it is passed along because it may be removed from // `authenticated` while the authorization is pending. - Option<string> principal = authenticated.get(from); + Option<Principal> principal = authenticated.contains(from) + ? Principal(authenticated.at(from)) + : Option<Principal>::none(); + + // Calling the `onAny` continuation below separately so we can move + // `reregisterSlaveMessage` without it being evaluated before it's used + // by `authorizeSlave`. + Future<bool> authorization = + authorizeSlave(reregisterSlaveMessage.slave(), principal); - authorizeSlave(principal) + authorization .onAny(defer(self(), &Self::_reregisterSlave, from, @@ -6371,7 +6413,7 @@ void Master::reregisterSlave( void Master::_reregisterSlave( const UPID& pid, ReregisterSlaveMessage&& reregisterSlaveMessage, - const Option<string>& principal, + const Option<Principal>& principal, const Future<bool>& authorized) { CHECK(!authorized.isDiscarded()); @@ -6385,9 +6427,10 @@ void Master::_reregisterSlave( authorizationError = "Authorization failure: " + authorized.failure(); } else if (!authorized.get()) { authorizationError = - "Not authorized to re-register as agent with principal " + + "Not authorized to re-register agent providing resources " + "'" + stringify(Resources(slaveInfo.resources())) + "' " + (principal.isSome() - ? "with principal '" + principal.get() + "'" + ? "with principal '" + stringify(principal.get()) + "'" : "without a principal"); } http://git-wip-us.apache.org/repos/asf/mesos/blob/ae515379/src/master/master.hpp ---------------------------------------------------------------------- diff --git a/src/master/master.hpp b/src/master/master.hpp index 651e130..0513678 100644 --- a/src/master/master.hpp +++ b/src/master/master.hpp @@ -604,7 +604,7 @@ protected: void _registerSlave( const process::UPID& pid, RegisterSlaveMessage&& registerSlaveMessage, - const Option<std::string>& principal, + const Option<process::http::authentication::Principal>& principal, const process::Future<bool>& authorized); void __registerSlave( @@ -615,7 +615,7 @@ protected: void _reregisterSlave( const process::UPID& pid, ReregisterSlaveMessage&& incomingMessage, - const Option<std::string>& principal, + const Option<process::http::authentication::Principal>& principal, const process::Future<bool>& authorized); void __reregisterSlave( @@ -757,8 +757,11 @@ protected: process::Future<bool> authorizeFramework( const FrameworkInfo& frameworkInfo); - // Returns whether the principal is authorized to (re-)register an agent. - process::Future<bool> authorizeSlave(const Option<std::string>& principal); + // Returns whether the principal is authorized to (re-)register an agent + // and whether the `SlaveInfo` is authorized. + process::Future<bool> authorizeSlave( + const SlaveInfo& slaveInfo, + const Option<process::http::authentication::Principal>& principal); // Returns whether the task is authorized. // Returns failure for transient authorization failures. http://git-wip-us.apache.org/repos/asf/mesos/blob/ae515379/src/tests/master_authorization_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/master_authorization_tests.cpp b/src/tests/master_authorization_tests.cpp index d06a50e..8e526fc 100644 --- a/src/tests/master_authorization_tests.cpp +++ b/src/tests/master_authorization_tests.cpp @@ -2407,7 +2407,7 @@ TEST_F(MasterAuthorizationTest, UnauthorizedToRegisterAgent) ASSERT_SOME(master); Future<Message> shutdownMessage = - FUTURE_MESSAGE(Eq(ShutdownMessage ().GetTypeName()), _, _); + FUTURE_MESSAGE(Eq(ShutdownMessage().GetTypeName()), _, _); Owned<MasterDetector> detector = master.get()->createDetector(); Try<Owned<cluster::Slave>> slave = StartSlave(detector.get()); @@ -2511,6 +2511,180 @@ TEST_F(MasterAuthorizationTest, RetryRegisterAgent) AWAIT_READY(slaveRegisteredMessage); } + +// This test verifies that the agent is shut down by the master if it is +// not authorized to statically reserve any resources. +TEST_F(MasterAuthorizationTest, UnauthorizedToStaticallyReserveResources) +{ + // Set up ACLs so that the agent can (re)register. + ACLs acls; + + { + mesos::ACL::RegisterAgent* acl = acls.add_register_agents(); + acl->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal()); + acl->mutable_agents()->set_type(ACL::Entity::ANY); + } + + // The agent cannot statically reserve resources of any role. + { + mesos::ACL::ReserveResources* acl = acls.add_reserve_resources(); + acl->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal()); + acl->mutable_roles()->set_type(ACL::Entity::NONE); + } + + master::Flags masterFlags = CreateMasterFlags(); + masterFlags.acls = acls; + + Try<Owned<cluster::Master>> master = StartMaster(masterFlags); + ASSERT_SOME(master); + + Future<Message> shutdownMessage = + FUTURE_MESSAGE(Eq(ShutdownMessage().GetTypeName()), _, _); + + Owned<MasterDetector> detector = master.get()->createDetector(); + + slave::Flags agentFlags = CreateSlaveFlags(); + agentFlags.resources = "cpus(foo):1;mem(foo):1"; + + Try<Owned<cluster::Slave>> agent = StartSlave(detector.get(), agentFlags); + ASSERT_SOME(agent); + + AWAIT_READY(shutdownMessage); +} + + +// This test verifies that the agent is shut down by the master if it is +// not authorized to statically reserve certain roles. +TEST_F(MasterAuthorizationTest, UnauthorizedToStaticallyReserveRole) +{ + // Set up ACLs so that the agent can (re)register. + ACLs acls; + + { + mesos::ACL::RegisterAgent* acl = acls.add_register_agents(); + acl->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal()); + acl->mutable_agents()->set_type(ACL::Entity::ANY); + } + + // Only `high-security-principal` (not the principal that the agent in + // this test has) can statically reserve resources for `high-security-role`. + { + mesos::ACL::ReserveResources* acl = acls.add_reserve_resources(); + acl->mutable_principals()->add_values("high-security-principal"); + acl->mutable_roles()->add_values("high-security-role"); + } + + // No other can statically reserve resources of this role. + { + mesos::ACL::ReserveResources* acl = acls.add_reserve_resources(); + acl->mutable_principals()->set_type(ACL::Entity::NONE); + acl->mutable_roles()->add_values("high-security-role"); + } + + master::Flags masterFlags = CreateMasterFlags(); + masterFlags.acls = acls; + + Try<Owned<cluster::Master>> master = StartMaster(masterFlags); + ASSERT_SOME(master); + + Future<Message> shutdownMessage = + FUTURE_MESSAGE(Eq(ShutdownMessage().GetTypeName()), _, _); + + Owned<MasterDetector> detector = master.get()->createDetector(); + + slave::Flags agentFlags = CreateSlaveFlags(); + agentFlags.resources = "cpus(high-security-role):1;mem(high-security-role):1"; + + Try<Owned<cluster::Slave>> agent = StartSlave(detector.get(), agentFlags); + ASSERT_SOME(agent); + + AWAIT_READY(shutdownMessage); +} + + +// This test verifies that the agent successfully registers while statically +// reserving resource for a certain role when permitted in the ACLs. +TEST_F(MasterAuthorizationTest, AuthorizedToStaticallyReserveRole) +{ + // Set up ACLs so that the agent can register. + ACLs acls; + + { + mesos::ACL::RegisterAgent* acl = acls.add_register_agents(); + acl->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal()); + acl->mutable_agents()->set_type(ACL::Entity::ANY); + } + + // The agent principal is allowed to statically reserve resources of + // `high-security-role`. + { + mesos::ACL::ReserveResources* acl = acls.add_reserve_resources(); + acl->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal()); + acl->mutable_roles()->add_values("high-security-role"); + } + + master::Flags masterFlags = CreateMasterFlags(); + masterFlags.acls = acls; + + Try<Owned<cluster::Master>> master = StartMaster(masterFlags); + ASSERT_SOME(master); + + // Start a slave with no static reservations. + slave::Flags agentFlags = CreateSlaveFlags(); + agentFlags.resources = "cpus(high-security-role):1;mem(high-security-role):1"; + + Owned<MasterDetector> detector = master.get()->createDetector(); + + Future<Message> slaveRegisteredMessage = + FUTURE_MESSAGE(Eq(SlaveRegisteredMessage().GetTypeName()), _, _); + + Try<Owned<cluster::Slave>> agent = StartSlave(detector.get(), agentFlags); + ASSERT_SOME(agent); + + AWAIT_READY(slaveRegisteredMessage); +} + + +// This test verifies that the agent is authorized if ACLs don't allow +// any agents to statically reserve resources but the agent only registers +// with unreserved resources. +TEST_F(MasterAuthorizationTest, AuthorizedToRegisterNoStaticReservations) +{ + // Set up ACLs so that the agent can (re)register. + ACLs acls; + + { + mesos::ACL::RegisterAgent* acl = acls.add_register_agents(); + acl->mutable_principals()->add_values(DEFAULT_CREDENTIAL.principal()); + acl->mutable_agents()->set_type(ACL::Entity::ANY); + } + + { + // No agent is allowed to statically reserve resources. + mesos::ACL::ReserveResources* acl = acls.add_reserve_resources(); + acl->mutable_principals()->set_type(ACL::Entity::ANY); + acl->mutable_roles()->set_type(ACL::Entity::NONE); + } + + master::Flags masterFlags = CreateMasterFlags(); + masterFlags.acls = acls; + + Try<Owned<cluster::Master>> master = StartMaster(masterFlags); + ASSERT_SOME(master); + + // Start a slave with no static reservations. + slave::Flags agentFlags = CreateSlaveFlags(); + Owned<MasterDetector> detector = master.get()->createDetector(); + + Future<Message> slaveRegisteredMessage = + FUTURE_MESSAGE(Eq(SlaveRegisteredMessage().GetTypeName()), _, _); + + Try<Owned<cluster::Slave>> agent = StartSlave(detector.get(), agentFlags); + ASSERT_SOME(agent); + + AWAIT_READY(slaveRegisteredMessage); +} + } // namespace tests { } // namespace internal { } // namespace mesos {
