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

Rob McKenna rob.mckenna at oracle.com
Tue Nov 6 15:52:03 UTC 2018

Thanks folks,

Vyom, I've updated service to be volatile.

On 30/10/18 14:25, Daniel Fuchs wrote:

Hi Rob,

LdapCtxFactory.java 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?

Yes, you're right, we should return the first successful result.

LdapDnsProviderService.java:

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

Good point, fixed.

LdapDnsProvider.java:

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?

So I've altered the signature of the method to take a Map as proposed. I've added a getLdapService(String domainName, Map environment) method to ServiceLocator which delegates to the existing method after conversion. Hopefully this addresses your concerns.

I'll update the CSR accordingly once this review is complete.

-Rob

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. 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