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

Rob McKenna rob.mckenna at oracle.com
Thu Apr 27 15:27:05 UTC 2017


Hi folks, I'd like to revisit this for JDK10 as it was judged to be a
bit late in the game for 9.

I've published a new webrev at:
http://cr.openjdk.java.net/~robm/8160768/webrev.05/ which incorporates
very helpful feedback from Michael Osipov.

Notes:

- The 9 request rejection noted that there were concerns about a
  "dependency on the DNS provider" - to be explicit, there should be no
  new dependencies from the original code. (the new DefaultDnsProvider
  is simply a repackaging of how we currently do things)
- As Michael noted, the assumption that the DN will map to the correct
  domain name is incorrect. The major implication from this is that the
  DNS provider will need to return both a domain name *AND* a list of
  urls corresponding to server endpoints.

The latter point results in some implementation issues. I wanted to be
able to provide this functionality with the following restrictions:

a) avoid a new public type to leave the door open to backporting the
change.

b) maintain type safety (i.e. avoid the use of Object in generic types)

I've thought of 3 possible approaches with the first of these being the
one implemented in the webrev. Per-approach, the DNS provider must
return a:

1) List<String> where the first element in the list corresponds to the
domain name and subsequent elements correspond to server endpoints.

2) Map<String, String> where the key represents the server endpoints and
the value represents the domain name. (this is particularly ungainly as the
map values will be identical but alongside option 3) is also the most
flexible)

3) Map<String, List<String>> where the key "domain" points to a list
with a single element (the domain name) and the key "urls" contains a
list of server endpoints.

Feedback (or alternative approaches) would be much appreciated!

    -Rob

On 03/02/17 01:44, Vyom Tewari wrote:
> Hi Rob,
> 
> Most of the code changes looks fine except new code will throw NPE if
> "ServiceLocator.mapDnToDomainName(ldapUrl.getDN())" is not able to map. you
> have to check for null in LdapCtxFactory.java class as follows.
> 
> if(ServiceLocator.mapDnToDomainName(ldapUrl.getDN())!=null){
>                 ((LdapCtx) ctx).setDomainName(
> ServiceLocator.mapDnToDomainName(ldapUrl.getDN()));
>                 }
> 
> 
> ##############################
> 
> Exception in thread "main" java.lang.NullPointerException
>     at java.base/java.util.Hashtable.put(Hashtable.java:474)
>     at
> java.naming/com.sun.jndi.ldap.LdapCtx.setDomainName(LdapCtx.java:2346)
>     at java.naming/com.sun.jndi.ldap.LdapCtxFactory.getUsingURLs(LdapCtxFactory.java:210)
> ################################
> 
> Thanks,
> 
> Vyom
> 
> 
> On Friday 03 February 2017 03:44 AM, Rob McKenna wrote:
> >Thanks Daniel,
> >
> >New webrev at http://cr.openjdk.java.net/~robm/8160768/webrev.04/
> >
> >In addition to your comments below I've added the package access check
> >we discussed.
> >
> >Note: when a provider returns multiple results we create an LdapCtx
> >based on the first of those. I think it might be useful to extend
> >LdapClient to accept a list of addresses which can be iterated over in the
> >event of a connection failure but I'll tackle that separately.
> >
> >     -Rob
> >
> >On 02/02/17 11:01, Daniel Fuchs wrote:
> >>Hi Rob,
> >>
> >>This is not a code I know well - but here are a couple
> >>of comments:
> >>
> >>LdapCtxFactory.java:
> >>
> >>168         NamingException ne = new NamingException();
> >>232         NamingException ne = new NamingException();
> >>
> >>Creating an exception is somewhat costly as it records the
> >>backtrace in the Throwable constructor. I don't know if
> >>it's an issue there but I thought I'd mention it. It might
> >>be better to create the exception only if you're actually
> >>going to throw it.
> >>
> >>  186         } catch (Exception e) {
> >>  187             ne.setRootCause(e);
> >>  188         }
> >>
> >>This will catch the NamingException thrown at line 173
> >>and set it at the cause of another NamingException that
> >>has no message. Maybe it would be better to have two
> >>catch clauses:
> >>
> >>  186         } catch (NamingException e) {
> >>  187             throw e;
> >>  186         } catch (Exception e) {
> >>                  ... transform it to NamingException ...
> >>
> >>In getDnsUrls:
> >>
> >>  198         if (env.containsKey(LdapCtx.DNS_PROVIDER)
> >>  199                 && env.get(LdapCtx.DNS_PROVIDER) != null
> >>  200                 && !env.get(LdapCtx.DNS_PROVIDER).equals(""))
> >>
> >>I'd suggest to simplify this and make a single lookup:
> >>
> >>              String providerClass = env.get(LdapCtx.DNS_PROVIDER);
> >>              if (providerClass != null && !providerClass.isEmpty()) {
> >>                  ...
> >>                  Class<?> cls = Class.forName(providerClass, true, cl);
> >>
> >>best regards,
> >>
> >>-- daniel
> >>
> >>
> >>On 01/02/17 20:05, Rob McKenna wrote:
> >>>New webrev with updated test here:
> >>>
> >>>http://cr.openjdk.java.net/~robm/8160768/webrev.03/
> >>>
> >>>    -Rob
> >>>
> >>>On 26/01/17 04:03, Rob McKenna wrote:
> >>>>Ack, apologies, thats what I get for rushing. (which I suppose I'm doing
> >>>>again) That code was actually supposed to be in the getDnsUrls method.
> >>>>Updated in place:
> >>>>
> >>>>http://cr.openjdk.java.net/~robm/8160768/webrev.02/
> >>>>
> >>>>I'll add a couple of tests for the SecurityManager along with some input
> >>>>checking tests too.
> >>>>
> >>>>    -Rob
> >>>>
> >>>>On 25/01/17 05:50, Daniel Fuchs wrote:
> >>>>>Hi Rob,
> >>>>>
> >>>>>I believe you should move the security check to before
> >>>>>the class is actually loaded, before the call to
> >>>>>171             List<String> urls = getDnsUrls(url, env);
> >>>>>
> >>>>>best regards,
> >>>>>
> >>>>>-- daniel
> >>>>>
> >>>>>On 25/01/17 17:44, Rob McKenna wrote:
> >>>>>>I neglected to include a security check so I've cribbed the one from
> >>>>>>OBJECT_FACTORIES (NamingManager.setObjectFactoryBuilder()) - see:
> >>>>>>
> >>>>>>http://cr.openjdk.java.net/~robm/8160768/webrev.02/
> >>>>>>
> >>>>>>Note, this wraps the SecurityException in a NamingException. Presumably
> >>>>>>its better to throw something than simply leave the default value in
> >>>>>>place, but feedback appreciated!
> >>>>>>
> >>>>>>   -Rob
> >>>>>>
> >>>>>>On 25/01/17 04:31, Rob McKenna wrote:
> >>>>>>>Hi folks,
> >>>>>>>
> >>>>>>>I'm looking for feedback on this suggested fix for the following bug:
> >>>>>>>
> >>>>>>>https://bugs.openjdk.java.net/browse/JDK-816076
> >>>>>>>http://cr.openjdk.java.net/~robm/8160768/webrev.01/
> >>>>>>>
> >>>>>>>This is something that has come up a few times. Basically in certain
> >>>>>>>environments (e.g. MS Active Directory) there is a dependence on
> >>>>>>>DNS records to provide a pointer to the actual ldap server to be used
> >>>>>>>for a given LdapCtx.PROVIDER_URL where the url itself simply points to the
> >>>>>>>domain name.
> >>>>>>>
> >>>>>>>This fix add a new Ldap context property which allows a user to specify a
> >>>>>>>class (implementing BiFunction) which can perform any necessary extra steps
> >>>>>>>to derive the ldap servers hostname/port from the PROVIDER_URL.
> >>>>>>>
> >>>>>>>   -Rob
> >>>>>>>
> 


More information about the core-libs-dev mailing list