code review nits and TrasnportContext constructor changes JDK-8145252-TLS13-branch
authorascarpino
Sat, 09 Jun 2018 13:38:27 -0700
branchJDK-8145252-TLS13-branch
changeset 56715 b152d06ed6a9
parent 56714 2d7e08d730b6
child 56716 38c2a4078033
code review nits and TrasnportContext constructor changes
src/java.base/share/classes/sun/security/ssl/HandshakeContext.java
src/java.base/share/classes/sun/security/ssl/SSLCipher.java
src/java.base/share/classes/sun/security/ssl/SSLHandshake.java
src/java.base/share/classes/sun/security/ssl/ServerHandshakeContext.java
src/java.base/share/classes/sun/security/ssl/ServerHello.java
src/java.base/share/classes/sun/security/ssl/TransportContext.java
--- a/src/java.base/share/classes/sun/security/ssl/HandshakeContext.java	Sat Jun 09 08:08:12 2018 -0700
+++ b/src/java.base/share/classes/sun/security/ssl/HandshakeContext.java	Sat Jun 09 13:38:27 2018 -0700
@@ -118,9 +118,6 @@
     int                                     clientHelloVersion;
     String                                  applicationProtocol;
 
-    // the preferable signature algorithm used by ServerKeyExchange message
-    SignatureScheme                         preferableSignatureAlgorithm;
-
     RandomCookie                            clientHelloRandom;
     RandomCookie                            serverHelloRandom;
     byte[]                                  certRequestContext;
@@ -228,7 +225,7 @@
     }
 
     // Initialize the non-final class variables.
-    private void initialize() throws IOException {
+    private void initialize() {
         ProtocolVersion inputHelloVersion;
         ProtocolVersion outputHelloVersion;
         if (conContext.isNegotiated) {
@@ -498,17 +495,6 @@
         return activeProtocols.contains(protocolVersion);
     }
 
-    boolean isNegotiable(byte majorVersion, byte minorVersion) {
-        ProtocolVersion pv =
-                ProtocolVersion.valueOf(majorVersion, minorVersion);
-        if (pv == null) {
-            // unsupported protocol version
-            return false;
-        }
-
-        return activeProtocols.contains(pv);
-    }
-
     /**
      * Set the active protocol version and propagate it to the SSLSocket
      * and our handshake streams. Called from ClientHandshaker
--- a/src/java.base/share/classes/sun/security/ssl/SSLCipher.java	Sat Jun 09 08:08:12 2018 -0700
+++ b/src/java.base/share/classes/sun/security/ssl/SSLCipher.java	Sat Jun 09 13:38:27 2018 -0700
@@ -468,8 +468,7 @@
         // shipped with the SunJCE  provider.  However, AES/256 is unavailable
         // when the default JCE policy jurisdiction files are installed because
         // of key length restrictions.
-        this.isAvailable =
-                allowed ? isUnlimited(keySize, transformation) : false;
+        this.isAvailable = allowed && isUnlimited(keySize, transformation);
 
         this.readCipherGenerators = readCipherGenerators;
         this.writeCipherGenerators = writeCipherGenerators;
@@ -525,17 +524,6 @@
         return null;
     }
 
-    public static final String getDefaultType() {
-        String prop = AccessController.doPrivileged(
-                new PrivilegedAction<String>() {
-            @Override
-            public String run() {
-                return Security.getProperty("jdk.tls.KeyLimits");
-            }
-        });
-        return prop;
-    }
-
     /**
      * Test if this bulk cipher is available. For use by CipherSuite.
      */
@@ -838,7 +826,7 @@
                     }
                     if (bb.position() != dup.position()) {
                         throw new RuntimeException(
-                                "Unexpected Bytebuffer position");
+                                "Unexpected ByteBuffer position");
                     }
                 } catch (ShortBufferException sbe) {
                     // catch BouncyCastle buffering error
@@ -931,7 +919,7 @@
                     }
                     if (bb.position() != dup.position()) {
                         throw new RuntimeException(
-                                "Unexpected Bytebuffer position");
+                                "Unexpected ByteBuffer position");
                     }
                 } catch (ShortBufferException sbe) {
                     // catch BouncyCastle buffering error
@@ -1024,7 +1012,7 @@
 
                     if (bb.position() != dup.position()) {
                         throw new RuntimeException(
-                                "Unexpected Bytebuffer position");
+                                "Unexpected ByteBuffer position");
                     }
                 } catch (ShortBufferException sbe) {
                     // catch BouncyCastle buffering error
@@ -1180,7 +1168,7 @@
 
                     if (bb.position() != dup.position()) {
                         throw new RuntimeException(
-                                "Unexpected Bytebuffer position");
+                                "Unexpected ByteBuffer position");
                     }
                 } catch (ShortBufferException sbe) {
                     // catch BouncyCastle buffering error
@@ -1296,7 +1284,7 @@
 
                     if (bb.position() != dup.position()) {
                         throw new RuntimeException(
-                                "Unexpected Bytebuffer position");
+                                "Unexpected ByteBuffer position");
                     }
                 } catch (ShortBufferException sbe) {
                     // catch BouncyCastle buffering error
@@ -1474,7 +1462,7 @@
 
                     if (bb.position() != dup.position()) {
                         throw new RuntimeException(
-                                "Unexpected Bytebuffer position");
+                                "Unexpected ByteBuffer position");
                     }
                 } catch (ShortBufferException sbe) {
                     // catch BouncyCastle buffering error
@@ -1602,8 +1590,7 @@
 
                 // DON'T decrypt the nonce_explicit for AEAD mode. The buffer
                 // position has moved out of the nonce_explicit range.
-                int len = bb.remaining();
-                int pos = bb.position();
+                int len, pos = bb.position();
                 ByteBuffer dup = bb.duplicate();
                 try {
                     len = cipher.doFinal(dup, bb);
@@ -1617,7 +1604,7 @@
                     throw new RuntimeException("Cipher buffering error in " +
                         "JCE provider " + cipher.getProvider().getName(), sbe);
                 }
-                // reset the limit to the end of the decryted data
+                // reset the limit to the end of the decrypted data
                 bb.position(pos);
                 bb.limit(pos + len);
 
@@ -1718,8 +1705,7 @@
                 bb.put(nonce);
 
                 // DON'T encrypt the nonce for AEAD mode.
-                int len = bb.remaining();
-                int pos = bb.position();
+                int len, pos = bb.position();
                 if (SSLLogger.isOn && SSLLogger.isOn("plaintext")) {
                     SSLLogger.fine(
                             "Plaintext before ENCRYPTION",
@@ -1868,8 +1854,7 @@
                                         contentType, bb.remaining(), sn);
                 cipher.updateAAD(aad);
 
-                int len = bb.remaining();
-                int pos = bb.position();
+                int len, pos = bb.position();
                 ByteBuffer dup = bb.duplicate();
                 try {
                     len = cipher.doFinal(dup, bb);
@@ -1883,7 +1868,7 @@
                     throw new RuntimeException("Cipher buffering error in " +
                         "JCE provider " + cipher.getProvider().getName(), sbe);
                 }
-                // reset the limit to the end of the decryted data
+                // reset the limit to the end of the decrypted data
                 bb.position(pos);
                 bb.limit(pos + len);
 
@@ -2000,8 +1985,7 @@
                                         contentType, outputSize, sn);
                 cipher.updateAAD(aad);
 
-                int len = bb.remaining();
-                int pos = bb.position();
+                int len, pos = bb.position();
                 if (SSLLogger.isOn && SSLLogger.isOn("plaintext")) {
                     SSLLogger.fine(
                             "Plaintext before ENCRYPTION",
@@ -2205,8 +2189,8 @@
 
         // The caller ensures there are enough bytes available in the buffer.
         // So we won't need to check the remaining of the buffer.
-        for (int i = 0; i < tag.length; i++) {
-            if (bb.get() != tag[i]) {
+        for (byte t : tag) {
+            if (bb.get() != t) {
                 results[0]++;       // mismatched bytes
             } else {
                 results[1]++;       // matched bytes
--- a/src/java.base/share/classes/sun/security/ssl/SSLHandshake.java	Sat Jun 09 08:08:12 2018 -0700
+++ b/src/java.base/share/classes/sun/security/ssl/SSLHandshake.java	Sat Jun 09 13:38:27 2018 -0700
@@ -466,18 +466,6 @@
         return name;
     }
 
-    /*
-    static SSLHandshake valueOf(byte id) {
-        for (SSLHandshake hs : SSLHandshake.values()) {
-            if (hs.id == id) {
-                return hs;
-            }
-        }
-
-        return null;
-    }
-    */
-
     static String nameOf(byte id) {
         // If two handshake message share the same handshake type, returns
         // the first handshake message name.
--- a/src/java.base/share/classes/sun/security/ssl/ServerHandshakeContext.java	Sat Jun 09 08:08:12 2018 -0700
+++ b/src/java.base/share/classes/sun/security/ssl/ServerHandshakeContext.java	Sat Jun 09 13:38:27 2018 -0700
@@ -29,16 +29,15 @@
 import java.security.AlgorithmConstraints;
 import java.security.AccessController;
 import sun.security.util.LegacyAlgorithmConstraints;
-import sun.security.action.GetPropertyAction;
 import sun.security.action.GetLongAction;
 
 class ServerHandshakeContext extends HandshakeContext {
     // To prevent the TLS renegotiation issues, by setting system property
     // "jdk.tls.rejectClientInitiatedRenegotiation" to true, applications in
-    // server side can disable all client initiated SSL renegotiations
+    // server side can disable all client initiated SSL renegotiation
     // regardless of the support of TLS protocols.
     //
-    // By default, allow client initiated renegotiations.
+    // By default, allow client initiated renegotiation.
     static final boolean rejectClientInitiatedRenego =
             Utilities.getBooleanProperty(
                 "jdk.tls.rejectClientInitiatedRenegotiation", false);
--- a/src/java.base/share/classes/sun/security/ssl/ServerHello.java	Sat Jun 09 08:08:12 2018 -0700
+++ b/src/java.base/share/classes/sun/security/ssl/ServerHello.java	Sat Jun 09 13:38:27 2018 -0700
@@ -48,7 +48,7 @@
 import sun.security.ssl.SupportedVersionsExtension.SHSupportedVersionsSpec;
 
 /**
- * Pack of the ServertHello/HelloRetryRequest handshake message.
+ * Pack of the ServerHello/HelloRetryRequest handshake message.
  */
 final class ServerHello {
     static final SSLConsumer handshakeConsumer =
@@ -79,7 +79,7 @@
         new T13HelloRetryRequestConsumer();
 
     /**
-     * The ServertHello handshake message.
+     * The ServerHello handshake message.
      */
     static final class ServerHelloMessage extends HandshakeMessage {
         final ProtocolVersion           serverVersion;      // TLS 1.3 legacy
@@ -115,7 +115,7 @@
             this.clientHello = clientHello;
 
             // The handshakeRecord field is used for HelloRetryRequest consumer
-            // only.  It's fine to set it to null for gnerating side of the
+            // only.  It's fine to set it to null for generating side of the
             // ServerHello/HelloRetryRequest message.
             this.handshakeRecord = null;
         }
@@ -131,7 +131,7 @@
             byte minor = m.get();
             this.serverVersion = ProtocolVersion.valueOf(major, minor);
             if (this.serverVersion == null) {
-                // The client should only request for known protovol versions.
+                // The client should only request for known protocol versions.
                 context.conContext.fatal(Alert.PROTOCOL_VERSION,
                     "Unsupported protocol version: " +
                     ProtocolVersion.nameOf(major, minor));
@@ -390,18 +390,18 @@
         private static KeyExchangeProperties chooseCipherSuite(
                 ServerHandshakeContext shc,
                 ClientHelloMessage clientHello) throws IOException {
-            List<CipherSuite> prefered;
+            List<CipherSuite> preferred;
             List<CipherSuite> proposed;
             if (shc.sslConfig.preferLocalCipherSuites) {
-                prefered = shc.activeCipherSuites;
+                preferred = shc.activeCipherSuites;
                 proposed = clientHello.cipherSuites;
             } else {
-                prefered = clientHello.cipherSuites;
+                preferred = clientHello.cipherSuites;
                 proposed = shc.activeCipherSuites;
             }
 
             List<CipherSuite> legacySuites = new LinkedList<>();
-            for (CipherSuite cs : prefered) {
+            for (CipherSuite cs : preferred) {
                 if (!HandshakeContext.isNegotiable(
                         proposed, shc.negotiatedProtocol, cs)) {
                     continue;
@@ -675,20 +675,20 @@
         private static CipherSuite chooseCipherSuite(
                 ServerHandshakeContext shc,
                 ClientHelloMessage clientHello) throws IOException {
-            List<CipherSuite> prefered;
+            List<CipherSuite> preferred;
             List<CipherSuite> proposed;
             if (shc.sslConfig.preferLocalCipherSuites) {
-                prefered = shc.activeCipherSuites;
+                preferred = shc.activeCipherSuites;
                 proposed = clientHello.cipherSuites;
             } else {
-                prefered = clientHello.cipherSuites;
+                preferred = clientHello.cipherSuites;
                 proposed = shc.activeCipherSuites;
             }
 
             CipherSuite legacySuite = null;
             AlgorithmConstraints legacyConstraints =
                     ServerHandshakeContext.legacyAlgorithmConstraints;
-            for (CipherSuite cs : prefered) {
+            for (CipherSuite cs : preferred) {
                 if (!HandshakeContext.isNegotiable(
                         proposed, shc.negotiatedProtocol, cs)) {
                     continue;
@@ -855,7 +855,6 @@
                     "No more message expected before ServerHello is processed");
             }
 
-            int startPos = message.position();
             ServerHelloMessage shm = new ServerHelloMessage(chc, message);
             if (SSLLogger.isOn && SSLLogger.isOn("ssl,handshake")) {
                 SSLLogger.fine("Consuming ServerHello handshake message", shm);
@@ -960,7 +959,7 @@
 
             if (serverHello.serverRandom.isVersionDowngrade(chc)) {
                 chc.conContext.fatal(Alert.ILLEGAL_PARAMETER,
-                    "A potential protocol versoin downgrade attack");
+                    "A potential protocol version downgrade attack");
             }
 
             // Consume the handshake message for the specific protocol version.
--- a/src/java.base/share/classes/sun/security/ssl/TransportContext.java	Sat Jun 09 08:08:12 2018 -0700
+++ b/src/java.base/share/classes/sun/security/ssl/TransportContext.java	Sat Jun 09 13:38:27 2018 -0700
@@ -35,7 +35,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import javax.crypto.SecretKey;
 import javax.net.ssl.HandshakeCompletedEvent;
 import javax.net.ssl.HandshakeCompletedListener;
 import javax.net.ssl.SSLEngineResult.HandshakeStatus;
@@ -60,109 +59,82 @@
 
     // connection status
     boolean                         isUnsureMode;
-    boolean                         isNegotiated;
-    boolean                         isBroken;
-    boolean                         isInputCloseNotified;
-    boolean                         isOutputCloseNotified;
-    Exception                       closeReason;
+    boolean                         isNegotiated = false;
+    boolean                         isBroken = false;
+    boolean                         isInputCloseNotified = false;
+    boolean                         isOutputCloseNotified = false;
+    Exception                       closeReason = null;
 
     // negotiated security parameters
     SSLSessionImpl                  conSession;
     ProtocolVersion                 protocolVersion;
-    String                          applicationProtocol;
+    String                          applicationProtocol= null;
 
     // handshake context
-    HandshakeContext                handshakeContext;
+    HandshakeContext                handshakeContext = null;
 
     // connection reserved status for handshake.
-    boolean                         secureRenegotiation;
+    boolean                         secureRenegotiation = false;
     byte[]                          clientVerifyData;
     byte[]                          serverVerifyData;
 
     // connection sensitive configuration
     List<NamedGroup>                serverRequestedNamedGroups;
 
-    SecretKey baseWriteSecret, baseReadSecret;
     CipherSuite cipherSuite;
+    private static final byte[] emptyByteArray = new byte[0];
 
     // Please never use the transport parameter other than storing a
     // reference to this object.
+    // Called by SSLEngineImpl
     TransportContext(SSLContextImpl sslContext, SSLTransport transport,
             InputRecord inputRecord, OutputRecord outputRecord) {
-        this.transport = transport;
-        this.sslContext = sslContext;
-        this.inputRecord = inputRecord;
-        this.outputRecord = outputRecord;
-        this.sslConfig = new SSLConfiguration(sslContext, true);
-        this.sslConfig.maximumPacketSize = outputRecord.getMaxPacketSize();
-        this.isUnsureMode = true;
-
-        initialize();
-
-        this.acc = AccessController.getContext();
-        this.consumers = new HashMap<>();
+        this(sslContext, transport, new SSLConfiguration(sslContext, true),
+                inputRecord, outputRecord, true);
     }
 
     // Please never use the transport parameter other than storing a
     // reference to this object.
+    // Called by SSLSocketImpl
     TransportContext(SSLContextImpl sslContext, SSLTransport transport,
             InputRecord inputRecord, OutputRecord outputRecord,
             boolean isClientMode) {
+        this(sslContext, transport,
+                new SSLConfiguration(sslContext, isClientMode),
+                inputRecord, outputRecord,false);
+    }
+
+    // Please never use the transport parameter other than storing a
+    // reference to this object.
+    // Called by SSLSocketImpl with an existing SSLConfig
+    TransportContext(SSLContextImpl sslContext, SSLTransport transport,
+            SSLConfiguration sslConfig,
+            InputRecord inputRecord, OutputRecord outputRecord) {
+        this(sslContext, transport, (SSLConfiguration)sslConfig.clone(),
+                inputRecord, outputRecord, false);
+    }
+
+    private TransportContext(SSLContextImpl sslContext, SSLTransport transport,
+            SSLConfiguration sslConfig, InputRecord inputRecord,
+            OutputRecord outputRecord, boolean isUnsureMode) {
         this.transport = transport;
         this.sslContext = sslContext;
         this.inputRecord = inputRecord;
         this.outputRecord = outputRecord;
-        this.sslConfig = new SSLConfiguration(sslContext, isClientMode);
-        this.sslConfig.maximumPacketSize = outputRecord.getMaxPacketSize();
-        this.isUnsureMode = false;
-
-        initialize();
-
-        this.acc = AccessController.getContext();
-        this.consumers = new HashMap<>();
-    }
-
-    // Please never use the transport parameter other than storing a
-    // reference to this object.
-    TransportContext(SSLContextImpl sslContext, SSLTransport transport,
-            SSLConfiguration sslConfig,
-            InputRecord inputRecord, OutputRecord outputRecord) {
-        this.transport = transport;
-        this.sslContext = sslContext;
-        this.inputRecord = inputRecord;
-        this.outputRecord = outputRecord;
-        this.sslConfig = (SSLConfiguration)sslConfig.clone();
+        this.sslConfig = sslConfig;
         if (this.sslConfig.maximumPacketSize == 0) {
             this.sslConfig.maximumPacketSize = outputRecord.getMaxPacketSize();
         }
-        this.isUnsureMode = false;
-
-        initialize();
+        this.isUnsureMode = isUnsureMode;
 
-        this.acc = AccessController.getContext();
-        this.consumers = new HashMap<>();
-    }
-
-    // Initialize the non-final class variables.
-    private void initialize() {
         // initial security parameters
         this.conSession = SSLSessionImpl.nullSession;
         this.protocolVersion = this.sslConfig.maximumProtocolVersion;
-        this.applicationProtocol = null;
-
-        // initial handshake context
-        this.handshakeContext = null;
+        this.clientVerifyData = emptyByteArray;
+        this.serverVerifyData = emptyByteArray;
 
-        // initial security parameters for secure renegotiation
-        this.secureRenegotiation = false;
-        this.clientVerifyData = new byte[0];
-        this.serverVerifyData = new byte[0];
-
-        this.isNegotiated = false;
-        this.isBroken = false;
-        this.isInputCloseNotified = false;
-        this.isOutputCloseNotified = false;
-        this.closeReason = null;
+        this.acc = AccessController.getContext();
+        this.consumers = new HashMap<>();
     }
 
     // Dispatch plaintext to a specific consumer.
@@ -317,7 +289,7 @@
             } else {    // unlikely, but just in case.
                 if (SSLLogger.isOn && SSLLogger.isOn("ssl")) {
                     SSLLogger.warning(
-                            "Closed transport, rethrowing (unexpected)", cause);
+                            "Closed transport, unexpected rethrowing", cause);
                 }
                 throw alert.createSSLException("Unexpected rethrowing", cause);
             }
@@ -390,7 +362,7 @@
             outputRecord.close();
         } catch (IOException ioe) {
             if (SSLLogger.isOn && SSLLogger.isOn("ssl")) {
-                SSLLogger.warning("Fatal: ouput record closure failed", ioe);
+                SSLLogger.warning("Fatal: output record closure failed", ioe);
             }
         }
 
@@ -473,7 +445,7 @@
         }
     }
 
-    void closeInbound() throws SSLException {
+    void closeInbound() {
         if (isInboundDone()) {
             return;
         }
@@ -512,11 +484,6 @@
             inputRecord.close();
         }
 
-        // For TLS 1.3, output closure is independent from input closure.
-//      if (isNegotiated && protocolVersion.useTLS13PlusSpec()) {
-//          return;
-//      }
-
         // For TLS 1.2 and prior version, it is required to respond with
         // a close_notify alert of its own and close down the connection
         // immediately, discarding any pending writes.
@@ -579,13 +546,6 @@
             }
         }
 
-        // For TLS 1.3, output closure is independent from input closure.
-//
-//      if (isNegotiated && protocolVersion.useTLS13PlusSpec()) {
-//          return;
-//      }
-//
-
         // It is not required for the initiator of the close to wait for the
         // responding close_notify alert before closing the read side of the
         // connection.  However, if the application protocol using TLS
@@ -641,8 +601,6 @@
         }
 
         handshakeContext = null;
-        // inputRecord and outputRecord shares the same handshakeHash
-        // inputRecord.handshakeHash.finish();
         outputRecord.handshakeHash.finish();
         inputRecord.finishHandshake();
         outputRecord.finishHandshake();
@@ -670,7 +628,7 @@
         handshakeContext = null;
 
         // Note: May need trigger handshake completion even for post-handshake
-        // authenticiation in the future.
+        // authentication in the future.
 
         return HandshakeStatus.FINISHED;
     }