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