Max,
Here are some comments for 6894072: always refresh keytab
General:
1) Copyright year should be 2011
2) You added new classes under javax.XX packages: Have you filed CCC for
them?
Krb5LoginModule.java
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.
KerberosKey.java
1) "at needed" probably should be "when needed" or "as needed"
46 * the {@link KeyTab} class, where latest keys can be read at needed.
Krb5AcceptCredential.java
1) you changed it to not extending KerberosKey, potential compatibility
concern?
Krb5Util.java
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.
2) when shall ServiceCreds.destroy() be called? For every re-read of
KeyTab object, e.g. line 257
3) You replaced the Krb5Util.getKeys(GSSCaller, ...) method w/
ServicesCreds.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.
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.
KrbAsReq.java
1) I can't see any change? Is it included in this webrev by mistake?
KrbAsReqBuilder.java
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?
2) Exception message at line 203 may be better phrased as "Required
password not provided".
sun.security.krb5.internal.ktab.KeyTab.java
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.
SharedSecrets.java, SubjectComber.java, Kinit.java, Klist.java,
Ktab.java, EncryptionKey.java, Config.java, Krb5ProxyImpl.java,
ServerHandshaker.java, KrbAsRep.java,
JavaxSecurityAuthKerberosAccessImpl.java,
JavaxSecurityAuthKerberosAccess.java
=> looks ok
javax.security.auth.KeyTab.java
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.
2) why not leverage getEncryptionKeys(PrincipalName) for the code of
line 148-155? They seem similar enough.
Thanks,
Valerie
*
*On 12/01/10 01:46 AM, Weijun Wang wrote:
Hi Valerie
The webrev is at --
http://cr.openjdk.java.net/~weijun/6894072/webrev.00/
Changes:
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.
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.
3. KrbAsReqBuilder was constructed with password or keys, now with
password or keytab. Kinit and Krb5LoginModule updated accordingly.
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.
Thanks
Max
-------- The Bug --------
*Change Request ID*: 6894072
*Synopsis*: always refresh keytab
Product: java
Category: jgss
Subcategory: krb5plugin
Type: RFE
=== *Description* ======================================
info from keytab should be refreshed at every security context
establishment in Kerberos.
