code review request: 6894072: always refresh keytab
weijun.wang at oracle.com
Sun Mar 20 22:28:55 PDT 2011
On 03/19/2011 07:54 AM, Valerie (Yu-Ching) Peng wrote:
>>> 1) you changed it to not extending KerberosKey, potential compatibility
>> 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
> 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.
I've searched thru the codes and didn't see this happen. The problem now
is, for a typical service, now we use KeyTab instead of KerberosKey as
the private credentials.
I don't suggest letting Krb5AcceptCredential extend anything now. For
safety, we can support Krb5AcceptCredential as a private credential,
this means the SubjectComber will search for it, and if it's there, we
can directly use the ServiceCreds inside.
>>> 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.
I think I've made some design error here.
In fact, the old code has never looked at the KerberosPrincipal inside a
Subject, when the specified server principal is null, it simply uses the
one inside KerberosKey (because a key is always for a pricipal, and this
principal is saved into the key in Krb5LoginModule). Since I thought a
KeyTab can be used by multiple principals, I haven't embedded a
This can lead to a problem: If there are 2 Krb5LoginModule in a JAAS
login process (I don't know if we can support it. Of course, in this
case, the specified principal cannot be null), and two different
principals using different keytab files. With the old code, keys are
extracted from keytabs and the principal name is attached to it, so the
SubjectComber.find(non-null principal) method can locate the correct
keys. But with the new code, two keytab files is put into private
credentials but we don't know which one is for who.
>>> 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
>> 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
> At least add some comment in the code to remind ourself about this.
>>> 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"?
Well, I want to express the meaning that the keytab is now missing but
the caller can still access its old content.
Yes, this is a little confusing. When I designed the new javax...KeyTab
class, I want to make sure the keytab is not affected by half-edited
keytab, bad communication etc. This is why I use these words everywhere:
* If there is any error (say, file missing, I/O error or
* file format error) during the reading process of the
* KeyTab file, a saved result should be returned.
So, "file missing" means "saved result". This is why I set the isMissing
field on but keep the old keytab.
(continue to my reply below).
> 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?
I was afraid that a file might suddenly disaappears during an update
(say, someone renames it and then copies a new one). But as you said,
this is a contradict to common sense. If the update is a direct copy
(without renaming the old one first), the file will not disappear.
I would treat "file missing" a valid case and remove it from the "error"
This would need some changes to the CCC:
1. Remove "file missing" from error lists.
2. Add principal into KeyTab. Theoretically, a user can put multiple
Krb5LoginModule all marked required for a JAAS login config named entry.
They can use different keytabs and the keytabs can be both empty at the
login time. We have no other way to guess which can be used by who and
have to add the principal label.
I'll send another mail when my edit is ready.
More information about the security-dev