<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
</head>
<body bgcolor="#ffffff" text="#000000">
Max,<br>
<br>
Here are some comments for 6894072: always refresh keytab<br>
General:<br>
1) Copyright year should be 2011<br>
2) You added new classes under javax.XX packages: Have you filed CCC
for them?<br>
<br>
Krb5LoginModule.java<br>
1) if useKeyTab is set to true in the config, then the if-cond on line
712 will always be false given the ktab assignment on line 692, and
then it won't get to the password prompting call on line 713? This
seems different than what the comment (line 671-689) described.<br>
<br>
KerberosKey.java <br>
1) "at needed" probably should be "when
needed" or "as needed"<br>
<pre><span class="new"> 46 * the {@link KeyTab} class, where latest keys can be read at needed.</span></pre>
Krb5AcceptCredential.java<br>
1) you changed it to not extending KerberosKey, potential compatibility
concern?<br>
<br>
Krb5Util.java<br>
1) When calling ServiceCreds.getInstance(), your change only checks
that kt, kk can't both be null, i.e. line 221. However, at line 237, it
uses kk when ktab file is missing. It looks to me that this may leads
NPE if the ServiceCreds is constructed w/ a non-null (but non-existent)
KeyTab object and null KerberosKey list.<br>
2) when shall ServiceCreds.destroy() be called? For every re-read of
KeyTab object, e.g. line 257<br>
3) You replaced the Krb5Util.getKeys(GSSCaller, ...) method w/
ServicesCreds.<span class="new">getServiceCreds(GSSCaller,...). If you
don't update JSSE source immediately, people will run into a
compilation failure. Perhaps it's better to keep the
Krb5Util.getKeys(GSSCaller,...) method and then remove this method
together w/ the updated JSSE sources.<br>
4) at line 291 and 299, will the KerberosPrincipal found off the
Subject be different from the specified server principal? Seems
somewhat strange that we just grab the first one.</span><br>
<br>
KrbAsReq.java<br>
1) I can't see any change? Is it included in this webrev by mistake?<br>
<br>
KrbAsReqBuilder.java<br>
1) Its destroy() method no longer destroys key nor clears password? Is
this intentional? If yes, then the method description should be
updated. Also, how/when will keys or keytab objects be destroyed and
password be cleared?<br>
2) Exception message at line 203 may be better phrased as "Required
password not provided".<br>
<br>
sun.security.krb5.internal.ktab.KeyTab.java<br>
1) your "isReading" flag is static to the KeyTab class but the way you
use it seems to suggest that it should be associated w/ each entry of
the "map" Hashmap. <br>
<br>
SharedSecrets.java, SubjectComber.java, Kinit.java, Klist.java,
Ktab.java, EncryptionKey.java, Config.java, Krb5ProxyImpl.java,
ServerHandshaker.java, KrbAsRep.java<font color="#000000">,
JavaxSecurityAuthKerberosAccessImpl.java, </font><font color="#000000">JavaxSecurityAuthKerberosAccess.java</font><br>
=> looks ok<br>
<br>
<font color="green"><font color="#000000">javax.security.auth.KeyTab.java</font><br>
</font><font color="green"></font>1) for its getInstance(File) method,
the javadoc specifies that a NPE is thrown if file argument is null.
However, the private constructor KeyTab(File) does not throw NPE when
null is specified. <br>
2) why not leverage getEncryptionKeys(PrincipalName) for the code of
line 148-155? They seem similar enough.<br>
<br>
Thanks,<br>
Valerie<br>
<font color="green"><b><br>
</b></font>On 12/01/10 01:46 AM, Weijun Wang wrote:
<blockquote cite="mid:4CF6195A.6000301@oracle.com" type="cite">Hi
Valerie <br>
<br>
The webrev is at -- <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eweijun/6894072/webrev.00/">http://cr.openjdk.java.net/~weijun/6894072/webrev.00/</a>
<br>
<br>
Changes: <br>
<br>
1. New javax..KeyTab, updated sun..KeyTab. As the impl note in
javax..KeyTab says: the former is a name with dynamic content, the
latter is a snapshot of a file. <br>
<br>
2. Now Subject can have private credentials with type KeyTab. Thus the
content of Krb5AcceptCredential is not only keys. Krb5Util defines an
expandable ServiceCreds class for this purpose. <br>
<br>
3. KrbAsReqBuilder was constructed with password or keys, now with
password or keytab. Kinit and Krb5LoginModule updated accordingly. <br>
<br>
4. Having parallel defined KerberosKey/KerberosPrincipal and
EncrytionKey/PrincipalName is complicated. Special Unsafe methods are
defined to get EncryptionKey thru a PrincipalName from new
javax..KeyTab. Might look into consolidate data types some day. <br>
<br>
Thanks <br>
Max <br>
<br>
<br>
-------- The Bug -------- <br>
*Change Request ID*: 6894072 <br>
*Synopsis*: always refresh keytab <br>
<br>
Product: java <br>
Category: jgss <br>
Subcategory: krb5plugin <br>
Type: RFE <br>
<br>
=== *Description* ====================================== <br>
info from keytab should be refreshed at every security context
establishment in Kerberos. <br>
<br>
</blockquote>
<br>
<br>
</body>
</html>