8002344 code review request: (was Re: [PATCH] for bug 2376501: Krb5LoginModule config class does not return proper KDC list from DNS)
Severin Gehwolf
sgehwolf at redhat.com
Mon Nov 19 11:37:52 UTC 2012
Max,
Can we get the fix[1] backported to JDK7? Patch applies cleanly on top
of JDK 7 sources for me.
Thanks,
Severin
[1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f740a9ac6eb6
On Wed, 2012-11-14 at 17:04 +0400, Dmitry Samersoff wrote:
> Weijun,
>
> On 2012-11-14 12:08, Weijun Wang wrote:
> >> On 2012-11-14 04:36, Weijun Wang wrote:
> >>> Yes, but with line 288, hostport will be "host:1". Isn't that expected?
> >>
> >> I'm OK with ll. 288 but against ll.285. I'm against comparing integers
> >> as strings - port could (imaginary) be specified as 0088 and comparison
> >> fails.
> >
> > I see.
> >
> > Actually, in KdcComm.java when the string "host:port" is really been
> > used, Integer.parseInt(port) is called and when an exception is thrown
> > host:88 is tried, although this might not be the correct guess.
> >
> > Are you ok with this "delayed" check?
>
> Yes.
>
> >
> > As for the possibility of 0088, I'll remove that equals check and stick
> > with
> >
> > hostport = tokenizer.nextToken() + ":" + port;
>
> I'm OK with it.
>
> -Dmitry
>
>
> >
> > You can see my DNS.java test already prepared for this by comparing to
> > both a.com.:88 and a.com. :)
> >
> > Thanks
> > Max
> >
> >>
> >>
> >>>> Customer allowed to use service name here instead of numeric port
> >>>> number
> >>>> if my memory is not bogus.
> >>>
> >>> rfc2782 [1] says it can be only a number:
> >>
> >> Thank you for clarification.
> >> -Dmitry
> >>
> >>>
> >>> Port
> >>> The port on this target host of this service. The range is 0-
> >>> 65535. This is a 16 bit unsigned integer in network byte
> >>> order.
> >>> This is often as specified in Assigned Numbers but need not be.
> >>>
> >>> If it's really a service name, I might have to ignore it at the moment
> >>> because I don't know an API to perform getportbyname().
> >>>
> >>>>
> >>>> 2.
> >>>>> shadow the real one by prepending it to the bootclasspath.
> >>>> jtreg allows you to change boot classpath in othervm mode
> >>>>
> >>>> e.g. :
> >>>>
> >>>> @run main/othervm -Xbootclasspath/a:../classes/serviceability
> >>>> -XX:+UnlockDiagnosticVMOptions ... ParserTest
> >>>
> >>> Good suggestion, but the class is built into test.classes and test run
> >>> in scratch. I cannot find a way to get the relative path to the class. I
> >>> reply on "javac -d ." to output the class into scratch.
> >>>
> >>> Thanks
> >>> Max
> >>>
> >>> [1] http://tools.ietf.org/html/rfc2782
> >>>
> >>>>
> >>>>
> >>>> -Dmitry
> >>>>
> >>>> On 2012-11-13 16:07, Weijun Wang wrote:
> >>>>>
> >>>>>
> >>>>> On 11/13/2012 07:44 PM, Dmitry Samersoff wrote:
> >>>>>> Weijun,
> >>>>>>
> >>>>>> Config.java:1162
> >>>>>> This code is unclear to me.
> >>>>>> if srvs[i] could be "" this code could insert extra space in
> >>>>>> the middle of kdcs string.
> >>>>>
> >>>>> It should never be empty. KrbServiceLocator.java:281 makes sure it's a
> >>>>> valid DNS SRV record.
> >>>>>
> >>>>>>
> >>>>>> if srvc[i] couldn't be empty we can return null just
> >>>>>> after line 1160 if srvs.length == 0
> >>>>>
> >>>>> Yes, we can.
> >>>>>
> >>>>>>
> >>>>>> KrbServiceLocator.java:285
> >>>>>>
> >>>>>> Kerberos could be run on non standard port - rare case but
> >>>>>> AFAIK we don't limit it. So it's better to use parseInt here.
> >>>>>
> >>>>> That's line 288. Are you suggesting that port string can be
> >>>>> non-numeric
> >>>>> and need a check?
> >>>>>
> >>>>>>
> >>>>>> dns.sh:
> >>>>>> Why we need Shell script here?
> >>>>>
> >>>>> I cannot use the real NamingManager inside JDK because it will attempt
> >>>>> to access a real DNS server, thus I write my own fake provider and
> >>>>> shadow the real one by prepending it to the bootclasspath.
> >>>>>
> >>>>> Thanks
> >>>>> Max
> >>>>>
> >>>>>
> >>>>>>
> >>>>>> Regards,
> >>>>>> -Dmitry
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 2012-11-13 15:05, Weijun Wang wrote:
> >>>>>>> Ping again.
> >>>>>>>
> >>>>>>> The webrev contains codes by myself so I need another reviewer.
> >>>>>>>
> >>>>>>> Thanks
> >>>>>>> Max
> >>>>>>>
> >>>>>>>
> >>>>>>> On 11/09/2012 08:38 AM, Weijun Wang wrote:
> >>>>>>>> Hi Severin
> >>>>>>>>
> >>>>>>>> I've created an OpenJDK bug and created a new webrev:
> >>>>>>>>
> >>>>>>>> http://cr.openjdk.java.net/~weijun/8002344/webrev.00/
> >>>>>>>>
> >>>>>>>> The Config.java change is identical to yours, and I added a small
> >>>>>>>> tweak
> >>>>>>>> in KrbServiceLocator, and a quite ugly regression test.
> >>>>>>>>
> >>>>>>>> Anyone can review all the changes?
> >>>>>>>>
> >>>>>>>> After the code review, I'll push the change to tl/jdk. I don't
> >>>>>>>> see an
> >>>>>>>> OpenJDK user id for you at http://db.openjdk.java.net/people, so I
> >>>>>>>> add
> >>>>>>>> your name in
> >>>>>>>>
> >>>>>>>> Contributed-by: Severin Gehwolf <sgehwolf at redhat.com>
> >>>>>>>>
> >>>>>>>> Thanks
> >>>>>>>> Max
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 11/08/2012 11:46 PM, Severin Gehwolf wrote:
> >>>>>>>>> Hi Max,
> >>>>>>>>>
> >>>>>>>>> Thanks for the review!
> >>>>>>>>>
> >>>>>>>>> On Wed, 2012-11-07 at 08:52 +0800, Weijun Wang wrote:
> >>>>>>>>>> The fix looks fine. There is one place it might get enhanced:
> >>>>>>>>>>
> >>>>>>>>>> if (value.charAt(j) == ':') {
> >>>>>>>>>> kdcs = (value.substring(0, j)).trim();
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>>> So this changes a.com:88 to a.com. If the port is really 88,
> >>>>>>>>>> it's OK.
> >>>>>>>>>> Otherwise, info gets lost. Maybe we can keep the whole string.
> >>>>>>>>>
> >>>>>>>>> I've removed the entire loop which strips the port from the
> >>>>>>>>> returned
> >>>>>>>>> record. Updated webrev is here:
> >>>>>>>>>
> >>>>>>>>> http://jerboaa.fedorapeople.org/bugs/openjdk/2376501/webrev.1/
> >>>>>>>>>
> >>>>>>>>>> BTW, are you OK with contributing the fix into OpenJDK main repo?
> >>>>>>>>>
> >>>>>>>>> Yes, of course :) Just let me know what's to be done to get it
> >>>>>>>>> pushed.
> >>>>>>>>>
> >>>>>>>>> Cheers,
> >>>>>>>>> Severin
> >>>>>>>>>
> >>>>>>>>>> On 11/06/2012 11:08 PM, Severin Gehwolf wrote:
> >>>>>>>>>>> Hi,
> >>>>>>>>>>>
> >>>>>>>>>>> In Config.java, line 1234 in method getKDCFromDNS(String realm)
> >>>>>>>>>>> there is
> >>>>>>>>>>> a loop which discards earlier values of KDCs returned rather
> >>>>>>>>>>> than
> >>>>>>>>>>> concatenating them. This results in a behaviour where only
> >>>>>>>>>>> one KDC
> >>>>>>>>>>> in a
> >>>>>>>>>>> seemingly random fashion is returned. In fact, the KDC returned
> >>>>>>>>>>> depends
> >>>>>>>>>>> on the order which KrbServiceLocator.getKerberosService(realm,
> >>>>>>>>>>> "_udp")
> >>>>>>>>>>> returns the servers. The correct behaviour should be to return a
> >>>>>>>>>>> String
> >>>>>>>>>>> containing ALL KDCs available via DNS (separated by spaces).
> >>>>>>>>>>>
> >>>>>>>>>>> The webrev is here:
> >>>>>>>>>>> http://jerboaa.fedorapeople.org/bugs/openjdk/2376501/webrev/
> >>>>>>>>>>>
> >>>>>>>>>>> Comments and suggestions very welcome!
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks,
> >>>>>>>>>>> Severin
> >>>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>
> >>>>>>
> >>>>
> >>>>
> >>
> >>
>
>
More information about the security-dev
mailing list