Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8204636

Improvement of TLS 1.3 implementation

    Details

    • Type: Enhancement
    • Status: Open
    • Priority: P3
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: tbd
    • Component/s: security-libs
    • Labels:
      None

      Description

      We could have some enhancement in the current TLS 1.3 implementation. This is a collection of them.

      1. sun/security/ssl/SignatureScheme.java
      Valerie:
      Do we need a constructor with both NamedGroup and SigAlgParamSpec? NamedGroup is for EC and SigAlgParamSpec is for RSASSA-PSS, they shouldn't co-exist in the same scheme, right?

      There are currently 5 constructors in SignatureScheme class, I find the ordering of arguments somewhat confusing. The required arguments should appear first and we can add optional ones after them. Easier to read this way, at least for me...

      2. cipher suites preference
      Brad: Since we're down in this code anyway, this is probably a good time to revisit our default ciphersuite ordering scheme.

      3. Performance impact of CipherSuite name look up
      Brad: I'm concerned of the performance impact of repeatedly iterating over 300+ ciphersuite ids/names using values(). You should benchmark and see if it makes sense to switch to using a HashMap (or even TreeMap) here. For the limited number of Protocols (< 10 for TLSv1.x), this approach is fine, but this has quite a bit larger search space.

      4. Remove the temporary tls13VN (done 2018/08/15)

      5. Combing SSLSocketImpl contructors
      Brad: Any chance of combining the commonalities of these different constructors into 1, and then do the constructor-specific things at the end? It will help with future maintenance to not have to make the same changes in multiple places.

      6. SSLLogger
      Weijun: Since SSLConsoleLogger is a Logger child, its log(..., params) method should follow the Logger::log spec, which has

              /**
               * Logs a message with resource bundle and an optional list of
               * parameters.
               * ...
               * @param format the string message format in {@link
               * java.text.MessageFormat} format, (or a key in the message
               * catalog if {@code bundle} is not {@code null}); can be {@code null}.
               * @param params an optional list of parameters to the message (may be
               * none).
               * ...
               */
              public void log(Level level, ResourceBundle bundle, String format,
                      Object... params);

      Especially, the String parameter is in MessageFormat format (that's why it's named "format" but not "message").

      Daniel Fuchs: Just a note that it might be a better idea to rework the implementation of SSLLogger/SSLConsoleLogger a bit in order to have SSLLogger implement System.Logger. This would ensure that the SSLLogger class is skipped when looking for the caller, when the underlying logger is a logger returned by System.getogger() and the backend is java.util.logging. Otherwise the caller will appear to be the static methods defined on the SSLLogger class itself.

      7. nameOf or ofName, or of().
      Weijun: nameOf() sounds like it would return a name. Given the return type is a value, maybe simply of()?
      Brad:
      SSLEngineImpl.java
      ------------------
      757: At line 735, you use

      CipherSuite.validValuesOf(suites);

      but here you use:

      ProtocolVersion.namesOf(protocols);

      Seems like it's basically the same kind of method, do you want to make the names consistent.

      8. SSLConfiguration, 'noSniExtension/Matcher' is confusing.
      Valerie: I have some doubt on line 191 and 198, does "noSniExtension/Matcher" means "no SNI Extension/Matcher"?

      9. Move setUseClientMode(boolean) implementation details to SSLConfiguration.
      Brad: (SSLServerSocketImpl.java) Can we move this sslConfig code (157-167) to a method in SSLConfiguration instead? The logic here in SSLServerSocketImpl uses sslContext.* 8 times (e.g. sslContext.getDefaultProtocolVersions(!useClientMode)). This code is very implementation-dependent on SSLConfiguration's internals, and thus a good encapsulation candidate.

      10. typos
      Brad:
      CertSignAlgsExtension.java: List<SignatureScheme> shemes =
      ->
      CertSignAlgsExtension.java: List<SignatureScheme> schemes =

      SignatureScheme.java: "Ignore disabled signature sheme: " + ss.name);
      SignatureScheme.java: "Ignore inactive signature sheme: " + ss.name);
      ->
      SignatureScheme.java: "Ignore disabled signature scheme: " + ss.name);
      SignatureScheme.java: "Ignore inactive signature scheme: " + ss.name);

      Missing "c"

      11. Brad
      I've noticed lots of enhanced for looping where the less common or preferred things like enums are listed first. If you're going to do this coding style, you might put the common ones up front, and the less common at the end. e.g.

              for (Map.Entry<HandshakeProducer,
                      ProtocolVersion[]> phe : handshakeProducers) {
                  for (ProtocolVersion pv : phe.getValue()) {
                      if (protocolVersion == pv) {
                          return phe.getKey();

      This (protocolVersion == pv) goes through the producers list, which has 6 elements first. So it cycles through TLSv1.2/1.1/1/SSLv3/DTLS12/DTLS10, then finally back to the single element TLSv1.3. I noticed this in several places and seems like it could be improved.

      12. Brad
       We don't implement the certificate_authorities extension, i.e. we only look at the signature types. Thus our calls to any X509KM currently don't have a way to choose a name based on server's CA preference. This seems pretty significant. (This is likely covered by JDK-8206925)

      13. Brad
      Moved to JDK-8210474.

      14. supported_versions extensions consumed twice. First during extension processing to determine which protocol to use, then again during actual T13 server_hello processing.

      SSLExtension.consumeOnLoad:542
      SSLExtensions.consumeOnLoad:186
      ServerHello$ServerHelloConsumer.onServerHello:941
      ServerHello$ServerHelloConsumer.consume:877
      SSLHandhsake.consume:392
      HandshakeContext.dispatch:441
      HandshakeContext.dispatch:419
      TransportContext.dispatch:177
      SSLTransport.decode:164
      SSLSocketImpl.decode:1180
      SSLSocketImpl.readHandshakeRecord:1091

      SSLExtension.consumeOnLoad:542
      SSLExtensions.consumeOnLoad:186
      ServerHello$T13ServerHelloConsumer.consume:1207
      ServerHello$ServerHelloConsumer.onServerHello:989
      ServerHello$ServerHelloConsumer.consume:877
      SSLHandhsake.consume:392
      HandshakeContext.dispatch:441
      HandshakeContext.dispatch:419
      TransportContext.dispatch:177
      SSLTransport.decode:164
      SSLSocketImpl.decode:1180
      SSLSocketImpl.readHandshakeRecord:1091

      We end up with two identical debug output messages.

        Attachments

          Issue Links

            Activity

              People

              • Assignee:
                Unassigned
                Reporter:
                xuelei Xue-Lei Fan
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated: