Make SSL False Start work with asynchronous certificate validation

net/socket/ssl_client_socket_nss.cc

Index: net/socket/ssl_client_socket_nss.cc
===================================================================
--- net/socket/ssl_client_socket_nss.cc	(revision 222312)
+++ net/socket/ssl_client_socket_nss.cc	(working copy)
@@ -711,9 +711,14 @@
                                      SECKEYPrivateKey** result_private_key);
 #endif
 
-  // Called by NSS once the handshake has completed.
+  // Called once the handshake has completed.
+  void HandshakeSucceeded();
+
+  // Called by NSS to determine if we can false start.

[agl, 2013/09/13 15:03:29]

In other comments, "false start" is written as "False Start".

[wtc, 2013/09/18 22:57:23]

Done.

   // |arg| contains a pointer to the current SSLClientSocketNSS::Core.
-  static void HandshakeCallback(PRFileDesc* socket, void* arg);
+  static SECStatus CanFalseStartCallback(PRFileDesc* socket,
+                                         void* arg,
+                                         PRBool* can_false_start);
 
   // Handles an NSS error generated while handshaking or performing IO.
   // Returns a network error code mapped from the original NSS error.
@@ -866,8 +871,6 @@
   bool channel_id_needed_;
   // True if the handshake state machine was interrupted for client auth.
   bool client_auth_cert_needed_;
-  // True if NSS has called HandshakeCallback.
-  bool handshake_callback_called_;
 
   HandshakeState nss_handshake_state_;
 
@@ -934,7 +937,6 @@
       channel_id_xtn_negotiated_(false),
       channel_id_needed_(false),
       client_auth_cert_needed_(false),
-      handshake_callback_called_(false),
       transport_recv_busy_(false),
       transport_recv_eof_(false),
       transport_send_busy_(false),
@@ -1029,10 +1031,10 @@
     }
   }
 
-  rv = SSL_HandshakeCallback(
-      nss_fd_, SSLClientSocketNSS::Core::HandshakeCallback, this);
+  rv = SSL_SetCanFalseStartCallback(
+      nss_fd_, SSLClientSocketNSS::Core::CanFalseStartCallback, this);
   if (rv != SECSuccess) {
-    LogFailedNSSFunction(*weak_net_log_, "SSL_HandshakeCallback", "");
+    LogFailedNSSFunction(*weak_net_log_, "SSL_SetCanFalseStartCallback", "");
     return false;
   }
 
@@ -1155,7 +1157,6 @@
   }
 
   DCHECK(OnNSSTaskRunner());
-  DCHECK(handshake_callback_called_);
   DCHECK_EQ(STATE_NONE, next_handshake_state_);
   DCHECK(user_read_callback_.is_null());
   DCHECK(user_connect_callback_.is_null());
@@ -1209,7 +1210,6 @@
   }
 
   DCHECK(OnNSSTaskRunner());
-  DCHECK(handshake_callback_called_);
   DCHECK_EQ(STATE_NONE, next_handshake_state_);
   DCHECK(user_write_callback_.is_null());
   DCHECK(user_connect_callback_.is_null());
@@ -1271,28 +1271,6 @@
     PRFileDesc* socket,
     PRBool checksig,
     PRBool is_server) {
-  Core* core = reinterpret_cast<Core*>(arg);
-  if (!core->handshake_callback_called_) {
-    // Only need to turn off False Start in the initial handshake. Also, it is
-    // unsafe to call SSL_OptionSet in a renegotiation because the "first
-    // handshake" lock isn't already held, which will result in an assertion
-    // failure in the ssl_Get1stHandshakeLock call in SSL_OptionSet.
-    PRBool negotiated_extension;
-    SECStatus rv = SSL_HandshakeNegotiatedExtension(socket,
-                                                    ssl_app_layer_protocol_xtn,
-                                                    &negotiated_extension);
-    if (rv != SECSuccess || !negotiated_extension) {
-      rv = SSL_HandshakeNegotiatedExtension(socket,
-                                            ssl_next_proto_nego_xtn,
-                                            &negotiated_extension);
-    }
-    if (rv != SECSuccess || !negotiated_extension) {
-      // If the server doesn't support NPN or ALPN, then we don't do False
-      // Start with it.
-      SSL_OptionSet(socket, SSL_ENABLE_FALSE_START, PR_FALSE);
-    }
-  }
-
   // Tell NSS to not verify the certificate.
   return SECSuccess;
 }
@@ -1605,37 +1583,53 @@
 }
 #endif  // NSS_PLATFORM_CLIENT_AUTH
 
-// static
-void SSLClientSocketNSS::Core::HandshakeCallback(
-    PRFileDesc* socket,
-    void* arg) {
-  Core* core = reinterpret_cast<Core*>(arg);
-  DCHECK(core->OnNSSTaskRunner());
+void SSLClientSocketNSS::Core::HandshakeSucceeded() {
+  DCHECK(OnNSSTaskRunner());
 
-  core->handshake_callback_called_ = true;
-
-  HandshakeState* nss_state = &core->nss_handshake_state_;
-
   PRBool last_handshake_resumed;
-  SECStatus rv = SSL_HandshakeResumedSession(socket, &last_handshake_resumed);
+  SECStatus rv = SSL_HandshakeResumedSession(nss_fd_, &last_handshake_resumed);
   if (rv == SECSuccess && last_handshake_resumed) {
-    nss_state->resumed_handshake = true;
+    nss_handshake_state_.resumed_handshake = true;
   } else {
-    nss_state->resumed_handshake = false;
+    nss_handshake_state_.resumed_handshake = false;
   }
 
-  core->RecordChannelIDSupport();
-  core->UpdateServerCert();
-  core->UpdateConnectionStatus();
-  core->UpdateNextProto();
+  RecordChannelIDSupport();
+  UpdateServerCert();
+  UpdateConnectionStatus();
+  UpdateNextProto();
 
   // Update the network task runners view of the handshake state whenever
   // a handshake has completed.
-  core->PostOrRunCallback(
-      FROM_HERE, base::Bind(&Core::OnHandshakeStateUpdated, core,
-                            *nss_state));
+  PostOrRunCallback(
+      FROM_HERE, base::Bind(&Core::OnHandshakeStateUpdated, this,
+                            nss_handshake_state_));
 }
 
+// static
+SECStatus SSLClientSocketNSS::Core::CanFalseStartCallback(
+    PRFileDesc* socket,
+    void* arg,
+    PRBool* can_false_start) {
+  // If the server doesn't support NPN or ALPN, then we don't do False Start
+  // with it.
+  PRBool negotiated_extension;
+  SECStatus rv = SSL_HandshakeNegotiatedExtension(socket,
+                                                  ssl_app_layer_protocol_xtn,
+                                                  &negotiated_extension);
+  if (rv != SECSuccess || !negotiated_extension) {
+    rv = SSL_HandshakeNegotiatedExtension(socket,
+                                          ssl_next_proto_nego_xtn,
+                                          &negotiated_extension);
+  }
+  if (rv != SECSuccess || !negotiated_extension) {
+    *can_false_start = PR_FALSE;
+    return SECSuccess;
+  }
+
+  return SSL_DefaultCanFalseStart(socket, can_false_start);
+}
+
 int SSLClientSocketNSS::Core::HandleNSSError(PRErrorCode nss_error,
                                              bool handshake_error) {
   DCHECK(OnNSSTaskRunner());
@@ -1708,7 +1702,6 @@
 
 int SSLClientSocketNSS::Core::DoReadLoop(int result) {
   DCHECK(OnNSSTaskRunner());
-  DCHECK(handshake_callback_called_);
   DCHECK_EQ(STATE_NONE, next_handshake_state_);
 
   if (result < 0)
@@ -1737,7 +1730,6 @@
 
 int SSLClientSocketNSS::Core::DoWriteLoop(int result) {
   DCHECK(OnNSSTaskRunner());
-  DCHECK(handshake_callback_called_);
   DCHECK_EQ(STATE_NONE, next_handshake_state_);
 
   if (result < 0)
@@ -1793,52 +1785,41 @@
     if (rv == SECSuccess && SSL_InvalidateSession(nss_fd_) != SECSuccess)
       LOG(WARNING) << "Couldn't invalidate SSL session: " << PR_GetError();
   } else if (rv == SECSuccess) {
-    if (!handshake_callback_called_) {
-      // Workaround for https://bugzilla.mozilla.org/show_bug.cgi?id=562434 -
-      // SSL_ForceHandshake returned SECSuccess prematurely.
-      rv = SECFailure;
-      net_error = ERR_SSL_PROTOCOL_ERROR;
-      PostOrRunCallback(
-          FROM_HERE,
-          base::Bind(&AddLogEventWithCallback, weak_net_log_,
-                     NetLog::TYPE_SSL_HANDSHAKE_ERROR,
-                     CreateNetLogSSLErrorCallback(net_error, 0)));
-    } else {
+    HandshakeSucceeded();
   #if defined(SSL_ENABLE_OCSP_STAPLING)
-      // TODO(agl): figure out how to plumb an OCSP response into the Mac
-      // system library and update IsOCSPStaplingSupported for Mac.
-      if (IsOCSPStaplingSupported()) {
-        const SECItemArray* ocsp_responses =
-            SSL_PeerStapledOCSPResponses(nss_fd_);
-        if (ocsp_responses->len) {
+    // TODO(agl): figure out how to plumb an OCSP response into the Mac
+    // system library and update IsOCSPStaplingSupported for Mac.
+    if (IsOCSPStaplingSupported()) {
+      const SECItemArray* ocsp_responses =
+          SSL_PeerStapledOCSPResponses(nss_fd_);
+      if (ocsp_responses->len) {
   #if defined(OS_WIN)
-          if (nss_handshake_state_.server_cert) {
-            CRYPT_DATA_BLOB ocsp_response_blob;
-            ocsp_response_blob.cbData = ocsp_responses->items[0].len;
-            ocsp_response_blob.pbData = ocsp_responses->items[0].data;
-            BOOL ok = CertSetCertificateContextProperty(
-                nss_handshake_state_.server_cert->os_cert_handle(),
-                CERT_OCSP_RESPONSE_PROP_ID,
-                CERT_SET_PROPERTY_IGNORE_PERSIST_ERROR_FLAG,
-                &ocsp_response_blob);
-            if (!ok) {
-              VLOG(1) << "Failed to set OCSP response property: "
-                      << GetLastError();
-            }
+        if (nss_handshake_state_.server_cert) {
+          CRYPT_DATA_BLOB ocsp_response_blob;
+          ocsp_response_blob.cbData = ocsp_responses->items[0].len;
+          ocsp_response_blob.pbData = ocsp_responses->items[0].data;
+          BOOL ok = CertSetCertificateContextProperty(
+              nss_handshake_state_.server_cert->os_cert_handle(),
+              CERT_OCSP_RESPONSE_PROP_ID,
+              CERT_SET_PROPERTY_IGNORE_PERSIST_ERROR_FLAG,
+              &ocsp_response_blob);
+          if (!ok) {
+            VLOG(1) << "Failed to set OCSP response property: "
+                    << GetLastError();
           }
+        }
   #elif defined(USE_NSS)
-          CacheOCSPResponseFromSideChannelFunction cache_ocsp_response =
-              GetCacheOCSPResponseFromSideChannelFunction();
+        CacheOCSPResponseFromSideChannelFunction cache_ocsp_response =
+            GetCacheOCSPResponseFromSideChannelFunction();
 
-          cache_ocsp_response(
-              CERT_GetDefaultCertDB(),
-              nss_handshake_state_.server_cert_chain[0], PR_Now(),
-              &ocsp_responses->items[0], NULL);
+        cache_ocsp_response(
+            CERT_GetDefaultCertDB(),
+            nss_handshake_state_.server_cert_chain[0], PR_Now(),
+            &ocsp_responses->items[0], NULL);
   #endif
-        }
       }
-  #endif
     }
+  #endif
     // Done!
   } else {
     PRErrorCode prerr = PR_GetError();