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:00 UTC
[kudu-CR] Add option to enforce FIPS approved mode
Attila Bukor has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16657
Change subject: Add option to enforce FIPS approved mode
......................................................................
Add option to enforce FIPS approved mode
When OpenSSL is initialized, Kudu now logs if FIPS approved mode is
enabled. If the KUDU_REQUIRE_FIPS_MODE is set, it fails to start without
FIPS mode being enabled. This needs to be an environment variable
instead of a flag so that this check can be used in the C++ client as
well.
Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
---
M src/kudu/security/openssl_util.cc
1 file changed, 11 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/16657/1
--
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: newchange
Gerrit-Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
Gerrit-Change-Number: 16657
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
[kudu-CR] KUDU-3210 Add option to enforce FIPS approved mode
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 12: Code-Review+2
--
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: 12
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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:40:52 +0000
Gerrit-HasComments: No
[kudu-CR] Add option to enforce FIPS approved mode
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16657 )
Change subject: Add option to enforce FIPS approved mode
......................................................................
Patch Set 4:
(1 comment)
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: 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 guess this check should also be performed in the control flow where the OpenSSL is initialized by the application itself (e.g., consider some Python app which uses Kudu client or a Kudu C++ application which uses Kudu C++ client).
Probably, the check for this invariant should be performed regardless of the g_disable_ssl_init's value, no?
--
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 <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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 19:01:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
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 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16657/1/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:
http://gerrit.cloudera.org:8080/#/c/16657/1/src/kudu/security/openssl_util.cc@127
PS1, Line 127: }
> warning: use nullptr [modernize-use-nullptr]
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: 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:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
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 <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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:00:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3210 Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor 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 11:
(1 comment)
> Patch Set 11: Code-Review+1
>
> (1 comment)
>
> Overall looks good, the only question left is about clearing errors which might left from a Kudu application that initialized openssl but didn't clear the error stack as needed (say, didn't do proper error handling of openssl errors).
Not sure I understand, are you referring to the comment you just added?
http://gerrit.cloudera.org:8080/#/c/16657/11/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:
http://gerrit.cloudera.org:8080/#/c/16657/11/src/kudu/security/openssl_util.cc@160
PS11, Line 160: // In case the user's thread has left some error around, clear it.
: ERR_clear_error();
: SCOPED_OPENSSL_NO_PENDING_ERRORS;
> I didn't notice that in the previous review round, but it seems the previou
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: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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:34:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
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 7:
(2 comments)
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: ERR_clear_error();
: return Status::RuntimeError(
: "SSL library appears uninitialized (cannot create SSL_CTX)");
> As another thought: if we have such a trouble determining whether KUDU_REQU
Good point, refactored it and added the check to CheckOpenSSLInitialized() too.
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: // LSAN warnings.
: debug::ScopedLeakCheckDisabler d;
> We have GetBooleanEnvironmentVariable() in test_util.cc to handle this.
Yea I just found it, should I move it to some other util?
--
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: 7
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3210 Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16657
to look at the new patch set (#8).
Change subject: KUDU-3210 Add option to enforce FIPS approved mode
......................................................................
KUDU-3210 Add option to enforce FIPS approved mode
When OpenSSL is initialized, Kudu now logs if FIPS approved mode is
enabled. If the KUDU_REQUIRE_FIPS_MODE is set, it fails to start without
FIPS mode being enabled.
Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
---
M src/kudu/security/openssl_util.cc
1 file changed, 21 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/16657/8
--
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: newpatchset
Gerrit-Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
Gerrit-Change-Number: 16657
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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 Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16657
to look at the new patch set (#11).
Change subject: KUDU-3210 Add option to enforce FIPS approved mode
......................................................................
KUDU-3210 Add option to enforce FIPS approved mode
If KUDU_REQUIRE_FIPS_MODE is set to "1", "true" or "yes", Kudu checks if
FIPS mode is enabled and crashes when initializing OpenSSL if not. This
is also true for Kudu C++ client applications.
On initializing OpenSSL, Kudu now also verbose logs (level 2) if FIPS
mode is enabled regardless of whether it's required or not.
Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
---
M src/kudu/security/openssl_util.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/test_util.cc
4 files changed, 45 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/16657/11
--
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: newpatchset
Gerrit-Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
Gerrit-Change-Number: 16657
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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] Add option to enforce FIPS approved mode
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16657 )
Change subject: Add option to enforce FIPS approved mode
......................................................................
Patch Set 6:
(2 comments)
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: #if OPENSSL_VERSION_NUMBER >= 0x10100000L
: // The OPENSSL_init_ssl manpage [1] says "As of version 1.1.0 OpenSSL will
: // automatically allocate all resources it needs so no explicit initialis
> I'm not sure about Python, it seems this is not called there, but in the C+
Let's start with a simple question: what should be the behavior when KUDU_REQUIRE_FIPS_MODE is set to 1 and a client application initialized OpenSSL on itself so that FIPS_mode() == 0?
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@198
PS6, Line 198: CHECK(fips_mode) << "FIPS mode required by environment variable "
: "KUDU_REQUIRE_FIPS_MODE, but it is not enabled.";
Does it make sense to add a test to track regressions, if any? We have a plenty of death tests (look for ASSERT_DEATH) in the project's codebase.
--
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 <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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:03:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
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 5:
> Patch Set 5: Verified-1
>
> Build Failed
>
> http://jenkins.kudu.apache.org/job/kudu-gerrit/22671/ : FAILURE
This is weird, getting this in master-test in LSAN:
==26632==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 32 byte(s) in 1 object(s) allocated from:
#0 0x561851 in malloc /home/jenkins-slave/workspace/kudu-master/3/thirdparty/src/llvm-9.0.0.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:145
#1 0x7fdbdd712db2 in CRYPTO_malloc (/lib/x86_64-linux-gnu/libcrypto.so.1.0.0+0x5fdb2)
Indirect leak of 2216 byte(s) in 2 object(s) allocated from:
#0 0x561851 in malloc /home/jenkins-slave/workspace/kudu-master/3/thirdparty/src/llvm-9.0.0.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:145
#1 0x7fdbdd712db2 in CRYPTO_malloc (/lib/x86_64-linux-gnu/libcrypto.so.1.0.0+0x5fdb2)
--
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: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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 15:05:28 +0000
Gerrit-HasComments: No
[kudu-CR] Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
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 7:
(1 comment)
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@198
PS6, Line 198: int num_locks = CRYPTO_num_locks();
: CHECK(!kCryptoLocks);
> I don't think we need to mock anything here when running against non-FIPS-e
Right, so you mean running this test conditionally, only if we're not running in FIPS mode?
--
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: 7
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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 12:19:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3210 Add option to enforce FIPS approved mode
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 11: Code-Review+1
(1 comment)
Overall looks good, the only question left is about clearing errors which might left from a Kudu application that initialized openssl but didn't clear the error stack as needed (say, didn't do proper error handling of openssl errors).
http://gerrit.cloudera.org:8080/#/c/16657/11/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:
http://gerrit.cloudera.org:8080/#/c/16657/11/src/kudu/security/openssl_util.cc@160
PS11, Line 160: // In case the user's thread has left some error around, clear it.
: ERR_clear_error();
: SCOPED_OPENSSL_NO_PENDING_ERRORS;
I didn't notice that in the previous review round, but it seems the previous version of the code did this clean-up if the library was initialized externally. If those errors are left on the OpenSSL's error stack, openssl_util might hit those with SCOPED_OPENSSL_NO_PENDING_ERRORS check even if those errors are irrelevant.
That was introduced here:
https://github.com/apache/kudu/commit/5f1ca4f3948a61b22946255e4ada895c77bc6adf#diff-c9f685fcd94c68485fe3abda7982e97f6b4ced85d48de9fc72b90aa2d72dd02dR87-R89
Maybe, we need to keep this? I don't see any tests failing, but I guess we don't have a test case to cover this edge case. And I guess the placement of these lines before 'if (g_disable_ssl_init)' check was intentional.
--
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: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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 18:58:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
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 6:
(2 comments)
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: #if OPENSSL_VERSION_NUMBER >= 0x10100000L
: // The OPENSSL_init_ssl manpage [1] says "As of version 1.1.0 OpenSSL will
: // automatically allocate all resources it needs so no explicit initialis
> Let's start with a simple question: what should be the behavior when KUDU_R
That's what I'm not sure about. On one hand, it would make sense that it crashes, as it's set to require, but on the other hand, the client application clearly indicated that it doesn't want Kudu to initialize OpenSSL, and Kudu should trust that OpenSSL is set up however the application wants to. In this case, these contradict each other.
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@198
PS6, Line 198: CHECK(fips_mode) << "FIPS mode required by environment variable "
: "KUDU_REQUIRE_FIPS_MODE, but it is not enabled.";
> Does it make sense to add a test to track regressions, if any? We have a p
Not sure. Are you suggesting to wrap the FIPS_mode() in another function call and mock it?
--
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 <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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:18:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
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 6: Verified+1
--
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 <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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 17:39:57 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3210 Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16657 )
Change subject: KUDU-3210 Add option to enforce FIPS approved mode
......................................................................
KUDU-3210 Add option to enforce FIPS approved mode
If KUDU_REQUIRE_FIPS_MODE is set to "1", "true" or "yes", Kudu checks if
FIPS mode is enabled and crashes when initializing OpenSSL if not. This
is also true for Kudu C++ client applications.
On initializing OpenSSL, Kudu now also verbose logs (level 2) if FIPS
mode is enabled regardless of whether it's required or not.
Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
Reviewed-on: http://gerrit.cloudera.org:8080/16657
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/security/openssl_util.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/test_util.cc
4 files changed, 48 insertions(+), 41 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
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: merged
Gerrit-Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
Gerrit-Change-Number: 16657
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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] Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has removed a vote on this change.
Change subject: Add option to enforce FIPS approved mode
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
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: deleteVote
Gerrit-Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
Gerrit-Change-Number: 16657
Gerrit-PatchSet: 7
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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] Add option to enforce FIPS approved mode
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16657 )
Change subject: Add option to enforce FIPS approved mode
......................................................................
Patch Set 4:
(1 comment)
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: if (getenv("KUDU_REQUIRE_FIPS_MODE")) {
: CHECK(fips_mode) << ": FIPS mode require by environment variable "
: "KUDU_REQUIRE_FIPS_MODE, but it is not enabled.";
> I think there is a misunderstanding here. The DisableOpenSSLInitialization
As another thought: if we have such a trouble determining whether KUDU_REQUIRE_FIPS_MODE should or should not affect client applications that have OpenSSL initialized by themselves, maybe it's a good sign that there should be no KUDU_REQUIRE_FIPS_MODE variable at all and these semantics should be enforces only in case of Kudu servers, i.e. this should be implemented as a gflag, not a env variable?
--
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 <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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:58:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3210 Add option to enforce FIPS approved mode
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 12:
> (1 comment)
>
> > Patch Set 11: Code-Review+1
> >
> > (1 comment)
> >
> > Overall looks good, the only question left is about clearing
> errors which might left from a Kudu application that initialized
> openssl but didn't clear the error stack as needed (say, didn't do
> proper error handling of openssl errors).
>
> Not sure I understand, are you referring to the comment you just
> added?
Right, that was the thing. Thank you for addressing that one!
--
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: 12
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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:39:32 +0000
Gerrit-HasComments: No
[kudu-CR] Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16657
to look at the new patch set (#6).
Change subject: Add option to enforce FIPS approved mode
......................................................................
Add option to enforce FIPS approved mode
When OpenSSL is initialized, Kudu now logs if FIPS approved mode is
enabled. If the KUDU_REQUIRE_FIPS_MODE is set, it fails to start without
FIPS mode being enabled.
Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
---
M src/kudu/security/openssl_util.cc
1 file changed, 16 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/16657/6
--
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: newpatchset
Gerrit-Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
Gerrit-Change-Number: 16657
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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] Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has removed a vote on this change.
Change subject: Add option to enforce FIPS approved mode
......................................................................
Removed -Verified by Kudu Jenkins (120)
--
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: deleteVote
Gerrit-Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
Gerrit-Change-Number: 16657
Gerrit-PatchSet: 7
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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] Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
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 7: Verified+1
--
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: 7
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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 08:31:20 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3210 Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16657
to look at the new patch set (#12).
Change subject: KUDU-3210 Add option to enforce FIPS approved mode
......................................................................
KUDU-3210 Add option to enforce FIPS approved mode
If KUDU_REQUIRE_FIPS_MODE is set to "1", "true" or "yes", Kudu checks if
FIPS mode is enabled and crashes when initializing OpenSSL if not. This
is also true for Kudu C++ client applications.
On initializing OpenSSL, Kudu now also verbose logs (level 2) if FIPS
mode is enabled regardless of whether it's required or not.
Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
---
M src/kudu/security/openssl_util.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/test_util.cc
4 files changed, 48 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/16657/12
--
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: newpatchset
Gerrit-Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
Gerrit-Change-Number: 16657
Gerrit-PatchSet: 12
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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] Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
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 6:
> Patch Set 5:
>
> > Patch Set 5: Verified-1
> >
> > Build Failed
> >
> > http://jenkins.kudu.apache.org/job/kudu-gerrit/22671/ : FAILURE
>
> This is weird, getting this in master-test in LSAN:
>
> ==26632==ERROR: LeakSanitizer: detected memory leaks
>
> Direct leak of 32 byte(s) in 1 object(s) allocated from:
> #0 0x561851 in malloc /home/jenkins-slave/workspace/kudu-master/3/thirdparty/src/llvm-9.0.0.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:145
> #1 0x7fdbdd712db2 in CRYPTO_malloc (/lib/x86_64-linux-gnu/libcrypto.so.1.0.0+0x5fdb2)
>
> Indirect leak of 2216 byte(s) in 2 object(s) allocated from:
> #0 0x561851 in malloc /home/jenkins-slave/workspace/kudu-master/3/thirdparty/src/llvm-9.0.0.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:145
> #1 0x7fdbdd712db2 in CRYPTO_malloc (/lib/x86_64-linux-gnu/libcrypto.so.1.0.0+0x5fdb2)
Probably because the FIPS_mode() is called before registering the callback. I moved it after OpenSSL init.
--
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 <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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 15:13:46 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3210 Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor 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 11:
(2 comments)
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: int num_locks = CRYPTO_num_locks();
: CHECK(!kCryptoLocks);
> Yep: that's some extra work, but I guess it's worth it once we started eval
Done
http://gerrit.cloudera.org:8080/#/c/16657/6/src/kudu/security/openssl_util.cc@198
PS6, Line 198: kCryptoLocks = new Mutex[num_locks];
:
> One more note: I think it's totally OK to add such a test in a separate cha
Yea I think I'll add it in a follow-up patch, there's something weird with ASSERT_DEATH and the mini cluster. I'll figure that out later.
--
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: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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:47:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3210 Add option to enforce FIPS approved mode
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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 18:08:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add option to enforce FIPS approved mode
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16657 )
Change subject: Add option to enforce FIPS approved mode
......................................................................
Patch Set 6:
(2 comments)
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) {
We have GetBooleanEnvironmentVariable() in test_util.cc to handle this.
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.";
> Not sure. Are you suggesting to wrap the FIPS_mode() in another function ca
I don't think we need to mock anything here when running against non-FIPS-enabled OpenSSL library. The idea is to set the KUDU_REQUIRE_FIPS_MODE environment variable in a test code and then try to start mini-cluster. It allowed not to crash only if FIPS_mode() returns 1. If there are some issues with running internal mini-cluster's components, I guess it might be possible to get cleaner results with external mini-cluster.
--
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 <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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:43:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has removed a vote on this change.
Change subject: Add option to enforce FIPS approved mode
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
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: deleteVote
Gerrit-Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
Gerrit-Change-Number: 16657
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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] Add option to enforce FIPS approved mode
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16657 )
Change subject: Add option to enforce FIPS approved mode
......................................................................
Patch Set 4:
(1 comment)
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: if (getenv("KUDU_REQUIRE_FIPS_MODE")) {
: CHECK(fips_mode) << ": FIPS mode require by environment variable "
: "KUDU_REQUIRE_FIPS_MODE, but it is not enabled.";
> That's what I'm not sure about. On one hand, it would make sense that it cr
I think there is a misunderstanding here. The DisableOpenSSLInitialization() function doesn't have any semantics like 'Kudu should _trust_ that OpenSSL has been initialized properly before', no. It simply means that Kudu should not try to init the library on its own because the initialization has already been done. If you look around, you can see that Kudu doesn't particularly "trust" that client initializes the library properly: there is a check in CheckOpenSSLInitialized() for properly installed locking callbacks: https://github.com/apache/kudu/blob/17d569b870e8fb3978a8c02bc1170057a42ca7cc/src/kudu/security/openssl_util.cc#L110-L112
--
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 <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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:54:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add option to enforce FIPS approved mode
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/16657
to look at the new patch set (#3).
Change subject: Add option to enforce FIPS approved mode
......................................................................
Add option to enforce FIPS approved mode
When OpenSSL is initialized, Kudu now logs if FIPS approved mode is
enabled. If the KUDU_REQUIRE_FIPS_MODE is set, it fails to start without
FIPS mode being enabled. This needs to be an environment variable
instead of a flag so that this check can be used in the C++ client as
well.
Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
---
M src/kudu/security/openssl_util.cc
1 file changed, 10 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/16657/3
--
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: newpatchset
Gerrit-Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
Gerrit-Change-Number: 16657
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>
[kudu-CR] KUDU-3210 Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16657
to look at the new patch set (#10).
Change subject: KUDU-3210 Add option to enforce FIPS approved mode
......................................................................
KUDU-3210 Add option to enforce FIPS approved mode
If KUDU_REQUIRE_FIPS_MODE is set to "1", "true" or "yes", Kudu checks if
FIPS mode is enabled and crashes when initializing OpenSSL if not. This
is also true for Kudu C++ client applications.
On initializing OpenSSL, Kudu now also verbose logs (level 2) if FIPS
mode is enabled regardless of whether it's required or not.
Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
---
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/security/openssl_util.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/test_util.cc
5 files changed, 87 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/16657/10
--
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: newpatchset
Gerrit-Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
Gerrit-Change-Number: 16657
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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] Add option to enforce FIPS approved mode
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/16657
to look at the new patch set (#2).
Change subject: Add option to enforce FIPS approved mode
......................................................................
Add option to enforce FIPS approved mode
When OpenSSL is initialized, Kudu now logs if FIPS approved mode is
enabled. If the KUDU_REQUIRE_FIPS_MODE is set, it fails to start without
FIPS mode being enabled. This needs to be an environment variable
instead of a flag so that this check can be used in the C++ client as
well.
Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
---
M src/kudu/security/openssl_util.cc
1 file changed, 11 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/16657/2
--
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: newpatchset
Gerrit-Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
Gerrit-Change-Number: 16657
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 Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16657
to look at the new patch set (#9).
Change subject: KUDU-3210 Add option to enforce FIPS approved mode
......................................................................
KUDU-3210 Add option to enforce FIPS approved mode
If KUDU_REQUIRE_FIPS_MODE is set to "1", "true" or "yes", Kudu checks if
FIPS mode is enabled and crashes when initializing OpenSSL if not. This
is also true for Kudu C++ client applications.
On initializing OpenSSL, Kudu now also verbose logs (level 2) if FIPS
mode is enabled regardless of whether it's required or not.
Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
---
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/security/openssl_util.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/test_util.cc
5 files changed, 70 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/16657/9
--
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: newpatchset
Gerrit-Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
Gerrit-Change-Number: 16657
Gerrit-PatchSet: 9
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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 Add option to enforce FIPS approved mode
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
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@198
PS6, Line 198: CHECK(fips_mode) << "FIPS mode required by environment variable "
: "KUDU_REQUIRE_FIPS_MODE, but it is not enabled.";
> I think ideally the test should make sure that when KUDU_REQUIRE_FIPS_MODE
One more note: I think it's totally OK to add such a test in a separate changelist later on if it makes more sense.
--
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 <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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 03:20:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16657
to look at the new patch set (#5).
Change subject: Add option to enforce FIPS approved mode
......................................................................
Add option to enforce FIPS approved mode
When OpenSSL is initialized, Kudu now logs if FIPS approved mode is
enabled. If the KUDU_REQUIRE_FIPS_MODE is set, it fails to start without
FIPS mode being enabled.
Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
---
M src/kudu/security/openssl_util.cc
1 file changed, 15 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/16657/5
--
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: newpatchset
Gerrit-Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
Gerrit-Change-Number: 16657
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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] Add option to enforce FIPS approved mode
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/16657
to look at the new patch set (#4).
Change subject: Add option to enforce FIPS approved mode
......................................................................
Add option to enforce FIPS approved mode
When OpenSSL is initialized, Kudu now logs if FIPS approved mode is
enabled. If the KUDU_REQUIRE_FIPS_MODE is set, it fails to start without
FIPS mode being enabled. This needs to be an environment variable
instead of a flag so that this check can be used in the C++ client as
well.
Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
---
M src/kudu/security/openssl_util.cc
1 file changed, 11 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/16657/4
--
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: newpatchset
Gerrit-Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
Gerrit-Change-Number: 16657
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>
[kudu-CR] Add option to enforce FIPS approved mode
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16657
to look at the new patch set (#7).
Change subject: Add option to enforce FIPS approved mode
......................................................................
Add option to enforce FIPS approved mode
When OpenSSL is initialized, Kudu now logs if FIPS approved mode is
enabled. If the KUDU_REQUIRE_FIPS_MODE is set, it fails to start without
FIPS mode being enabled.
Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
---
M src/kudu/security/openssl_util.cc
1 file changed, 21 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/16657/7
--
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: newpatchset
Gerrit-Change-Id: I98a6a8b3330ea0b372b188690fadd4d312d8bf93
Gerrit-Change-Number: 16657
Gerrit-PatchSet: 7
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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] Add option to enforce FIPS approved mode
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16657 )
Change subject: Add option to enforce FIPS approved mode
......................................................................
Patch Set 4:
(5 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 gets checked?
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?
http://gerrit.cloudera.org:8080/#/c/16657/4/src/kudu/security/openssl_util.cc@131
PS4, Line 131: :
nit: drop the ":"?
http://gerrit.cloudera.org:8080/#/c/16657/4/src/kudu/security/openssl_util.cc@131
PS4, Line 131: require
nit: required
http://gerrit.cloudera.org:8080/#/c/16657/4/src/kudu/security/openssl_util.cc@132
PS4, Line 132:
nit: spacing
--
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 <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@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:53:34 +0000
Gerrit-HasComments: Yes