code review request: 6894072: always refresh keytab

[Weijun Wang]

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.
>>
>
>