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
Mon Nov 19 14:04:33 UTC 2012
Sure. I'll do some basic test before requesting for backport. Tomorrow.
10pm here in Beijing.
Thanks
Max
On 11/19/2012 07:37 PM, Severin Gehwolf wrote:
> Max,
>
> Can we get the fix[1] backported to JDK7? Patch applies cleanly on top
> of JDK 7 sources for me.
>
> Thanks,
> Severin
>
> [1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f740a9ac6eb6
>
> On Wed, 2012-11-14 at 17:04 +0400, Dmitry Samersoff wrote:
>> 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
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
>
>
>
More information about the security-dev
mailing list