Code review request: 8005523: Unbound krb5 for TLS

[Xuelei Fan]

On 4/15/2013 2:18 PM, Weijun Wang wrote:
> 
> 
> On 4/15/13 1:38 PM, Xuelei Fan wrote:
>>>> Do you want to file a simple enhancement request (CCC)?
>>>
>>> Why CCC? This is all internal.
>>>
>> Yes, it is optional. I think, now it can accept unbound principal in
>> server side, it is an enhancement. It would be nice to have the
>> community and SQE know the improvement.
> 
> I consider it to be a natural benefit of
> http://ccc.us.oracle.com/8001104. The release note will mention SASL,
> GSS-API, JAAS, TLS all.
> 
OK.

>>
>>>>
>>>> . KerberosClientKeyExchangeImpl.java
>>>> ------------------------------------
>>>> Do you want to check the return value to make sure it is non-null or
>>>> empty? Otherwise, it is possible to run into NPE when using the
>>>> serverKeys.
>>>>
>>>> 188  KerberosKey[] serverKeys = AccessController.doPrivileged(
>>>>
>>>> An IOException will be thrown if the principal is not matched. I think
>>>> we need to reserve the behavior.
> 
> I see.
> 
> Webrev updated
> 
>     http://cr.openjdk.java.net/~weijun/8005523/webrev.01/
> 
> 3 changes:
> 
> 1. Pass AccessControlContext instead of Handshaker (so Handshaker can be
> protected again).
> 
> 2. New IOE when there is no keys for princ in ServiceCreds
> 
> 3. Enhance findkey in KerberosClientKeyExchangeImpl to include the logic
> of 7197159: accept different kvno if there no match.
> 
> 4. Smaller change in SSL.java the test. Add a case instead of change to
> all.
> 

setupKerberosKeys() @ServerHandshaker.java:
-------------------------------------------
I would like to reserve the permission checking for bound krb5 here.
The checking is done while parse ClientHello, and is useful to select
the right one from a list of cipher suites.  It is too later to check it
during KerberosClientKeyExchange.

I think it is OK to get the krb5 principal for bound krb5, right?

BTW, how to set "accept" service policy for unbound krb5 in server side?
We used to have a particular server principal. Is the "*" acceptable in
policy configuration?


findKey() @KerberosClientKeyExchangeImpl.java
--------------------------------------------------
Thanks for the version matching update.

I think it might be OK to remove this line:
 449             //throw new KrbException(Krb5.KRB_AP_ERR_BADKEYVER);

Otherwise, looks fine to me.

Xuelei

> Thanks
> Max
> 
>>>
>>> If the returned serverKeys is empty (it won't be null), line 208 will
>>> return a null and line 213 will throw the IOE. Is that enough?
>> The exception message will be confusing if the check is done in line 208
>> and 213.  I like to show principal mismatch message when using bound
>> principals.
>>
>> Xuelei
>>
>>>>
>>>>
>>>> Is it possible to add a new test for the unbound krb5 in TLS?
>>>
>>> It's already there. Note the "principal=*" in the updated SSL.java test.
>>> Maybe I can provide 2 test cases, one bound, one unbound.
>>>
>>> Thanks
>>> Max
>>>
>>>>
>>>> Thanks,
>>>> Xuelei
>>>>
>>>> On 4/1/2013 9:16 PM, Weijun Wang wrote:
>>>>> Ping again.
>>>>>
>>>>> On 3/14/13 4:42 PM, Weijun Wang wrote:
>>>>>> Hi Xuelei
>>>>>>
>>>>>> You might know that krb5 now supports unbound acceptor, which
>>>>>> means if
>>>>>> you set "principal=*" in an acceptor's JAAS login config file, it can
>>>>>> serve as any service. The acceptor would read initiator's request,
>>>>>> find
>>>>>> out what the intended service name is, and then find a key for it
>>>>>> from
>>>>>> its keytab file.
>>>>>>
>>>>>> Currently TLS's krb5 ciphersuites must know the service principal at
>>>>>> the
>>>>>> beginning, it uses the info to read keys and then wait for incoming
>>>>>> requests. This must be changed if it also want to be "unbound".
>>>>>>
>>>>>> I have a primitive patch here
>>>>>>
>>>>>>       http://cr.openjdk.java.net/~weijun/8005523/webrev.00
>>>>>>
>>>>>> You can see it gets a ServiceCreds instead of KerberosKey at the
>>>>>> beginning. This ServiceCreds encapsulates keytabs and JAAS settings,
>>>>>> and
>>>>>> it can be used to find keys for any service name later.
>>>>>>
>>>>>> The fix is quite ugly. Especially, I make Handshaker public and
>>>>>> pass it
>>>>>> to KerberosClientKeyExchangeImpl so that its context can be used to
>>>>>> check permissions. Is this necessary? I mean, is the context any
>>>>>> different from the one inside KerberosClientKeyExchangeImpl?
>>>>>>
>>>>>> Thanks
>>>>>> Max
>>>>
>>