code review request: 6894072: always refresh keytab
Valerie (Yu-Ching) Peng
valerie.peng at oracle.com
Mon Apr 18 18:58:51 UTC 2011
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
>>>>>>>>>>>>
>>>
>>>
>>
More information about the security-dev
mailing list