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/02/01 02:54:17 UTC
[3/3] kudu git commit: [security] avoid crashing when importing
invalid TSKs
[security] avoid crashing when importing invalid TSKs
Kudu typically tries to avoid crashing when validating data from
external processes. In this case the data is from another Kudu server,
but it's still better not to crash.
Change-Id: I51dece30df0ec612417917f6855d57432fe2f7ec
Reviewed-on: http://gerrit.cloudera.org:8080/5843
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/e67289ed
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/e67289ed
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/e67289ed
Branch: refs/heads/master
Commit: e67289edfbca4b1d189c1b42406a5a8bc5602fc5
Parents: e6e3029
Author: Dan Burkert <da...@apache.org>
Authored: Tue Jan 31 16:20:41 2017 -0800
Committer: Alexey Serbin <as...@cloudera.com>
Committed: Wed Feb 1 02:20:01 2017 +0000
----------------------------------------------------------------------
src/kudu/security/token-test.cc | 8 ++++----
src/kudu/security/token_verifier.cc | 18 ++++++++++++++----
src/kudu/security/token_verifier.h | 3 ++-
3 files changed, 20 insertions(+), 9 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/e67289ed/src/kudu/security/token-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/security/token-test.cc b/src/kudu/security/token-test.cc
index d99cc85..0bb751d 100644
--- a/src/kudu/security/token-test.cc
+++ b/src/kudu/security/token-test.cc
@@ -136,7 +136,7 @@ TEST_F(TokenTest, TestEndToEnd_Valid) {
// Try to verify it.
TokenVerifier verifier;
- verifier.ImportPublicKeys(signer.GetTokenSigningPublicKeys(0));
+ ASSERT_OK(verifier.ImportPublicKeys(signer.GetTokenSigningPublicKeys(0)));
ASSERT_EQ(VerificationResult::VALID, verifier.VerifyTokenSignature(token));
}
@@ -147,7 +147,7 @@ TEST_F(TokenTest, TestEndToEnd_InvalidCases) {
ASSERT_OK(signer.RotateSigningKey());
TokenVerifier verifier;
- verifier.ImportPublicKeys(signer.GetTokenSigningPublicKeys(0));
+ ASSERT_OK(verifier.ImportPublicKeys(signer.GetTokenSigningPublicKeys(0)));
// Make and sign a token, but corrupt the data in it.
{
@@ -195,8 +195,8 @@ TEST_F(TokenTest, TestEndToEnd_InvalidCases) {
FLAGS_token_signing_key_validity_seconds = -10;
ASSERT_OK(signer.RotateSigningKey());
ASSERT_OK(signer.RotateSigningKey());
- verifier.ImportPublicKeys(signer.GetTokenSigningPublicKeys(
- verifier.GetMaxKnownKeySequenceNumber()));
+ ASSERT_OK(verifier.ImportPublicKeys(signer.GetTokenSigningPublicKeys(
+ verifier.GetMaxKnownKeySequenceNumber())));
{
SignedTokenPB token = MakeUnsignedToken(WallTime_Now() + 600);
ASSERT_OK(signer.SignToken(&token));
http://git-wip-us.apache.org/repos/asf/kudu/blob/e67289ed/src/kudu/security/token_verifier.cc
----------------------------------------------------------------------
diff --git a/src/kudu/security/token_verifier.cc b/src/kudu/security/token_verifier.cc
index 51bd398..80744de 100644
--- a/src/kudu/security/token_verifier.cc
+++ b/src/kudu/security/token_verifier.cc
@@ -52,15 +52,24 @@ int64_t TokenVerifier::GetMaxKnownKeySequenceNumber() const {
// Import a set of public keys provided by the token signer (typically
// running on another node).
-void TokenVerifier::ImportPublicKeys(const vector<TokenSigningPublicKeyPB>& public_keys) {
+Status TokenVerifier::ImportPublicKeys(const vector<TokenSigningPublicKeyPB>& public_keys) {
// Do the copy construction outside of the lock, to avoid holding the
// lock while doing lots of allocation.
vector<unique_ptr<TokenSigningPublicKey>> tsks;
for (const auto& pb : public_keys) {
// Sanity check the key.
- CHECK(pb.has_rsa_key_der());
- CHECK(pb.has_key_seq_num());
- CHECK(pb.has_expire_unix_epoch_seconds());
+ if (!pb.has_rsa_key_der()) {
+ return Status::RuntimeError(
+ "token-signing public key message must include the signing key");
+ }
+ if (!pb.has_key_seq_num()) {
+ return Status::RuntimeError(
+ "token-signing public key message must include the signing key sequence number");
+ }
+ if (!pb.has_expire_unix_epoch_seconds()) {
+ return Status::RuntimeError(
+ "token-signing public key message must include an expiration time");
+ }
tsks.emplace_back(new TokenSigningPublicKey { pb });
}
@@ -68,6 +77,7 @@ void TokenVerifier::ImportPublicKeys(const vector<TokenSigningPublicKeyPB>& publ
for (auto&& tsk_ptr : tsks) {
keys_by_seq_.emplace(tsk_ptr->pb().key_seq_num(), std::move(tsk_ptr));
}
+ return Status::OK();
}
// Verify the signature on the given token.
http://git-wip-us.apache.org/repos/asf/kudu/blob/e67289ed/src/kudu/security/token_verifier.h
----------------------------------------------------------------------
diff --git a/src/kudu/security/token_verifier.h b/src/kudu/security/token_verifier.h
index d068416..a35af22 100644
--- a/src/kudu/security/token_verifier.h
+++ b/src/kudu/security/token_verifier.h
@@ -61,7 +61,8 @@ class TokenVerifier {
// Import a set of public keys provided by a TokenSigner instance (which might
// be running on a remote node). If any public keys already exist with matching key
// sequence numbers, they are replaced by the new keys.
- void ImportPublicKeys(const std::vector<TokenSigningPublicKeyPB>& public_keys);
+ Status ImportPublicKeys(const std::vector<TokenSigningPublicKeyPB>& public_keys)
+ WARN_UNUSED_RESULT;
// Verify the signature on the given token.
VerificationResult VerifyTokenSignature(const SignedTokenPB& signed_token) const;