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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
