code review request: 6894072: always refresh keytab

Weijun Wang weijun.wang at oracle.com
Thu Apr 14 04:36:02 UTC 2011


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
>>>>>>>>>>>
>>
>>
>



More information about the security-dev mailing list