[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Todd Lipcon has submitted this change and it was merged. Change subject: [security] Add per-connection nonce for Kerberos replay resistance .. [security] Add per-connection nonce for Kerberos replay resistance Kerberos is susceptible to replay attacks, which it attempts to mitigate by using a server-side replay cache. The cache is not 100% effective, and is extremely slow. This commit introduces an effective and efficient method of mitigating replay attacks by using a server-generated nonce which the client must send back to the server, wrapped in SASL integrity protection. This will allow Kudu to disable the replay cache without negatively affecting security. No tests are provided, but the codepath is well covered by existing Kerberos negotiation tests. I intend to write simulated mitm tests to check this and the channel binding protections soon. Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Reviewed-on: http://gerrit.cloudera.org:8080/6137 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin Reviewed-by: Todd Lipcon --- M docs/design-docs/rpc.md M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java M src/kudu/rpc/client_negotiation.cc M src/kudu/rpc/client_negotiation.h M src/kudu/rpc/negotiation-test.cc M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h M src/kudu/security/crypto-test.cc M src/kudu/security/crypto.cc M src/kudu/security/crypto.h 13 files changed, 254 insertions(+), 106 deletions(-) Approvals: Todd Lipcon: Looks good to me, approved Alexey Serbin: Looks good to me, but someone else must approve Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/6137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Todd Lipcon has posted comments on this change. Change subject: [security] Add per-connection nonce for Kerberos replay resistance .. Patch Set 10: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/6137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Alexey Serbin has posted comments on this change. Change subject: [security] Add per-connection nonce for Kerberos replay resistance .. Patch Set 10: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/6137/7/src/kudu/security/crypto-test.cc File src/kudu/security/crypto-test.cc: PS7, Line 247: kNonceSize, '\0'), nonce > I don't think it's possible to have a static std::string. Well, of course the constructor is non-trivial, but the instance would be allocated just once during initialization or entering the scope the very first time. However, I agree that it does not make much sense since these tests usually executed just once (i.e. almost nobody uses that --gtest-iterations=... parameter). -- To view, visit http://gerrit.cloudera.org:8080/6137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6137 to look at the new patch set (#10). Change subject: [security] Add per-connection nonce for Kerberos replay resistance .. [security] Add per-connection nonce for Kerberos replay resistance Kerberos is susceptible to replay attacks, which it attempts to mitigate by using a server-side replay cache. The cache is not 100% effective, and is extremely slow. This commit introduces an effective and efficient method of mitigating replay attacks by using a server-generated nonce which the client must send back to the server, wrapped in SASL integrity protection. This will allow Kudu to disable the replay cache without negatively affecting security. No tests are provided, but the codepath is well covered by existing Kerberos negotiation tests. I intend to write simulated mitm tests to check this and the channel binding protections soon. Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 --- M docs/design-docs/rpc.md M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java M src/kudu/rpc/client_negotiation.cc M src/kudu/rpc/client_negotiation.h M src/kudu/rpc/negotiation-test.cc M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h M src/kudu/security/crypto-test.cc M src/kudu/security/crypto.cc M src/kudu/security/crypto.h 13 files changed, 254 insertions(+), 106 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/6137/10 -- To view, visit http://gerrit.cloudera.org:8080/6137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Dan Burkert has posted comments on this change.
Change subject: [security] Add per-connection nonce for Kerberos replay
resistance
..
Patch Set 7:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/6137/7//COMMIT_MSG
Commit Message:
PS7, Line 9: suceptible
> seems to be a typo: susceptible ?
Done
http://gerrit.cloudera.org:8080/#/c/6137/7/docs/design-docs/rpc.md
File docs/design-docs/rpc.md:
PS7, Line 540: tls
> nit: TLS ?
tls-server-end-point is the name defined by the RFC.
http://gerrit.cloudera.org:8080/#/c/6137/7/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
Line 351: Status SaslEncode(sasl_conn_t* conn, const std::string& plaintext,
std::string* encoded) {
> It seems you have just moved this method here, but while you are at it, con
Done
PS7, Line 353: const char* out;
: unsigned out_len;
> Consider making these local for the scope of the while() loop below.
Done
PS7, Line 358: plaintext.size()
> nit: maybe, create a constant for the scope of this method so it would not
I think this should be hoisted by the compiler, since plaintext is const.
PS7, Line 374: const char* out;
: unsigned out_len;
> Ditto: consider making there local for the while() loop below.
Done
Line 378: // have to call decode multiple times if our input is larger than
this max.
> nit: does it make sense to call something like
The plaintext is usually shorter, since there are length delimiters and hash
overhead. We're only using these functions for very short tokens, so I'm not
sure it matters anyway.
http://gerrit.cloudera.org:8080/#/c/6137/7/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
PS7, Line 788: CHECK_OK(s);
> DCHECK_OK(s) ? Otherwise, why to have that
Done
http://gerrit.cloudera.org:8080/#/c/6137/7/src/kudu/security/crypto-test.cc
File src/kudu/security/crypto-test.cc:
PS7, Line 246: ASSERT_EQ
> nit here and below: the 'expected' parameter comes first in {ASSERT,EXPECT}
Done
PS7, Line 247: string(kNonceSize, '\0')
> nit: consider making this a constant (or event static constant) for this te
I don't think it's possible to have a static std::string.
--
To view, visit http://gerrit.cloudera.org:8080/6137
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6137 to look at the new patch set (#9). Change subject: [security] Add per-connection nonce for Kerberos replay resistance .. [security] Add per-connection nonce for Kerberos replay resistance Kerberos is susceptible to replay attacks, which it attempts to mitigate by using a server-side replay cache. The cache is not 100% effective, and is extremely slow. This commit introduces an effective and efficient method of mitigating replay attacks by using a server-generated nonce which the client must send back to the server, wrapped in SASL integrity protection. This will allow Kudu to disable the replay cache without negatively affecting security. No tests are provided, but the codepath is well covered by existing Kerberos negotiation tests. I intend to write simulated mitm tests to check this and the channel binding protections soon. Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 --- M docs/design-docs/rpc.md M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java M src/kudu/rpc/client_negotiation.cc M src/kudu/rpc/client_negotiation.h M src/kudu/rpc/negotiation-test.cc M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h M src/kudu/security/crypto-test.cc M src/kudu/security/crypto.cc M src/kudu/security/crypto.h 13 files changed, 253 insertions(+), 104 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/6137/9 -- To view, visit http://gerrit.cloudera.org:8080/6137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6137 to look at the new patch set (#8). Change subject: [security] Add per-connection nonce for Kerberos replay resistance .. [security] Add per-connection nonce for Kerberos replay resistance Kerberos is suceptible to replay attacks, which it attempts to mitigate by using a server-side replay cache. The cache is not 100% effective, and is extremely slow. This commit introduces an effective and efficient method of mitigating replay attacks by using a server-generated nonce which the client must send back to the server, wrapped in SASL integrity protection. This will allow Kudu to disable the replay cache without negatively affecting security. No tests are provided, but the codepath is well covered by existing Kerberos negotiation tests. I intend to write simulated mitm tests to check this and the channel binding protections soon. Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 --- M docs/design-docs/rpc.md M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java M src/kudu/rpc/client_negotiation.cc M src/kudu/rpc/client_negotiation.h M src/kudu/rpc/negotiation-test.cc M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h M src/kudu/security/crypto-test.cc M src/kudu/security/crypto.cc M src/kudu/security/crypto.h 13 files changed, 253 insertions(+), 104 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/6137/8 -- To view, visit http://gerrit.cloudera.org:8080/6137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Alexey Serbin has posted comments on this change.
Change subject: [security] Add per-connection nonce for Kerberos replay
resistance
..
Patch Set 7:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/6137/7//COMMIT_MSG
Commit Message:
PS7, Line 9: suceptible
seems to be a typo: susceptible ?
http://gerrit.cloudera.org:8080/#/c/6137/7/docs/design-docs/rpc.md
File docs/design-docs/rpc.md:
PS7, Line 540: tls
nit: TLS ?
http://gerrit.cloudera.org:8080/#/c/6137/7/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
Line 351: Status SaslEncode(sasl_conn_t* conn, const std::string& plaintext,
std::string* encoded) {
It seems you have just moved this method here, but while you are at it,
consider addressing the comments below. It's up to you whether to address that
in a separate changelist or here.
PS7, Line 353: const char* out;
: unsigned out_len;
Consider making these local for the scope of the while() loop below.
PS7, Line 358: plaintext.size()
nit: maybe, create a constant for the scope of this method so it would not be
necessary to call the string::size() method twice per loop?
PS7, Line 374: const char* out;
: unsigned out_len;
Ditto: consider making there local for the while() loop below.
Line 378: // have to call decode multiple times if our input is larger than
this max.
nit: does it make sense to call something like
plaintext->reserve(encoded.size()) ?
http://gerrit.cloudera.org:8080/#/c/6137/7/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
PS7, Line 788: CHECK_OK(s);
DCHECK_OK(s) ? Otherwise, why to have that
if (!s.ok()) {} closure below.
http://gerrit.cloudera.org:8080/#/c/6137/7/src/kudu/security/crypto-test.cc
File src/kudu/security/crypto-test.cc:
PS7, Line 246: ASSERT_EQ
nit here and below: the 'expected' parameter comes first in
{ASSERT,EXPECT}_EQ() macros.
PS7, Line 247: string(kNonceSize, '\0')
nit: consider making this a constant (or event static constant) for this test.
--
To view, visit http://gerrit.cloudera.org:8080/6137
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6137 to look at the new patch set (#7). Change subject: [security] Add per-connection nonce for Kerberos replay resistance .. [security] Add per-connection nonce for Kerberos replay resistance Kerberos is suceptible to replay attacks, which it attempts to mitigate by using a server-side replay cache. The cache is not 100% effective, and is extremely slow. This commit introduces an effective and efficient method of mitigating replay attacks by using a server-generated nonce which the client must send back to the server, wrapped in SASL integrity protection. This will allow Kudu to disable the replay cache without negatively affecting security. No tests are provided, but the codepath is well covered by existing Kerberos negotiation tests. I intend to write simulated mitm tests to check this and the channel binding protections soon. Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 --- M docs/design-docs/rpc.md M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java M src/kudu/rpc/client_negotiation.cc M src/kudu/rpc/client_negotiation.h M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h M src/kudu/security/crypto-test.cc M src/kudu/security/crypto.cc M src/kudu/security/crypto.h 12 files changed, 232 insertions(+), 104 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/6137/7 -- To view, visit http://gerrit.cloudera.org:8080/6137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6137 to look at the new patch set (#6). Change subject: [security] Add per-connection nonce for Kerberos replay resistance .. [security] Add per-connection nonce for Kerberos replay resistance Kerberos is suceptible to replay attacks, which it attempts to mitigate by using a server-side replay cache. The cache is not 100% effective, and is extremely slow. This commit introduces an effective and efficient method of mitigating replay attacks by using a server-generated nonce which the client must send back to the server, wrapped in SASL integrity protection. This will allow Kudu to disable the replay cache without negatively affecting security. No tests are provided, but the codepath is well covered by existing Kerberos negotiation tests. I intend to write simulated mitm tests to check this and the channel binding protections soon. Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 --- M docs/design-docs/rpc.md M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java M src/kudu/rpc/client_negotiation.cc M src/kudu/rpc/client_negotiation.h M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h M src/kudu/security/crypto-test.cc M src/kudu/security/crypto.cc M src/kudu/security/crypto.h 12 files changed, 220 insertions(+), 104 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/6137/6 -- To view, visit http://gerrit.cloudera.org:8080/6137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Dan Burkert has posted comments on this change.
Change subject: [security] Add per-connection nonce for Kerberos replay
resistance
..
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/6137/5/src/kudu/security/crypto-test.cc
File src/kudu/security/crypto-test.cc:
PS5, Line 247: string('\0', kNonceSize)
> consider addressing this TidyBot's comment
Done
http://gerrit.cloudera.org:8080/#/c/6137/5/src/kudu/security/crypto.cc
File src/kudu/security/crypto.cc:
Line 41: const size_t kNonceSize = 8;
> http://security.stackexchange.com/questions/1952/how-long-should-a-random-n
Done
PS5, Line 250: s->resize(kNonceSize);
: OPENSSL_RET_NOT_OK(RAND_bytes(reinterpret_cast(&s->front()), s->size()),
: "failed to generate nonce");
:
> if you want to avoid the C++ sketchiness, why not just add a local buffer?
Done
--
To view, visit http://gerrit.cloudera.org:8080/6137
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Todd Lipcon has posted comments on this change. Change subject: [security] Add per-connection nonce for Kerberos replay resistance .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/6137/5/src/kudu/security/crypto.cc File src/kudu/security/crypto.cc: Line 41: const size_t kNonceSize = 8; http://security.stackexchange.com/questions/1952/how-long-should-a-random-nonce-be has some people suggesting more than 64 bits are a good idea. Would it cost us anything to go to 16 bytes just for kicks? PS5, Line 250: s->resize(kNonceSize); : OPENSSL_RET_NOT_OK(RAND_bytes(reinterpret_cast(&s->front()), s->size()), : "failed to generate nonce"); : if you want to avoid the C++ sketchiness, why not just add a local buffer? char buf[kNonceSize]; ...RAND_BYTES... s->assign(buf, kNonceSize) ? I don't think the extra copy costs anything. -- To view, visit http://gerrit.cloudera.org:8080/6137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Alexey Serbin has posted comments on this change.
Change subject: [security] Add per-connection nonce for Kerberos replay
resistance
..
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6137/5/src/kudu/security/crypto-test.cc
File src/kudu/security/crypto-test.cc:
PS5, Line 247: string('\0', kNonceSize)
consider addressing this TidyBot's comment
--
To view, visit http://gerrit.cloudera.org:8080/6137
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6137 to look at the new patch set (#5). Change subject: [security] Add per-connection nonce for Kerberos replay resistance .. [security] Add per-connection nonce for Kerberos replay resistance Kerberos is suceptible to replay attacks, which it attempts to mitigate by using a server-side replay cache. The cache is not 100% effective, and is extremely slow. This commit introduces an effective and efficient method of mitigating replay attacks by using a server-generated nonce which the client must send back to the server, wrapped in SASL integrity protection. This will allow Kudu to disable the replay cache without negatively affecting security. No tests are provided, but the codepath is well covered by existing Kerberos negotiation tests. I intend to write simulated mitm tests to check this and the channel binding protections soon. Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 --- M docs/design-docs/rpc.md M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java M src/kudu/rpc/client_negotiation.cc M src/kudu/rpc/client_negotiation.h M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h M src/kudu/security/crypto-test.cc M src/kudu/security/crypto.cc M src/kudu/security/crypto.h 12 files changed, 219 insertions(+), 103 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/6137/5 -- To view, visit http://gerrit.cloudera.org:8080/6137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Dan Burkert has posted comments on this change.
Change subject: [security] Add per-connection nonce for Kerberos replay
resistance
..
Patch Set 3:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/6137/3//COMMIT_MSG
Commit Message:
PS3, Line 11: and is extremely slow in older versions of the library.
> I think it's extremely slow in all versions of the library (the "avoid fsyn
Done
http://gerrit.cloudera.org:8080/#/c/6137/3/docs/design-docs/rpc.md
File docs/design-docs/rpc.md:
PS3, Line 543: against Kerberos replay attacks.
> think it's worth adding here something like "Kerberos's built-in replay att
Done
http://gerrit.cloudera.org:8080/#/c/6137/3/src/kudu/rpc/client_negotiation.cc
File src/kudu/rpc/client_negotiation.cc:
PS3, Line 619: if (!response.has_channel_bindings()) {
: return Status::NotAuthorized("no channel bindings
provided by server");
: }
> nit: Is it worth retrieving remote certificate and generating channel bindi
Done
PS3, Line 625:
response.channel_bindings(),
:
&received_channel_bindings),
> nit: off-by-one shift
Done
http://gerrit.cloudera.org:8080/#/c/6137/3/src/kudu/rpc/rpc_header.proto
File src/kudu/rpc/rpc_header.proto:
PS3, Line 57: negotatition
> nit: typo
Done
PS3, Line 60: nonce
> rename this field to "wrapped_nonce" or "nonce_reply" or something? it seem
Done
http://gerrit.cloudera.org:8080/#/c/6137/3/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
PS3, Line 719: nonce_ = string(kNonceSize, '\0');
: RETURN_NOT_OK(security::GenerateNonce(*nonce_));
:
: // Sanity check the nonce.
: DCHECK_EQ(kNonceSize, nonce_->size());
: DCHECK_NE(*nonce_, "\0\0\0\0\0\0\0\0");
:
> this smells a little goofy to me. why do you have to pre-initialize nonce_?
Done
http://gerrit.cloudera.org:8080/#/c/6137/3/src/kudu/security/crypto.cc
File src/kudu/security/crypto.cc:
PS3, Line 246: Slice
> nit: consider using Slice* to conform with the style guide.
changed to string*
--
To view, visit http://gerrit.cloudera.org:8080/6137
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6137 to look at the new patch set (#4). Change subject: [security] Add per-connection nonce for Kerberos replay resistance .. [security] Add per-connection nonce for Kerberos replay resistance Kerberos is suceptible to replay attacks, which it attempts to mitigate by using a server-side replay cache. The cache is not 100% effective, and is extremely slow. This commit introduces an effective and efficient method of mitigating replay attacks by using a server-generated nonce which the client must send back to the server, wrapped in SASL integrity protection. This will allow Kudu to disable the replay cache without negatively affecting security. No tests are provided, but the codepath is well covered by existing Kerberos negotiation tests. I intend to write simulated mitm tests to check this and the channel binding protections soon. Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 --- M docs/design-docs/rpc.md M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java M src/kudu/rpc/client_negotiation.cc M src/kudu/rpc/client_negotiation.h M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h M src/kudu/security/crypto-test.cc M src/kudu/security/crypto.cc M src/kudu/security/crypto.h 12 files changed, 219 insertions(+), 103 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/6137/4 -- To view, visit http://gerrit.cloudera.org:8080/6137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Alexey Serbin has posted comments on this change.
Change subject: [security] Add per-connection nonce for Kerberos replay
resistance
..
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/6137/3/src/kudu/rpc/client_negotiation.cc
File src/kudu/rpc/client_negotiation.cc:
PS3, Line 619: if (!response.has_channel_bindings()) {
: return Status::NotAuthorized("no channel bindings
provided by server");
: }
nit: Is it worth retrieving remote certificate and generating channel bindings
for the cert if the response does not have channel bindings? Consider moving
this check before the certificate-related activity.
PS3, Line 625:
response.channel_bindings(),
:
&received_channel_bindings),
nit: off-by-one shift
http://gerrit.cloudera.org:8080/#/c/6137/3/src/kudu/security/crypto.cc
File src/kudu/security/crypto.cc:
PS3, Line 246: Slice
nit: consider using Slice* to conform with the style guide.
http://gerrit.cloudera.org:8080/#/c/6137/2/src/kudu/security/crypto.h
File src/kudu/security/crypto.h:
PS2, Line 89: Slice
nit: consider using Slice* to conform with the style guide?
--
To view, visit http://gerrit.cloudera.org:8080/6137
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Todd Lipcon has posted comments on this change. Change subject: [security] Add per-connection nonce for Kerberos replay resistance .. Patch Set 3: should this patch also add the setenv() call to disable the replay cache, btw? Also, tag the commit message with the JIRA? -- To view, visit http://gerrit.cloudera.org:8080/6137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Dan Burkert has posted comments on this change. Change subject: [security] Add per-connection nonce for Kerberos replay resistance .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/6137/3/src/kudu/security/crypto.h File src/kudu/security/crypto.h: PS3, Line 88: // Generates a nonce, and writes it to the provided slice. : Status GenerateNonce(Slice slice); > yea, but we rarely use Slice as a parameter for an output parameter like th OK I'll change it to string*. The thing I like about Slice is that it's completely unambiguous. string* has ambiguity due to including a length and capacity. -- To view, visit http://gerrit.cloudera.org:8080/6137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Todd Lipcon has posted comments on this change. Change subject: [security] Add per-connection nonce for Kerberos replay resistance .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/6137/3/src/kudu/security/crypto.h File src/kudu/security/crypto.h: PS3, Line 88: // Generates a nonce, and writes it to the provided slice. : Status GenerateNonce(Slice slice); > Isn't that effectively a slice? yea, but we rarely use Slice as a parameter for an output parameter like this (plus it makes the call site look pretty strange). Since we only have one use case, I think it's also reasonable to just have a 'string*' parameter and hard-code the length to 8 or 16 or whatever -- To view, visit http://gerrit.cloudera.org:8080/6137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Dan Burkert has posted comments on this change. Change subject: [security] Add per-connection nonce for Kerberos replay resistance .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/6137/3/src/kudu/security/crypto.h File src/kudu/security/crypto.h: PS3, Line 88: // Generates a nonce, and writes it to the provided slice. : Status GenerateNonce(Slice slice); > Pretty strange signature. Why not just a string* and maybe an 'int length' Isn't that effectively a slice? -- To view, visit http://gerrit.cloudera.org:8080/6137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Todd Lipcon has posted comments on this change. Change subject: [security] Add per-connection nonce for Kerberos replay resistance .. Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/6137/3//COMMIT_MSG Commit Message: PS3, Line 11: and is extremely slow in older versions of the library. I think it's extremely slow in all versions of the library (the "avoid fsync" thing is a project plan but I dont think it's implemented yet?) http://gerrit.cloudera.org:8080/#/c/6137/3/docs/design-docs/rpc.md File docs/design-docs/rpc.md: PS3, Line 543: against Kerberos replay attacks. think it's worth adding here something like "Kerberos's built-in replay attack mitigation is extremely slow, so this allows much faster connection negotiation" or something to that effect http://gerrit.cloudera.org:8080/#/c/6137/3/src/kudu/rpc/rpc_header.proto File src/kudu/rpc/rpc_header.proto: PS3, Line 57: negotatition nit: typo PS3, Line 60: nonce rename this field to "wrapped_nonce" or "nonce_reply" or something? it seems odd to name this 'nonce' because it's not actually the nonce anymore http://gerrit.cloudera.org:8080/#/c/6137/3/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: PS3, Line 719: nonce_ = string(kNonceSize, '\0'); : RETURN_NOT_OK(security::GenerateNonce(*nonce_)); : : // Sanity check the nonce. : DCHECK_EQ(kNonceSize, nonce_->size()); : DCHECK_NE(*nonce_, "\0\0\0\0\0\0\0\0"); : this smells a little goofy to me. why do you have to pre-initialize nonce_? also seems a little strange to defensively check that GenerateNonce does what it's sposed to do http://gerrit.cloudera.org:8080/#/c/6137/3/src/kudu/security/crypto.h File src/kudu/security/crypto.h: PS3, Line 88: // Generates a nonce, and writes it to the provided slice. : Status GenerateNonce(Slice slice); Pretty strange signature. Why not just a string* and maybe an 'int length' -- To view, visit http://gerrit.cloudera.org:8080/6137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6137 to look at the new patch set (#3). Change subject: [security] Add per-connection nonce for Kerberos replay resistance .. [security] Add per-connection nonce for Kerberos replay resistance Kerberos is suceptible to replay attacks, which it attempts to mitigate by using a server-side replay cache. The cache is not 100% effective, and is extremely slow in older versions of the library. This commit introduces an effective and efficient method of mitigating replay attacks by using a server-generated nonce which the client must send back to the server, wrapped in SASL integrity protection. This will allow Kudu to disable the replay cache without negatively affecting security. No tests are provided, but the codepath is well covered by existing Kerberos negotiation tests. I intend to write simulated mitm tests to check this and the channel binding protections soon. Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 --- M docs/design-docs/rpc.md M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java M src/kudu/rpc/client_negotiation.cc M src/kudu/rpc/client_negotiation.h M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h M src/kudu/security/crypto.cc M src/kudu/security/crypto.h 11 files changed, 205 insertions(+), 103 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/6137/3 -- To view, visit http://gerrit.cloudera.org:8080/6137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Dan Burkert has uploaded a new patch set (#2). Change subject: [security] Add per-connection nonce for Kerberos replay resistance .. [security] Add per-connection nonce for Kerberos replay resistance Kerberos is suceptible to replay attacks, which it attempts to mitigate by using a server-side replay cache. The cache is not 100% effective, and is extremely slow in older versions of the library. This commit introduces an effective and efficient method of mitigating replay attacks by using a server-generated nonce which the client must send back to the server, wrapped in SASL integrity protection. This will allow Kudu to disable the replay cache without negatively affecting security. No tests are provided, but the codepath is well covered by existing Kerberos negotiation tests. I intend to write simulated mitm tests to check this and the channel binding protections soon. Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 --- M docs/design-docs/rpc.md M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java M src/kudu/rpc/client_negotiation.cc M src/kudu/rpc/client_negotiation.h M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h M src/kudu/security/crypto.cc M src/kudu/security/crypto.h 11 files changed, 199 insertions(+), 103 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/6137/2 -- To view, visit http://gerrit.cloudera.org:8080/6137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [security] Add per-connection nonce for Kerberos replay resistance
Hello Todd Lipcon, Alexey Serbin, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/6137 to review the following change. Change subject: [security] Add per-connection nonce for Kerberos replay resistance .. [security] Add per-connection nonce for Kerberos replay resistance Kerberos is suceptible to replay attacks, which it attempts to mitigate by using a server-side replay cache. The cache is not 100% effective, and is extremely slow in older versions of the library. This commit introduces an effective and efficient method of mitigating replay attacks by using a server-generated nonce which the client must send back to the server, wrapped in SASL integrity protection. This will allow Kudu to disable the replay cache without negatively affecting security. No tests are provided, but the codepath is well covered by existing Kerberos negotiation tests. I intend to write simulated mitm tests to check this and the channel binding protections soon. Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 --- M docs/design-docs/rpc.md M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java M src/kudu/rpc/client_negotiation.cc M src/kudu/rpc/client_negotiation.h M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/sasl_common.cc M src/kudu/rpc/sasl_common.h M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h M src/kudu/security/crypto.cc M src/kudu/security/crypto.h 11 files changed, 193 insertions(+), 98 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/6137/1 -- To view, visit http://gerrit.cloudera.org:8080/6137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Todd Lipcon
