Hi Valerie
Webrev updated:
http://cr.openjdk.java.net/~weijun/6894072/webrev.01
On 03/12/2011 06:06 AM, Valerie (Yu-Ching) Peng wrote:
Max,
Here are some comments for 6894072: always refresh keytab
General:
1) Copyright year should be 2011
Updated.
2) You added new classes under javax.XX packages: Have you filed CCC for
them?
Yes, http://ccc.sfbay/6894072
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.
You're correct. "ktab = null;" added into the if check block. Also, the
LoginModuleOptions.java regression test is reverted.
I don't know why I made this mistake and even updated the test to "hide"
it. Maybe there was a time I really wanted the opposite behavior.
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.
OK.
Krb5AcceptCredential.java
1) you changed it to not extending KerberosKey, potential compatibility
concern?
Not compatibility concern. I only think that now Krb5AcceptCredential
can be something else other than simply KerberosKey.
If fact, I have no idea why the original Krb5AcceptCredential needs to
extends KerberosKey. I didn't see any place where it's used as a
KerberosKey.
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.
When ktab file is missing, ktab is not null, and line 237 will not be
executed.
2) when shall ServiceCreds.destroy() be called? For every re-read of
KeyTab object, e.g. line 257
Yes, it's better to be called here.
It's also called in Krb5AcceptCredential.destroy(). Hopefully this
method will be called somewhere.
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.
JSSE codes are also updated in the same changeset. I think such problems
only happen when a pre-compiled jar is involved. Or, did I miss anything?
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.
Well, if every credentials inside the subject is a result of
Krb5LoginModule commit() call, then there should be one
KerberosPrincipal there. I am not using the specified server principal
because it has a chance to be null.
KrbAsReq.java
1) I can't see any change? Is it included in this webrev by mistake?
Removed. There was a small whitespace change and "hg diff" automatically
added it.
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?
Fixed. Password is now cleared. There is no need to destroy the keytab.
Maybe the sun.security...KeyTab class needs a static destroy method to
clean up the map, but I am not sure when to call that.
2) Exception message at line 203 may be better phrased as "Required
password not provided".
Updated.
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.
Updated. I gave up the multi-thread tuning. There is a chance an old
keytab is returned even if it was updated long time ago. Now I simply
made the getInstance0 method synchronized. As long as the keytab file's
timestamp does not change, this method should be very fast.
During the CCC approval, Dmitry Miltsov did like the "try its best"
words in "Implementation of getKeys() method should try its best to get
the latest info". Therefore I removed it and change the implementation
as well.
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.
Fixed. getInstance(file) now throws NPE when file == null.
2) why not leverage getEncryptionKeys(PrincipalName) for the code of
line 148-155? They seem similar enough.
Updated.
I've also added @run main/othervm to all jgss/krb5 tests which call
System.setProperty().
Thanks
Max
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.
