On Jul 21, 2014, at 23:09, Alan Bateman <alan.bate...@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.
