On 03/19/2011 07:54 AM, Valerie (Yu-Ching) Peng wrote:
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.
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.
Krb5Util.java
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
principal inside.
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.
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.
OK.
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"?
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"
list.
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.
Thanks
Max
Thanks,
Valerie
