Tim Armstrong has posted comments on this change. Change subject: IMPALA-4118: extract encryption utils from BufferedBlockMgr ......................................................................
Patch Set 6: (13 comments) http://gerrit.cloudera.org:8080/#/c/4389/6/be/src/common/init.cc File be/src/common/init.cc: Line 29: #include "util/openssl-util.h" > move before "util/os-info.h" Ran clang-format to fix the ordering. http://gerrit.cloudera.org:8080/#/c/4389/6/be/src/util/openssl-util-test.cc File be/src/util/openssl-util-test.cc: Line 79: TEST_F(OpenSSLUtilTest, EncryptInPlace) { > Is there an easy way to inspect the RNG? Would be nice to validate that we Turns out there's an OpenSSL function that tells us whether its happy. Added a new test that checks this while generating keys. Line 154: impala::InitCommonRuntime(argc, argv, true, impala::TestInfo::BE_TEST); > I think you can pass "false" here since you don't need an embedded JVM Done http://gerrit.cloudera.org:8080/#/c/4389/6/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: Line 31: using strings::Substitute; > taken care of by common/names.h? It's not in there, so I added it and removed all the cases where it wasn't needed elsewhere. Line 78: SeedOpenSSLRNG(); > Is the RNG a thread-local or global thing? I'm wondering if we can make key The RNG is global. Was the concern performance? Line 130: // Finalize encryption. > Need to update comment since it may be finalising decryption too. Done http://gerrit.cloudera.org:8080/#/c/4389/6/be/src/util/openssl-util.h File be/src/util/openssl-util.h: Line 23: #include <sstream> > move to .cc Done Line 33: /// The hash of a data buffer used for checking integrity. Augmented this comment with details of hash algo. Line 35: public: > Add an initialized_ or similar to make sure we don't Verify() against an un I thought about it and it seemed mostly not useful, since verification against the garbage hash will fail. Line 41: bool Verify(const uint8_t* data, int64_t len); > const method Done Line 48: /// data. This should be regenerated for each buffer of data. Augmented this comment with details of encryption algo. Line 68: Status Encrypt(const uint8_t* data, int64_t len, uint8_t* out); > const function? Done Line 74: Status Decrypt(const uint8_t* data, int64_t len, uint8_t* out); > const function? Done -- To view, visit http://gerrit.cloudera.org:8080/4389 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibebe431f69c3c569cbff68171beaa32ef2ab03b6 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
