8002344 code review request: (was Re: [PATCH] for bug 2376501: Krb5LoginModule config class does not return proper KDC list from DNS)
Dmitry Samersoff
dmitry.samersoff at oracle.com
Wed Nov 14 13:04:55 UTC 2012
Weijun,
On 2012-11-14 12:08, Weijun Wang wrote:
>> 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?
Yes.
>
> As for the possibility of 0088, I'll remove that equals check and stick
> with
>
> hostport = tokenizer.nextToken() + ":" + port;
I'm OK with it.
-Dmitry
>
> 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
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer
More information about the security-dev
mailing list