code review request: 6894072: always refresh keytab

Valerie (Yu-Ching) Peng valerie.peng at oracle.com
Fri Mar 18 23:54:33 UTC 2011


Max,
>> 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.
I am somewhat concerned about this change.
If any Krb5AcceptCredential objects are added into the Subject's private 
credential set, with this change, they will no longer be found by apps 
which search for KerberosKey. We'd better be careful here.

>>
>> 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.
My comment applies to your webrev.00. I saw you changed it in your 
latest webrev, i.e. 01, so that the NPE won't happen.

>
> 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?
Never mind, somehow I missed that JSSE file and thought that you didn't 
update it.

>> 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.
Then, how about we use the specified server principal if it's not null 
and if it is null, then we grab the KerberosPrincipal from the Subject?
I think we should use the explicitly specified value whenever possible 
since that's what we'd expect.

>>
>> 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.
Yes, it's not obvious when to clean up the map.
Well, static map w/ only add and no removal may lead to memory problems 
later...
At least add some comment in the code to remind ourself about this.

>>
>> 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.
The changes look good to me.
Question regarding line 124: why update the "isMissing" field of the old 
keytab? Also, what about the other field such as "lastModified"?
Typo on line 108: "instead if" should be "instead of"
Lastly, regarding "if a user want to revoke all keys, he should empty 
the keytab instead if deleting the file.", do you think that's what most 
users do?
Can't we detect this by checking the existence of the KeyTab file when 
there is a copy in the cache?
If a KeyTab file was found earlier, but later disappeared/removed, can't 
we take it as a sign to dispose it by calling the dispose() method and 
remove it from the cache?

Thanks,
Valerie



More information about the security-dev mailing list