On Apr 17, 2009, at 10:08 PM, 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.
In fact, the handler is called inside JDK. Two of them, one with one
NameCallback, and another with one PasswordCallback. However, I would
like handle() to look more general.
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.
Correct.
Thanks
Max
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
