Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23429 )
Change subject: OpenSSL 3.x compatibility adaptation ...................................................................... Patch Set 7: (8 comments) http://gerrit.cloudera.org:8080/#/c/23429/7/src/kudu/security/crypto.cc File src/kudu/security/crypto.cc: http://gerrit.cloudera.org:8080/#/c/23429/7/src/kudu/security/crypto.cc@340 PS7, Line 340: auto key = ssl_make_unique(EVP_PKEY_new()); Is this needed for OpenSSL 3.0 and newer? http://gerrit.cloudera.org:8080/#/c/23429/7/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/23429/7/src/kudu/util/jwt-util.cc@146 PS7, Line 146: struct EvpPublicKeyTraits { : static constexpr auto kFreeFunc = &EVP_PKEY_free; : static constexpr auto kWritePemFunc = &PEM_write_bio_PUBKEY; : static constexpr auto kWriteDerFunc = &i2d_PUBKEY_bio; : }; Is it possible to reconcile this and SslTypeTraits<EVP_PKEY> from openssl_util.h, so SslTypeTraits<EVP_PKEY> might be used here instead of this EvpPublicKeyTraits? http://gerrit.cloudera.org:8080/#/c/23429/7/src/kudu/util/jwt-util.cc@462 PS7, Line 462: auto mod = ssl_make_unique(BN_bin2bn( : reinterpret_cast<const unsigned char*>(str_n.c_str()), : static_cast<int>(str_n.size()), : nullptr)); : auto exp = ssl_make_unique(BN_bin2bn( : reinterpret_cast<const unsigned char*>(str_e.c_str()), : static_cast<int>(str_e.size()), : nullptr)); This part looks exactly the same as at lines 494 -- 501. Is it possible to reuse it? http://gerrit.cloudera.org:8080/#/c/23429/7/src/kudu/util/jwt-util.cc@488 PS7, Line 488: rc Isn't it necessary to check 'rc' for an error? http://gerrit.cloudera.org:8080/#/c/23429/7/src/kudu/util/jwt-util.cc@615 PS7, Line 615: auto x = ssl_make_unique(BN_bin2bn( : reinterpret_cast<const unsigned char*>(ascii_x.c_str()), : static_cast<int>(ascii_x.size()), : nullptr)); : auto y = ssl_make_unique(BN_bin2bn( : reinterpret_cast<const unsigned char*>(ascii_y.c_str()), : static_cast<int>(ascii_y.size()), : nullptr)); This block looks exactly the same as at lines 678-685. Is it possible to reuse it? http://gerrit.cloudera.org:8080/#/c/23429/7/src/kudu/util/jwt-util.cc@664 PS7, Line 664: OPENSSL_CHECK_OK(OSSL_PKEY_PARAM_PUB_KEY != nullptr); : OPENSSL_CHECK_OK(OSSL_PARAM_BLD_push_octet_string( nit: please add a comment to explain why these shouldn't fail http://gerrit.cloudera.org:8080/#/c/23429/7/src/kudu/util/jwt-util.cc@672 PS7, Line 672: rc Isn't it necessary to check for an error here? http://gerrit.cloudera.org:8080/#/c/23429/7/src/kudu/util/openssl_util.cc File src/kudu/util/openssl_util.cc: http://gerrit.cloudera.org:8080/#/c/23429/7/src/kudu/util/openssl_util.cc@251 PS7, Line 251: #if OPENSSL_VERSION_NUMBER >= 0x10100000L : auto ctx = ssl_make_unique(SSL_CTX_new(TLS_method())); : #else : auto ctx = ssl_make_unique(SSL_CTX_new(SSLv23_method())); : #endif If this is already under the 'else' part of the '#if OPENSSL_VERSION_NUMBER >= 0x10100000L ... #else ...' outer macro clause, why to add this? -- To view, visit http://gerrit.cloudera.org:8080/23429 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic587a85e6b9088ffd353f9119b75431f1ec60b5c Gerrit-Change-Number: 23429 Gerrit-PatchSet: 7 Gerrit-Owner: Yan-Daojiang <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yan-Daojiang <[email protected]> Gerrit-Comment-Date: Tue, 30 Sep 2025 05:08:15 +0000 Gerrit-HasComments: Yes
