Repository: mesos Updated Branches: refs/heads/master 7026fc7b7 -> 70725089e
Keep track of the principals for authenticated pids in Master and verify FramworkInfo::principal during (re-)registration. Review: https://reviews.apache.org/r/21787 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/70725089 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/70725089 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/70725089 Branch: refs/heads/master Commit: 70725089e9c20c2d0a85ad9b24bb018a01181ae3 Parents: 7026fc7 Author: Jiang Yan Xu <y...@jxu.me> Authored: Wed May 21 14:54:07 2014 -0700 Committer: Jiang Yan Xu <y...@jxu.me> Committed: Tue May 27 08:13:51 2014 -0700 ---------------------------------------------------------------------- include/mesos/mesos.proto | 5 ++ src/master/master.cpp | 83 +++++++++++++------ src/master/master.hpp | 4 +- src/tests/authentication_tests.cpp | 141 ++++++++++++++++++++++++++++++++ src/tests/mesos.hpp | 13 +-- 5 files changed, 214 insertions(+), 32 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/70725089/include/mesos/mesos.proto ---------------------------------------------------------------------- diff --git a/include/mesos/mesos.proto b/include/mesos/mesos.proto index ce780ca..82388e1 100644 --- a/include/mesos/mesos.proto +++ b/include/mesos/mesos.proto @@ -111,6 +111,10 @@ message ContainerID { * decisions, depending on the allocation policy being used. * If the hostname field is set to an empty string Mesos will * automagically set it to the current hostname. + * The principal field should match the credential the framework uses + * in authentication. This field is used for framework API rate + * exporting and limiting and should be set even if authentication is + * not enabled if these features are desired. */ message FrameworkInfo { required string user = 1; @@ -120,6 +124,7 @@ message FrameworkInfo { optional bool checkpoint = 5 [default = false]; optional string role = 6 [default = "*"]; optional string hostname = 7; + optional string principal = 8; } http://git-wip-us.apache.org/repos/asf/mesos/blob/70725089/src/master/master.cpp ---------------------------------------------------------------------- diff --git a/src/master/master.cpp b/src/master/master.cpp index e5d8edf..ae738bb 100644 --- a/src/master/master.cpp +++ b/src/master/master.cpp @@ -980,17 +980,34 @@ void Master::registerFramework( return; } - if (flags.authenticate_frameworks && !authenticated.contains(from)) { - // This could happen if another authentication request came - // through before we are here or if a framework tried to register - // without authentication. - LOG(WARNING) << "Refusing registration of framework at " << from - << " because it is not authenticated"; - FrameworkErrorMessage message; - message.set_message("Framework at " + stringify(from) + - " is not authenticated."); - send(from, message); - return; + if (flags.authenticate_frameworks) { + if (!authenticated.contains(from)) { + // This could happen if another authentication request came + // through before we are here or if a framework tried to register + // without authentication. + LOG(WARNING) << "Refusing registration of framework at " << from + << " because it is not authenticated"; + FrameworkErrorMessage message; + message.set_message("Framework at " + stringify(from) + + " is not authenticated."); + send(from, message); + return; + } else if (frameworkInfo.has_principal() && + frameworkInfo.principal() != authenticated[from]) { + LOG(WARNING) << "Refusing registration of framework at " << from + << " because its principal '" << frameworkInfo.principal() + << "' does not match what it used in authentication: '" + << authenticated[from] << "'"; + FrameworkErrorMessage message; + message.set_message("Framework principal " + frameworkInfo.principal() + + " does not match what was used in authentication: " + + authenticated[from]); + send(from, message); + return; + } else if (!frameworkInfo.has_principal()) { + LOG(WARNING) << "Framework at " << from + << " does not specify principal in its FrameworkInfo"; + } } if (!roles.contains(frameworkInfo.role())) { @@ -1064,17 +1081,34 @@ void Master::reregisterFramework( return; } - if (flags.authenticate_frameworks && !authenticated.contains(from)) { - // This could happen if another authentication request came - // through before we are here or if a framework tried to - // re-register without authentication. - LOG(WARNING) << "Refusing re-registration of framework at " << from - << " because it is not authenticated"; - FrameworkErrorMessage message; - message.set_message("Framework '" + frameworkInfo.id().value() + "' at " + - stringify(from) + " is not authenticated."); - send(from, message); - return; + if (flags.authenticate_frameworks) { + if (!authenticated.contains(from)) { + // This could happen if another authentication request came + // through before we are here or if a framework tried to + // re-register without authentication. + LOG(WARNING) << "Refusing re-registration of framework at " << from + << " because it is not authenticated"; + FrameworkErrorMessage message; + message.set_message("Framework '" + frameworkInfo.id().value() + "' at " + + stringify(from) + " is not authenticated."); + send(from, message); + return; + } else if (frameworkInfo.has_principal() && + frameworkInfo.principal() != authenticated[from]) { + LOG(WARNING) << "Refusing re-registration of framework at " << from + << " because its principal '" << frameworkInfo.principal() + << "' does not match what it used in authentication: '" + << authenticated[from] << "'"; + FrameworkErrorMessage message; + message.set_message("Framework principal " + frameworkInfo.principal() + + " does not match what was used in authentication: " + + authenticated[from]); + send(from, message); + return; + } else if (!frameworkInfo.has_principal()) { + LOG(WARNING) << "Framework at " << from + << " does not specify principal in its FrameworkInfo"; + } } if (!roles.contains(frameworkInfo.role())) { @@ -2899,10 +2933,11 @@ void Master::_authenticate( promise->fail(error); } else { - LOG(INFO) << "Successfully authenticated " << pid; + LOG(INFO) << "Successfully authenticated principal '" << future.get().get() + << "' at " << pid; promise->set(Nothing()); - authenticated.insert(pid); + authenticated.put(pid, future.get().get()); } authenticators.erase(pid); http://git-wip-us.apache.org/repos/asf/mesos/blob/70725089/src/master/master.hpp ---------------------------------------------------------------------- diff --git a/src/master/master.hpp b/src/master/master.hpp index 99c7063..847d7c7 100644 --- a/src/master/master.hpp +++ b/src/master/master.hpp @@ -456,8 +456,8 @@ private: hashmap<process::UPID, process::Owned<sasl::Authenticator> > authenticators; - // Authenticated frameworks/slaves keyed by PID. - hashset<process::UPID> authenticated; + // Principals of authenticated frameworks/slaves keyed by PID. + hashmap<process::UPID, std::string> authenticated; Option<process::Owned<Authorizer> > authorizer; http://git-wip-us.apache.org/repos/asf/mesos/blob/70725089/src/tests/authentication_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/authentication_tests.cpp b/src/tests/authentication_tests.cpp index b60e595..5cf2da4 100644 --- a/src/tests/authentication_tests.cpp +++ b/src/tests/authentication_tests.cpp @@ -157,6 +157,76 @@ TEST_F(AuthenticationTest, DisableSlaveAuthentication) } +// This test verifies that an authenticated framework is denied +// registration by the master if it uses a different +// FrameworkInfo::principal. +TEST_F(AuthenticationTest, MismatchedFrameworkInfoPrincipal) +{ + Try<PID<Master> > master = StartMaster(); + ASSERT_SOME(master); + + MockScheduler sched; + FrameworkInfo frameworkInfo; // Bug in gcc 4.1.*, must assign on next line. + frameworkInfo = DEFAULT_FRAMEWORK_INFO; + frameworkInfo.set_principal("mismatched-principal"); + + MesosSchedulerDriver driver( + &sched, + frameworkInfo, + master.get(), + DEFAULT_CREDENTIAL); + + Future<Nothing> error; + EXPECT_CALL(sched, error(&driver, _)) + .WillOnce(FutureSatisfy(&error)); + + driver.start(); + + // Scheduler should get error message from the master. + AWAIT_READY(error); + + driver.stop(); + driver.join(); + + Shutdown(); +} + + +// This test verifies that if a Framework successfully authenticates +// but does not set FrameworkInfo::principal, it is allowed to +// register. +TEST_F(AuthenticationTest, UnspecifiedFrameworkInfoPrincipal) +{ + Try<PID<Master> > master = StartMaster(); + ASSERT_SOME(master); + + MockScheduler sched; + FrameworkInfo frameworkInfo; // Bug in gcc 4.1.*, must assign on next line. + frameworkInfo = DEFAULT_FRAMEWORK_INFO; + frameworkInfo.clear_principal(); + + MesosSchedulerDriver driver( + &sched, + frameworkInfo, + master.get(), + DEFAULT_CREDENTIAL); + + Future<Nothing> registered; + EXPECT_CALL(sched, registered(&driver, _, _)) + .WillOnce(FutureSatisfy(®istered)); + + driver.start(); + + // Scheduler should be able to get registered. + AWAIT_READY(registered); + + driver.stop(); + driver.join(); + + Shutdown(); +} + + // This test verifies that when the master is started with // authentication disabled, it registers authenticated frameworks. TEST_F(AuthenticationTest, AuthenticatedFramework) @@ -690,3 +760,74 @@ TEST_F(AuthenticationTest, SchedulerFailover) Shutdown(); } + + +// This test verifies that a scheduler's re-registration will be +// rejected if it specifies a principal different from what's used in +// authentication. +TEST_F(AuthenticationTest, RejectedSchedulerFailover) +{ + Try<PID<Master> > master = StartMaster(); + ASSERT_SOME(master); + + // Launch the first scheduler. + MockScheduler sched1; + Owned<StandaloneMasterDetector> detector( + new StandaloneMasterDetector(master.get())); + TestingMesosSchedulerDriver driver1(&sched1, detector.get()); + + Future<FrameworkID> frameworkId; + EXPECT_CALL(sched1, registered(&driver1, _, _)) + .WillOnce(FutureArg<1>(&frameworkId)); + + driver1.start(); + + AWAIT_READY(frameworkId); + + // Drop the AuthenticationStepMessage from authenticator + // to stop authentication from succeeding. + Future<AuthenticationStepMessage> authenticationStepMessage = + DROP_PROTOBUF(AuthenticationStepMessage(), _, _); + + EXPECT_CALL(sched1, disconnected(&driver1)); + + // Appoint a new master and inform the scheduler about it. + detector->appoint(master.get()); + + AWAIT_READY(authenticationStepMessage); + + // Attempt to failover to scheduler 2 while scheduler 1 is still + // up. We use the framework id recorded from scheduler 1 but change + // the principal in FrameworInfo and it will be denied. Scheduler 1 + // will not be asked to shutdown. + MockScheduler sched2; + + FrameworkInfo framework2; // Bug in gcc 4.1.*, must assign on next line. + framework2 = DEFAULT_FRAMEWORK_INFO; + framework2.mutable_id()->MergeFrom(frameworkId.get()); + framework2.set_principal("mismatched-principal"); + + MesosSchedulerDriver driver2( + &sched2, framework2, master.get(), DEFAULT_CREDENTIAL); + + Future<Nothing> sched1Error; + EXPECT_CALL(sched1, error(&driver1, _)) + .Times(0); + + Future<Nothing> sched2Error; + EXPECT_CALL(sched2, error(&driver2, _)) + .WillOnce(FutureSatisfy(&sched2Error)); + + driver2.start(); + + // Scheduler 2 should get error message from the master. + AWAIT_READY(sched2Error); + + driver2.stop(); + driver2.join(); + + driver1.stop(); + driver1.join(); + + Shutdown(); +} http://git-wip-us.apache.org/repos/asf/mesos/blob/70725089/src/tests/mesos.hpp ---------------------------------------------------------------------- diff --git a/src/tests/mesos.hpp b/src/tests/mesos.hpp index a1a7d79..c31199a 100644 --- a/src/tests/mesos.hpp +++ b/src/tests/mesos.hpp @@ -263,12 +263,6 @@ protected: executor; }) -#define DEFAULT_FRAMEWORK_INFO \ - ({ FrameworkInfo framework; \ - framework.set_name("default"); \ - framework; }) - - #define DEFAULT_CREDENTIAL \ ({ Credential credential; \ credential.set_principal("test-principal"); \ @@ -276,6 +270,13 @@ protected: credential; }) +#define DEFAULT_FRAMEWORK_INFO \ + ({ FrameworkInfo framework; \ + framework.set_name("default"); \ + framework.set_principal(DEFAULT_CREDENTIAL.principal()); \ + framework; }) + + #define DEFAULT_EXECUTOR_ID \ DEFAULT_EXECUTOR_INFO.executor_id()