Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16657 )
Change subject: KUDU-3210 Add option to enforce FIPS approved mode ...................................................................... Patch Set 6: (4 comments) http://gerrit.cloudera.org:8080/#/c/16657/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16657/6//COMMIT_MSG@9 PS6, Line 9: Kudu now logs if FIPS approved mode is : enabled It seems this is no longer true in PS8 unless verbose debug level is set to 2 or higher. http://gerrit.cloudera.org:8080/#/c/16657/6//COMMIT_MSG@10 PS6, Line 10: KUDU_REQUIRE_FIPS_MODE Thinking about this approach once more made me curios about the anticipated use cases where we need to enforce FIPS-enabled mode directly on the client side. My thoughts are the following: if in a Kudu cluster the FIPS-mode is enforced on the server side only, that automatically makes only FIPS-approved ciphers and the rest TLS-related stuff available to the client side for TLS-related communications. The client would not be able to communicate with a FIPS-enabled server side because servers would refuse to use other non-FIPS ciphers and algorithms even if they are available at the client side. With that, it seems that an environment variable is necessary only if we want to enforce using FIPS-approved stuff in a Kudu cluster where the server side is running in non-FIPS mode. Do we ever want to handle such a situation at all? If not, maybe we are better off with a gflag to enforce FIPS compliance only on the server side? http://gerrit.cloudera.org:8080/#/c/16657/6/src/kudu/security/openssl_util.cc File src/kudu/security/openssl_util.cc: http://gerrit.cloudera.org:8080/#/c/16657/6/src/kudu/security/openssl_util.cc@196 PS6, Line 196: auto require_fips_mode = getenv("KUDU_REQUIRE_FIPS_MODE"); : if (require_fips_mode && strcmp("1", require_fips_mode) == 0) { > Yea I just found it, should I move it to some other util? Yep: that's some extra work, but I guess it's worth it once we started evaluated boolean env variables like that in non-test code as well. I think kudu_util might be a good place to move GetBooleanEnvironmentVariable() into since it's linked both into kudu_client and kserver links kudu_util as well (master and tserver link kserver library). http://gerrit.cloudera.org:8080/#/c/16657/6/src/kudu/security/openssl_util.cc@198 PS6, Line 198: CHECK(fips_mode) << "FIPS mode required by environment variable " : "KUDU_REQUIRE_FIPS_MODE, but it is not enabled."; > Right, so you mean running this test conditionally, only if we're not runni I think ideally the test should make sure that when KUDU_REQUIRE_FIPS_MODE environment variable is set then a Kudu application: (a) does not crash when run in FIPS mode (b) does crash when run in non-FIPS mode I guess it's easier to implement the mentioned checks in a separate scenarios, where each scenario is run conditionally depending on FIPS mode, yes. The point is to have a unit-like test for the expected behavior related to the newly introduced environment variable. I guess (a) might be covered as well by some integration-level test to run a FIPS-enabled environment, but I guess (b) is a perfect candidate to have unit-like test coverage only. -- 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: 6 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: Thu, 29 Oct 2020 18:08:32 +0000 Gerrit-HasComments: Yes
