-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8349546: Linux support for Kerberos "nativeccache" functionality #28075
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
8349546: Linux support for Kerberos "nativeccache" functionality #28075
Conversation
|
👋 Welcome back nrhall! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
|
Is there a particular reason for build.sh in the tests or are you just not familiar with how native test code gets automatically compiled by the makefiles based on file naming conventions? In short, any file |
Definitely the unfamiliar bit, although the build.sh was useful for doing some quick test runs. Is there an example you could point me at - I'd be happy to fix that. Edit: found some examples - fix incoming. |
|
@erikj79 I've had a go at the suggested changes - hope that's more what you were looking for? |
make/autoconf/libraries.m4
Outdated
| if test "x$OPENJDK_TARGET_OS" = xlinux -o "x$OPENJDK_TARGET_OS" = xmacosx; then | ||
| NEEDS_LIB_KRB5=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that this external dependency is only needed on Linux. The resulting variable even has linux in the name.
make/autoconf/lib-krb5.m4
Outdated
| AC_ARG_WITH(krb5, [AS_HELP_STRING([--with-krb5], | ||
| [enable krb5 support (default=yes), or "no" to disable])]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is an optional dependency, this option should take three values: yes/no/auto. The default should be "auto".
- yes: Fail if the library cannot be found
- no: Disable the feature
- auto: Look for the library and enable if found, otherwise disable
Also the help text should make it clear that this is Linux only.
For other library dependency options, the with- option can also be used to point to the location of the installation, usually combined with individual --with--include and --with--lib. I think something like this might be good to support, but I'm not familiar with this lib and possible installation options.
make/autoconf/lib-krb5.m4
Outdated
| ENABLE_LIBLINUXKRB5=false | ||
| else | ||
| # First try pkg-config (most modern approach) | ||
| AC_PATH_TOOL([PKG_CONFIG], [pkg-config], [no]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PKG_CONFIG should already be setup in pkg.m4.
make/autoconf/lib-krb5.m4
Outdated
| ENABLE_LIBLINUXKRB5=true | ||
| else | ||
| # Fallback: try krb5-config | ||
| AC_PATH_TOOL([KRB5CONF], [krb5-config], [no]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have our own internal macro UTIL_LOOKUP_PROGS that is preferred over the AC variants.
make/autoconf/lib-krb5.m4
Outdated
| if test "x${with_krb5}" = xno; then | ||
| AC_MSG_NOTICE([krb5 explicitly disabled]) | ||
| KRB5_DISABLED=yes | ||
| elif test "x$NEEDS_LIB_KRB5" = xfalse; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is an optional dependency, perhaps use a different variable name?
make/autoconf/spec.gmk.template
Outdated
| ALSA_CFLAGS := @ALSA_CFLAGS@ | ||
| KRB5_LIBS := @KRB5_LIBS@ | ||
| KRB5_CFLAGS := @KRB5_CFLAGS@ | ||
| ENABLE_LIBLINUXKRB5 := @ENABLE_LIBLINUXKRB5@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better name would perhaps be ENABLE_LIBKRB5_LINUX or ENABLE_LINUX_LIBKRB5, maybe even just ENABLE_LIBKRB5?
make/autoconf/lib-krb5.m4
Outdated
| AC_PATH_TOOL([PKG_CONFIG], [pkg-config], [no]) | ||
| use_pkgconfig_for_krb5=no | ||
| if test "x$PKG_CONFIG" != "xno"; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If SYSROOT is set, we should not try to use pkg-config or any other similar tool, as that would try to pick it up from the build host rather than the target sysroot. See how other external dependencies handle this.
make/test/JtregNativeJdk.gmk
Outdated
| ifeq ($(call isTargetOs, macosx), true) | ||
| # On macOS, disable deprecation warnings for krb5 API | ||
| BUILD_JDK_JTREG_LIBRARIES_CFLAGS_libNativeCredentialCacheHelper += -Wno-deprecated-declarations | ||
| endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this enabled and should be tested on macos regardless of ENABLE_LIBLINUXKRB5 as support was there already?
|
Thanks for all the help and pointers @erikj79 - I've pushed a commit to (hopefully) address all of your comments! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a build point of view this is definitely starting to look better. I would like to get input from a reviewer in the component team at this point to comment on the validity of the change before I spend more time reviewing the build aspects.
| endif | ||
|
|
||
| ifeq ($(call isTargetOs, linux), true) | ||
| ifeq ($(ENABLE_LIBLINUXKRB5), true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ifeq ($(ENABLE_LIBLINUXKRB5), true) | |
| ifeq ($(ENABLE_LIBKRB5_LINUX), true) |
make/autoconf/lib-krb5.m4
Outdated
| fi | ||
| AC_MSG_RESULT([$KRB5_FOUND]) | ||
| else | ||
| PKG_CHECK_MODULES(KRB5, krb5, [KRB5_FOUND=yes], [KRB5_FOUND=no]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In lib-freetype.m4 we first check if PKG_CONFIG has a value before trying to use it. We also print a result because it seems PKG_CHECK_MODULES is quiet. I tried your patch here and configure doesn't produce any output in this case, so I think we should print something.
|
Thanks for these @erikj79, no idea how I managed to miss at least one of those...! All addressed in latest commit. |
Purpose
This PR allows Linux based applications using JAAS to acquire Kerberos TGTs natively using the local system's Kerberos libraries/configuration, building on existing support on Windows/MacOSX.
Rationale
Currently the (pure java) JAAS codebase only supports file-based credential caches (ccaches). There are many other useful types of ccache accessible via the local system libraries; this change allows credentials to be acquired natively using those libraries, and thus adds support for all other ccache types supported by the local system (e.g. KCM, in-memory and kernel types), This support already exists on MacOSX and Windows.
The code change here largely uses the MacOSX code, edited for Linux with associated build system changes. It also adds an appropriate jtreg test which uses some native test helper code to manufacture an in-memory cache, and then uses the new code to acquire these credentials natively. This has been tested on Linux/Mac and the jtreg test passes on each (I couldn't see any existing tests on MacOSX for this feature).
Additionally this PR fixes a bug that's existed for a while (see L585-588 in
nativeccache.c) - without this code, this is a 100% reproducible segfault on Linux (it's unclear why this hasn't affected the Mac JVMs up to now, probably just no calling code that provides an empty list of addresses). It also fixes a (non problem) typo in the variable name in a function prototype.Implementation Detail
Note that there were multiple possible ways of doing this:
Duplicate the MacOSX
nativeccache.c, edit lightly for Linux and build a new library on Linux only (liblinuxkrb5), leaving MacOSX largely unchanged, but at the expense of this code duplication.Create a new shared library used on both platforms with conditional compilation to manage the differences. This necessitates a library name change on MacOSX and potentially knock-on packaging changes on that platform, which seemed a potentially expensive side-effect.
Create a shared
nativeccache.c(usingEXTRA_SRCin the build) and build separate MacOSX/Linux libraries. This allows the MacOSX library name to remain unchanged, and only adds a new library in Linux.I tried all three options; 3 seemed to be the best compromise all around, although is one of the options that effectively introduces a "no-op" change on MacOSX as a result. Hopefully the additional jtreg test is sufficient to compensate for this.
Interested to hear if anyone else has any suggestions for better ideas!
Notes
It wasn't clear to me what I should do with copyright headers/updating dates in headers. I've added similar boilerplate headers seen in other files to some of the new files, but please let me know what the usual form is here.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28075/head:pull/28075$ git checkout pull/28075Update a local copy of the PR:
$ git checkout pull/28075$ git pull https://git.openjdk.org/jdk.git pull/28075/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28075View PR using the GUI difftool:
$ git pr show -t 28075Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28075.diff
Using Webrev
Link to Webrev Comment