[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-security-dev
Subject:    [security-dev 00787]: Re: Code review request: Undefined requesting URL in	"java.net.Authenticator.g
From:       Weijun.Wang () Sun ! COM (Weijun Wang)
Date:       2009-04-28 2:10:32
Message-ID: 49F66598.4010604 () sun ! com
[Download RAW message or body]

Hi Valerie

> 6578647: Undefined requesting URL in java.net.Authenticator.getPasswordAuthentication()
> 6829283: HTTP/Negotiate: Autheticator triggered again when user cancels the first one
> 

Chris has reviewed the net part of the two webrevs and I've updated the
fixes and added tests. Can you take a look at the JGSS side?

The useful changes in JGSS is in GSSUtil.java. All other changes are
simply s/int/GSSCaller/ or s/GSSUtil/GSSCaller/ when the pre-defined
callers are used.

The updated webrevs are at:

http://cr.openjdk.java.net/~weijun/6578647/webrev.02/
http://cr.openjdk.java.net/~weijun/6829283/webrev.01/

Thanks
Max

Christopher Hegarty - Sun Microsystems Ireland wrote:
> Hi Max,
> 
> I'm not overly familiar with this code, so another reviewer would be
> prudent.
> 
> The changes look fine. I have just two minor comments:
> 1) In handle(Callback[]) I'd move the call to getAnswer from L83 and
>    L86 and put it before the if statement. I expect that an
>    unsupported callback would be rare.
> 2) I don't see that you need to set the default values for the class
>    members username and answered. I actually believe that Suns javac
>    generates more unnecessary bytecode to set these values.
> 
> As I said the comments are minor (feel free to ignore them). Otherwise
> looks good.
> 
> -Chris.
> 
> Weijun Wang wrote:
>> Hi Chris/Valerie
>>
>> Can you take a review on a related bug. I found it when I wrote the test
>> for the previous one.
>>
>> 6829283: HTTP/Negotiate: Authenticator triggered again when user cancels
>> the first one
>> http://cr.openjdk.java.net/~weijun/6829283/webrev.00/
>>
>> Basically, it's because for HTTP/Negotiate, it's
>>
>>    ... -> Callback -> Authenticator
>>
>> We have 2 callbacks (user and pass) in JAAS, but there's only 1
>> Authenticator (doing user and pass at the same time). If user cancels
>> the first call, we shouldn't bother her again.
>>
>> Thanks
>> Max
>>
>> Max Wang (Weijun) wrote:
>>> Hi Chris
>>>
>>> A new webrev is created at
>>>     http://cr.openjdk.java.net/~weijun/6578647/webrev.01
>>>
>>> Now all HttpCallerInfo creations are inline, so the diff is much
>>> clearer. There's one place I didn't call toLowerCase(), the call is
>>> moved into NegotiatorImpl right before the service principal name is
>>> created.
>>>
>>> I also add a test, putting two Kerberos KDC, one HTTP server, one proxy
>>> server in a single regression test is fun!
>>>
>>> Thanks
>>> Mx
>>>
>>> On Apr 14, 2009, at 8:55 PM, Max (Weijun) Wang wrote:
>>>
>>>> On Apr 14, 2009, at 5:59 PM, Christopher Hegarty - Sun Microsystems
>>>> Ireland wrote:
>>>>
>>>>> Hi Max,
>>>>>
>>>>> I only looked at the networking part of the changes. They look fine,
>>>>> I just have a few questions/comments:
>>>>>
>>>>> 1) sun.net.www.protocol.http.HttpURLConnection
>>>>>  Can you use the same HttpCallerInfo instance for proxy authentication
>>>>>  at line 1108? This instance has been created using the single arg
>>>>>  constructor therefore it is has authType = RequestorType.SERVER,
>>>>>  right?
>>>> Yes, you're right. Will update tomorrow.
>>>>
>>>>> 2) sun.net.www.protocol.http.HttpCallerInfo
>>>>>  It is just my preference, but I would prefer to see all the fields of
>>>>>  HttpCallerInfo private and have simple accessors:
>>>>>      private final String host;
>>>>>      ......
>>>>>
>>>>>      public String host() {
>>>>>          return host;
>>>>>      }
>>>>>      ......
>>>> Your suggestion is more formal. But I think making all fields final is
>>>> also sufficient to make it immutable.
>>>>
>>>>> 3) Are the changes to use HttpCallerInfo in AuthenticationHeader,
>>>>>  HttpURLConnection, NegotiateAuthentication and NegotiatorImpl
>>>>>  strictly necessary? They seem to be changed just for consistency of
>>>>>  using the new class. I only see that NegotiateCallbackHandler is
>>>>>  required to use this new class on the networking side.
>>>> There needs a way to transfer these info into the JGSS underneath (so
>>>> that NegotiateCallbackHandler has a chance to know them), and the only
>>>> bridge is inside NegotiatorImpl. I don't know if there's a better way
>>>> to do this. The HttpClient class seems having similar info but
>>>> sometimes it's null and I don't know why. Sorry if I reinvent a
>>>> wheel-cart to carry these info.
>>>>
>>>> Thanks
>>>> Max
>>>>
>>>>>  This is not a problem just a question to see if I understand
>>>>>  correctly the changes.
>>>>>
>>>>> -Chris.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On 04/13/09 03:27, Weijun Wang wrote:
>>>>>> Hi Valerie and Networking guys
>>>>>> Please take a review at this bug fix:
>>>>>>   http://cr.openjdk.java.net/~weijun/6578647/webrev.00/
>>>>>> The bug is
>>>>>>   http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6578647
>>>>>> The bug report says that no URL-related info is available in
>>>>>> Authenticator when using HTTP/Negotiate. The reason is that in the
>>>>>> long
>>>>>> stack of
>>>>>>  HTTP/Negotiate -> JGSS -> JAAS -> Krb5LoginModule
>>>>>>      -> Callback -> Authenticator
>>>>>> The URL info is lost. In order to support special actions for
>>>>>> HTTP/Negotiate calls in JGSS (say, using Authenticator instead of
>>>>>> text-based callback, honor the OK-AS-DELEGATE flag...), we already
>>>>>> used
>>>>>> an integer field (caller) to tell the codes deep below who initiates
>>>>>> the
>>>>>> JGSS calls. It seems an integer is not enough to carry too much
>>>>>> information. (oh, I love the C void*)
>>>>>> The fix is simple: change the caller from integer to a Java class:
>>>>>> GSSCaller, which includes as much as info it likes. For
>>>>>> HTTP/Negotiate,
>>>>>> a child class HttpCaller, encapsulates all info an Authenticator
>>>>>> needs.
>>>>>> The fix includes three parts:
>>>>>> 1. Three new classes:
>>>>>>  sun.sec.jgss.GSSCaller:
>>>>>>      the new caller
>>>>>>  sun.sec.jgss.HttpCaller:
>>>>>>      a child of GSSCaller, knows everything about HTTP
>>>>>>  sun.net.www.protocol.http.HttpCallerInfo:
>>>>>>      the info GSSCaller knows, this class is created on the
>>>>>>      network side so that no sun.security.jgss.* codes are
>>>>>>      dragged into the bootstrap building process.
>>>>>> 2. On the network side:
>>>>>>  Refactoring HTTP codes in sun.net.www.protocol.http.* to fill info
>>>>>>  into the HttpCallerInfo class.
>>>>>> 3. On the JGSS side:
>>>>>>  Multiple changes in sun.security.jgss.* classes. *All* the
>>>>>>  code changes are simply s/int/GSSCaller/g changes.
>>>>>>  I also moved the pre-defined callers from GSSUtil to
>>>>>>  GSSCaller.
>>>>>> Thanks
>>>>>> Max


[prev in list] [next in list] [prev in thread] [next in thread]