Update security-related TODOs heartbeater.cc: We decided not to implement rotation of the IPKI CA cert. This removes a TODO referring to this unimplemented feature.
master.proto: Remove a TODO about whether the client should specify whether it wants a certificate and CA info. No compelling reason to do this that we've seen so far. token_verifier.cc: Removed a TODO about triggering an out-of-band heartbeat to fetch a new TSK if a client provides an unknown one. Given that we don't start using new TSKs until a long time period has elapsed, it seems unlikely that an expedited heartbeat would increase our chances of fetching it beyond the normal heartbeats that we're always running. token_verifier.h: Removed a TODO about expiring old token keys from the storage. Given each key is only a few hundred bytes, and we rotate once a day, it doesn't seem like a real concern for current use cases. token_signer.h: Remove a TODO about attempting to enforce the constraint of rotation intevals and validity interals. Given the way that the user-facing configuration ended up, it doesn't seem to be a real concern anymore. For all other cases of TODO(ipki) or TODO(security), filed JIRAs and changed the TODO to point to the JIRA. Change-Id: Ibcbef1c1ec75a1e78e6bc892880f6e986508e8f1 Reviewed-on: http://gerrit.cloudera.org:8080/6337 Tested-by: Kudu Jenkins Reviewed-by: Dan Burkert <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/abf772c7 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/abf772c7 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/abf772c7 Branch: refs/heads/master Commit: abf772c7cc5ade8e0fbfc09a8a500f88be5d21c4 Parents: 6db5400 Author: Todd Lipcon <[email protected]> Authored: Thu Mar 9 14:44:47 2017 -0800 Committer: Todd Lipcon <[email protected]> Committed: Fri Mar 10 01:13:22 2017 +0000 ---------------------------------------------------------------------- src/kudu/integration-tests/delete_table-test.cc | 2 +- src/kudu/master/catalog_manager.cc | 4 ++-- src/kudu/master/master.proto | 4 ---- src/kudu/master/master_service.cc | 2 +- src/kudu/rpc/client_negotiation.cc | 12 ++++++------ src/kudu/rpc/messenger.cc | 4 ++-- src/kudu/rpc/messenger.h | 2 +- src/kudu/rpc/server_negotiation.cc | 6 +++--- src/kudu/security/tls_context.cc | 2 +- src/kudu/security/tls_handshake.cc | 2 +- src/kudu/security/token_signer.h | 2 -- src/kudu/security/token_verifier.cc | 2 -- src/kudu/security/token_verifier.h | 11 +++++------ src/kudu/tserver/heartbeater.cc | 3 --- src/kudu/tserver/scanners.cc | 5 +++-- 15 files changed, 26 insertions(+), 37 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/abf772c7/src/kudu/integration-tests/delete_table-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/integration-tests/delete_table-test.cc b/src/kudu/integration-tests/delete_table-test.cc index 0f1a6ff..8f8af2d 100644 --- a/src/kudu/integration-tests/delete_table-test.cc +++ b/src/kudu/integration-tests/delete_table-test.cc @@ -1091,7 +1091,7 @@ TEST_F(DeleteTableTest, TestUnknownTabletsAreNotDeleted) { // won't be able to authenticate to the new master, due to it having a new // CA certificate which the old tserver doesn't trust. // - // TODO(PKI): perhaps this is actually a feature? should we have tablet servers + // TODO(KUDU-65): perhaps this is actually a feature? should we have tablet servers // remember the CA cert persistently so that it's impossible to connect an // old tserver to a new cluster? cluster_->Shutdown(); http://git-wip-us.apache.org/repos/asf/kudu/blob/abf772c7/src/kudu/master/catalog_manager.cc ---------------------------------------------------------------------- diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc index d859507..daf974d 100644 --- a/src/kudu/master/catalog_manager.cc +++ b/src/kudu/master/catalog_manager.cc @@ -845,8 +845,8 @@ void CatalogManager::VisitTablesAndTabletsTask() { CHECK_OK(VisitTablesAndTabletsUnlocked()); } - // TODO(PKI): this should not be done in case of external PKI. - // TODO(PKI): should be there a flag to reset already existing CA info? + // TODO(KUDU-1920): this should not be done in case of external PKI. + // TODO(KUDU-1919): some kind of tool to rotate the IPKI CA LOG(INFO) << "Loading CA info into memory..."; LOG_SLOW_EXECUTION(WARNING, 1000, LogPrefix() + "Loading CA info into memory") { http://git-wip-us.apache.org/repos/asf/kudu/blob/abf772c7/src/kudu/master/master.proto ---------------------------------------------------------------------- diff --git a/src/kudu/master/master.proto b/src/kudu/master/master.proto index 79de60f..1a61320 100644 --- a/src/kudu/master/master.proto +++ b/src/kudu/master/master.proto @@ -585,10 +585,6 @@ message GetTableSchemaResponsePB { } message ConnectToMasterRequestPB { - // TODO(PKI): should the client specify whether it wants an authn token and CA - // information or not? - // Or should the server always send this info back, even if the client already has - // what it needs? } message ConnectToMasterResponsePB { http://git-wip-us.apache.org/repos/asf/kudu/blob/abf772c7/src/kudu/master/master_service.cc ---------------------------------------------------------------------- diff --git a/src/kudu/master/master_service.cc b/src/kudu/master/master_service.cc index 7ba1bcf..58628c5 100644 --- a/src/kudu/master/master_service.cc +++ b/src/kudu/master/master_service.cc @@ -399,7 +399,7 @@ void MasterServiceImpl::ConnectToMaster(const ConnectToMasterRequestPB* /*req*/, resp->set_role(role); if (l.leader_status().ok()) { - // TODO(PKI): it seems there is some window when 'role' is LEADER but + // TODO(KUDU-1924): it seems there is some window when 'role' is LEADER but // in fact we aren't done initializing (and we don't have a CA cert). // In that case, if we respond with the 'LEADER' role to a client, but // don't pass back the CA cert, then the client won't be able to trust http://git-wip-us.apache.org/repos/asf/kudu/blob/abf772c7/src/kudu/rpc/client_negotiation.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/client_negotiation.cc b/src/kudu/rpc/client_negotiation.cc index eb282fa..e7951df 100644 --- a/src/kudu/rpc/client_negotiation.cc +++ b/src/kudu/rpc/client_negotiation.cc @@ -168,7 +168,7 @@ Status ClientNegotiation::Negotiate() { } // Step 3: if both ends support TLS, do a TLS handshake. - // TODO(PKI): allow the client to require TLS. + // TODO(KUDU-1921): allow the client to require TLS. if (ContainsKey(server_features_, TLS)) { RETURN_NOT_OK(tls_context_->InitiateHandshake(security::TlsHandshakeType::CLIENT, &tls_handshake_)); @@ -266,8 +266,8 @@ Status ClientNegotiation::SendConnectionHeader() { Status ClientNegotiation::InitSaslClient() { RETURN_NOT_OK(SaslInit()); - // TODO(unknown): Support security flags. - unsigned secflags = 0; + // TODO(KUDU-1922): consider setting SASL_SUCCESS_DATA + unsigned flags = 0; sasl_conn_t* sasl_conn = nullptr; RETURN_NOT_OK_PREPEND(WrapSaslCall(nullptr /* no conn */, [&]() { @@ -277,7 +277,7 @@ Status ClientNegotiation::InitSaslClient() { nullptr, // Local and remote IP address strings. (we don't use nullptr, // any mechanisms which require this info.) &callbacks_[0], // Connection-specific callbacks. - secflags, // Security flags. + flags, &sasl_conn); }), "Unable to create new SASL client"); sasl_conn_.reset(sasl_conn); @@ -307,7 +307,7 @@ Status ClientNegotiation::SendNegotiate() { msg.add_authn_types()->mutable_certificate(); } if (authn_token_ && tls_context_->has_trusted_cert()) { - // TODO(PKI): check that the authn token is not expired. Can this be done + // TODO(KUDU-1924): check that the authn token is not expired. Can this be done // reliably on clients? msg.add_authn_types()->mutable_token(); } @@ -414,7 +414,7 @@ Status ClientNegotiation::HandleNegotiate(const NegotiatePB& response) { return Status::NotAuthorized(msg); } - // TODO(PKI): allow the client to require authentication. + // TODO(KUDU-1921): allow the client to require authentication. if (ContainsKey(common_mechs, SaslMechanism::GSSAPI)) { negotiated_mech_ = SaslMechanism::GSSAPI; } else { http://git-wip-us.apache.org/repos/asf/kudu/blob/abf772c7/src/kudu/rpc/messenger.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/messenger.cc b/src/kudu/rpc/messenger.cc index 9f9574a..09ccbbe 100644 --- a/src/kudu/rpc/messenger.cc +++ b/src/kudu/rpc/messenger.cc @@ -249,8 +249,8 @@ Status MessengerBuilder::Build(shared_ptr<Messenger> *msgr) { if (!FLAGS_rpc_certificate_file.empty()) { CHECK(!FLAGS_rpc_private_key_file.empty()); CHECK(!FLAGS_rpc_ca_certificate_file.empty()); - // TODO(PKI): should we try and enforce that the server UUID and/or - // hostname is in the subject or alt names of the cert? + // TODO(KUDU-1920): should we try and enforce that the server + // is in the subject or alt names of the cert? RETURN_NOT_OK(tls_context->LoadCertificateAuthority(FLAGS_rpc_ca_certificate_file)); RETURN_NOT_OK(tls_context->LoadCertificateAndKey(FLAGS_rpc_certificate_file, FLAGS_rpc_private_key_file)); http://git-wip-us.apache.org/repos/asf/kudu/blob/abf772c7/src/kudu/rpc/messenger.h ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/messenger.h b/src/kudu/rpc/messenger.h index 2ada341..b88f7a1 100644 --- a/src/kudu/rpc/messenger.h +++ b/src/kudu/rpc/messenger.h @@ -268,7 +268,7 @@ class Messenger { // Whether to require authentication and encryption on the connections managed // by this messenger. - // TODO(PKI): scope these to individual proxies, so that messengers can be + // TODO(KUDU-1928): scope these to individual proxies, so that messengers can be // reused by different clients. RpcAuthentication authentication_; RpcEncryption encryption_; http://git-wip-us.apache.org/repos/asf/kudu/blob/abf772c7/src/kudu/rpc/server_negotiation.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/server_negotiation.cc b/src/kudu/rpc/server_negotiation.cc index ab31c0b..3f9cc6e 100644 --- a/src/kudu/rpc/server_negotiation.cc +++ b/src/kudu/rpc/server_negotiation.cc @@ -377,7 +377,7 @@ Status ServerNegotiation::HandleNegotiate(const NegotiatePB& request) { tls_context_->has_signed_cert()) { // If the client supports it and we are locally configured with TLS and have // a CA-signed cert, choose cert authn. - // TODO(PKI): consider adding the fingerprint of the CA cert which signed + // TODO(KUDU-1924): consider adding the fingerprint of the CA cert which signed // the client's cert to the authentication message. negotiated_authn_ = AuthenticationType::CERTIFICATE; } else if (ContainsKey(authn_types, AuthenticationType::TOKEN) && @@ -385,7 +385,7 @@ Status ServerNegotiation::HandleNegotiate(const NegotiatePB& request) { tls_context_->has_signed_cert()) { // If the client supports it, we have a TSK to verify the client's token, // and we have a signed-cert so the client can verify us, choose token authn. - // TODO(PKI): consider adding the TSK sequence number to the authentication + // TODO(KUDU-1924): consider adding the TSK sequence number to the authentication // message. negotiated_authn_ = AuthenticationType::TOKEN; } else { @@ -548,7 +548,7 @@ Status ServerNegotiation::AuthenticateByToken(faststring* recv_buf) { Status s = Status::NotAuthorized("TOKEN_EXCHANGE message must include an authentication token"); } - // TODO(PKI): propagate the specific token verification failure back to the client, + // TODO(KUDU-1924): propagate the specific token verification failure back to the client, // so it knows how to intelligently retry. security::TokenPB token; auto verification_result = token_verifier_->VerifyTokenSignature(pb.authn_token(), &token); http://git-wip-us.apache.org/repos/asf/kudu/blob/abf772c7/src/kudu/security/tls_context.cc ---------------------------------------------------------------------- diff --git a/src/kudu/security/tls_context.cc b/src/kudu/security/tls_context.cc index 17b7f69..b50fdcf 100644 --- a/src/kudu/security/tls_context.cc +++ b/src/kudu/security/tls_context.cc @@ -164,7 +164,7 @@ Status TlsContext::Init() { #endif #endif - // TODO(PKI): is it possible to disable client-side renegotiation? it seems there + // TODO(KUDU-1926): is it possible to disable client-side renegotiation? it seems there // have been various CVEs related to this feature that we don't need. return Status::OK(); } http://git-wip-us.apache.org/repos/asf/kudu/blob/abf772c7/src/kudu/security/tls_handshake.cc ---------------------------------------------------------------------- diff --git a/src/kudu/security/tls_handshake.cc b/src/kudu/security/tls_handshake.cc index f57f24f..324e42c 100644 --- a/src/kudu/security/tls_handshake.cc +++ b/src/kudu/security/tls_handshake.cc @@ -141,7 +141,7 @@ Status TlsHandshake::Verify(const Socket& socket) const { return Status::OK(); } - // TODO(PKI): KUDU-1886: Do hostname verification. + // TODO(KUDU-1886): Do hostname verification. /* TRACE("Verifying peer cert"); http://git-wip-us.apache.org/repos/asf/kudu/blob/abf772c7/src/kudu/security/token_signer.h ---------------------------------------------------------------------- diff --git a/src/kudu/security/token_signer.h b/src/kudu/security/token_signer.h index b7e637f..768ff53 100644 --- a/src/kudu/security/token_signer.h +++ b/src/kudu/security/token_signer.h @@ -124,8 +124,6 @@ class TokenVerifier; // period longer than or equal to 3 days, without risking that the // signing/verification key would expire before the token. // -// TODO(PKI): should we try to enforce this constraint in code? -// // NOTE: One other result of the above is that the first key (Key 1) is actually // active for longer than the rest. This has some potential security // implications, so it's worth considering rolling twice at startup. http://git-wip-us.apache.org/repos/asf/kudu/blob/abf772c7/src/kudu/security/token_verifier.cc ---------------------------------------------------------------------- diff --git a/src/kudu/security/token_verifier.cc b/src/kudu/security/token_verifier.cc index 37e72e8..8f3d5ad 100644 --- a/src/kudu/security/token_verifier.cc +++ b/src/kudu/security/token_verifier.cc @@ -127,8 +127,6 @@ 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) { http://git-wip-us.apache.org/repos/asf/kudu/blob/abf772c7/src/kudu/security/token_verifier.h ---------------------------------------------------------------------- diff --git a/src/kudu/security/token_verifier.h b/src/kudu/security/token_verifier.h index 2f779f8..be7936c 100644 --- a/src/kudu/security/token_verifier.h +++ b/src/kudu/security/token_verifier.h @@ -48,6 +48,11 @@ enum class VerificationResult; // and is not yet expired. Any business rules around authorization or // authentication are left up to callers. // +// NOTE: old tokens are never removed from the underlying storage of this +// class. The assumption is that tokens rotate so infreqeuently that this +// slow leak is not worrisome. If this class is adopted for any use cases +// with frequent rotation, GC of expired tokens will need to be added. +// // This class is thread-safe. class TokenVerifier { public: @@ -77,12 +82,6 @@ class TokenVerifier { VerificationResult VerifyTokenSignature(const SignedTokenPB& signed_token, TokenPB* token) const; - // TODO(PKI): should expire out old key versions at some point. eg only - // keep old key versions until their expiration is an hour or two in the past? - // Not sure where we'll call this from, and likely the "slow leak" isn't - // critical for first implementation. - // void ExpireOldKeys(); - private: typedef std::map<int64_t, std::unique_ptr<TokenSigningPublicKey>> KeysMap; http://git-wip-us.apache.org/repos/asf/kudu/blob/abf772c7/src/kudu/tserver/heartbeater.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tserver/heartbeater.cc b/src/kudu/tserver/heartbeater.cc index 093399e..bfbd246 100644 --- a/src/kudu/tserver/heartbeater.cc +++ b/src/kudu/tserver/heartbeater.cc @@ -372,9 +372,6 @@ Status Heartbeater::Thread::DoHeartbeat() { VLOG(1) << "Sending a CSR to the master in the next heartbeat"; } - // TODO(PKI): send the version number of the latest CA cert which we know about. - // The response should include new CA certs. - // Send the most recently known TSK sequence number so that the master can // send us knew ones if they exist. req.set_latest_tsk_seq_num(server_->token_verifier().GetMaxKnownKeySequenceNumber()); http://git-wip-us.apache.org/repos/asf/kudu/blob/abf772c7/src/kudu/tserver/scanners.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tserver/scanners.cc b/src/kudu/tserver/scanners.cc index 9786c65..e8e79d5 100644 --- a/src/kudu/tserver/scanners.cc +++ b/src/kudu/tserver/scanners.cc @@ -108,9 +108,10 @@ void ScannerManager::NewScanner(const scoped_refptr<TabletPeer>& tablet_peer, // Keep trying to generate a unique ID until we get one. bool success = false; while (!success) { - // TODO(security): are these UUIDs predictable? If so, we should + // TODO(KUDU-1918): are these UUIDs predictable? If so, we should // probably generate random numbers instead, since we can safely - // just retry until we avoid a collision. + // just retry until we avoid a collision. Alternatively we could + // verify that the requestor userid does not change mid-scan. string id = oid_generator_.Next(); scanner->reset(new Scanner(id, tablet_peer, requestor_string, metrics_.get()));
