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

[Weijun Wang]

Hi Dmitry

On 11/13/2012 08:18 PM, Dmitry Samersoff wrote:
> 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.

Yes, but with line 288, hostport will be "host:1". Isn't that expected?

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

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