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
