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

[Weijun Wang]

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