code review request: 6894072: always refresh keytab

[Valerie (Yu-Ching) Peng]

Max,

Here are some comments for 6894072: always refresh keytab
General:
1) Copyright year should be 2011
2) You added new classes under javax.XX packages: Have you filed CCC for 
them?

Krb5LoginModule.java
1) if useKeyTab is set to true in the config, then the if-cond on line 
712 will always be false given the ktab assignment on line 692, and then 
it won't get to the password prompting call on line 713? This seems 
different than what the comment (line 671-689) described.

KerberosKey.java
1) "at needed" probably should be "when needed" or "as needed"

  46  * the {@link KeyTab} class, where latest keys can be read at needed.

Krb5AcceptCredential.java
1) you changed it to not extending KerberosKey, potential compatibility 
concern?

Krb5Util.java
1) When calling ServiceCreds.getInstance(), your change only checks that 
kt, kk can't both be null, i.e. line 221. However, at line 237, it uses 
kk when ktab file is missing. It looks to me that this may leads NPE if 
the ServiceCreds is constructed w/ a non-null (but non-existent) KeyTab 
object and null KerberosKey list.
2) when shall ServiceCreds.destroy() be called? For every re-read of 
KeyTab object, e.g. line 257
3) You replaced the Krb5Util.getKeys(GSSCaller, ...) method w/ 
ServicesCreds.getServiceCreds(GSSCaller,...). If you don't update JSSE 
source immediately, people will run into a compilation failure. Perhaps 
it's better to keep the Krb5Util.getKeys(GSSCaller,...) method and then 
remove this method together w/ the updated JSSE sources.
4) at line 291 and 299, will the KerberosPrincipal found off the Subject 
be different from the specified server principal? Seems somewhat strange 
that we just grab the first one.

KrbAsReq.java
1) I can't see any change? Is it included in this webrev by mistake?

KrbAsReqBuilder.java
1) Its destroy() method no longer destroys key nor clears password? Is 
this intentional? If yes, then the method description should be updated. 
Also, how/when will keys or keytab objects be destroyed and password be 
cleared?
2) Exception message at line 203 may be better phrased as "Required 
password not provided".

sun.security.krb5.internal.ktab.KeyTab.java
1) your "isReading" flag is static to the KeyTab class but the way you 
use it seems to suggest that it should be associated w/ each entry of 
the "map" Hashmap.

SharedSecrets.java, SubjectComber.java, Kinit.java, Klist.java, 
Ktab.java, EncryptionKey.java, Config.java, Krb5ProxyImpl.java, 
ServerHandshaker.java, KrbAsRep.java, 
JavaxSecurityAuthKerberosAccessImpl.java, 
JavaxSecurityAuthKerberosAccess.java
=> looks ok

javax.security.auth.KeyTab.java
1) for its getInstance(File) method, the javadoc specifies that a NPE is 
thrown if file argument is null. However, the private constructor 
KeyTab(File) does not throw NPE when null is specified.
2) why not leverage getEncryptionKeys(PrincipalName) for the code of 
line 148-155? They seem similar enough.

Thanks,
Valerie
*
*On 12/01/10 01:46 AM, Weijun Wang wrote:
> Hi Valerie
>
> The webrev is at --
>
>   http://cr.openjdk.java.net/~weijun/6894072/webrev.00/
>
> Changes:
>
> 1. New javax..KeyTab, updated sun..KeyTab. As the impl note in 
> javax..KeyTab says: the former is a name with dynamic content, the 
> latter is a snapshot of a file.
>
> 2. Now Subject can have private credentials with type KeyTab. Thus the 
> content of Krb5AcceptCredential is not only keys. Krb5Util defines an 
> expandable ServiceCreds class for this purpose.
>
> 3. KrbAsReqBuilder was constructed with password or keys, now with 
> password or keytab. Kinit and Krb5LoginModule updated accordingly.
>
> 4. Having parallel defined KerberosKey/KerberosPrincipal and 
> EncrytionKey/PrincipalName is complicated. Special Unsafe methods are 
> defined to get EncryptionKey thru a PrincipalName from new 
> javax..KeyTab. Might look into consolidate data types some day.
>
> Thanks
> Max
>
>
> -------- The Bug --------
> *Change Request ID*: 6894072
> *Synopsis*: always refresh keytab
>
>   Product: java
>   Category: jgss
>   Subcategory: krb5plugin
>   Type: RFE
>
> === *Description* ======================================
> info from keytab should be refreshed at every security context 
> establishment in Kerberos.
>


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20110311/7b4a08ab/attachment.htm>