code review request: 6894072: always refresh keytab

[Weijun Wang]

Thanks for the careful review. It has been a long one.

Max

On 04/19/2011 02:58 AM, Valerie (Yu-Ching) Peng wrote:
>
> Ok, I have no more comments.
> Thanks,
> Valerie
>
> On 04/13/11 09:36 PM, Weijun Wang wrote:
>> webrev updated at
>>
>> http://cr.openjdk.java.net/~weijun/6894072/webrev.06/
>>
>> changes:
>>
>> 1. Remove Krb5AcceptCredentials in manipulation cred priv set
>> 2. Add codes to ServiceCreds.destroy() (See below)
>> 3. javadoc of sun..KeyTab.getInstance(s) (mentioned in your other mail)
>> 4. Copyright year of Handshaker and ServerHandsshaker (mentioned in
>> your other mail)
>>
>> Comments inline below:
>>
>> On 04/14/2011 09:45 AM, Valerie (Yu-Ching) Peng wrote:
>>>
>>>
>>> On 04/09/11 03:00 AM, Weijun Wang wrote:
>>>>> src/share/classes/sun/security/jgss/krb5/Krb5Util.java
>>>>> => 1) So, since when do we populate the Subject w/
>>>>> Krb5AcceptCredential objects? I thought only Krb5LoginModule would
>>>>> "write" the subject's private cred set and I didn't find
>>>>> Krb5AcceptCredential objects added there.
>>>>> 224 Krb5AcceptCredential k5ac = SubjectComber.find(
>>>>> 225 subj, serverPrincipal, null, Krb5AcceptCredential.class);
>>>>>
>> <snip>
>>>> So, I'd prefer we just remove this special handling altogether.
>>
>> OK. Removed.
>>
>>>
>>>>
>>>>> => 2) Inside the getKKeys() method, you refresh the set of
>>>>> KerberoKey objects in the subject when the subject isn't read only.
>>>>> However, in Krb5LoginModule, KerberosKey objects are only stored
>>>>> into the Subject's private cred set when "storeKey" is true. Seems
>>>>> inconsistent?
>>>>> 271 public KerberosKey[] getKKeys() {
>>>>>
>>>>
>>>> I think not. In a normal krb5 login with JAAS, KeyTab and
>>>> KerberosKey are only stored when both "storeKey" and
>>>> subject.isReadOnly() are true. Since we now only deal with
>>>> KeysFromKeyTab type of keys, we can be sure they are read from
>>>> KeyTab. On the other hand, the reason we still store keys here is
>>>> because we are afraid that the user is directly manipulating the
>>>> subject, which means he/she is not using JAAS and therefore no
>>>> Krb5LoginModule involved at all.
>>>>
>>>>
>>> Let me rephrase my question: If he/she is only using JAAS, e.g.
>>> Krb5LoginModule and has storeKey set to false, will
>>> ServiceCreds.getKKeys() put KerberosKey objects into Subject's priv cred
>>> set? If yes, is this the expected behavior?
>>
>> If storeKey=false, there will be no KeyTab object in the priv cred
>> set, and there is no chance for these KerberosKey objects to be added.
>>
>>>
>>>>> => 3) I don't quite understand why there is a destroy() method that
>>>>> does nothing... At a minimum, I'd expect we need to reset the
>>>>> fields, so that they aren't usable or empty, right? Perhaps, we
>>>>> also need to destroy the individual KeyTab and KerberosKey objects
>>>>> in the lists?
>>>>> 316 public void destroy() {
>>>>> 317 // Nothing to do now
>>>>> 318 }
>>>>>
>>>>
>>>> I'll think about this.
>>>>
>>> Ok, let me know if you have more thoughts.
>>
>> I added
>>
>> kp = null;
>> ktabs = null;
>> kk = null;
>>
>> to the method. Since the keys and keytabs are directly taken out of
>> subject's priv cred set, I cannot destroy them. Otherwise, the same
>> subject cannot doAs another JGSS session.
>>
>> The destroy() method should be called by
>> Krb5AcceptCredential.dispose() and then called by
>> Krb5Context.dispose(). Unfortunately, Krb5Context has not made the
>> call. This is something we should fix later, but I don't want to touch
>> it now.
>>
>> Thanks
>> Max
>>
>>
>>> Thanks,
>>> Valerie
>>>>
>>>>> src/share/classes/sun/security/jgss/krb5/SubjectComber.java
>>>>>
>>>>> => I wonder why do you add this? The previous impl only search for
>>>>> publicly defined Kerberos objects, i.e. KerberosKey,
>>>>> KerberosTicket, etc. I thought you said in previous email that we
>>>>> don't populate Subject's private credential set w/
>>>>> Krb5AcceptCredential objects?
>>>>>
>>>>> 100 credClass == Krb5AcceptCredential.class) {
>>>>> src/share/classes/sun/security/krb5/Config.java
>>>>> src/share/classes/sun/security/krb5/EncryptionKey.java
>>>>> src/share/classes/sun/security/krb5/KrbAsRep.java
>>>>> => All look fine
>>>>>
>>>>> src/share/classes/sun/security/krb5/KrbAsReqBuilder.java
>>>>> => 1) This class does store its own copy of password, so shouldn't
>>>>> you move the "does not" on line 49 to line 50?
>>>>> 49 * This class does not:
>>>>>
>>>>> 50 * 1. Deal with real communications (KdcComm does it, and TGS-REQ)
>>>>> 51 * a. Name of KDCs for a realm
>>>>> 52 * b. Server availability, timeout, UDP or TCP
>>>>> 53 * d. KRB_ERR_RESPONSE_TOO_BIG
>>>>>
>>>>> 54 * 2. Stores its own copy of password, this means:
>>>>> 55 * a. Do not change/wipe it before Builder finish
>>>>> 56 * b. Builder will not wipe it for you
>>>>>
>>>>
>>>> I'll remove lines 54-56. Maybe I meant to do that some time ago.
>>>>
>>>> Thanks
>>>> Max
>>>>
>>>>
>>>>> I am still looking at the rest of changes, just want to send what I
>>>>> have now, so you don't wait too long.
>>>>>
>>>>> Thanks,
>>>>> Valerie
>>>>>
>>>>> On 04/02/11 02:18 AM, Weijun Wang wrote:
>>>>>
>>>>>> Updated again:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~weijun/6894072/webrev.05/
>>>>>>
>>>>>> Changes:
>>>>>>
>>>>>> 1. New Krb5Util.KeysFromKeyTab as a special kind of KerebrosKey we
>>>>>> will add to and remove from private credentials set. Add and
>>>>>> remove are only done when !subject.isReadOnly(). Only remove keys
>>>>>> for this principal.
>>>>>>
>>>>>> 2. Use the class above in KeyTab.getKeys().
>>>>>>
>>>>>> 3. Remove a uselss method in KDC.java test.
>>>>>>
>>>>>> 4. Update new test KeyTabCompat.java, make sure after keytab
>>>>>> refresh, the old key in priv cred set is removed.
>>>>>>
>>>>>> Thanks
>>>>>> Max
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 04/02/2011 03:02 AM, Valerie (Yu-Ching) Peng wrote:
>>>>>>
>>>>>>> I think we need to clean up the old ones if we added it there.
>>>>>>> Conceptually, this would fit closer w/ the "dynamic key tab"
>>>>>>> support.
>>>>>>> One straightforward way for us to do this is to subclass KerberosKey
>>>>>>> class and then we can remove all KerberosKey objects which are
>>>>>>> implemented using this class at refresh time.
>>>>>>> Just an idea.
>>>>>>> Valerie
>>>>>>>
>>>>>>> On 04/01/11 02:14 AM, Weijun Wang wrote:
>>>>>>>
>>>>>>>> Hi Valerie
>>>>>>>>
>>>>>>>> Updated again:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~weijun/6894072/webrev.04/
>>>>>>>>
>>>>>>>> 1. KeyTab can be used by anyone
>>>>>>>> 2. The two compatibility support
>>>>>>>>
>>>>>>>> As for adding keys (from keytab) into private credentials set, I
>>>>>>>> haven't cleaned up old ones. Since it's a set, this means if old
>>>>>>>> keys
>>>>>>>> are removed from the keytab, they stay in the set. The set is never
>>>>>>>> really used by our code, so I think it's harmless. I really
>>>>>>>> don't know
>>>>>>>> how to clean up. Remove all keys for this principal? But we do this
>>>>>>>> because we want to keep compatibility and worry about people
>>>>>>>> directly
>>>>>>>> manipulating the set, and I cannot predict what they will do
>>>>>>>> with the
>>>>>>>> set.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Max
>>>>>>>>
>>>>>>>>
>>>>>>>> On 04/01/2011 10:23 AM, Weijun Wang wrote:
>>>>>>>>
>>>>>>>>> On 04/01/2011 10:09 AM, Valerie (Yu-Ching) Peng wrote:
>>>>>>>>>
>>>>>>>>>> Max,
>>>>>>>>>>
>>>>>>>>>> I like this new approach of yours better.
>>>>>>>>>> As for compatibility, you mentioned only one aspect, i.e. apps
>>>>>>>>>> which
>>>>>>>>>> put
>>>>>>>>>> KerberosKeys inside a subject's private cred set.
>>>>>>>>>> There is also a possibility that apps may read the subject's
>>>>>>>>>> private
>>>>>>>>>> credentials set for KerberosKeys that we used to put in and
>>>>>>>>>> they won't
>>>>>>>>>> find the keys anymore since it's the KeyTab objects that we
>>>>>>>>>> put into
>>>>>>>>>> the
>>>>>>>>>> private cred set after this dynamic keytab support. Thoughts?
>>>>>>>>>>
>>>>>>>>> No, I haven't thought about it.
>>>>>>>>>
>>>>>>>>> We can put a snapshot of keys there at the beginning and
>>>>>>>>> refresh them
>>>>>>>>> whenever a getKeys() is called. This should be harmless because
>>>>>>>>> we don't
>>>>>>>>> really use the keys if keytab objects (not keytab files) exist.
>>>>>>>>> I can do
>>>>>>>>> that.
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> Max
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Valerie
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 03/31/11 03:41 AM, Weijun Wang wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi Valerie
>>>>>>>>>>>
>>>>>>>>>>> Sorry for the late reply. I've considered some alternatives.
>>>>>>>>>>>
>>>>>>>>>>> A "to-be-resolved" KerberosKey is quite difficult. When it's
>>>>>>>>>>> resolved,
>>>>>>>>>>> we have a list of keys with different etypes as the private
>>>>>>>>>>> credentials. If it's not resolved, we can only create one, whose
>>>>>>>>>>> encoding and etype are both unresolved, and when it finally gets
>>>>>>>>>>> resolved, it's resolved into multiple keys. Also, there was a
>>>>>>>>>>> simple
>>>>>>>>>>> mapping between KerberosKey and EncryptionKey. The resolving
>>>>>>>>>>> process
>>>>>>>>>>> is not always at the same time as the mapping (converting
>>>>>>>>>>> from one to
>>>>>>>>>>> another) time, so it seems EncryptionKey might also needs to be
>>>>>>>>>>> unresolved.
>>>>>>>>>>>
>>>>>>>>>>> Another solution is to revert back to my original KeyTab
>>>>>>>>>>> without an
>>>>>>>>>>> intended user. This means several changes:
>>>>>>>>>>>
>>>>>>>>>>> In my latest version of ServiceCreds, there were multiple
>>>>>>>>>>> keys and
>>>>>>>>>>> *one* keytab, now we also need multiple keytabs, because
>>>>>>>>>>> there might
>>>>>>>>>>> be multiple keytabs in the subject's private credentials set
>>>>>>>>>>> and we
>>>>>>>>>>> cannot tell which is for who. Therefore we collect all of
>>>>>>>>>>> them, when
>>>>>>>>>>> the keys are needed at AP-REP time, we call getKeys() on all
>>>>>>>>>>> of them,
>>>>>>>>>>> and return the combination. Hopefully there won't be two keys
>>>>>>>>>>> for the
>>>>>>>>>>> same principal with same kvno and same etype. I regard that
>>>>>>>>>>> as an
>>>>>>>>>>> abuse.
>>>>>>>>>>>
>>>>>>>>>>> Currently when there is no serverPrincipal specified in the
>>>>>>>>>>> Krb5AcceptCredential construction, we pick a KerberosKey from
>>>>>>>>>>> the
>>>>>>>>>>> private credentials set and use the KerberosPrincipal info
>>>>>>>>>>> inside, and
>>>>>>>>>>> then get all KerberosKeys for the same principal. We have
>>>>>>>>>>> never really
>>>>>>>>>>> looked at any KerberosPrincipal objects in the subject's
>>>>>>>>>>> principal
>>>>>>>>>>> set. I had tried to do the same to KeyTabs in my webrev.02.
>>>>>>>>>>> Now this
>>>>>>>>>>> will have a big change: the first step is always finding a
>>>>>>>>>>> KerberosPrincipal in the principal sets first. If
>>>>>>>>>>> serverPrincipal is
>>>>>>>>>>> specified, we find a matched one, otherwise, we just pick one
>>>>>>>>>>> and use
>>>>>>>>>>> it. And then, from the private credentials set, we fetch all
>>>>>>>>>>> KerberosKeys for this principal and all KeyTabs.
>>>>>>>>>>>
>>>>>>>>>>> I think this is the correct way to go. Of course, for
>>>>>>>>>>> compatibility
>>>>>>>>>>> reason, we assume there are third party codes that put
>>>>>>>>>>> KerberosKeys
>>>>>>>>>>> inside a subject's private credentials set but hasn't put any
>>>>>>>>>>> KerberosPrincipal there. Our Krb5LoginModule will never do
>>>>>>>>>>> it, but we
>>>>>>>>>>> can accept it.
>>>>>>>>>>>
>>>>>>>>>>> Here is a partial webrev:
>>>>>>>>>>>
>>>>>>>>>>> http://cr.openjdk.java.net/~weijun/6894072/webrev.03
>>>>>>>>>>>
>>>>>>>>>>> It only contains changes for Krb5Util.java, and hasn't
>>>>>>>>>>> included the
>>>>>>>>>>> compatibility codes I mentioned above. As you can see, the
>>>>>>>>>>> KeyTab
>>>>>>>>>>> objects are now retrieved by
>>>>>>>>>>>
>>>>>>>>>>> + sc.ktabs = SubjectComber.findMany(
>>>>>>>>>>> + subj, null, null, KeyTab.class);
>>>>>>>>>>>
>>>>>>>>>>> so no principal name is used, and we retrive "many".
>>>>>>>>>>>
>>>>>>>>>>> If you think this is OK, I'll clean up other codes. One
>>>>>>>>>>> benefit is
>>>>>>>>>>> that we don't need to update CCC with this solution. Yes we do
>>>>>>>>>>> introduce new hashCode/equals/toString methods, but I think
>>>>>>>>>>> they do
>>>>>>>>>>> not deserve a CCC.
>>>>>>>>>>>
>>>>>>>>>>> Thanks
>>>>>>>>>>> Max
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 03/26/2011 08:20 AM, Valerie (Yu-Ching) Peng wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Max,
>>>>>>>>>>>>
>>>>>>>>>>>> Well, I find it a bit awkward that the KeyTab class has to
>>>>>>>>>>>> have the
>>>>>>>>>>>> KerberosPrincipal info which "intends" to use it.
>>>>>>>>>>>> Have you considered a different approach like:
>>>>>>>>>>>> Instead of adding the whole KeyTab object into the Subject's
>>>>>>>>>>>> private
>>>>>>>>>>>> credential set, we add a "to-be-resolved" KerberosKey
>>>>>>>>>>>> object. When we
>>>>>>>>>>>> need to use this kind of key, we'd check the associated KeyTab
>>>>>>>>>>>> object to
>>>>>>>>>>>> re-fresh its value if needed. This approach is conceptually
>>>>>>>>>>>> closer to
>>>>>>>>>>>> what we had and the changes aren't as dramatic and seems to
>>>>>>>>>>>> meet the
>>>>>>>>>>>> need required by 6894072.
>>>>>>>>>>>>
>>>>>>>>>>>> I'll continue to review your webrev, but just want to kick
>>>>>>>>>>>> this idea
>>>>>>>>>>>> off
>>>>>>>>>>>> w/ you and see if it may work.
>>>>>>>>>>>> Valerie
>>>>>>>>>>>>
>>>>>>>>>>>> On 03/23/11 02:00 AM, Weijun Wang wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Valerie
>>>>>>>>>>>>>
>>>>>>>>>>>>> Updated webrev:
>>>>>>>>>>>>>
>>>>>>>>>>>>> http://cr.openjdk.java.net/~weijun/6894072/webrev.02
>>>>>>>>>>>>>
>>>>>>>>>>>>> Changes since last version:
>>>>>>>>>>>>>
>>>>>>>>>>>>> 1. A KerberosPrincipal inside javax..KeyTab class. New
>>>>>>>>>>>>> getInstance()
>>>>>>>>>>>>> arguments, new getPrincipal() method.
>>>>>>>>>>>>>
>>>>>>>>>>>>> It can only be non-null now, but I didn't say anything in
>>>>>>>>>>>>> the spec.
>>>>>>>>>>>>> I'm hoping it can be null in the future to support multiple
>>>>>>>>>>>>> service
>>>>>>>>>>>>> principal in a single service.
>>>>>>>>>>>>>
>>>>>>>>>>>>> 2. toString(), hashCode(), equals() for KeyTab, since it
>>>>>>>>>>>>> will be put
>>>>>>>>>>>>> inside private credentials set.
>>>>>>>>>>>>>
>>>>>>>>>>>>> 3. Enhancement to SubjectComber:
>>>>>>>>>>>>> a) Generics for find() and findMany()
>>>>>>>>>>>>> b) findAux() now support Krb5AcceptCredential
>>>>>>>>>>>>>
>>>>>>>>>>>>> 4. Krb5Util.ServiceCreds: since principal is already inside
>>>>>>>>>>>>> both
>>>>>>>>>>>>> KeyTab and KerberosKey, no more KerberosPrincipal argument in
>>>>>>>>>>>>> getInstance(), there is still a field inside to save the
>>>>>>>>>>>>> value.
>>>>>>>>>>>>>
>>>>>>>>>>>>> 5. sun..KeyTab and javax..KeyTab: isMissing==true is now
>>>>>>>>>>>>> valid.
>>>>>>>>>>>>> Changes to the javadoc of javax..KeyTab.getKeys().
>>>>>>>>>>>>>
>>>>>>>>>>>>> 6. New TwoPrinces.java test, a subject with 2
>>>>>>>>>>>>> KerberosPrincipal
>>>>>>>>>>>>> after
>>>>>>>>>>>>> JAAS commit.
>>>>>>>>>>>>>
>>>>>>>>>>>>> This time I'd like to first make sure implementation is
>>>>>>>>>>>>> correct, and
>>>>>>>>>>>>> then I'll update the CCC. Is this OK?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>> Max
>>>>>>>>>>>>>
>>>>
>>>>
>>>
>