Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16658 )
Change subject: KUDU-3210 Increase key size in tests and EMC ...................................................................... Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/16658/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16658/3//COMMIT_MSG@11 PS3, Line 11: This commit removes the test-only key size > The master test, for example, has an assertion that would fail depending on OK, then that's the only difference in https://gerrit.cloudera.org/c/16658/3/src/kudu/master/master-test.cc#1970 so far. I guess developers adding new tests now and in the future don't need to worry about key sizes, unless that's something directly related to encryption and TLS. If they touch something related to TLS and encryption, by definition they should know what they are doing and should care about key size, so I'm not buying your argument. I think the verification that our tests pass in FIPS-approved mode doesn't require running these on pre-commit builds and elsewhere. Running FIPS-enabled mode time to time and gating a new release on the results seems like a viable approach to me. I think that unit tests and other tests we have in our test suite are targeted to verify specific functionality and features, and track regressions, if any. I don't think we have a need to run _all_ the suite with longer keys all the time. If there a need to test some particular crypto-related aspects of using TLS and wire encryption in Kudu, let's figure out what we need to focus and add a targeted scenario that would run as a pre-commit verification. To mirror real-life behavior we should run integration-level tests. Unit tests are not exactly for that purpose, IMO. http://gerrit.cloudera.org:8080/#/c/16658/3//COMMIT_MSG@11 PS3, Line 11: This commit removes the test-only key size > In the meantime I ran the full test suite (without slow tests) serially on Thank you for benchmarking those! I guess perf statistics for ctest itself isn't that important in this context (rather it's important to see how each test affected since that's where the crypto ops are actually taking place), but time elapsed gives us good information on what's the overall increase in time for the test suite. BTW, I also run the suite several times at CentOS 7.4 with OpenSSL 1.0.2k-fips 26 Jan 2017, both in FIPS mode with the patch and in non-FIPS mode without the patch, in both cases with KUDU_ALLOW_SLOW_TESTS=1, RELEASE build configuration. I ran them as 'ctest -j4' on a machine that has more than 4 CPU cores available. I got the following results from the latest runs (I also confirmed that the variation from run to run was very small): non-FIPS mode, without the patch: Total Test time (real) = 7707.15 sec FIPS mode, with the patch: Total Test time (real) = 9319.28 sec The difference is about 1.21 times increase in run-time, i.e. more than 20% increase from current. With that, I'm not sure we want to increase runtime of the tests we run at every pre-commit check, even if we run those via dist-test. http://gerrit.cloudera.org:8080/#/c/16658/3/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/16658/3/src/kudu/master/master-test.cc@a1970 PS3, Line 1970: Why don't to keep this comment explaining the semantics behind the magic number of the signature size? Yes, it changes with the key size, but I think there is a simple correspondence between the RSA key size of the size of the signature if using SHA256 digest. -- To view, visit http://gerrit.cloudera.org:8080/16658 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I45b96e0b4499ea1d80db5871a529c732ad41220e Gerrit-Change-Number: 16658 Gerrit-PatchSet: 3 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Alexey Serbin <[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: Mon, 02 Nov 2020 18:22:13 +0000 Gerrit-HasComments: Yes
