code review request: 6894072: always refresh keytab
Valerie (Yu-Ching) Peng
valerie.peng at oracle.com
Wed Apr 13 23:54:32 UTC 2011
The remaining half of the review:
*src/share/classes/sun/security/krb5/internal/ktab/KeyTab.java*
=> 1) inconsistency between javadoc and the source code, i.e. the
parameter name is not "file" but "s". Also, what do you mean by source?
It's very vague.
137 * @param file the key tab source.
||*src/share/classes/sun/security/ssl/Handshaker.java
*=>1) the only change is the copyright year? I think you meant to update
the copyright year of *ServerHandshaker.java* which has some source
changes.*
**src/share/classes/sun/security/ssl/ServerHandshaker.java
**src/share/classes/sun/security/ssl/krb5/Krb5ProxyImpl.java
**src/windows/classes/sun/security/krb5/internal/tools/Kinit.java*
*src/windows/classes/sun/security/krb5/internal/tools/Klist.java*
*src/windows/classes/sun/security/krb5/internal/tools/Ktab.java
**src/share/classes/javax/security/auth/kerberos/JavaxSecurityAuthKerberosAccessImpl.java
**src/share/classes/javax/security/auth/kerberos/KeyTab.java
**src/share/classes/sun/misc/JavaxSecurityAuthKerberosAccess.java
*=> looks fine***
**
*Valerie*
**
*On 04/08/11 05:33 PM, Valerie (Yu-Ching) Peng wrote:
>
> *src/share/classes/com/sun/security/auth/module/Krb5LoginModule.java*
> *src/share/classes/javax/security/auth/kerberos/KerberosKey.java*
> *src/share/classes/sun/misc/SharedSecrets.java*
> *src/share/classes/sun/security/jgss/krb5/Krb5AcceptCredential.java
> *=> All look fine.*
>
> **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);
>
> => 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() {
> => 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 }
>
> ||*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 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
>>>>>>>>
>>>>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20110413/a2e714de/attachment.htm>
More information about the security-dev
mailing list