You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by to...@apache.org on 2017/03/10 05:04:12 UTC
[2/3] kudu git commit: Update security-related TODOs
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 <da...@apache.org>
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 <to...@apache.org>
Authored: Thu Mar 9 14:44:47 2017 -0800
Committer: Todd Lipcon <to...@apache.org>
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()));