[kudu-CR] [security] sign/verify data using RSA key pair

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged.

Change subject: [security] sign/verify data using RSA key pair
..


[security] sign/verify data using RSA key pair

Added functionality to make a signature of a data chunk and
verify signature of a data chunk using RSA keys.  As for now,
supporting SHA256 and SHA512 message digests with test coverage
for SHA512.

Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Reviewed-on: http://gerrit.cloudera.org:8080/5805
Reviewed-by: Dan Burkert 
Tested-by: Kudu Jenkins
---
M src/kudu/security/crypto-test.cc
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 234 insertions(+), 7 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] sign/verify data using RSA key pair

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [security] sign/verify data using RSA key pair
..


Patch Set 14: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
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] sign/verify data using RSA key pair

[Alexey Serbin (Code Review)]

Hello Dan Burkert, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5805

to look at the new patch set (#14).

Change subject: [security] sign/verify data using RSA key pair
..

[security] sign/verify data using RSA key pair

Added functionality to make a signature of a data chunk and
verify signature of a data chunk using RSA keys.  As for now,
supporting SHA256 and SHA512 message digests with test coverage
for SHA512.

Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
---
M src/kudu/security/crypto-test.cc
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 234 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/5805/14
-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] sign/verify data using RSA key pair

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [security] sign/verify data using RSA key pair
..


Patch Set 13:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5805/13//COMMIT_MSG
Commit Message:

PS13, Line 11: SHA1
> Looks like maybe SHA1 was removed in later iterations?
right. thanks -- will update


http://gerrit.cloudera.org:8080/#/c/5805/13/src/kudu/security/crypto.cc
File src/kudu/security/crypto.cc:

Line 93: default:
> Wasn't the consensus to remove the default case?
ugh, this one has leaked.  will fix


-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
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] sign/verify data using RSA key pair

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [security] sign/verify data using RSA key pair
..


Patch Set 13:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5805/13//COMMIT_MSG
Commit Message:

PS13, Line 11: SHA1
Looks like maybe SHA1 was removed in later iterations?


http://gerrit.cloudera.org:8080/#/c/5805/13/src/kudu/security/crypto.cc
File src/kudu/security/crypto.cc:

Line 93: default:
Wasn't the consensus to remove the default case?


-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
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] sign/verify data using RSA key pair

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [security] sign/verify data using RSA key pair
..


Patch Set 10:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/5805/10/src/kudu/security/crypto-test.cc
File src/kudu/security/crypto-test.cc:

PS10, Line 231: // data. The idea is to verify the modified data yields 
different signatures.
  : T
> I feel this test is a little excessive -- we should already be able to rely
removed


Line 265: TEST_F(CryptoTest, VerifySignatureRef) {
> maybe just combine this with the top test case that generates the signature
Done


PS10, Line 299:   // Corrupt the reference data a bit.
  :   const int num_changes = rand() % iter_num + 1;
  :   for (int j = 0; j < num_changes; ++j) {
  : const size_t idx = rand() % data.size();
  : // Using the fact that the reference data contains only 
ASCII symbols.
  : data[idx] |= 0x80;
  :   }
> what about just using a constant string data = "not-the-original-data"? Or 
Done


http://gerrit.cloudera.org:8080/#/c/5805/10/src/kudu/security/crypto.cc
File src/kudu/security/crypto.cc:

Line 127: Status DoMakeSignature(DigestType digest,
> what's the purpose of these helper methods? they're only called from a sing
I separated them for better readability.  ok, if it does not look good, I'll 
inline them.


Line 141:   auto buf = ssl_make_unique(OPENSSL_malloc(sig_len));
> why use OPENSSL_malloc here instead of just a normal C++ allocation? (even 
It's a good question.

I was thinking about replacing it with memory on the stack, but decided to keep 
OPENSSL_malloc (i.e. CRYPTO_malloc) because I thought there were some 
restrictions on how the memory for EVP_DigestSignXXX() should be allocated.  
However, after replacing it with stack-allocated memory everything was OK.

Signature length depends on the message digest, key type, and number of bits in 
the key.  The wiki page says it's 64 bytes for SHA2 digests, but openssl 
produces 256 bytes for SHA512 and RSA2048 bit key (64 bytes is for RSA512 and 
SHA224).  For SHA512 and RSA4096 -- 512 bytes, SHA512 and RSA8192 -- 1024 bytes.

I think we can safely put 4K for now -- the EVP_DigestSignFinal() should report 
an error if the buffer is too short.


PS10, Line 166: #if OPENSSL_VERSION_NUMBER < 0x10002000L
  :   unsigned char* sig_data = reinterpret_cast(
  :   const_cast(signature.data()));
  : #else
  :   const unsigned char* sig_data = reinterpret_cast(
  :   signature.data());
  : #endif
> we have this kind of messiness in a bunch of places. let's consider a separ
yes, I would better do that in a separate patch.


Line 174:   if (rc < 0) {
> being extra paranoid here: EVP_DigestVerifyFinal explicitly says that it re
Done


http://gerrit.cloudera.org:8080/#/c/5805/10/src/kudu/security/crypto.h
File src/kudu/security/crypto.h:

Line 36:   SHA1,
> Do we need SHA1 at all? It's known to be relatively weak. eg the NIST guide
SHA1 is faster than SHA2 digests -- we could use it for tests.However, if 
we are about to use it only for tests, may be we can drop it at all.


Line 84:   virtual Status MakeSignature(DigestType digest,
> can you add a note whether the signature is already base64-ed or whether th
Done


http://gerrit.cloudera.org:8080/#/c/5805/2/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:

PS2, Line 436: 
 : 
> btw http://llvm.org/docs/CodingStandards.html#don-t-use-default-labels-in-f
sounds good


-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
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] sign/verify data using RSA key pair

[Alexey Serbin (Code Review)]

Hello Dan Burkert, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5805

to look at the new patch set (#11).

Change subject: [security] sign/verify data using RSA key pair
..

[security] sign/verify data using RSA key pair

Added functionality to make a signature of a data chunk and
verify signature of a data chunk using RSA keys.  As for now,
supporting SHA1, SHA256 and SHA512 message digests with test coverage
for SHA512.

Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
---
M src/kudu/security/crypto-test.cc
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 236 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/5805/11
-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] sign/verify data using RSA key pair

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [security] sign/verify data using RSA key pair
..


Patch Set 10:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/5805/10/src/kudu/security/crypto-test.cc
File src/kudu/security/crypto-test.cc:

PS10, Line 231: // data. The idea is to verify the modified data yields 
different signatures.
  : T
I feel this test is a little excessive -- we should already be able to rely on 
the fact that, if OpenSSL is telling us it's doing a signature, then it has the 
properties of doing a signature. You already verify that the signature 
generated by OpenSSL matches known good values in the test case above. So, 
here, I don't think we're really covering any new interesting code paths. (I 
think we can trust that OpenSSL hasn't "gamed the system" by hard-coding the 
results for our test cases ;-)"


Line 265: TEST_F(CryptoTest, VerifySignatureRef) {
maybe just combine this with the top test case that generates the signatures 
for each bit of reference data?


PS10, Line 299:   // Corrupt the reference data a bit.
  :   const int num_changes = rand() % iter_num + 1;
  :   for (int j = 0; j < num_changes; ++j) {
  : const size_t idx = rand() % data.size();
  : // Using the fact that the reference data contains only 
ASCII symbols.
  : data[idx] |= 0x80;
  :   }
what about just using a constant string data = "not-the-original-data"? Or use 
the original data but a constant signature? Either one would shorten the test 
without any loss of coverage (since again, per above, we should be trusting the 
underlying library that a single bit changing is no different than all the bits 
changing)


http://gerrit.cloudera.org:8080/#/c/5805/10/src/kudu/security/crypto.cc
File src/kudu/security/crypto.cc:

Line 127: Status DoMakeSignature(DigestType digest,
what's the purpose of these helper methods? they're only called from a single 
place each, unless I missed something.


Line 141:   auto buf = ssl_make_unique(OPENSSL_malloc(sig_len));
why use OPENSSL_malloc here instead of just a normal C++ allocation? (even on 
the stack uint8_t buf[sig_len] since we know that the signature lengths are 64 
bytes max)


PS10, Line 166: #if OPENSSL_VERSION_NUMBER < 0x10002000L
  :   unsigned char* sig_data = reinterpret_cast(
  :   const_cast(signature.data()));
  : #else
  :   const unsigned char* sig_data = reinterpret_cast(
  :   signature.data());
  : #endif
we have this kind of messiness in a bunch of places. let's consider a separate 
patch later to add some kind of utility function like 
OPENSSL_CONST_BUFFER(const char* foo) which cleans them up?


Line 174:   if (rc < 0) {
being extra paranoid here: EVP_DigestVerifyFinal explicitly says that it 
returns 1 for success, and any other value is failure to verify. Here, if it 
happened to return 2, we'd still act as if it were a success.

I don't think it can actually return 2, but I think given the results would be 
silent signature verification skipping if it did, we should do a CHECK_LE(rc, 
1); or adjust the if statement to 'if (rc < 0 || rc > 1)'


http://gerrit.cloudera.org:8080/#/c/5805/10/src/kudu/security/crypto.h
File src/kudu/security/crypto.h:

Line 36:   SHA1,
Do we need SHA1 at all? It's known to be relatively weak. eg the NIST 
guidelines say to use SHA-256 for digital signatures. 
(https://www.keylength.com/en/4/)


Line 84:   virtual Status MakeSignature(DigestType digest,
can you add a note whether the signature is already base64-ed or whether the 
output is binary?


-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
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] sign/verify data using RSA key pair

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [security] sign/verify data using RSA key pair
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5805/2/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:

PS2, Line 436: default:
 :   CHECK(false) << "unsupported digest operation";
> Sure, but it's two extra lines of code, and we only get the runtime error i
btw 
http://llvm.org/docs/CodingStandards.html#don-t-use-default-labels-in-fully-covered-switches-over-enumerations
 has some interesting bits on this.


-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
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] sign/verify data using RSA key pair

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [security] sign/verify data using RSA key pair
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5805/2/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:

PS2, Line 436: default:
 :   CHECK(false) << "unsupported digest operation";
> But at least there will be run-time error, right?
Sure, but it's two extra lines of code, and we only get the runtime error if we 
remember to exercise it with a test. If we leave off the 'default' then we get 
a compile time error immediately. In general I think reducing the lines of code 
here rather than being defensive against something that can't happen is 
beneficial.


-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
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] sign/verify data using RSA key pair

[Alexey Serbin (Code Review)]

Hello Dan Burkert,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5805

to look at the new patch set (#9).

Change subject: [security] sign/verify data using RSA key pair
..

[security] sign/verify data using RSA key pair

Added functionality to make a signature of a data chunk and
verify signature of a data chunk using RSA keys.  As for now,
supporting SHA1, SHA256 and SHA512 message digests with test coverage
for SHA512.

Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
---
M src/kudu/security/crypto-test.cc
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 310 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/5805/9
-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] sign/verify data using RSA key pair

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [security] sign/verify data using RSA key pair
..


Patch Set 8: Verified+1

unrelated breakage: boost_date_time issues

-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [security] sign/verify data using RSA key pair

[Alexey Serbin (Code Review)]

Hello Dan Burkert, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5805

to look at the new patch set (#7).

Change subject: [security] sign/verify data using RSA key pair
..

[security] sign/verify data using RSA key pair

Added functionality to make a signature of a data chunk and
verify signature of a data chunk using RSA keys.  As for now,
supporting SHA1, SHA256 and SHA512 message digests with test coverage
for SHA512.

Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
---
M src/kudu/security/crypto-test.cc
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 315 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/5805/7
-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] sign/verify data using RSA key pair

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [security] sign/verify data using RSA key pair
..


Patch Set 6: Code-Review+1

-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
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] sign/verify data using RSA key pair

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5805

to look at the new patch set (#5).

Change subject: [security] sign/verify data using RSA key pair
..

[security] sign/verify data using RSA key pair

Added functionality to make a signature of a data chunk and
verify signature of a data chunk using RSA keys.  As for now,
supporting SHA1, SHA256 and SHA512 message digests with test coverage
for SHA512.

Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
---
M src/kudu/security/crypto-test.cc
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 311 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/5805/5
-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] sign/verify data using RSA key pair

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5805

to look at the new patch set (#4).

Change subject: [security] sign/verify data using RSA key pair
..

[security] sign/verify data using RSA key pair

Added functionality to make a signature of a data chunk and
verify signature of a data chunk using RSA keys.  As for now,
supporting SHA1, SHA256 and SHA512 message digests with test coverage
for SHA512.

Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
---
M src/kudu/security/crypto-test.cc
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 288 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/5805/4
-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] sign/verify data using RSA key pair

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5805

to look at the new patch set (#3).

Change subject: [security] sign/verify data using RSA key pair
..

[security] sign/verify data using RSA key pair

Added functionality to make a signature of a data chunk and
verify signature of a data chunk using RSA keys.  As for now,
supporting SHA1, SHA256 and SHA512 message digests with test coverage
for SHA512.

Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
---
M src/kudu/security/crypto-test.cc
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 300 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/5805/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] sign/verify data using RSA key pair

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [security] sign/verify data using RSA key pair
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5805/2/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:

PS2, Line 436: default:
 :   CHECK(false) << "unsupported digest operation";
> nope, because then we'd have to have a 'default:', and then we don't get co
But at least there will be run-time error, right?


PS2, Line 440:   const void* mdata = reinterpret_cast(data.data());
 :   auto membio = ssl_make_unique(BIO_new_mem_buf(
 : #if OPENSSL_VERSION_NUMBER < 0x10002000L
 :   const_cast(mdata),
 : #else
 :   mdata,
 : #endif
 :   data.size()));
 :   BIO* inp = BIO_push(bmd.get(), membio.get());
 :   while (true) {
 : unsigned char buf[1024];
 : const int rc = BIO_read(inp, buf, sizeof(buf));
 : if (rc < 0) {
 :   return Status::RuntimeError(Substitute("error reading 
data: $0",
 :  
GetOpenSSLErrors()));
 : }
 : if (rc == 0) {
 :   break;
 : }
 :   }
> hrm, why not? also, our use case is only signing short token strings (on th
Hundred of bytes were too much for RSA-related functions.  Oh, wait -- that was 
not envelope-based stuff.  Hopefully, with EVP it should be all right.


-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
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] sign/verify data using RSA key pair

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [security] sign/verify data using RSA key pair
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5805/2/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:

PS2, Line 436: default:
 :   CHECK(false) << "unsupported digest operation";
> ok, will remove.  You think we event don't need DCHECK() here?
nope, because then we'd have to have a 'default:', and then we don't get 
compilation warnings if we added a new enum case but forgot to update this 
function


PS2, Line 440:   const void* mdata = reinterpret_cast(data.data());
 :   auto membio = ssl_make_unique(BIO_new_mem_buf(
 : #if OPENSSL_VERSION_NUMBER < 0x10002000L
 :   const_cast(mdata),
 : #else
 :   mdata,
 : #endif
 :   data.size()));
 :   BIO* inp = BIO_push(bmd.get(), membio.get());
 :   while (true) {
 : unsigned char buf[1024];
 : const int rc = BIO_read(inp, buf, sizeof(buf));
 : if (rc < 0) {
 :   return Status::RuntimeError(Substitute("error reading 
data: $0",
 :  
GetOpenSSLErrors()));
 : }
 : if (rc == 0) {
 :   break;
 : }
 :   }
> I don't think EVP_DigestSignUpdate() would work here in case of big data ch
hrm, why not? also, our use case is only signing short token strings (on the 
order of hundreds of bytes max is my guess) so does it matter?


http://gerrit.cloudera.org:8080/#/c/5805/2/src/kudu/security/openssl_util.h
File src/kudu/security/openssl_util.h:

PS2, Line 194: template
 : Status MakeSignature(DigestType digest, const KeyType& key,
> I thought we like stand-alone functions more.  Don't we? :)
I think we like standalone functions for utility code rather than getting 
utility code by implementation inheritance. But when there's a function that is 
closely tied to a single class instance, I think it belongs in that class.


-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
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] sign/verify data using RSA key pair

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [security] sign/verify data using RSA key pair
..


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5805/2/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:

PS2, Line 436: default:
 :   CHECK(false) << "unsupported digest operation";
> per comment on the review the other day, I think it's better to just not li
ok, will remove.  You think we event don't need DCHECK() here?


PS2, Line 440:   const void* mdata = reinterpret_cast(data.data());
 :   auto membio = ssl_make_unique(BIO_new_mem_buf(
 : #if OPENSSL_VERSION_NUMBER < 0x10002000L
 :   const_cast(mdata),
 : #else
 :   mdata,
 : #endif
 :   data.size()));
 :   BIO* inp = BIO_push(bmd.get(), membio.get());
 :   while (true) {
 : unsigned char buf[1024];
 : const int rc = BIO_read(inp, buf, sizeof(buf));
 : if (rc < 0) {
 :   return Status::RuntimeError(Substitute("error reading 
data: $0",
 :  
GetOpenSSLErrors()));
 : }
 : if (rc == 0) {
 :   break;
 : }
 :   }
> hrm, not quite following what this BIO stuff is doing. Can we just use EVP_
I don't think EVP_DigestSignUpdate() would work here in case of big data 
chunks.  But I'll clarify on that.


http://gerrit.cloudera.org:8080/#/c/5805/2/src/kudu/security/openssl_util.h
File src/kudu/security/openssl_util.h:

Line 185: // Supported message digests.
> Could you put this digest functionality in a different file, say digest.h/c
Moved into crypto.{cc,h}


PS2, Line 194: template
 : Status MakeSignature(DigestType digest, const KeyType& key,
> hrm, why are these standalone templated functions instead of just being mem
I thought we like stand-alone functions more.  Don't we? :)

Being them members of corresponding key class looks a bit strange to me.  But 
it you really think that's the better place, I'll do that.


-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
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] sign/verify data using RSA key pair

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [security] sign/verify data using RSA key pair
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5805/2/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:

PS2, Line 436: default:
 :   CHECK(false) << "unsupported digest operation";
per comment on the review the other day, I think it's better to just not list a 
default, since this is an internally defined enum


PS2, Line 440:   const void* mdata = reinterpret_cast(data.data());
 :   auto membio = ssl_make_unique(BIO_new_mem_buf(
 : #if OPENSSL_VERSION_NUMBER < 0x10002000L
 :   const_cast(mdata),
 : #else
 :   mdata,
 : #endif
 :   data.size()));
 :   BIO* inp = BIO_push(bmd.get(), membio.get());
 :   while (true) {
 : unsigned char buf[1024];
 : const int rc = BIO_read(inp, buf, sizeof(buf));
 : if (rc < 0) {
 :   return Status::RuntimeError(Substitute("error reading 
data: $0",
 :  
GetOpenSSLErrors()));
 : }
 : if (rc == 0) {
 :   break;
 : }
 :   }
hrm, not quite following what this BIO stuff is doing. Can we just use 
EVP_DigestSignUpdate(mctx, [0], data.size()); ? I'm looking at the bottom 
section of https://wiki.openssl.org/index.php/EVP_Signing_and_Verifying which 
seems much simpler than the code in this function


http://gerrit.cloudera.org:8080/#/c/5805/2/src/kudu/security/openssl_util.h
File src/kudu/security/openssl_util.h:

PS2, Line 194: template
 : Status MakeSignature(DigestType digest, const KeyType& key,
hrm, why are these standalone templated functions instead of just being members 
of the Key/PrivateKey classes?


-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [security] sign/verify data using RSA key pair

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [security] sign/verify data using RSA key pair
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5805/2/src/kudu/security/openssl_util.h
File src/kudu/security/openssl_util.h:

Line 185: // Supported message digests.
Could you put this digest functionality in a different file, say digest.h/cc?  
The interface doesn't look OpenSSL specific.


-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [security] sign/verify data using RSA key pair

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new patch set (#2).

Change subject: [security] sign/verify data using RSA key pair
..

[security] sign/verify data using RSA key pair

Added functionality to make a signature of a data chunk and
verify signature of a data chunk using RSA key pair.  As for now,
supporting SHA1, SHA256 and SHA512 message digests with test coverage
for SHA512.

Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
---
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/security/test/crypto-test.cc
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 332 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/5805/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] sign/verify data using RSA key pair

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/5805

Change subject: [security] sign/verify data using RSA key pair
..

[security] sign/verify data using RSA key pair

Added functionality to make a signature of a data chunk and
verify signature of a data chunk using RSA key pair.  As for now,
supporting SHA1, SHA256 and SHA512 message digests with test coverage
for SHA512.

Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
---
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/security/test/crypto-test.cc
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 327 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/5805/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin