[RFR] 8160768: Add capability to custom resolve host/domain names within the default JDNI LDAP provider

Daniel Fuchs daniel.fuchs at oracle.com
Tue Oct 30 14:25:18 UTC 2018

Hi Rob,


  187             for (String u : r.getEndpoints()) {
  188                 try {
  189                     ctx = getLdapCtxFromUrl(
  190                             r.getDomainName(), new LdapURL(u), env);
  191                 } catch (NamingException e) {
  192                     // try the next element
  193                     lastException = e;
  194                 }
  195             }

is a break statement missing after line 190?

If not then can you add a comment explaining why only the last
context is retained (and returned?)

Alternatively, if a break is indeed missing, these three lines
could be moved into the for loop above:

  206             // Record the URL that created the context
  207             ctx.setProviderUrl(url);
  208             return ctx;

and maybe lines 206-207 could be moved into the
getLdapCtxFromUrl() method?


Why is this class non final? If it can be made final then
the protected methods should probably become package.


It is strange to see new APIs with Hashtable in the method
signature. I understand that our implementation will need
an Hashtable at some point to call javax.naming.spi.NamingProvider,
but how likely is it that a clean room implementation of
a LdapDnsProvider will need to do that?

Maybe we could have Map<?,?> in the signature instead - and
leave the burden to our implementation - somewhere in ServiceLocator,
to adapt back to Hashtable where it needs to?

best regards,

-- daniel

On 25/10/2018 17:34, Rob McKenna wrote:
> This recently received CSR approval, so it seems like a good time to pick
> the codereview up again:
> http://cr.openjdk.java.net/~robm/8160768/webrev.08/
> Referencing:
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050794.html
> 1) I'm copying the behaviour from
> LdapCtxFactory.getInitialContext(Hashtable<?,?>) where an empty String is
> the default value used when the provider url is null.
> I don't think HostnameChecker (by way of SNIHostname) will accept either
> empty or null values when doing the comparison.
> Somewhat oddly however, LdapCtx.extendedOperation(ExtendedRequest)
> appears to pass the String "null" to
> StartTlsResponseImpl.setConnection(Connection, String), which attempts
> to substitute null values with the Connections host so there may be a bug
> here.
> I'm happy to allow null returns here if necessary. Sean, can you
> comment on the distinction between null & "" as far as hostname
> verification is concerned?
> 2) In the latest iteration lookupEndpoints() returns an
> Optional<LdapDnsResult>. Currently neither getEndpoints() nor
> getDomainName() can be null. (they can be an empty list and/or an empty
> String however)
> 3) Corrected.
> 4) See https://www.oracle.com/technetwork/java/seccodeguide-139067.html#4-5
>      -Rob

More information about the core-libs-dev mailing list