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

[Dmitry Samersoff]

Weijun,

1.
> That's line 288. Are you suggesting that port string can be
> non-numeric and need a check?

Port could be not a default kerberos port but another one - e.g.
customer can setup kerberos on port *1* if they want.

Customer allowed to use service name here instead of numeric port number
if my memory is not bogus.

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


-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