8002344 code review request: (was Re: [PATCH] for bug 2376501: Krb5LoginModule config class does not return proper KDC list from DNS)

[Dmitry Samersoff]

Weijun,

Sorry. I was not clean enough. See below.

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.


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


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer