Re: code review request: 6894072: always refresh keytab

Wed, 13 Apr 2011 17:03:37 -0700 

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







































					Reply via email to