On Mon, 20 Apr 2026 11:00:30 GMT, Kieran Farrell <[email protected]> wrote:
> In DNS-based KDC discovery failures are exposed as generic 'KrbException: > Cannot locate KDC / Unable to locate KDC for realm <REALM>' with no > indication whether the underlying DNS SRV lookup failed due to NXDOMAIN, > SERVFAIL, or a communication timeout. > > To improve supportability, this patch updates > `KrbServiceLocator.getKerberosService(realm, protocol)` to rethrow the > original JNDI NamingException from the SRV lookup and attach a sanitized > failure category to the existing KrbException when both udp and tcp discovery > attempts fail, while preserving the original top level exception message. > `Config.getKDCFromDNS()` is updated to catch exception, sanitize it into the > relevant category to prevent leaking any senistive information and attach it > to the existing KrbException. > > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). Just some minor questions src/java.security.jgss/share/classes/sun/security/krb5/Config.java line 2: > 1: /* > 2: * Copyright (c) 2000, 2025, Oracle and/or its affiliates. All rights > reserved. Suggestion: * Copyright (c) 2000, 2026, Oracle and/or its affiliates. All rights reserved. src/java.security.jgss/share/classes/sun/security/krb5/Config.java line 1391: > 1389: String kdcs = ""; > 1390: String[] srvs = null; > 1391: NamingException udpNE = null; I think this might be a bit hard to read, what do you think about making the name longer like `udpNamingException` so it's clear at a glance? If you prefer as is i'm fine with it src/java.security.jgss/share/classes/sun/security/krb5/KrbServiceLocator.java line 2: > 1: /* > 2: * Copyright (c) 2006, 2024, Oracle and/or its affiliates. All rights > reserved. Suggestion: * Copyright (c) 2006, 2026, Oracle and/or its affiliates. All rights reserved. test/jdk/sun/security/krb5/SRVLookupFailureTest.java line 36: > 34: */ > 35: > 36: import javax.security.auth.callback.*; minor: could you please change wildcard import to explicit? test/jdk/sun/security/krb5/SRVLookupFailureTest.java line 44: > 42: static class Handler implements CallbackHandler { > 43: @Override > 44: public void handle(Callback[] callbacks) throws > UnsupportedCallbackException { nit: 80 chars line Suggestion: public void handle(Callback[] callbacks) throws UnsupportedCallbackException { test/jdk/sun/security/krb5/SRVLookupFailureTest.java line 66: > 64: if (!containsSanitizedCause(e)) { > 65: e.printStackTrace(System.out); > 66: throw new RuntimeException("Expected DNS SRV failure > cause not found"); Suggestion: throw new RuntimeException( "Expected DNS SRV failure cause not found"); test/jdk/sun/security/krb5/SRVLookupFailureTest.java line 76: > 74: // check for the DNS SRV failure cause > 75: if (msg != null && msg.contains("DNS SRV lookup failed:")) { > 76: if (msg.contains("NXDOMAIN") || msg.contains("SERVFAIL") > || msg.contains("COMMUNICATION_ERROR")) { Suggestion: if (msg.contains("NXDOMAIN") || msg.contains("SERVFAIL") || msg.contains("COMMUNICATION_ERROR")) { test/jdk/sun/security/krb5/SRVLookupFailureTest.java line 84: > 82: return false; > 83: } > 84: } Nit: new lines in the end of the files ------------- PR Review: https://git.openjdk.org/jdk/pull/30824#pullrequestreview-4163479461 PR Review Comment: https://git.openjdk.org/jdk/pull/30824#discussion_r3131736055 PR Review Comment: https://git.openjdk.org/jdk/pull/30824#discussion_r3131751798 PR Review Comment: https://git.openjdk.org/jdk/pull/30824#discussion_r3131734050 PR Review Comment: https://git.openjdk.org/jdk/pull/30824#discussion_r3131697525 PR Review Comment: https://git.openjdk.org/jdk/pull/30824#discussion_r3131721989 PR Review Comment: https://git.openjdk.org/jdk/pull/30824#discussion_r3131724148 PR Review Comment: https://git.openjdk.org/jdk/pull/30824#discussion_r3131728927 PR Review Comment: https://git.openjdk.org/jdk/pull/30824#discussion_r3131685227
