Code review request: 8005523: Unbound krb5 for TLS

[Xuelei Fan]

On 4/15/2013 8:57 PM, Weijun Wang wrote:
>>>      http://cr.openjdk.java.net/~weijun/8005523/webrev.01/
>>>
>>
>> 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.
> 
> So it can fail early? That's OK.
> 
It's more about to select the right cipher suite.  If it is not failed
during cipher suite selection, the server may select a cipher suite that
it cannot support, and does not have the chance to select a right cipher
suite (e.g. non krb5 cipher suites). It's too bad.

>>
>> I think it is OK to get the krb5 principal for bound krb5, right?
> 
> Yes.
> 
>>
>> 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?
> 
> "*" is acceptable in ServicePermission even before unbound krb5 is
> introduced. It works nicely now.
> 
I asked this question because I think we might not need to double check
the permission in ClientKeyExchange.  Do we really need to double check
the permissions in clientHello and ClientKeyExchange?

Xuelei

>>
>>
>> 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);
> 
> Haha. I'll remove it.
> 
> I'm refining the test to make sure permissions are granted correctly.
> Also, it seems there are useless requests made to server. Will double
> check.
> 
> Thanks
> Max
> 
>>
>> 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
>>>>>>
>>>>
>>