Alexey Serbin has posted comments on this change. Change subject: [security] method to extract public part of RSA key ......................................................................
Patch Set 6: (7 comments) http://gerrit.cloudera.org:8080/#/c/5783/6/src/kudu/security/ca/cert_management.cc File src/kudu/security/ca/cert_management.cc: Line 89: "error setting X509 public key"); > Indentation here and below. Done http://gerrit.cloudera.org:8080/#/c/5783/6/src/kudu/security/crypto.cc File src/kudu/security/crypto.cc: PS6, Line 94: // The 'using' is here to overcome issues with RETURN_NOT_OK_PREPEND() macro. : using func = Status(*)(BIO*, DataFormat, c_unique_ptr<EVP_PKEY>*); : func f = ::kudu::security::FromBIO<EVP_PKEY, RSAPublicKeyTraits>; : > huh, that's weird... what's going on? The pre-processor stuck with parsing that otherwise -- it cannot distinguish template parameter from function parameters. Yep, the auto will do it, right. I was playing with ways doing that call, auto seems to be better. Another workaround is to enclose the call of the specialized template function into extra-braces. Probably, that's the best option. http://gerrit.cloudera.org:8080/#/c/5783/6/src/kudu/security/crypto.h File src/kudu/security/crypto.h: Line 29: class RSAPublicKey : public BasicWrapper { > Are these classes specific to RSA keys? Would we want to reuse them if we There is some specific due to FromBIO/ToBIO at least. Yes, making this template is an option, and I was thinking to do that later when adding EC keys. At that stage, we would have better understanding what is needed here. Let me just change RSAPrivateKey --> RsaPrivateKey at this phase. PS6, Line 33: RawDataType > I think you should be able to use EVP_PKEY instead of RawDataType everywher Done Line 40: Status FromBIO(BIO* bio, DataFormat format) override; > given that this is just moved code I think my preference is not to change i ok -- let me to that in a separate patch: I'll do that for all crypto- and cert-related code in the security library. PS6, Line 71: template<typename KeyType> : S > hrm, I think I remember asking but I can't find the answer now... why's thi This is a bit of forward thinking -- I was thinking that using ECDHA keys for token signing might be a good option. That's why the RSA prefix for keys and template method here. http://gerrit.cloudera.org:8080/#/c/5783/6/src/kudu/security/openssl_util.h File src/kudu/security/openssl_util.h: Line 95: enum class DataFormat { > eh, I think this is used everywhere, like keys too, right? Yep, I would prefer to keep it here since it's common for both keys & certs. -- To view, visit http://gerrit.cloudera.org:8080/5783 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8ac61a08c8165152d0046468c0fa655131ff8fef Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
