Qpid
  1. Qpid
  2. QPID-3739

Java properties qpid.ssl.keyStoreCertType and qpid.ssl.trustStoreCertType have misleading names and would be better called qpid.ssl.[Key|Trust]ManagerFactory.algorithm

    Details

      Description

      The Java client supports two system properties, qpid.ssl.trustStoreCertType and qpid.ssl.keyStoreCertType that the Programming-In-Apache-Qpid docbook describe as "the certificate type". These properties are defaulted to SunX509 in ConnectionSettings.java and SSLContextFactory.java.

      Similarly, the Java broker supports a configuration item connector/ssl/certType which is again defaulted to SunX509 in ServerConfiguration.

      On all code paths, these values are passed down to {{javax.net.ssl.KeyManagerFactory
      #getInstance()}} and javax.net.ssl.TrustManagerFactory#getInstance()

      The confusion is that KeyManagerFactory#getInstance()/TrustManagerFactory#getInstance() do not accept a certificate type at all. It accepts a key/trust manager factory algorithm name.

      It would be better if the existing property names were deprecated and a more accurate name used, such as
      qpid.ssl.KeyManagerFactory.algorithm/qpid.ssl.TrustManagerFactory.algorithm. We would continue to support the existing properties, with a warning for a time period.

      I also notice that other projects tend to default the algorithm to Security.getProperty("ssl.KeyManagerFactory.algorithm" and only fallback to SunX509 if that is null. This plays better with non Sun JDKs such as IBMs. See: http://jira.codehaus.org/browse/JETTY-70

        Activity

        Hide
        Keith Wall added a comment -

        Committed patch.

        • Introduced two properties qpid.ssl.KeyManagerFactory.algorithm and qpid.ssl.TrustManagerFactory.algorithm to allow a client user to override the algorithm name used w
        • Continued to support qpid.ssl.keyStoreCertType and qpid.ssl.trustStoreCertType (now marked as deprecated)
        • Introduced a new Java Broker configuration key connector/ssl/keyManagerFactoryAlgorithm
        • Continued to support broker configuration key connector/ssl/certType (now marked as deprecated and will issue warning if used).
        • Changed the default from hardcoded 'SunX509' to the value(s) returned by KeyManagerFactory#getDefaultAlgorithm() and TrustManagerFactory#getDefaultAlgorithm(). This
        • Updated client docbook documentation.

        Tested both Java Broker and Client on IBM JDK and ensured all 0-10 and 0-9-1 profiles pass (including SSLTest which was failing prior to this change).

        Show
        Keith Wall added a comment - Committed patch. Introduced two properties qpid.ssl.KeyManagerFactory.algorithm and qpid.ssl.TrustManagerFactory.algorithm to allow a client user to override the algorithm name used w Continued to support qpid.ssl.keyStoreCertType and qpid.ssl.trustStoreCertType (now marked as deprecated) Introduced a new Java Broker configuration key connector/ssl/keyManagerFactoryAlgorithm Continued to support broker configuration key connector/ssl/certType (now marked as deprecated and will issue warning if used). Changed the default from hardcoded 'SunX509' to the value(s) returned by KeyManagerFactory#getDefaultAlgorithm() and TrustManagerFactory#getDefaultAlgorithm(). This Updated client docbook documentation. Tested both Java Broker and Client on IBM JDK and ensured all 0-10 and 0-9-1 profiles pass (including SSLTest which was failing prior to this change).
        Hide
        Keith Wall added a comment -

        Hi Robbie, could you review this commit please?

        Show
        Keith Wall added a comment - Hi Robbie, could you review this commit please?
        Hide
        Robbie Gemmell added a comment -

        Changes seem good, just a couple tiny niggles:

        • The DEFAULT_ALGORITHM_NAME constant added in in ConnectionSettings seems unused.
        • The JETTY Jira mentions that the Security.getProperty() calls actually do return null on some JVMs, so it might be an idea to guard for that.
        Show
        Robbie Gemmell added a comment - Changes seem good, just a couple tiny niggles: The DEFAULT_ALGORITHM_NAME constant added in in ConnectionSettings seems unused. The JETTY Jira mentions that the Security.getProperty() calls actually do return null on some JVMs, so it might be an idea to guard for that.
        Hide
        Keith Wall added a comment -

        I'll remove the DEFAULT_ALGORITHM_NAME, we don't need it.

        I decided to default the algorithm names using KeyManagerFactory#getDefaultAlgorithm() and TrustManagerFactory#getDefaultAlgorithm() rather than using the system properties ssl.KeyManagerFactory.algorithm and ssl.TrustManagerFactory.algorithm to avoid the possibility that those system properties can return null on some JVM platforms. I think it reasonable to assume that all platforms will return a default value.

        The Jetty defect mentions the GnuJVM. The classpath docs say that getDefaultAlgorithm() returns JessieX509.

        Show
        Keith Wall added a comment - I'll remove the DEFAULT_ALGORITHM_NAME, we don't need it. I decided to default the algorithm names using KeyManagerFactory#getDefaultAlgorithm() and TrustManagerFactory#getDefaultAlgorithm() rather than using the system properties ssl.KeyManagerFactory.algorithm and ssl.TrustManagerFactory.algorithm to avoid the possibility that those system properties can return null on some JVM platforms. I think it reasonable to assume that all platforms will return a default value. The Jetty defect mentions the GnuJVM. The classpath docs say that getDefaultAlgorithm() returns JessieX509.
        Hide
        Robbie Gemmell added a comment -

        Seems reasonable then, closing out.

        Show
        Robbie Gemmell added a comment - Seems reasonable then, closing out.

          People

          • Assignee:
            Keith Wall
            Reporter:
            Keith Wall
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development