On 8038089: TLS optional support for Kerberos cipher suites needs to be re-examine

[Wang Weijun]

On Jul 21, 2014, at 23:09, Alan Bateman <Alan.Bateman at oracle.com> wrote:

> On 21/07/2014 09:22, Wang Weijun wrote:
>> Please review the updated webrev at
>> 
>>   
>> http://cr.openjdk.java.net/~weijun/8038089/webrev.01
>> 
>> 
>> Some comment changes. Some arguments rearrangement between classes.
>> 
>> The interface is still in sun.security.ssl. It will be easy to move it to somewhere else later. When module is introduced, we may need to export the interface from java.base to java.security.jgss.
>> 
>> 
> I'm skimmed over the changes (not a detailed review yet) and just want to check one thing - would I be correct to say that this isn't a general solution for plugging in addition cipher suites, the ServiceLoader usage in Krb5Helper looks specifically for a provider that supports TLS_KRB5_XXX from what I can establish.

It's meant to be a general solution, and that's why I add a support() method. Said that, we have no idea what a non-KRB5 not-certificate-based cipher suite would look like.

As for Krb5Helper, it's only used by KRB5 cipher suites. I don't want to modify too many SSL codes where the helper class is called like

  case KRB5: Krb5Helper.doXXX(); break;

Ideally, it should be something like:

  case RSA: bla-bla-bla(); break;
  default: getHelper(ciphersuite).doXXX(); break;

We will do this if we have more provider(s) later.

> 
> One other thing about the ServiceLoader usage in Krb5Helper is that it's using the one-arg load method, hence the TCCL will be used to locate the cipher suite providers. As the provider is cached system-wide then I assume you meant to specify the system class loader here.

Yes.

> 
> The profiles build currently uses a simple dependency checker to ensure that there aren't any dependencies in a compact N build on something that is in a larger profile or the JRE. It allows a few exceptions, due to the need to keep jsse.jar and these are maintained in jdk/make/data/checkdeps/refs.allowed. I think this     file will need to be updated to drop references to classes that no longer exist.

I didn't know that file. Now only one line left there.

> 
> A minor comment is that the new files are missing a copyright header, I assume you'll fix that up before pushing.

Sure.

Thanks
Max

> 
> -Alan.