RFC: Fix for PR1161: X509VariableTrustManager does not work correctly with OpenJDK7
Omair Majid
omajid at redhat.com
Tue Sep 11 11:46:22 PDT 2012
On 09/07/2012 05:33 PM, Deepak Bhole wrote:
> Hi,
>
> This patch fixes PR1161:
> http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=1161
>
> As specified there, the issue is that Java 7 changed what parent class
> a trust manager needs to implement in order to correctly intercept
> issues such as CN mismatch. As a result, icedtea-web currently cannot
> load sites whose certificate has a CN mismatch (proprietary tools work
> fine in such cases).
>
> Attached patch adds support for Java 7 while maintaining compatibility
> with Java 6 (loading one trust manager for 6 and a different one for 7+).
>
> I would like to see this backported to 1.3 as well given its importance, but we
> can let it bake in HEAD for a couple of weeks.
If I may make a request, when you are pushing this, could you push this
as two changesets? One that deals with the JDK version problems and the
other that changes the check for hostname (this patch includes both but
mixes them).
> OK for HEAD?
>
> ChangeLog:
> 2012-09-07 Deepak Bhole <dbhole at redhat.com>
>
> PR1161: X509VariableTrustManager does not work correctly with OpenJDK7
> * Makefile.am: If building with JDK 6, don't build
> VariableX509TrustManagerJDK7.
> * NEWS: Updated.
> * acinclude.m4: In addition to setting VERSION_DEFS, also set HAVE_JAVA7
> if building with JDK7.
> * netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java (initialize): Use new
> getSSLSocketTrustManager() method to get the trust manager.
> (getSSLSocketTrustManager): New method. Depending on runtime JRE version,
> returns the appropriate trust manager.
> * netx/net/sourceforge/jnlp/security/HttpsCertVerifier.java: Removed
> unused tm variable.
> * netx/net/sourceforge/jnlp/security/VariableX509TrustManager.java: No
> longer extends com.sun.net.ssl.internal.ssl.X509ExtendedTrustManager.
> (checkClientTrusted): Renamed to checkTrustClient and removed overloaded
> implementations.
> (checkServerTrusted): Renamed to checkTrustServer. Also, modified to
> accept socket and engine (may be null). Assume that CN is mismatched by
> default, rather than matched. If explicitly trusted, bypass other checks,
> including CN mismatch.
> (checkAllManagers): Modified to accept socket and engine. Modified to work
> for both JDK6 and JDK7.
> (getAcceptedIssuers): Make protected (called by others in package).
> * netx/net/sourceforge/jnlp/security/VariableX509TrustManagerJDK6.java:
> New class -- X509TrustManager for JDK6.
> * netx/net/sourceforge/jnlp/security/VariableX509TrustManagerJDK7.java:
> New class -- X509TrustManager for JDK7.
>
> Thanks,
> Deepak
>
>
> PR1161.patch
>
>
> diff -r 0db02eca94bf Makefile.am
> --- a/Makefile.am Fri Sep 07 13:52:23 2012 +0200
> +++ b/Makefile.am Fri Sep 07 17:28:56 2012 -0400
> @@ -348,6 +348,9 @@
> if !WITH_RHINO
> sed -i '/RhinoBasedPacEvaluator/ d' $@
> endif
> +if !HAVE_JAVA7
> + sed -i '/VariableX509TrustManagerJDK7/ d' $@
> +endif
>
> stamps/netx.stamp: netx-source-files.txt stamps/bootstrap-directory.stamp
> mkdir -p $(NETX_DIR)
> diff -r 0db02eca94bf NEWS
> --- a/NEWS Fri Sep 07 13:52:23 2012 +0200
> +++ b/NEWS Fri Sep 07 17:28:56 2012 -0400
> @@ -17,6 +17,7 @@
> * Common
> - PR1049: Extension jnlp's signed jar with the content of only META-INF/* is considered
> - PR955: regression: SweetHome3D fails to run
> + - PR1161: X509VariableTrustManager does not work correctly with OpenJDK7
>
> New in release 1.3 (2012-XX-XX):
> * NetX
> diff -r 0db02eca94bf acinclude.m4
> --- a/acinclude.m4 Fri Sep 07 13:52:23 2012 +0200
> +++ b/acinclude.m4 Fri Sep 07 17:28:56 2012 -0400
> @@ -714,10 +714,13 @@
> fi
> AC_MSG_RESULT(${JAVA})
> AC_SUBST(JAVA)
> - JAVA_VERSION=`$JAVA -version 2>&1 | sed -n '1s/@<:@^"@:>@*"\(.*\)"$/\1/p'`
> - case "${JAVA_VERSION}" in
> - 1.7*) VERSION_DEFS='-DHAVE_JAVA7';;
> - esac
> + JAVA_VERSION=`$JAVA -version 2>&1 | sed -n -e '1s/@<:@^"@:>@*"\(.*\)"$/\1/p'`
We should try and make this regex readable at some point :)
> + HAVE_JAVA7=`echo $JAVA_VERSION | awk '{if ($(0) >= 1.7) print "yes"}'`
> + if ! test -z "$HAVE_JAVA7" ; then
> + VERSION_DEFS='-DHAVE_JAVA7'
> + fi
> +
> + AM_CONDITIONAL([HAVE_JAVA7], test x"${HAVE_JAVA7}" = "xyes" )
> AC_SUBST(VERSION_DEFS)
> ])
>
> diff -r 0db02eca94bf netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java Fri Sep 07 13:52:23 2012 +0200
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java Fri Sep 07 17:28:56 2012 -0400
> @@ -248,6 +269,42 @@
> }
>
> /**
> + * Returns a TrustManager ideal for the running VM.
> + *
> + * @return TrustManager the trust manager to use for verifying https certificates
> + */
Some indention seems to be off here.
> + private static TrustManager getSSLSocketTrustManager() throws
> + ClassNotFoundException, IllegalAccessException, InstantiationException, InvocationTargetException {
> +
> + try {
> +
> + Class<?> trustManagerClass;
> + Constructor<?> tmCtor = null;
Perhaps it might be better to use X509TrustManager as the type argument
to make thing clearer? You can chain it like
Class.forName().asSubClass(X509TrustManager.class) to make the code more
explicit.
> +
> + if (System.getProperty("java.version").startsWith("1.6")) { // Java 6
> + trustManagerClass = Class.forName("net.sourceforge.jnlp.security.VariableX509TrustManagerJDK6");
> + } else { // Java 7 or more (technically could be <= 1.5 but <= 1.5 is unsupported)
> + trustManagerClass = Class.forName("net.sourceforge.jnlp.security.VariableX509TrustManagerJDK7");
> + }
> +
> + Constructor<?>[] tmCtors = trustManagerClass.getDeclaredConstructors();
> + tmCtor = tmCtors[0];
> +
> + for (Constructor<?> ctor : tmCtors) {
> + if (tmCtor.getGenericParameterTypes().length == 0) {
> + tmCtor = ctor;
> + break;
> + }
> + }
> +
> + return (TrustManager) tmCtor.newInstance();
> + } catch (RuntimeException e) {
> + System.err.println("Unable to load JDK-specific TrustManager. Was this version of IcedTea-Web compiled with JDK6?");
Is it worth i18n'ing this message?
Also, would it be better to print the "compiled with wrong jdk" message
(which really indicates that the wanted class is not found") in the
place where we do the jdk check?
> diff -r 0db02eca94bf netx/net/sourceforge/jnlp/security/VariableX509TrustManager.java
> --- a/netx/net/sourceforge/jnlp/security/VariableX509TrustManager.java Fri Sep 07 13:52:23 2012 +0200
> +++ b/netx/net/sourceforge/jnlp/security/VariableX509TrustManager.java Fri Sep 07 17:28:56 2012 -0400
> /**
> - * Check if the server is trusted
> + * Check if the server is trusted.
> *
> * @param chain The cert chain
> * @param authType The auth type algorithm
> - * @param checkOnly Whether to "check only" i.e. no user prompt, or to prompt for permission
> + * @param hostName The expected hostName that the server should have
> + * @param socket The SSLSocket in use (may be null)
> + * @param ending The SSLEngine in use (may be null)
> */
> - public synchronized void checkServerTrusted(X509Certificate[] chain,
> + public synchronized void checkTrustServer(X509Certificate[] chain,
> String authType, String hostName,
> - boolean checkOnly) throws CertificateException {
> + SSLSocket socket, SSLEngine engine) throws CertificateException {
> CertificateException ce = null;
> boolean trusted = true;
> - boolean CNMatched = true;
> + boolean CNMatched = false;
>
> try {
> - checkAllManagers(chain, authType);
> + checkAllManagers(chain, authType, socket, engine);
> } catch (CertificateException e) {
> trusted = false;
> ce = e;
> @@ -226,59 +214,92 @@
> // If the certificate is not explicitly trusted, we
> // need to prompt the user
> if (!isExplicitlyTrusted(chain, authType)) {
> -
> - if (hostName == null) {
> - CNMatched = false;
> - } else {
> + if (hostName != null) {
> try {
> HostnameChecker checker = HostnameChecker
> .getInstance(HostnameChecker.TYPE_TLS);
>
> - checker.match(hostName, chain[0]); // only need to match @ 0 for
> - // CN
> + checker.match(hostName, chain[0]); // only need to match @ 0 for CN
>
> + CNMatched = true;
> } catch (CertificateException e) {
> - CNMatched = false;
> ce = e;
> }
> }
> + } else {
> + // Else it is explicitly trusted. Return here.
> + return;
> }
I don't like this else block: if the code below ever adds more checks,
they will be skipped. Why not let the code below handle this?
>
> if (!trusted || !CNMatched) {
> - if (checkOnly) {
> - throw ce;
> - } else {
> - if (!isTemporarilyUntrusted(chain[0])) {
> - boolean b = askUser(chain, authType, trusted, CNMatched, hostName);
> + if (!isTemporarilyUntrusted(chain[0])) {
> + boolean b = askUser(chain, authType, trusted, CNMatched, hostName);
>
> - if (b) {
> - temporarilyTrust(chain[0]);
> - } else {
> - temporarilyUntrust(chain[0]);
> - }
> + if (b) {
> + temporarilyTrust(chain[0]);
> + return;
> + } else {
> + temporarilyUntrust(chain[0]);
> }
> + }
>
> - checkAllManagers(chain, authType);
> - }
> + throw ce;
> }
> }
>
> /**
> - * Check system, user and custom trust manager
> + * Check system, user and custom trust manager.
> + *
> + * This method is intended to work with both, JRE6 and JRE7. If socket
> + * and engine are null, it assumes that the call is for JRE6 (i.e. not
> + * javax.net.ssl.X509ExtendedTrustManager which is Java 7 specific). If
> + * either of those are not null, it will assume that the caTrustManagers
> + * are javax.net.ssl.X509ExtendedTrustManager instances and will
> + * invoke their check methods.
> + *
> + * @param chain The certificate chain
> + * @param authType The authentication type
> + * @param socket the SSLSocket being used for the connection
> + * @param engine the SSLEngine being used for the connection
> */
> - private void checkAllManagers(X509Certificate[] chain, String authType) throws CertificateException {
> + private void checkAllManagers(X509Certificate[] chain, String authType, Socket socket, SSLEngine engine) throws CertificateException {
> +
> // first try CA TrustManagers
> boolean trusted = false;
> ValidatorException savedException = null;
> for (int i = 0; i < caTrustManagers.length; i++) {
> try {
> - caTrustManagers[i].checkServerTrusted(chain, authType);
> + if (socket == null && engine == null) {
> + caTrustManagers[i].checkServerTrusted(chain, authType);
> + } else {
> +
> + try {
> + Class<?> x509ETMClass = Class.forName("javax.net.ssl.X509ExtendedTrustManager");
> + if (engine == null) {
> + Method mcheckServerTrusted = x509ETMClass.getDeclaredMethod("checkServerTrusted", X509Certificate[].class, String.class, Socket.class);
> + mcheckServerTrusted.invoke(caTrustManagers[i], chain, authType, socket);
> + } else {
> + Method mcheckServerTrusted = x509ETMClass.getDeclaredMethod("checkServerTrusted", X509Certificate[].class, String.class, SSLEngine.class);
> + mcheckServerTrusted.invoke(caTrustManagers[i], chain, authType, engine);
> + }
> + } catch (NoSuchMethodException nsme) {
> + throw new ValidatorException(nsme.getMessage());
> + } catch (InvocationTargetException ite) {
> + throw new ValidatorException(ite.getMessage());
> + } catch (IllegalAccessException iae) {
> + throw new ValidatorException(iae.getMessage());
> + } catch (ClassNotFoundException cnfe) {
> + throw new ValidatorException(cnfe.getMessage());
> + }
> + }
> +
Since this is not common code, why not put it in the JDK6/7 specific class?
> diff -r 0db02eca94bf netx/net/sourceforge/jnlp/security/VariableX509TrustManagerJDK6.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/security/VariableX509TrustManagerJDK6.java Fri Sep 07 17:28:56 2012 -0400
> @@ -0,0 +1,38 @@
> +package net.sourceforge.jnlp.security;
Missing license header.
> +import java.security.cert.CertificateException;
> +import java.security.cert.X509Certificate;
> +
> +import com.sun.net.ssl.internal.ssl.X509ExtendedTrustManager;
> +
> +public class VariableX509TrustManagerJDK6 extends X509ExtendedTrustManager {
> +
> + VariableX509TrustManager vX509TM = VariableX509TrustManager.getInstance();
Could you make this private? I don't think package-private is a problem,
but just to be safe.
> diff -r 0db02eca94bf netx/net/sourceforge/jnlp/security/VariableX509TrustManagerJDK7.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/security/VariableX509TrustManagerJDK7.java Fri Sep 07 17:28:56 2012 -0400
> @@ -0,0 +1,122 @@
> +package net.sourceforge.jnlp.security;
Missing license header.
> +public class VariableX509TrustManagerJDK7 extends X509ExtendedTrustManager {
> +
> + VariableX509TrustManager vX509TM = VariableX509TrustManager.getInstance();
Please make this private too.
> + /**
> + * Check if the server is trusted
> + *
> + * @param chain The cert chain
> + * @param authType The auth type algorithm
> + * @param socket the SSLSocket, if provided
> + * @param engine the SSLEngine, if provided
> + */
s/if provided/may be null/ ?
> + private void checkTrustServer(X509Certificate[] chain,
> + String authType, Socket socket,
> + SSLEngine engine) throws CertificateException {
> +
> + String hostName = null;
> +
> + if (socket != null) {
> + hostName = ((SSLSocket) socket).getHandshakeSession().getPeerHost();
> + } else if (engine != null) {
> + hostName = engine.getHandshakeSession().getPeerHost();
> + }
> +
> + vX509TM.checkTrustServer(chain, authType, hostName, (SSLSocket) socket, engine);
> + }
> +
> + /**
> + * Check if the server is trusted
/me looks at method name
> + * @param chain The cert chain
> + * @param authType The auth type algorithm
> + * @param socket the SSLSocket, if provided
> + * @param engine the SSLEngine, if provided
> + */
> + private void checkTrustClient(X509Certificate[] chain,
> + String authType, Socket socket,
> + SSLEngine engine) throws CertificateException {
> +
> + String hostName = null;
> +
> + try {
> + if (socket != null) {
> + Method mgetHandshakeSession = SSLSocket.class.getDeclaredMethod("getHandshakeSession");
> + SSLSession session = (SSLSession) mgetHandshakeSession.invoke(socket);
> +
> + if (session == null) {
> + throw new CertificateException("No handshake session");
> + }
> +
> + hostName = session.getPeerHost();
> + } else if (engine != null) {
> + Method mgetHandshakeSession = SSLEngine.class.getDeclaredMethod("getHandshakeSession");
> + SSLSession session = (SSLSession) mgetHandshakeSession.invoke(engine);
> +
> + if (session == null) {
> + throw new CertificateException("No handshake session");
> + }
> +
> + hostName = session.getPeerHost();
> + }
I don't see why we are using reflection here. Could you explain?
> + } catch (NoSuchMethodException nsme) {
> + // Do nothing. hostName will be null
> + } catch (InvocationTargetException ite) {
> + //Do nothing. hostName will be null
> + } catch (IllegalAccessException iae) {
> + //Do nothing. hostName will be null
> + }
> +
> + vX509TM.checkTrustClient(chain, authType, hostName);
> + }
> +}
>
Thanks,
Omair
More information about the distro-pkg-dev
mailing list