You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Attila Bukor (Code Review)" <ge...@cloudera.org> on 2020/10/27 13:54:07 UTC
[kudu-CR] Increase key size in tests and EMC
Attila Bukor has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16658
Change subject: Increase key size in tests and EMC
......................................................................
Increase key size in tests and EMC
The cryptographic key sizes were lowered in tests to speed up execution.
As FIPS approved mode requires larger key sizes, these tests fail with
the small key size. This commit removes the test-only key size
reductions in test util and ExternalMiniCluster to make sure that a) the
tests pass; b) if we decide to default to a small keysize somewhere in
the production code, it would be caught by some tests when running in
FIPS approved mode.
Change-Id: I45b96e0b4499ea1d80db5871a529c732ad41220e
---
M src/kudu/integration-tests/authz_token-itest.cc
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/security-unknown-tsk-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/token-test.cc
M src/kudu/tserver/tablet_server_authorization-test.cc
M src/kudu/util/test_util.cc
9 files changed, 8 insertions(+), 45 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/16658/1
--
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: newchange
Gerrit-Change-Id: I45b96e0b4499ea1d80db5871a529c732ad41220e
Gerrit-Change-Number: 16658
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3210 Increase key size in tests and EMC
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor 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 13:
(6 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 introduces a
> Thank you for benchmarking those! I guess perf statistics for ctest itself
As agreed on the call yesterday, I put all these behind an environment variable.
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 nu
Done
http://gerrit.cloudera.org:8080/#/c/16658/12/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/16658/12/src/kudu/mini-cluster/external_mini_cluster.cc@1011
PS12, Line 1011: are
> nit: are not ? Or that was a part of an incomplete sentence?
Done
http://gerrit.cloudera.org:8080/#/c/16658/12/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:
http://gerrit.cloudera.org:8080/#/c/16658/12/src/kudu/util/test_util.cc@119
PS12, Line 119: // necessary to override it to use the key length specified above (which are
: // considered lax and don't work in case of security level 2 or higher).
: flags_for_tests.emplace("openssl_security_level_override", "0");
> nit: do you mind moving this to precede the line with openssl_security_leve
Done
http://gerrit.cloudera.org:8080/#/c/16658/12/src/kudu/util/test_util.cc@129
PS12, Line 129: }
> nit: it seems unnecessary extra indent was added
Done
http://gerrit.cloudera.org:8080/#/c/16658/12/src/kudu/util/test_util.cc@133
PS12, Line 133: FLAGS_log_dir = GetTestDataDirectory();
> I guess this needs to be set only once, not sure we need to place it under
Done
--
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: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Nov 2020 19:46:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] Increase key size in tests and EMC
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16658 )
Change subject: Increase key size in tests and EMC
......................................................................
Patch Set 5:
(2 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
> Given that the majority of Kudu test runs (pre-commit, benchmarks, etc.) ar
Grant and I were discussing more regular, potentially even pre-commit builds on a FIPS-approved Docker environment. In addition to making these tests pass, if we don't use different key sizes in tests, we can flag potential issues in the production code when running the test suite in FIPS-approved mode.
Also, I wouldn't compare this to slow tests, this build took 46 minutes which is well within the normal range for the time it takes to run them: http://jenkins.kudu.apache.org/job/kudu-gerrit/buildTimeTrend
As for benchmarks, I know it's good to compare them over time, but I also believe that they should be as close to reality as they can be, and choosing a small key size isn't realistic.
http://gerrit.cloudera.org:8080/#/c/16658/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16658/4//COMMIT_MSG@10
PS4, Line 10: these tests
> In addition to these tests, did you try to run Java tests as well? I remem
I did, all tests passed even with this added to tests.gradle:
jvmArgs += "-Dcom.safelogic.cryptocomply.fips.approved_only=true"
jvmArgs += "-Djdk.tls.ephemeralDHKeySize=2048"
The only security-related tweaks I found in MiniKuduCluster are these:
Security.setProperty("jdk.certpath.disabledAlgorithms", "MD2, RC4, MD5");
Security.setProperty("jdk.tls.disabledAlgorithms", "SSLv3, RC4, MD5");
https://github.com/apache/kudu/blob/master/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java#L150-L151
This is disabling insecure algorithms only, so it should be fine for FIPS 140-2.
--
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: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 28 Oct 2020 14:01:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3210 Increase key size in tests and EMC
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor 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 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16658/3/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/16658/3/src/kudu/mini-cluster/external_mini_cluster.cc@a990
PS3, Line 990:
> I'm not sure I understand this correctly: if FIPS 140-2 requires minimum RS
Hm interesting. I'm not sure to be honest. It was definitely necessary to change them, I found where I need to change it by looking at the failed test cases. I'll try running these tests on CentOS 8.
--
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: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Oct 2020 19:44:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3210 Increase key size in tests and EMC
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Grant Henke, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16658
to look at the new patch set (#8).
Change subject: KUDU-3210 Increase key size in tests and EMC
......................................................................
KUDU-3210 Increase key size in tests and EMC
The cryptographic key sizes were lowered in tests to speed up execution.
As FIPS approved mode requires larger key sizes, these tests fail with
the small key size. This commit removes the test-only key size
reductions in test util and ExternalMiniCluster to make sure that a) the
tests pass; b) if we decide to default to a small keysize somewhere in
the production code, it would be caught by some tests when running in
FIPS approved mode.
Change-Id: I45b96e0b4499ea1d80db5871a529c732ad41220e
---
M src/kudu/integration-tests/authz_token-itest.cc
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/security-unknown-tsk-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/token-test.cc
M src/kudu/tserver/tablet_server_authorization-test.cc
M src/kudu/util/test_util.cc
9 files changed, 9 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/16658/8
--
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: newpatchset
Gerrit-Change-Id: I45b96e0b4499ea1d80db5871a529c732ad41220e
Gerrit-Change-Number: 16658
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-3210 Increase key size in tests and EMC
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor 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 10:
(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 what the key size is set to: https://gerrit.cloudera.org/c/16658/3/src/kudu/master/master-test.cc#1970
There are other examples in this patch that are more straight-forward, but developers adding new tests in the future would still need to make sure they use two different key sizes, both chosen correctly.
> I'm cautious about bumping up key sizes because I don't know how much impact that would be for our tests. If taking about speed and what part of overall costs of benchmarks crypto-related stuff is taking, I'm curious how much is that if running the whole suite of the tests on the same machine with short and FIPS-required key length? Or at least, maybe the the following tests:
Of course, there is a significant difference in the runtime of some of the tests you mention:
Performance counter stats for './bin/token-test': Performance counter stats for './bin/token-test':
12.309491423 seconds time elapsed | 15.027583910 seconds time elapsed
Performance counter stats for './bin/registration-test': Performance counter stats for './bin/registration-test':
3.844352371 seconds time elapsed | 10.652710483 seconds time elapsed
Performance counter stats for './bin/tls_socket-test': Performance counter stats for './bin/tls_socket-test':
4.630019880 seconds time elapsed | 4.626133437 seconds time elapsed
Performance counter stats for './bin/token_signer-itest': Performance counter stats for './bin/token_signer-itest':
7.316916991 seconds time elapsed | 10.807419288 seconds time elapsed
Performance counter stats for './bin/master_cert_authority-i Performance counter stats for './bin/master_cert_authority-i
13.531314727 seconds time elapsed | 18.915764233 seconds time elapsed
I'm just saying this makes up for only a small part of the overall runtime of the test suite. I mentioned above in this thread that on Jenkins, the runtime was 46 minutes which is well within the normal range. I compared perf stats on these test
> I guess we are interested to see not only time taken, but also CPU consumed (so, running some of the tests above under perf-stat might be a good idea as well). I'd think that if the overall increase is less than 3%, we probably should switch to bigger keys.
I understand we care about even small performance impacts, and I agree that we should, in production. This is, however, a test-only change, that in addition to fulfilling its primary goal to make the tests pass in FIPS-approved mode, it also changes the tests' behavior to mirror real-life behavior more closely.
http://gerrit.cloudera.org:8080/#/c/16658/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16658/4//COMMIT_MSG@10
PS4, Line 10: these tests
> I see; I thought those tweaks were necessary to pass (I missed the importan
Done
http://gerrit.cloudera.org:8080/#/c/16658/3/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/16658/3/src/kudu/mini-cluster/external_mini_cluster.cc@a990
PS3, Line 990:
> I'd suggest you run the tests on RHEL/CentOS 8.1 if you are about to remove
FIPS 140-2 does too, and that is our default RSA key size too. This serves to underline my point though - if we use the default key sizes in security levels in our tests, it's easier to catch real problems.
--
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: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Oct 2020 16:18:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3210 Increase key size in tests and EMC
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16658/3/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/16658/3/src/kudu/mini-cluster/external_mini_cluster.cc@a990
PS3, Line 990:
> FIPS 140-2 does too, and that is our default RSA key size too. This serves
I'm not sure I understand this correctly: if FIPS 140-2 requires minimum RSA key size of 2048, how are we getting away with RSA key size 1024 in this patch (there several places where the size was bumped from 512 to 1024)? Does that mean it wasn't necessary to change those key sizes from 512 to 1024 then?
--
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 <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Oct 2020 19:04:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] Increase key size in tests and EMC
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16658 )
Change subject: Increase key size in tests and EMC
......................................................................
Patch Set 4:
(2 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
> Grant and I were discussing more regular, potentially even pre-commit build
As I understand, adding a gate like KUDU_ALLOW_SLOW_TESTS would allow to run pre-commit builds in FIPS-approved or whatever environment is, so it wouldn't hurt. And as you mentioned, running test with different key length might show potential issues, so adding a gate for different key length is a win-win, IMO.
http://gerrit.cloudera.org:8080/#/c/16658/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16658/4//COMMIT_MSG@10
PS4, Line 10: these tests
> I did, all tests passed even with this added to tests.gradle:
Cool! Why not to include those changes for tests.gradle in this changelist as well?
--
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: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 28 Oct 2020 19:09:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3210 Increase key size in tests and EMC
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16658/3/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/16658/3/src/kudu/mini-cluster/external_mini_cluster.cc@a990
PS3, Line 990:
> Yep, it would be great if you could verify how this patch works on CentOS 8
From the other side, that information comes from the documentation of NSS module :) Not sure how much relevance is there w.r.t. OpenSSL library and its FIPS module.
--
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 <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Oct 2020 23:32:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3210 Increase key size in tests and EMC
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16658/3/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/16658/3/src/kudu/mini-cluster/external_mini_cluster.cc@a990
PS3, Line 990:
> Hm interesting. I'm not sure to be honest. It was definitely necessary to c
Yep, it would be great if you could verify how this patch works on CentOS 8. Last time I ran the whole set of tests on CentOS 8.1 a few month ago it passed. Unfortunately, I don't have a machine with CentOS 8.1 anymore. Maybe, we could try to leave the override for security level even with FIPS-enabled library?
BTW, I tried to run a few unit tests under OpenSSL 1.0.2k-fips 26 Jan 2017 on CentOS 7.4 with FIPS mode enabled. Indeed, 1024 bit RSA keys are accepted there. However, 512 and 768 bit keys were rejected with the `key too short` error. So, indeed: even if the declared minimum is 2048 bits for RSA keys in FIPS 140-2, 1024 bit keys are considered OK, at least for signature generation and verification. And that matches the information in section '9.2.2. RSA and DSA keys' of this document: https://csrc.nist.gov/csrc/media/projects/cryptographic-module-validation-program/documents/security-policies/140sp2564.pdf :
The Module allows the use of 1024 bit RSA and DSA keys for legacy purposes, including signature generation.
[ RUN ] CertManagementTest.SignerInitWithMismatchedCertAndKey [1557/1903]
WARNING: Logging before InitGoogleLogging() is written to STDERR
F1030 15:46:34.131778 455785 cert_management-test.cc:75] Check failed: _s.ok() Bad status: Runtime e
rror: error generating RSA key: error:2D07406C:FIPS routines:RSA_BUILTIN_KEYGEN:key too short:rsa_ge
n.c:438
--
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 <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Oct 2020 23:18:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] Increase key size in tests and EMC
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16658 )
Change subject: Increase key size in tests and EMC
......................................................................
Patch Set 6:
(2 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
> As I understand, adding a gate like KUDU_ALLOW_SLOW_TESTS would allow to ru
Hm I guess that would make sense. Do you think KUDU_REQUIRE_FIPS_MODE should be reused for this or should we have a separate test-only env var?
http://gerrit.cloudera.org:8080/#/c/16658/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16658/4//COMMIT_MSG@10
PS4, Line 10: these tests
> Cool! Why not to include those changes for tests.gradle in this changelist
These aren't required to make the tests pass in a FIPS environment, I only added them to simulate a more strict environment. Furthermore, the first one only makes sense if SafeLogic's security policies and classes are used.
--
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: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 28 Oct 2020 19:26:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3210 Increase key size in tests and EMC
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 13: Code-Review+2
(1 comment)
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 introduces a
> As agreed on the call yesterday, I put all these behind an environment vari
Thank you!
--
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: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Nov 2020 20:18:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3210 Increase key size in tests and EMC
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 14: Code-Review+2
--
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: 14
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Nov 2020 22:17:36 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3210 Increase key size in tests and EMC
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Grant Henke, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16658
to look at the new patch set (#12).
Change subject: KUDU-3210 Increase key size in tests and EMC
......................................................................
KUDU-3210 Increase key size in tests and EMC
The cryptographic key sizes were lowered in tests to speed up execution.
As FIPS approved mode requires larger key sizes, these tests fail with
the small key size. This commit introduces a
KUDU_USE_LARGE_KEYS_IN_TESTS environment variable for tests. If it's set
to true, the test-only key size restrictions are removed.
Change-Id: I45b96e0b4499ea1d80db5871a529c732ad41220e
---
M src/kudu/integration-tests/authz_token-itest.cc
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/security-unknown-tsk-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/token-test.cc
M src/kudu/tserver/tablet_server_authorization-test.cc
M src/kudu/util/test_util.cc
M src/kudu/util/test_util.h
10 files changed, 68 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/16658/12
--
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: newpatchset
Gerrit-Change-Id: I45b96e0b4499ea1d80db5871a529c732ad41220e
Gerrit-Change-Number: 16658
Gerrit-PatchSet: 12
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-3210 Increase key size in tests and EMC
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor 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 11:
(1 comment)
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
In the meantime I ran the full test suite (without slow tests) serially on a single host, here's the difference:
Before (cbdd66a6a97b0baa286530c67d6c204d3a070396):
Performance counter stats for 'ctest':
13155631.327516 task-clock (msec) # 2.493 CPUs utilized
31,113,793 context-switches # 0.002 M/sec
2,011,761 cpu-migrations # 0.153 K/sec
108,051,423 page-faults # 0.008 M/sec
26,531,408,698,301 cycles # 2.017 GHz
27,598,427,400,957 instructions # 1.04 insn per cycle
5,460,757,077,779 branches # 415.089 M/sec
135,745,019,971 branch-misses # 2.49% of all branches
5276.523142389 seconds time elapsed
After (fd9f2ed172936d351932bd884ae4bc7270323d09):
Performance counter stats for 'ctest':
14133059.898799 task-clock (msec) # 2.354 CPUs utilized
31,297,801 context-switches # 0.002 M/sec
2,005,327 cpu-migrations # 0.142 K/sec
108,193,193 page-faults # 0.008 M/sec
28,630,693,494,760 cycles # 2.026 GHz
30,878,714,897,060 instructions # 1.08 insn per cycle
5,628,824,177,225 branches # 398.274 M/sec
137,309,827,516 branch-misses # 2.44% of all branches
6003.471902752 seconds time elapsed
The difference is somewhat more significant than I expected, but I think this is still worth it. We normally use dist-test if we run the whole test suite, except before releases.
--
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: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Oct 2020 17:11:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] Increase key size in tests and EMC
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16658 )
Change subject: Increase key size in tests and EMC
......................................................................
Patch Set 7:
(1 comment)
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
I've been pondering over this and think it's still weird to gate this. We wouldn't need to gate only some startup flags, but we would need to use different assertions based on this in various tests. I feel this would make things needlessly complicated.
> And as you mentioned, running test with different key length might show potential issues, so adding a gate for different key length is a win-win, IMO.
We don't need to care about small key sizes though, we only need to make sure that we support, and default to key sizes that and algorithms that are considered secure by modern standards.
As I understand, the main concern here, at least based on comments such as the one below is speed:
// Generate smaller RSA keys -- generating a 768-bit key is faster
// than generating the default 2048-bit key, and we don't care about
// strong encryption in tests. Setting it lower (e.g. 512 bits) results
// in OpenSSL errors RSA_sign:digest too big for rsa key:rsa_sign.c:122
// since we are using strong/high TLS v1.2 cipher suites, so the minimum
// size of TLS-related RSA key is 768 bits (due to the usage of
// the ECDHE-RSA-AES256-GCM-SHA384 suite).
While it is true that generating and using larger keys is technically more expensive, it doesn't seem to cause any problems for us, and it shouldn't either, as it's still fast enough[1], and the resources spent on crypto is only a tiny part of the overall costs of tests and benchmarks. I'd be surprised if the difference would be statistically significant over our benchmark and test suites.
The above comment also mentions that it's set to the minimum key size required by TLS v1.2 cipher suites that we support. We can think of this as raising the tested key sizes to the minimum as required by FIPS 140-2, which is also something that we should support.
[1] https://blog.cloudflare.com/how-expensive-is-crypto-anyway/
--
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: 7
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Oct 2020 07:40:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] Increase key size in tests and EMC
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16658 )
Change subject: Increase key size in tests and EMC
......................................................................
Patch Set 4:
(2 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
Given that the majority of Kudu test runs (pre-commit, benchmarks, etc.) are run non-FIPS enabled environment, I don't think it makes sense to have this blanket-style solution. What does it buy us besides making these tests pass in a FIPS-enabled environment, which is once-in-a-while manual run?
Instead, I'd rather vote for bumping up key size only if some special variable is provided, something similar to KUDU_ALLOW_SLOW_TESTS.
http://gerrit.cloudera.org:8080/#/c/16658/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16658/4//COMMIT_MSG@10
PS4, Line 10: these tests
In addition to these tests, did you try to run Java tests as well? I remember we did some tweaks to default Java security policies in java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java
I'm just curious whether FIPS-enabled mode somehow affects the Java (client) side as well given we added those tweaks.
--
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: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 27 Oct 2020 18:33:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3210 Increase key size in tests and EMC
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 12: Code-Review+1
(4 comments)
Overall looks good to me, a few nits.
http://gerrit.cloudera.org:8080/#/c/16658/12/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/16658/12/src/kudu/mini-cluster/external_mini_cluster.cc@1011
PS12, Line 1011: are
nit: are not ? Or that was a part of an incomplete sentence?
http://gerrit.cloudera.org:8080/#/c/16658/12/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:
http://gerrit.cloudera.org:8080/#/c/16658/12/src/kudu/util/test_util.cc@119
PS12, Line 119: // Some OS distros set the default security level higher than 0, so it's
: // necessary to override it to use the key length specified above (which are
: // considered lax and don't work in case of security level 2 or higher).
nit: do you mind moving this to precede the line with openssl_security_level_override setting, so it's follows the same notation as other documented flags?
http://gerrit.cloudera.org:8080/#/c/16658/12/src/kudu/util/test_util.cc@129
PS12, Line 129:
nit: it seems unnecessary extra indent was added
http://gerrit.cloudera.org:8080/#/c/16658/12/src/kudu/util/test_util.cc@133
PS12, Line 133: FLAGS_log_dir = GetTestDataDirectory();
I guess this needs to be set only once, not sure we need to place it under the this 'for ()' cycle.
--
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: 12
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Nov 2020 18:05:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] Increase key size in tests and EMC
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16658 )
Change subject: Increase key size in tests and EMC
......................................................................
Patch Set 3:
(1 comment)
> Patch Set 1:
>
> (1 comment)
http://gerrit.cloudera.org:8080/#/c/16658/1/src/kudu/integration-tests/security-unknown-tsk-itest.cc
File src/kudu/integration-tests/security-unknown-tsk-itest.cc:
http://gerrit.cloudera.org:8080/#/c/16658/1/src/kudu/integration-tests/security-unknown-tsk-itest.cc@125
PS1, Line 125: static Status GenerateTsk(TokenSigningPrivateKeyPB* tsk, int64_t seq_num = 100) {
> warning: method 'GenerateTsk' can be made static [readability-convert-membe
Done
--
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 <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 27 Oct 2020 15:54:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3210 Increase key size in tests and EMC
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Grant Henke, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16658
to look at the new patch set (#13).
Change subject: KUDU-3210 Increase key size in tests and EMC
......................................................................
KUDU-3210 Increase key size in tests and EMC
The cryptographic key sizes were lowered in tests to speed up execution.
As FIPS approved mode requires larger key sizes, these tests fail with
the small key size. This commit introduces a
KUDU_USE_LARGE_KEYS_IN_TESTS environment variable for tests. If it's set
to true, the test-only key size restrictions are removed.
Change-Id: I45b96e0b4499ea1d80db5871a529c732ad41220e
---
M src/kudu/integration-tests/authz_token-itest.cc
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/security-unknown-tsk-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/token-test.cc
M src/kudu/tserver/tablet_server_authorization-test.cc
M src/kudu/util/test_util.cc
M src/kudu/util/test_util.h
10 files changed, 63 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/16658/13
--
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: newpatchset
Gerrit-Change-Id: I45b96e0b4499ea1d80db5871a529c732ad41220e
Gerrit-Change-Number: 16658
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-3210 Increase key size in tests and EMC
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 02 Nov 2020 18:22:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3210 Increase key size in tests and EMC
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16658 )
Change subject: KUDU-3210 Increase key size in tests and EMC
......................................................................
KUDU-3210 Increase key size in tests and EMC
The cryptographic key sizes were lowered in tests to speed up execution.
As FIPS approved mode requires larger key sizes, these tests fail with
the small key size. This commit introduces a
KUDU_USE_LARGE_KEYS_IN_TESTS environment variable for tests. If it's set
to true, the test-only key size restrictions are removed.
Change-Id: I45b96e0b4499ea1d80db5871a529c732ad41220e
Reviewed-on: http://gerrit.cloudera.org:8080/16658
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/integration-tests/authz_token-itest.cc
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/security-unknown-tsk-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/token-test.cc
M src/kudu/tserver/tablet_server_authorization-test.cc
M src/kudu/util/test_util.cc
M src/kudu/util/test_util.h
10 files changed, 63 insertions(+), 41 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: I45b96e0b4499ea1d80db5871a529c732ad41220e
Gerrit-Change-Number: 16658
Gerrit-PatchSet: 15
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] Increase key size in tests and EMC
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Grant Henke, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16658
to look at the new patch set (#2).
Change subject: Increase key size in tests and EMC
......................................................................
Increase key size in tests and EMC
The cryptographic key sizes were lowered in tests to speed up execution.
As FIPS approved mode requires larger key sizes, these tests fail with
the small key size. This commit removes the test-only key size
reductions in test util and ExternalMiniCluster to make sure that a) the
tests pass; b) if we decide to default to a small keysize somewhere in
the production code, it would be caught by some tests when running in
FIPS approved mode.
Change-Id: I45b96e0b4499ea1d80db5871a529c732ad41220e
---
M src/kudu/integration-tests/authz_token-itest.cc
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/security-unknown-tsk-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/token-test.cc
M src/kudu/tserver/tablet_server_authorization-test.cc
M src/kudu/util/test_util.cc
9 files changed, 9 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/16658/2
--
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: newpatchset
Gerrit-Change-Id: I45b96e0b4499ea1d80db5871a529c732ad41220e
Gerrit-Change-Number: 16658
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-3210 Increase key size in tests and EMC
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke 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 15: Code-Review+2
This is likely a separate patch, but do we also want to add KUDU_USE_LARGE_KEYS_IN_TESTS support to the Java tests?
It might just be removing this section when KUDU_USE_LARGE_KEYS_IN_TESTS is true: https://github.com/apache/kudu/blob/3343144fefaad5a30e95e21297c64c78e308fa1f/java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java#L145-L151
--
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: 15
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Nov 2020 01:49:36 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3210 Increase key size in tests and EMC
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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
> I've been pondering over this and think it's still weird to gate this. We w
Right: smaller key sizes result in less CPU resources consumed not only during key generation, but also during signature signing tokens, signature verification, encryption on the wire, etc. We kept the key sizes as small as possible in tests because the majority of our tests are about functionality not directly related to crypto stuff, so the idea is to have bare minimum at the crypto side, when possible.
I guess we should switch to bigger keys if that complexity is something substantial, but I don't see why it would be so. What complexity do you see regarding using different assertions in various tests based on the gate flag? E.g., I don't see any changes in assertions in this changelist, so I'm not sure why would they appear once we start using different key length based on the proposed gate flag? We don't assume to run different parts of our tests suite with different value of the FIPS mode gate flag.
I'm cautious about bumping up key sizes because I don't know how much impact that would be for our tests. If taking about speed and what part of overall costs of benchmarks crypto-related stuff is taking, I'm curious how much is that if running the whole suite of the tests on the same machine with short and FIPS-required key length? Or at least, maybe the the following tests:
* token-test
* registration-test
* tls_socket-test
* token_signer-itest
* master_cert_authority-itest
I guess we are interested to see not only time taken, but also CPU consumed (so, running some of the tests above under perf-stat might be a good idea as well). I'd think that if the overall increase is less than 3%, we probably should switch to bigger keys.
http://gerrit.cloudera.org:8080/#/c/16658/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16658/4//COMMIT_MSG@10
PS4, Line 10: these tests
> These aren't required to make the tests pass in a FIPS environment, I only
I see; I thought those tweaks were necessary to pass (I missed the important 'even' part).
http://gerrit.cloudera.org:8080/#/c/16658/3/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/16658/3/src/kudu/mini-cluster/external_mini_cluster.cc@a990
PS3, Line 990:
I'd suggest you run the tests on RHEL/CentOS 8.1 if you are about to remove this (security level 2 mandates size for RSA keys to be at least 2048). See https://github.com/apache/kudu/commit/93e85876f472b2668604ce5c15eafb17ce303989 for details.
--
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 <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Oct 2020 08:25:47 +0000
Gerrit-HasComments: Yes