server_negotiation: improve error handling Unlike in the client negotiator, the server negotiator should send an error response back to the client on authentication failure. We were missing this in a few cases, notably around token and cert authentication failures. This also adds a new token-specific rpc error code which is used when the client should discard its token and request a new one from the master.
Change-Id: I64f9c0f59aa608bf5078d65883a7d9a6fb186c04 Reviewed-on: http://gerrit.cloudera.org:8080/6154 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/9591957b Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/9591957b Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/9591957b Branch: refs/heads/master Commit: 9591957b80f1a5c4f7b785dbcd051100f8ee5b85 Parents: 815fd0a Author: Dan Burkert <[email protected]> Authored: Fri Feb 24 17:06:26 2017 -0800 Committer: Dan Burkert <[email protected]> Committed: Wed Mar 1 05:58:49 2017 +0000 ---------------------------------------------------------------------- src/kudu/rpc/negotiation-test.cc | 3 +- src/kudu/rpc/rpc_header.proto | 8 +++++ src/kudu/rpc/server_negotiation.cc | 55 ++++++++++++++++++++++---------- src/kudu/security/token_verifier.cc | 24 ++++++++++++++ src/kudu/security/token_verifier.h | 1 + 5 files changed, 73 insertions(+), 18 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/9591957b/src/kudu/rpc/negotiation-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/negotiation-test.cc b/src/kudu/rpc/negotiation-test.cc index b713339..5265e28 100644 --- a/src/kudu/rpc/negotiation-test.cc +++ b/src/kudu/rpc/negotiation-test.cc @@ -744,7 +744,8 @@ TEST_F(TestNegotiation, TestGSSAPIInvalidNegotiation) { [](const Status& s, ClientNegotiation& client) { CHECK(s.IsNotAuthorized()); #ifndef KRB5_VERSION_LE_1_10 - CHECK_EQ(s.message().ToString(), "No Kerberos credentials available"); + ASSERT_STR_MATCHES(s.ToString(), + "Not authorized: No Kerberos credentials available.*"); #endif })); http://git-wip-us.apache.org/repos/asf/kudu/blob/9591957b/src/kudu/rpc/rpc_header.proto ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/rpc_header.proto b/src/kudu/rpc/rpc_header.proto index b295fed..ea2dd66 100644 --- a/src/kudu/rpc/rpc_header.proto +++ b/src/kudu/rpc/rpc_header.proto @@ -291,6 +291,10 @@ message ErrorStatusPB { // longer cached. It's unknown whether the request was executed or not. ERROR_REQUEST_STALE = 6; + // The server is not able to complete the connection or request at this + // time. The client may try again later. + ERROR_UNAVAILABLE = 7; + // FATAL_* errors indicate that the client should shut down the connection. //------------------------------------------------------------ // The RPC server is already shutting down. @@ -303,6 +307,10 @@ message ErrorStatusPB { FATAL_VERSION_MISMATCH = 14; // Auth failed. FATAL_UNAUTHORIZED = 15; + + // The authentication token is invalid or expired; + // the client should obtain a new one. + FATAL_INVALID_AUTHENTICATION_TOKEN = 16; } required string message = 1; http://git-wip-us.apache.org/repos/asf/kudu/blob/9591957b/src/kudu/rpc/server_negotiation.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/server_negotiation.cc b/src/kudu/rpc/server_negotiation.cc index 2549132..2901795 100644 --- a/src/kudu/rpc/server_negotiation.cc +++ b/src/kudu/rpc/server_negotiation.cc @@ -92,8 +92,7 @@ ServerNegotiation::ServerNegotiation(unique_ptr<Socket> socket, } Status ServerNegotiation::EnablePlain() { - RETURN_NOT_OK(helper_.EnablePlain()); - return Status::OK(); + return helper_.EnablePlain(); } Status ServerNegotiation::EnableGSSAPI() { @@ -320,8 +319,10 @@ Status ServerNegotiation::InitSaslServer() { Status ServerNegotiation::HandleNegotiate(const NegotiatePB& request) { if (request.step() != NegotiatePB::NEGOTIATE) { - return Status::NotAuthorized("expected NEGOTIATE step", - NegotiatePB::NegotiateStep_Name(request.step())); + Status s = Status::NotAuthorized("expected NEGOTIATE step", + NegotiatePB::NegotiateStep_Name(request.step())); + RETURN_NOT_OK(SendError(ErrorStatusPB::FATAL_UNAUTHORIZED, s)); + return s; } TRACE("Received NEGOTIATE request from client"); @@ -550,29 +551,47 @@ Status ServerNegotiation::AuthenticateByToken(faststring* recv_buf) { // TODO(PKI): propagate the specific token verification failure back to the client, // so it knows how to intelligently retry. security::TokenPB token; - switch (token_verifier_->VerifyTokenSignature(pb.authn_token(), &token)) { + auto verification_result = token_verifier_->VerifyTokenSignature(pb.authn_token(), &token); + switch (verification_result) { case security::VerificationResult::VALID: break; + case security::VerificationResult::INVALID_TOKEN: - return Status::NotAuthorized("invalid authentication token"); case security::VerificationResult::INVALID_SIGNATURE: - return Status::NotAuthorized("invalid authentication token signature"); case security::VerificationResult::EXPIRED_TOKEN: - return Status::NotAuthorized("authentication token expired"); - case security::VerificationResult::EXPIRED_SIGNING_KEY: - return Status::NotAuthorized("authentication token signing key expired"); - case security::VerificationResult::UNKNOWN_SIGNING_KEY: - return Status::NotAuthorized("authentication token signed with unknown key"); - case security::VerificationResult::INCOMPATIBLE_FEATURE: - return Status::NotAuthorized("authentication token uses incompatible feature"); + case security::VerificationResult::EXPIRED_SIGNING_KEY: { + // These errors indicate the client should get a new token and try again. + Status s = Status::NotAuthorized(VerificationResultToString(verification_result)); + RETURN_NOT_OK(SendError(ErrorStatusPB::FATAL_INVALID_AUTHENTICATION_TOKEN, s)); + return s; + } + + case security::VerificationResult::UNKNOWN_SIGNING_KEY: { + // The server doesn't recognize the signing key. This indicates that the + // server has not been updated with the most recent TSKs, so tell the + // client to try again later. + Status s = Status::NotAuthorized(VerificationResultToString(verification_result)); + RETURN_NOT_OK(SendError(ErrorStatusPB::ERROR_UNAVAILABLE, s)); + return s; + } + case security::VerificationResult::INCOMPATIBLE_FEATURE: { + Status s = Status::NotAuthorized(VerificationResultToString(verification_result)); + // These error types aren't recoverable by having the client get a new token. + RETURN_NOT_OK(SendError(ErrorStatusPB::FATAL_UNAUTHORIZED, s)); + return s; + } } if (!token.has_authn()) { - return Status::NotAuthorized("non-authentication token presented for authentication"); + Status s = Status::NotAuthorized("non-authentication token presented for authentication"); + RETURN_NOT_OK(SendError(ErrorStatusPB::FATAL_UNAUTHORIZED, s)); + return s; } if (!token.authn().has_username()) { // This is a runtime error because there should be no way a client could // get a signed authn token without a subject. - return Status::RuntimeError("authentication token has no username"); + Status s = Status::RuntimeError("authentication token has no username"); + RETURN_NOT_OK(SendError(ErrorStatusPB::FATAL_INVALID_AUTHENTICATION_TOKEN, s)); + return s; } authenticated_user_.SetAuthenticatedByToken(token.authn().username()); @@ -595,7 +614,9 @@ Status ServerNegotiation::AuthenticateByCertificate() { boost::optional<string> principal = cert.KuduKerberosPrincipal(); if (!user_id) { - return Status::NotAuthorized("did not find expected X509 userId extension in cert"); + Status s = Status::NotAuthorized("did not find expected X509 userId extension in cert"); + RETURN_NOT_OK(SendError(ErrorStatusPB::FATAL_INVALID_AUTHENTICATION_TOKEN, s)); + return s; } authenticated_user_.SetAuthenticatedByClientCert(*user_id, std::move(principal)); http://git-wip-us.apache.org/repos/asf/kudu/blob/9591957b/src/kudu/security/token_verifier.cc ---------------------------------------------------------------------- diff --git a/src/kudu/security/token_verifier.cc b/src/kudu/security/token_verifier.cc index 2f6ba03..37e72e8 100644 --- a/src/kudu/security/token_verifier.cc +++ b/src/kudu/security/token_verifier.cc @@ -27,6 +27,7 @@ #include "kudu/security/token.pb.h" #include "kudu/security/token_signing_key.h" #include "kudu/util/locks.h" +#include "kudu/util/logging.h" using std::lock_guard; using std::string; @@ -116,6 +117,8 @@ VerificationResult TokenVerifier::VerifyTokenSignature(const SignedTokenPB& sign for (auto flag : token->incompatible_features()) { if (!TokenPB::Feature_IsValid(flag)) { + KLOG_EVERY_N_SECS(WARNING, 60) << "received authentication token with unknown feature; " + "server needs to be updated"; return VerificationResult::INCOMPATIBLE_FEATURE; } } @@ -124,6 +127,8 @@ VerificationResult TokenVerifier::VerifyTokenSignature(const SignedTokenPB& sign shared_lock<RWMutex> l(lock_); auto* tsk = FindPointeeOrNull(keys_by_seq_, signed_token.signing_key_seq_num()); if (!tsk) { + // TODO(pki): should this notify the heartbeater to send out a heartbeat + // immediately, since we are not up to date with TSKs? return VerificationResult::UNKNOWN_SIGNING_KEY; } if (tsk->pb().expire_unix_epoch_seconds() < now) { @@ -137,6 +142,25 @@ VerificationResult TokenVerifier::VerifyTokenSignature(const SignedTokenPB& sign return VerificationResult::VALID; } +const char* VerificationResultToString(VerificationResult r) { + switch (r) { + case security::VerificationResult::VALID: + return "valid"; + case security::VerificationResult::INVALID_TOKEN: + return "invalid authentication token"; + case security::VerificationResult::INVALID_SIGNATURE: + return "invalid authentication token signature"; + case security::VerificationResult::EXPIRED_TOKEN: + return "authentication token expired"; + case security::VerificationResult::EXPIRED_SIGNING_KEY: + return "authentication token signing key expired"; + case security::VerificationResult::UNKNOWN_SIGNING_KEY: + return "authentication token signed with unknown key"; + case security::VerificationResult::INCOMPATIBLE_FEATURE: + return "authentication token uses incompatible feature"; + } +} + } // namespace security } // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/9591957b/src/kudu/security/token_verifier.h ---------------------------------------------------------------------- diff --git a/src/kudu/security/token_verifier.h b/src/kudu/security/token_verifier.h index 2879f8c..2f779f8 100644 --- a/src/kudu/security/token_verifier.h +++ b/src/kudu/security/token_verifier.h @@ -114,6 +114,7 @@ enum class VerificationResult { INCOMPATIBLE_FEATURE }; +const char* VerificationResultToString(VerificationResult r); } // namespace security } // namespace kudu
