On Fri, 31 Oct 2025 01:16:01 GMT, Nick Hall <[email protected]> wrote:
>> _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: >> >> 1) 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. >> >> 2) 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. >> >> 3) Create a shared `nativeccache.c` (using `EXTRA_SRC` in 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 >> compensat... > > Nick Hall has updated the pull request incrementally with one additional > commit since the last revision: > > Fixes to ensure that the jtreg test is not built or executed if krb5 is not > installed make/autoconf/lib-krb5.m4 line 32: > 30: [ > 31: AC_ARG_WITH(krb5, [AS_HELP_STRING([--with-krb5], > 32: [enable krb5 support (default=yes), or "no" to disable])]) 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-<lib> option can also be used to point to the location of the installation, usually combined with individual --with-<lib>-include and --with-<lib>-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 line 39: > 37: AC_MSG_NOTICE([krb5 explicitly disabled]) > 38: KRB5_DISABLED=yes > 39: elif test "x$NEEDS_LIB_KRB5" = xfalse; then Since this is an optional dependency, perhaps use a different variable name? make/autoconf/lib-krb5.m4 line 52: > 50: else > 51: # First try pkg-config (most modern approach) > 52: AC_PATH_TOOL([PKG_CONFIG], [pkg-config], [no]) PKG_CONFIG should already be setup in pkg.m4. make/autoconf/lib-krb5.m4 line 55: > 53: use_pkgconfig_for_krb5=no > 54: > 55: if test "x$PKG_CONFIG" != "xno"; then 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/autoconf/lib-krb5.m4 line 78: > 76: else > 77: # Fallback: try krb5-config > 78: AC_PATH_TOOL([KRB5CONF], [krb5-config], [no]) We have our own internal macro `UTIL_LOOKUP_PROGS` that is preferred over the AC variants. make/autoconf/libraries.m4 line 87: > 85: # Check if krb5 is needed > 86: if test "x$OPENJDK_TARGET_OS" = xlinux -o "x$OPENJDK_TARGET_OS" = > xmacosx; then > 87: NEEDS_LIB_KRB5=true My understanding is that this external dependency is only needed on Linux. The resulting variable even has linux in the name. make/autoconf/spec.gmk.template line 440: > 438: KRB5_LIBS := @KRB5_LIBS@ > 439: KRB5_CFLAGS := @KRB5_CFLAGS@ > 440: ENABLE_LIBLINUXKRB5 := @ENABLE_LIBLINUXKRB5@ A better name would perhaps be `ENABLE_LIBKRB5_LINUX` or `ENABLE_LINUX_LIBKRB5`, maybe even just `ENABLE_LIBKRB5`? make/test/JtregNativeJdk.gmk line 121: > 119: # On macOS, disable deprecation warnings for krb5 API > 120: BUILD_JDK_JTREG_LIBRARIES_CFLAGS_libNativeCredentialCacheHelper += > -Wno-deprecated-declarations > 121: endif Isn't this enabled and should be tested on macos regardless of `ENABLE_LIBLINUXKRB5` as support was there already? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/28075#discussion_r2481585933 PR Review Comment: https://git.openjdk.org/jdk/pull/28075#discussion_r2481626000 PR Review Comment: https://git.openjdk.org/jdk/pull/28075#discussion_r2481600747 PR Review Comment: https://git.openjdk.org/jdk/pull/28075#discussion_r2482215190 PR Review Comment: https://git.openjdk.org/jdk/pull/28075#discussion_r2481617382 PR Review Comment: https://git.openjdk.org/jdk/pull/28075#discussion_r2481572420 PR Review Comment: https://git.openjdk.org/jdk/pull/28075#discussion_r2481631167 PR Review Comment: https://git.openjdk.org/jdk/pull/28075#discussion_r2482218069
