Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16657 )
Change subject: Add option to enforce FIPS approved mode ...................................................................... Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/16657/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16657/4//COMMIT_MSG@11 PS4, Line 11: This needs to be an environment variable : instead of a flag so that this check can be used in the C++ client as : well. > nit: can you add this as a comment around where the environment variable ge Done http://gerrit.cloudera.org:8080/#/c/16657/4/src/kudu/security/openssl_util.cc File src/kudu/security/openssl_util.cc: http://gerrit.cloudera.org:8080/#/c/16657/4/src/kudu/security/openssl_util.cc@130 PS4, Line 130: "KUDU_REQUIRE_FIPS_MODE" > Do we need to check what its value is? I thought it should be enough to check if the variable is set, but you're right it's better to make sure someone doesn't try to specifically disable it by setting it to "0" or something. http://gerrit.cloudera.org:8080/#/c/16657/4/src/kudu/security/openssl_util.cc@131 PS4, Line 131: require > nit: required Done http://gerrit.cloudera.org:8080/#/c/16657/4/src/kudu/security/openssl_util.cc@131 PS4, Line 131: : > nit: drop the ":"? Done http://gerrit.cloudera.org:8080/#/c/16657/4/src/kudu/security/openssl_util.cc@130 PS4, Line 130: if (getenv("KUDU_REQUIRE_FIPS_MODE")) { : CHECK(fips_mode) << ": FIPS mode require by environment variable " : "KUDU_REQUIRE_FIPS_MODE, but it is not enabled."; > If this is so important in case if Kudu initialized the OpenSSL library, I I'm not sure about Python, it seems this is not called there, but in the C++ client it is. Running the C++ example on my Mac with KUDU_REQUIRE_FIPS_MODE set to 1 crashes: charger@abukor-mbp16 cpp % KUDU_REQUIRE_FIPS_MODE=1 ./example abukor-kudu-fips-1.vpc.cloudera.com Running with Kudu client version: kudu 1.14.0-SNAPSHOT (rev 417ac51bd3881cb65ddb4b2ec9396358124f9897-dirty) Long version info: kudu 1.14.0-SNAPSHOT revision 417ac51bd3881cb65ddb4b2ec9396358124f9897-dirty build type DEBUG built by charger at 28 Oct 2020 12:37:40 CET on abukor-mbp16.local F1028 12:59:06.543414 342699456 openssl_util.cc:132] Check failed: fips_mode FIPS mode required by environment variable KUDU_REQUIRE_FIPS_MODE, but it is not enabled. Received log message from Kudu client library Severity: 3 Filename: ../../src/kudu/security/openssl_util.cc Line number: 132 Time: Wed Oct 28 12:59:06 2020 Message: Check failed: fips_mode FIPS mode required by environment variable KUDU_REQUIRE_FIPS_MODE, but it is not enabled. *** Check failure stack trace: *** @ 0x108c4334f google::LogMessageFatal::~LogMessageFatal() @ 0x108c40299 google::LogMessageFatal::~LogMessageFatal() @ 0x108cb90bc kudu::security::(anonymous namespace)::DoInitializeOpenSSL() @ 0x7fff65147bea std::__1::__call_once() @ 0x108cb8e3c _ZNSt3__1L9call_onceIRFvvEJEEEvRNS_9once_flagEOT_DpOT0_ @ 0x108cb8dd7 kudu::security::InitializeOpenSSL() @ 0x108cc0d28 kudu::security::TlsContext::TlsContext() @ 0x108cc0dd5 kudu::security::TlsContext::TlsContext() @ 0x1087f67d2 kudu::rpc::Messenger::Messenger() @ 0x1087f3d3d kudu::rpc::Messenger::Messenger() @ 0x1087f32bb kudu::rpc::MessengerBuilder::Build() @ 0x1085a672f kudu::client::KuduClientBuilder::Build() @ 0x10854b846 CreateClient() @ 0x108549868 main @ 0x7fff67e46cc9 start @ 0x2 (unknown) zsh: abort KUDU_REQUIRE_FIPS_MODE=1 ./example abukor-kudu-fips-1.vpc.cloudera.com As for whether this check should be done even if g_disable_ssl_init is true, I'm not convinced, although I'm not sure what the correct behavior should be. If it is set to true, we don't init OpenSSL and expect the client to handle everything. http://gerrit.cloudera.org:8080/#/c/16657/4/src/kudu/security/openssl_util.cc@132 PS4, Line 132: > nit: spacing Done -- To view, visit http://gerrit.cloudera.org:8080/16657 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93 Gerrit-Change-Number: 16657 Gerrit-PatchSet: 4 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Wed, 28 Oct 2020 14:00:57 +0000 Gerrit-HasComments: Yes
