You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/11/08 05:32:58 UTC
[kudu-CR] WIP: workarounds for el6/krb5 1.10
Hello Dan Burkert,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4990
to review the following change.
Change subject: WIP: workarounds for el6/krb5 1.10
......................................................................
WIP: workarounds for el6/krb5 1.10
Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/init.cc
A src/kudu/security/init.h
A src/kudu/security/test/krb5_realm_override.cc
M src/kudu/server/CMakeLists.txt
M src/kudu/server/server_base.cc
M src/kudu/util/test_main.cc
9 files changed, 224 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/4990/1
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 5:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4990/5/src/kudu/security/init.cc
File src/kudu/security/init.cc:
PS5, Line 88: krb5_creds creds
> ah, good catch, I think you're right. Wonder why LSAN didn't catch this
Interesting, indeed. May be, this is the case when the creds do not contain much at all?
PS5, Line 112: "KRB5CCNAME"
Does it make sense to introduce constants for those Kerberos-related env variables? It seems we have those string literals in multiple places now. At least, it could help to avoid mistakes related to typos/etc.
http://gerrit.cloudera.org:8080/#/c/4990/5/src/kudu/security/test/krb5_realm_override.cc
File src/kudu/security/test/krb5_realm_override.cc:
PS5, Line 78: int
nit: they have krb5_error_code type for that, if you wish.
PS5, Line 92: ret_realms
nit: probably it's too paranoid, but does it make sense to check for non-null result of malloc(), and, may be, strdup() as well (since they are plain C functions and don't throw std::bad_alloc if no memory available)?
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4990
to look at the new patch set (#7).
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Workaround test failures running with MIT krb5 1.10
This fixes a variety of issues seen when running on el6, which has MIT
krb5 1.10.
* This version doesn't support KRB5_CLIENT_KTNAME. So, we need to
explicitly login servers from their provided keytabs when the server
starts. This ensures that servers have proper client credentials in
order to send kerberized RPCs to other servers. Along the way,
I changed the ExternalMiniCluster to provide IP-specific keytabs to
the server.
* In our minicluster, we use strange local IPs like '127.x.y.z' for
testing. These don't have any associated hostnames in reverse DNS. MIT
krb5 has a bug (or lack of feature) whereby it cannot handle service
principals whose hostname component is a numeric IP rather than a
hostname. Specifically, it fails to determine the Kerberos Realm for a
principal with a numeric host.
This patch works around the issue by overriding the
krb5_get_host_realm() function using a combination of LD_PRELOAD and
statically including this symbol in our tests. See the comments at the
top of krb5_realm_override.cc for details.
With this patch, external_mini_cluster-test passes on el6 whereas it
used to fail.
Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
---
M CMakeLists.txt
M cmake_modules/FindKerberos.cmake
A cmake_modules/FindKerberosPrograms.cmake
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/rpc/sasl_server.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/init.cc
A src/kudu/security/init.h
A src/kudu/security/test/krb5_realm_override.cc
M src/kudu/security/test/mini_kdc-test.cc
M src/kudu/server/CMakeLists.txt
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/test_main.cc
17 files changed, 427 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/4990/7
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4990/7/CMakeLists.txt
File CMakeLists.txt:
PS7, Line 855: include_directories(${KERBEROS_INCLUDE_DIR})
> hm, but doesn't it already dedupe these? typically all of the libraries get
If it's a custom location (like in third-party), there is nothing do dedupe here. In that case, for most sources, that -I <custom kerberos dir> is not needed at all.
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4990/5/src/kudu/security/init.cc
File src/kudu/security/init.cc:
PS5, Line 112: "KRB5CCNAME"
> actually I think I'll do this in a follow-up since this patch is already a
Yep, it would be better to separate that into its own changelist.
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4990/7/CMakeLists.txt
File CMakeLists.txt:
PS7, Line 855: include_directories(${KERBEROS_INCLUDE_DIR})
> Shorter command line for clang invocations since no useless -I flags, so le
hm, but doesn't it already dedupe these? typically all of the libraries get found in the same place :)
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 5:
(6 comments)
It makes sense to stretch ourselves to accommodate various openssl versions, because we don't want to ship our own openssl due to the high number of updates that it gets.
But now we're stretching ourselves to accommodate el6's libkrb5. Does libkrb5 also receive a high number of updates? If not, I'd argue we should ship it ourselves and use a known good version.
I asked Dan about this and he said that due to how libsasl uses dlopen() to get to libkrb5, it's really hard (or maybe impossible) to get it to prefer a different version. Is that something you've looked into? Is there some way to customize it?
If it's impossible, we'd have to ship libsasl too, and patch it to load libkrb5 differently. I don't think that'd be the worst thing in the world (provided libsasl, like libkrb5 and unlike openssl, does not receive updates), but it'd be a lot more work than what you're doing here.
Anyway, I want to at least have this conversation, if only so I'll understand how many CVEs and the like libkrb5/libsasl receive.
http://gerrit.cloudera.org:8080/#/c/4990/5/src/kudu/security/CMakeLists.txt
File src/kudu/security/CMakeLists.txt:
PS5, Line 23: krb5
Can you add a find_library(REQUIRED) for this in the main CMakeLists.txt?
http://gerrit.cloudera.org:8080/#/c/4990/5/src/kudu/security/init.cc
File src/kudu/security/init.cc:
Line 109:
I'm kind of surprised you need to "sanitize" these environment variables given that we're calling into the krb5 library directly here. I would expect that the env variables have an effect on the krb5 CLI tools, but surprised that they would on direct library calls.
http://gerrit.cloudera.org:8080/#/c/4990/5/src/kudu/security/test/krb5_realm_override.cc
File src/kudu/security/test/krb5_realm_override.cc:
PS5, Line 74: CHECK(g_orig_krb5_get_host_realm);
: CHECK(g_orig_krb5_get_default_realm);
: CHECK(g_orig_krb5_free_default_realm);
Maybe have these return the right krb5_error_code on failure? Then you could remove the glog dependency and keep this code more "pristine" (i.e. less Kudu-dependent).
http://gerrit.cloudera.org:8080/#/c/4990/5/src/kudu/util/test_main.cc
File src/kudu/util/test_main.cc:
Line 20: #include <dlfcn.h>
What's this for?
Line 24: #include <string>
And this?
Line 39: using std::string;
And this.
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4990/5/src/kudu/security/init.cc
File src/kudu/security/init.cc:
PS5, Line 88: krb5_creds creds
> Interesting, indeed. May be, this is the case when the creds do not contai
My guess is the issue is that this code is only run by external minicluster daemons, and we shut those down by 'kill -9' rather than gracefully. So, the leak sanitizer doesn't actually get a chance to run on exit. I'll see if I can add a test case that repros the leak before I fix it.
Line 109:
> I'm kind of surprised you need to "sanitize" these environment variables gi
Consider yourself surprised, then. The library reads configuration files and environment variables to get configuration, because typical usage is deeply embedded within apps (eg just like us, people embed SASL which uses GSSAPI which uses libkrb5).
There are direct library calls you can make (as we do above) to manually grab credentials, store them in ticket caches, etc, but when you're interacting with Kerberos through the above-mentioned layers, it's basically impossible to do anything but pass the configuration through the process's environment.
PS5, Line 112: "KRB5CCNAME"
> Does it make sense to introduce constants for those Kerberos-related env va
will do
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4990/5/src/kudu/security/init.cc
File src/kudu/security/init.cc:
PS5, Line 88: krb5_creds creds
What about calling krb5_free_cred_contents() on creds for clean up?
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 7:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/4990/7/CMakeLists.txt
File CMakeLists.txt:
Line 854: find_package(Kerberos REQUIRED)
Is there a particular minimum version we should enforce?
http://gerrit.cloudera.org:8080/#/c/4990/7/cmake_modules/FindKerberos.cmake
File cmake_modules/FindKerberos.cmake:
PS7, Line 23: # KERBEROS_INCLUDE_DIR - where to find krb5.h, etc.
: # KERBEROS_LIBRARY - List of libraries when using krb5.
: # KERBEROS_FOUND - True if krb5 found.
Nit: mind fixing the alignment and capitalization here?
PS7, Line 27: IF (KERBEROS_INCLUDE_DIR)
: # Already in cache, be silent
: SET(KERBEROS_FIND_QUIETLY TRUE)
: ENDIF (KERBEROS_INCLUDE_DIR)
I think we can drop this; not sure why we'd care to avoid another find call if the result was cached.
Line 32: FIND_PATH(KERBEROS_INCLUDE_DIR krb5.h)
Built-in cmake directives should be lower-cased (find_path, set, etc.).
Line 34: SET(KERBEROS_NAMES krb5 k5crypto com_err)
Do we need to find all of these? Or just one of them? I think the single find_library() call passes if just one is found, but I imagine you wanted all of them?
PS7, Line 40: KERBEROS
Should be Kerberos
Line 42: MARK_AS_ADVANCED(KERBEROS_LIBRARY KERBEROS_INCLUDE_DIR)
I've never understood what this actually does, and I think we can omit it.
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4990
to look at the new patch set (#5).
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Workaround test failures running with MIT krb5 1.10
This fixes a variety of issues seen when running on el6, which has MIT
krb5 1.10.
* This version doesn't support KRB5_CLIENT_KTNAME. So, we need to
explicitly login servers from their provided keytabs when the server
starts. This ensures that servers have proper client credentials in
order to send kerberized RPCs to other servers.
* In our minicluster, we use strange local IPs like '127.x.y.z' for
testing. These don't have any associated hostnames in reverse DNS. MIT
krb5 has a bug (or lack of feature) whereby it cannot handle service
principals whose hostname component is a numeric IP rather than a
hostname. Specifically, it fails to determine the Kerberos Realm for a
principal with a numeric host.
This patch works around the issue by overriding the
krb5_get_host_realm() function using a combination of LD_PRELOAD and
statically including this symbol in our tests. See the comments at the
top of krb5_realm_override.cc for details.
With this patch, external_mini_cluster-test passes on el6 whereas it
used to fail.
Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/init.cc
A src/kudu/security/init.h
A src/kudu/security/test/krb5_realm_override.cc
M src/kudu/server/CMakeLists.txt
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/test_main.cc
11 files changed, 304 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/4990/5
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Workaround test failures running with MIT krb5 1.10
This fixes a variety of issues seen when running on el6, which has MIT
krb5 1.10.
* This version doesn't support KRB5_CLIENT_KTNAME. So, we need to
explicitly login servers from their provided keytabs when the server
starts. This ensures that servers have proper client credentials in
order to send kerberized RPCs to other servers. Along the way,
I changed the ExternalMiniCluster to provide IP-specific keytabs to
the server.
* In our minicluster, we use strange local IPs like '127.x.y.z' for
testing. These don't have any associated hostnames in reverse DNS. MIT
krb5 has a bug (or lack of feature) whereby it cannot handle service
principals whose hostname component is a numeric IP rather than a
hostname. Specifically, it fails to determine the Kerberos Realm for a
principal with a numeric host.
This patch works around the issue by overriding the
krb5_get_host_realm() function using a combination of LD_PRELOAD and
statically including this symbol in our tests. See the comments at the
top of krb5_realm_override.cc for details.
With this patch, external_mini_cluster-test passes on el6 whereas it
used to fail.
Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Reviewed-on: http://gerrit.cloudera.org:8080/4990
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M CMakeLists.txt
M cmake_modules/FindKerberos.cmake
A cmake_modules/FindKerberosPrograms.cmake
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/security/CMakeLists.txt
A src/kudu/security/init.cc
A src/kudu/security/init.h
A src/kudu/security/test/krb5_realm_override.cc
M src/kudu/security/test/mini_kdc-test.cc
M src/kudu/server/CMakeLists.txt
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/test_main.cc
16 files changed, 416 insertions(+), 41 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4990/5/src/kudu/security/init.cc
File src/kudu/security/init.cc:
PS5, Line 88: krb5_creds creds
> What about calling krb5_free_cred_contents() on creds for clean up?
ah, good catch, I think you're right. Wonder why LSAN didn't catch this
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4990/5/src/kudu/security/init.cc
File src/kudu/security/init.cc:
PS5, Line 88: krb5_creds creds
> My guess is the issue is that this code is only run by external minicluster
OK, new revision adds a test case which uses this new function outside of a minicluster, and it exposed the leak (which I then fixed as you suggested)
PS5, Line 112: "KRB5CCNAME"
> will do
actually I think I'll do this in a follow-up since this patch is already a bit sprawling. adding a TODO.
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] WIP: workarounds for el6/krb5 1.10
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4990
to look at the new patch set (#4).
Change subject: WIP: workarounds for el6/krb5 1.10
......................................................................
WIP: workarounds for el6/krb5 1.10
Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/init.cc
A src/kudu/security/init.h
A src/kudu/security/test/krb5_realm_override.cc
M src/kudu/server/CMakeLists.txt
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/test_main.cc
10 files changed, 298 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/4990/4
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 8: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4990/7/CMakeLists.txt
File CMakeLists.txt:
PS7, Line 855: include_directories(${KERBEROS_INCLUDE_DIR})
> If it's a custom location (like in third-party), there is nothing do dedupe
It seems in case of Linux builds it does not matter -- the path is in /usr/include, so nothing special.
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4990
to look at the new patch set (#8).
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Workaround test failures running with MIT krb5 1.10
This fixes a variety of issues seen when running on el6, which has MIT
krb5 1.10.
* This version doesn't support KRB5_CLIENT_KTNAME. So, we need to
explicitly login servers from their provided keytabs when the server
starts. This ensures that servers have proper client credentials in
order to send kerberized RPCs to other servers. Along the way,
I changed the ExternalMiniCluster to provide IP-specific keytabs to
the server.
* In our minicluster, we use strange local IPs like '127.x.y.z' for
testing. These don't have any associated hostnames in reverse DNS. MIT
krb5 has a bug (or lack of feature) whereby it cannot handle service
principals whose hostname component is a numeric IP rather than a
hostname. Specifically, it fails to determine the Kerberos Realm for a
principal with a numeric host.
This patch works around the issue by overriding the
krb5_get_host_realm() function using a combination of LD_PRELOAD and
statically including this symbol in our tests. See the comments at the
top of krb5_realm_override.cc for details.
With this patch, external_mini_cluster-test passes on el6 whereas it
used to fail.
Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
---
M CMakeLists.txt
M cmake_modules/FindKerberos.cmake
A cmake_modules/FindKerberosPrograms.cmake
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/security/CMakeLists.txt
A src/kudu/security/init.cc
A src/kudu/security/init.h
A src/kudu/security/test/krb5_realm_override.cc
M src/kudu/security/test/mini_kdc-test.cc
M src/kudu/server/CMakeLists.txt
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/test_main.cc
16 files changed, 416 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/4990/8
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4990
to look at the new patch set (#6).
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Workaround test failures running with MIT krb5 1.10
This fixes a variety of issues seen when running on el6, which has MIT
krb5 1.10.
* This version doesn't support KRB5_CLIENT_KTNAME. So, we need to
explicitly login servers from their provided keytabs when the server
starts. This ensures that servers have proper client credentials in
order to send kerberized RPCs to other servers. Along the way,
I changed the ExternalMiniCluster to provide IP-specific keytabs to
the server.
* In our minicluster, we use strange local IPs like '127.x.y.z' for
testing. These don't have any associated hostnames in reverse DNS. MIT
krb5 has a bug (or lack of feature) whereby it cannot handle service
principals whose hostname component is a numeric IP rather than a
hostname. Specifically, it fails to determine the Kerberos Realm for a
principal with a numeric host.
This patch works around the issue by overriding the
krb5_get_host_realm() function using a combination of LD_PRELOAD and
statically including this symbol in our tests. See the comments at the
top of krb5_realm_override.cc for details.
With this patch, external_mini_cluster-test passes on el6 whereas it
used to fail.
Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/rpc/sasl_server.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/init.cc
A src/kudu/security/init.h
A src/kudu/security/test/krb5_realm_override.cc
M src/kudu/security/test/mini_kdc-test.cc
M src/kudu/server/CMakeLists.txt
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/test_main.cc
14 files changed, 358 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/4990/6
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 5:
> > But now we're stretching ourselves to accommodate el6's libkrb5.
> Does libkrb5 also receive a high number of updates? If not, I'd
> argue we should ship it ourselves and use a known good version.
>
> I don't know what classifies as a high number, but looking at the
> Debian changelog for Ubuntu 14.04, I see 18 CVEs in there (12 from
> 2014, 6 from 2015). Looking at a RHEL 6.6 system, I see 4 releases
> in 2015 and a bunch from 2014 as well. So, empirically, this
> library does get updated fairly frequently by distro vendors, and I
> can imagine that a security-conscious admin would be nervous about
> taking an embedded version.
I think that does qualify as high, actually. I was hoping for a very low number, perhaps even 0.
> Also keep in mind that this stuff is used by the client library,
> and having our client try to static-link in its own krb5 and sasl
> seems like a dangerous proposition since it is very likely to be
> running embedded in another process which is also using krb5/sasl
> (and where we actively want to be picking up credentials kinitted
> by the embedder, etc).
Why? I would expect that credential passing between the embedder and the client will be done explicitly (via client API calls) and abstractly enough that the fact that the client uses MIT krb5 is just an implementation detail. Is that not going to be the case? I'm not a fan of dependencies dictating our client APIs, to the point where we've even discussed a C API to elide STL dependencies.
Anyway, if this interface is abstract, the client's use of symbol hiding should prevent the embedder from caring about the krb5/sasl usage of the client, regardless of its linkage.
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] WIP: workarounds for el6/krb5 1.10
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4990
to look at the new patch set (#3).
Change subject: WIP: workarounds for el6/krb5 1.10
......................................................................
WIP: workarounds for el6/krb5 1.10
Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/init.cc
A src/kudu/security/init.h
A src/kudu/security/test/krb5_realm_override.cc
M src/kudu/server/CMakeLists.txt
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/test_main.cc
10 files changed, 288 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/4990/3
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] WIP: workarounds for el6/krb5 1.10
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: WIP: workarounds for el6/krb5 1.10
......................................................................
Patch Set 3:
Awesome, this is looking a lot better. CI linker issues look legit.
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 5:
> But now we're stretching ourselves to accommodate el6's libkrb5. Does libkrb5 also receive a high number of updates? If not, I'd argue we should ship it ourselves and use a known good version.
I don't know what classifies as a high number, but looking at the Debian changelog for Ubuntu 14.04, I see 18 CVEs in there (12 from 2014, 6 from 2015). Looking at a RHEL 6.6 system, I see 4 releases in 2015 and a bunch from 2014 as well. So, empirically, this library does get updated fairly frequently by distro vendors, and I can imagine that a security-conscious admin would be nervous about taking an embedded version.
>I asked Dan about this and he said that due to how libsasl uses dlopen() to get to libkrb5, it's really hard (or maybe impossible) to get it to prefer a different version. Is that something you've looked into? Is there some way to customize it?
If it's impossible, we'd have to ship libsasl too, and patch it to load libkrb5 differently. I don't think that'd be the worst thing in the world (provided libsasl, like libkrb5 and unlike openssl, does not receive updates), but it'd be a lot more work than what you're doing here.
Yea, this stuff gets nasty, too, because once we're shipping libsasl, we also need to make sure that libsasl loads our own sasl modules, and not the ones from the system SASL path. We went down this route before but reversed it in 152ff259de7ea44c81e17c59fd4a7c5f41ba712e a couple years ago, and I recall it was a big pain in the butt.
Also keep in mind that this stuff is used by the client library, and having our client try to static-link in its own krb5 and sasl seems like a dangerous proposition since it is very likely to be running embedded in another process which is also using krb5/sasl (and where we actively want to be picking up credentials kinitted by the embedder, etc).
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 5:
> Why? I would expect that credential passing between the embedder and the client will be done explicitly (via client API calls) and abstractly enough that the fact that the client uses MIT krb5 is just an implementation detail. Is that not going to be the case? I'm not a fan of dependencies dictating our client APIs, to the point where we've even discussed a C API to elide STL dependencies.
That's not the typical usage pattern for Kerberos. Clients are expected to pick up credentials from the environment -- eg a user can use Kerberos APIs or environment variables to set the ticket cache location for the process, and underlying libraries are expected to read credentials from that ticket cache. In other words, you can just 'kinit' and then any program that uses Kudu can see those credentials.
If we use a different version of Kerberos compared to what's on the system, then there's little to no guarantee that when the user logs in via kinit that we can pick up the credentials from their ticket cache as configured by their environment. This is especially the case if the system kerberos is newer than the library's static-linked Kerberos: as a specific example, back in 2010 or so we had an issue where the kinit on RHEL6 would produce a ticket cache that Java 6's Kerberos implementation couldn't read (https://www.cloudera.com/documentation/enterprise/5-7-x/topics/cm_sg_sec_troubleshooting.html#topic_17_1_1). In the case that the library's Kerberos is newer than the system Kerberos, I think you could still get this sort of compatibility problem in reverse.
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 7: Code-Review+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4990/7/CMakeLists.txt
File CMakeLists.txt:
PS7, Line 855: include_directories(${KERBEROS_INCLUDE_DIR})
nit: if possible, consider adding this for relevant sources only (i.e. moving this directive into src/security/CMakeLists.txt).
http://gerrit.cloudera.org:8080/#/c/4990/5/src/kudu/security/init.cc
File src/kudu/security/init.cc:
PS5, Line 88:
> OK, new revision adds a test case which uses this new function outside of a
Cool, thanks for verifying and fixing this!
http://gerrit.cloudera.org:8080/#/c/4990/6/src/kudu/security/test/mini_kdc-test.cc
File src/kudu/security/test/mini_kdc-test.cc:
PS6, Line 20: #include <gflags/gflags.h>
nit: consider using <gflags/gflags_declare.h> instead to have less header file dependencies.
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] WIP: workarounds for el6/krb5 1.10
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4990
to look at the new patch set (#2).
Change subject: WIP: workarounds for el6/krb5 1.10
......................................................................
WIP: workarounds for el6/krb5 1.10
Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/init.cc
A src/kudu/security/init.h
A src/kudu/security/test/krb5_realm_override.cc
M src/kudu/server/CMakeLists.txt
M src/kudu/server/server_base.cc
M src/kudu/util/test_main.cc
9 files changed, 239 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/4990/2
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 5:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/4990/5/src/kudu/security/CMakeLists.txt
File src/kudu/security/CMakeLists.txt:
PS5, Line 23: krb5
> Can you add a find_library(REQUIRED) for this in the main CMakeLists.txt?
Done. Had to rename the current FindKerberos cmake script to FindKerberosPrograms, and made a new FindKerberos which locates the library.
http://gerrit.cloudera.org:8080/#/c/4990/5/src/kudu/security/test/krb5_realm_override.cc
File src/kudu/security/test/krb5_realm_override.cc:
PS5, Line 74: CHECK(g_orig_krb5_get_host_realm);
: CHECK(g_orig_krb5_get_default_realm);
: CHECK(g_orig_krb5_free_default_realm);
> Maybe have these return the right krb5_error_code on failure? Then you coul
I don't know what the right krb5 error code could be. This would be a very odd case -- somehow we have managed to call the krb5_get_host_realm function with no actual krb5 library present, even though our binaries all have linkage against libkrb5.so. So, it would be a true bug, and I'd rather crash and surface the area in an obvious way than try to shoehorn this into some error like KRB5_ERR_NO_SERVICE which would probably result in some confusing error message that would take a long time to track down.
PS5, Line 78: int
> nit: they have krb5_error_code type for that, if you wish.
Done
PS5, Line 92: ret_realms
> nit: probably it's too paranoid, but does it make sense to check for non-nu
done, returned ENOMEM which the original implementation also returns (funny that they return a mix of posix and krb5-specific error codes, but that's how it is!)
http://gerrit.cloudera.org:8080/#/c/4990/5/src/kudu/util/test_main.cc
File src/kudu/util/test_main.cc:
Line 20: #include <dlfcn.h>
> What's this for?
Done
Line 24: #include <string>
> And this?
Done
Line 39: using std::string;
> And this.
Done
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4990/7/CMakeLists.txt
File CMakeLists.txt:
PS7, Line 855: include_directories(${KERBEROS_INCLUDE_DIR})
> hrm, that's not very standard compared to what we do with all of the other
Shorter command line for clang invocations since no useless -I flags, so less searching for the include files.
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4990/4/src/kudu/security/test/krb5_realm_override.cc
File src/kudu/security/test/krb5_realm_override.cc:
Line 74: CHECK(g_orig_krb5_get_host_realm)
> Is there any way get_host_realm would be found, but not get_default_realm o
Done
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 8: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4990/7/cmake_modules/FindKerberos.cmake
File cmake_modules/FindKerberos.cmake:
PS7, Line 40:
> Really? All of the other ones in cmake_modules are all-caps here (eg in Fin
Okay. I thought they weren't (i.e. FindFoo.cmake would use "Foo" here).
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] WIP: workarounds for el6/krb5 1.10
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: WIP: workarounds for el6/krb5 1.10
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4990/4/src/kudu/security/test/krb5_realm_override.cc
File src/kudu/security/test/krb5_realm_override.cc:
Line 74: CHECK(g_orig_krb5_get_host_realm)
Is there any way get_host_realm would be found, but not get_default_realm or free_default_realm? May want to check all three just to be safe.
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Workaround test failures running with MIT krb5 1.10
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Workaround test failures running with MIT krb5 1.10
......................................................................
Patch Set 7:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/4990/7/CMakeLists.txt
File CMakeLists.txt:
Line 854: find_package(Kerberos REQUIRED)
> Is there a particular minimum version we should enforce?
I don't think the thing we're using to detect is is smart enough to know the version number (and doubt it's worth trying to build it since we'll just know when our tests fail)
PS7, Line 855: include_directories(${KERBEROS_INCLUDE_DIR})
> nit: if possible, consider adding this for relevant sources only (i.e. movi
hrm, that's not very standard compared to what we do with all of the other thirdparty things, so I'd prefer not to. What's the rationale?
http://gerrit.cloudera.org:8080/#/c/4990/7/cmake_modules/FindKerberos.cmake
File cmake_modules/FindKerberos.cmake:
PS7, Line 23: # KERBEROS_INCLUDE_DIR - where to find krb5.h, etc.
: # KERBEROS_LIBRARY - List of libraries when using krb5.
: # KERBEROS_FOUND - True if krb5 found.
> Nit: mind fixing the alignment and capitalization here?
Done
PS7, Line 27: IF (KERBEROS_INCLUDE_DIR)
: # Already in cache, be silent
: SET(KERBEROS_FIND_QUIETLY TRUE)
: ENDIF (KERBEROS_INCLUDE_DIR)
> I think we can drop this; not sure why we'd care to avoid another find call
Done
Line 32: FIND_PATH(KERBEROS_INCLUDE_DIR krb5.h)
> Built-in cmake directives should be lower-cased (find_path, set, etc.).
Done
Line 34: SET(KERBEROS_NAMES krb5 k5crypto com_err)
> Do we need to find all of these? Or just one of them? I think the single fi
I copied this from quickstep, but I'll change to just krb5 since that's the only one we're explicitly linking
PS7, Line 40: KERBEROS
> Should be Kerberos
Really? All of the other ones in cmake_modules are all-caps here (eg in FindSnappy, FindBitshuffle etc)
Line 42: MARK_AS_ADVANCED(KERBEROS_LIBRARY KERBEROS_INCLUDE_DIR)
> I've never understood what this actually does, and I think we can omit it.
Done
--
To view, visit http://gerrit.cloudera.org:8080/4990
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I708334cbbee35d2629a38a369e63c1dc309ed91b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes