[security-dev 00761]: Re: Code review request: Undefined requesting URL in "java.net.Authenticator.getPasswordAuthentication()"

[Christopher Hegarty - Sun Microsystems Ireland]

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