8002344 code review request: (was Re: [PATCH] for bug 2376501: Krb5LoginModule config class does not return proper KDC list from DNS)
Weijun Wang
weijun.wang at oracle.com
Wed Nov 14 08:08:38 UTC 2012
On 11/14/2012 03:28 PM, Dmitry Samersoff wrote:
> 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.
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?
As for the possibility of 0088, I'll remove that equals check and stick with
hostport = tokenizer.nextToken() + ":" + port;
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