Unwind the SSL connection holdback experiment and remove related code

net/socket/ssl_client_socket_openssl.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef NET_SOCKET_SSL_CLIENT_SOCKET_OPENSSL_H_
 #define NET_SOCKET_SSL_CLIENT_SOCKET_OPENSSL_H_
 
 #include <string>
 
 #include "base/compiler_specific.h"
@@ skipping 42 matching lines @@
                          const SSLConfig& ssl_config,
                          const SSLClientSocketContext& context);
   ~SSLClientSocketOpenSSL() override;
 
   const HostPortPair& host_and_port() const { return host_and_port_; }
   const std::string& ssl_session_cache_shard() const {
     return ssl_session_cache_shard_;
   }
 
   // SSLClientSocket implementation.
-  std::string GetSessionCacheKey() const override;
-  bool InSessionCache() const override;
-  void SetHandshakeCompletionCallback(const base::Closure& callback) override;
   void GetSSLCertRequestInfo(SSLCertRequestInfo* cert_request_info) override;
   NextProtoStatus GetNextProto(std::string* proto) override;
   ChannelIDService* GetChannelIDService() const override;
 
   // SSLSocket implementation.
   int ExportKeyingMaterial(const base::StringPiece& label,
                            bool has_context,
                            const base::StringPiece& context,
                            unsigned char* out,
                            unsigned int outlen) override;
@@ skipping 31 matching lines @@
  private:
   class PeerCertificateChain;
   class SSLContext;
   friend class SSLClientSocket;
   friend class SSLContext;
 
   int Init();
   void DoReadCallback(int result);
   void DoWriteCallback(int result);
 
-  void OnHandshakeCompletion();
-
   bool DoTransportIO();
   int DoHandshake();
   int DoChannelIDLookup();
   int DoChannelIDLookupComplete(int result);
   int DoVerifyCert(int result);
   int DoVerifyCertComplete(int result);
   void DoConnectCallback(int result);
   void UpdateServerCert();
   void VerifyCT();
 
@@ skipping 35 matching lines @@
                                  const char *argp, int argi, long argl,
                                  long retvalue);
 
   // Callback from the SSL layer when an operation is performed on
   // |transport_bio_|'s peer.
   static long BIOCallback(BIO *bio,
                           int cmd,
                           const char *argp, int argi, long argl,
                           long retvalue);
 
-  // Callback that is used to obtain information about the state of the SSL
-  // handshake.
-  static void InfoCallback(const SSL* ssl, int type, int val);
-
-  void CheckIfHandshakeFinished();
-
   // Adds the SignedCertificateTimestamps from ct_verify_result_ to |ssl_info|.
   // SCTs are held in three separate vectors in ct_verify_result, each
   // vetor representing a particular verification state, this method associates
   // each of the SCTs with the corresponding SCTVerifyStatus as it adds it to
   // the |ssl_info|.signed_certificate_timestamps list.
   void AddSCTInfoToSSLInfo(SSLInfo* ssl_info) const;
 
   bool transport_send_busy_;
   bool transport_recv_busy_;
 
@@ skipping 31 matching lines @@
 
   // Used by TransportReadComplete() to signify an error reading from the
   // transport socket. A value of OK indicates the socket is still
   // readable. EOFs are mapped to ERR_CONNECTION_CLOSED.
   int transport_read_error_;
 
   // Used by TransportWriteComplete() and TransportReadComplete() to signify an
   // error writing to the transport socket. A value of OK indicates no error.
   int transport_write_error_;
 
-  // Set when Connect finishes.
+  // Set when handshake finishes.
   scoped_ptr<PeerCertificateChain> server_cert_chain_;
   scoped_refptr<X509Certificate> server_cert_;
   CertVerifyResult server_cert_verify_result_;
-  bool completed_connect_;
+  bool completed_handshake_;

[davidben, 2015/03/09 18:02:36]

I think the LHS naming is actually more accurate in the face of False Start.
(Maybe even be worth an explanatory comment. Or we can leave that for another
CL.)

 
   // Set when Read() or Write() successfully reads or writes data to or from the
   // network.
   bool was_ever_used_;
 
   // Stores client authentication information between ClientAuthHandler and
   // GetSSLCertRequestInfo calls.
   bool client_auth_cert_needed_;
   // List of DER-encoded X.509 DistinguishedName of certificate authorities
   // allowed by the server.
   std::vector<std::string> cert_authorities_;
   // List of SSLClientCertType values for client certificates allowed by the
   // server.
   std::vector<SSLClientCertType> cert_key_types_;
 
   CertVerifier* const cert_verifier_;
   scoped_ptr<SingleRequestCertVerifier> verifier_;
   base::TimeTicks start_cert_verification_time_;
 
   // Certificate Transparency: Verifier and result holder.
   ct::CTVerifyResult ct_verify_result_;
   CTVerifier* cert_transparency_verifier_;
 
   // The service for retrieving Channel ID keys.  May be NULL.
   ChannelIDService* channel_id_service_;
 
-  // Callback that is invoked when the connection finishes.
-  //
-  // Note: this callback will be run in Disconnect(). It will not alter
-  // any member variables of the SSLClientSocketOpenSSL.
-  base::Closure handshake_completion_callback_;
-
   // OpenSSL stuff
   SSL* ssl_;
   BIO* transport_bio_;
 
   scoped_ptr<ClientSocketHandle> transport_;
   const HostPortPair host_and_port_;
   SSLConfig ssl_config_;
   // ssl_session_cache_shard_ is an opaque string that partitions the SSL
   // session cache. i.e. sessions created with one value will not attempt to
   // resume on the socket with a different value.
@@ skipping 11 matching lines @@
     STATE_VERIFY_CERT_COMPLETE,
   };
   State next_handshake_state_;
   NextProtoStatus npn_status_;
   std::string npn_proto_;
   // Written by the |channel_id_service_|.
   std::string channel_id_private_key_;
   std::string channel_id_cert_;
   // True if channel ID extension was negotiated.
   bool channel_id_xtn_negotiated_;
-  // True if InfoCallback has been run with result = SSL_CB_HANDSHAKE_DONE.
-  bool handshake_succeeded_;
-  // True if MarkSSLSessionAsGood has been called for this socket's
-  // SSL session.
-  bool marked_session_as_good_;

[davidben, 2015/03/09 18:02:36]

Fine to remove this now, but my session cache CL will probably bring some form
of it back. I think the verify + false start resolving should happen in
SSLClientSocketOpenSSL rather than in the session cache; the session cache will
just have an Insert method and it'll be the caller's responsibility to not
insert a session until both handshake and verification are done. Interface is
simpler that way.

(Also, I doubt we can actually change it in BoringSSL, but, in the False Start
case, I don't like SSL_get_session being called before the handshake is
completely finished. It gets into weirdnesses of, if there's a pending renego,
should SSL_get_session return the session we'd currently using or the session
for the current connection. With session tickets, you're even accessing the
SSL_SESSION before the ticket's filled in.)

   // The request handle for |channel_id_service_|.
   ChannelIDService::RequestHandle channel_id_request_handle_;
 
   TransportSecurityState* transport_security_state_;
 
   CertPolicyEnforcer* const policy_enforcer_;
 
   // pinning_failure_log contains a message produced by
   // TransportSecurityState::CheckPublicKeyPins in the event of a
   // pinning failure. It is a (somewhat) human-readable string.
   std::string pinning_failure_log_;
 
   BoundNetLog net_log_;
   base::WeakPtrFactory<SSLClientSocketOpenSSL> weak_factory_;
 };
 
 }  // namespace net
 
 #endif  // NET_SOCKET_SSL_CLIENT_SOCKET_OPENSSL_H_