Sign CertificateVerify messages on a background thread.

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 <openssl/base.h>
+#include <openssl/ssl.h>

[davidben, 2015/06/12 21:39:01]

base.h's main purpose in life is to forward-declare just about everything. But
ssl.h needs to be included because you can't forward-declare enums.

+
 #include <string>
+#include <vector>
 
 #include "base/compiler_specific.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "net/base/completion_callback.h"
 #include "net/base/io_buffer.h"
 #include "net/cert/cert_verifier.h"
 #include "net/cert/cert_verify_result.h"
 #include "net/cert/ct_verify_result.h"
 #include "net/socket/client_socket_handle.h"
 #include "net/socket/ssl_client_socket.h"
 #include "net/ssl/channel_id_service.h"
 #include "net/ssl/openssl_ssl_util.h"
 #include "net/ssl/ssl_client_cert_type.h"
 #include "net/ssl/ssl_config_service.h"
 #include "net/ssl/ssl_failure_state.h"
 
-// Avoid including misc OpenSSL headers, i.e.:
-// <openssl/bio.h>
-typedef struct bio_st BIO;
-// <openssl/evp.h>
-typedef struct evp_pkey_st EVP_PKEY;
-// <openssl/ssl.h>
-typedef struct ssl_st SSL;
-// <openssl/x509.h>
-typedef struct x509_st X509;
-// <openssl/ossl_type.h>
-typedef struct x509_store_ctx_st X509_STORE_CTX;
-
 namespace net {
 
 class CertVerifier;
 class CTVerifier;
 class SSLCertRequestInfo;
 class SSLInfo;
+class SSLPrivateKey;
 
 // An SSL client socket implemented with OpenSSL.
 class SSLClientSocketOpenSSL : public SSLClientSocket {
  public:
   // Takes ownership of the transport_socket, which may already be connected.
   // The given hostname will be compared with the name(s) in the server's
   // certificate during the SSL handshake.  ssl_config specifies the SSL
   // settings.
   SSLClientSocketOpenSSL(scoped_ptr<ClientSocketHandle> transport_socket,
                          const HostPortPair& host_and_port,
@@ skipping 76 matching lines @@
   void OnHandshakeIOComplete(int result);
   void OnSendComplete(int result);
   void OnRecvComplete(int result);
 
   int DoHandshakeLoop(int last_io_result);
   int DoReadLoop();
   int DoWriteLoop();
   int DoPayloadRead();
   int DoPayloadWrite();
 
+  // Runs both the Read and Write loops in response to an event that either or
+  // both may have been blocked on. This may occur during a renegotiation, at
+  // which point both state machines will block on the new handshake.
+  void RunReadWriteLoops();
+
   int BufferSend();
   int BufferRecv();
   void BufferSendComplete(int result);
   void BufferRecvComplete(int result);
   void TransportWriteComplete(int result);
   int TransportReadComplete(int result);
 
   // Callback from the SSL layer that indicates the remote server is requesting
   // a certificate for this client.
   int ClientCertRequestCallback(SSL* ssl);
@@ skipping 41 matching lines @@
   // the |ssl_info|.signed_certificate_timestamps list.
   void AddSCTInfoToSSLInfo(SSLInfo* ssl_info) const;
 
   // Returns a unique key string for the SSL session cache for
   // this socket.
   std::string GetSessionCacheKey() const;
 
   // Returns true if renegotiations are allowed.
   bool IsRenegotiationAllowed() const;
 
+  // Callbacks for operations with the private key.
+  static int PrivateKeyTypeCallback(SSL* ssl);
+  static int PrivateKeySupportsDigestCallback(SSL* ssl, const EVP_MD* md);
+  static size_t PrivateKeyMaxSignatureLenCallback(SSL* ssl);
+  static ssl_private_key_result_t PrivateKeySignCallback(SSL* ssl,
+                                                         uint8_t* out,
+                                                         size_t* out_len,
+                                                         size_t max_out,
+                                                         const EVP_MD* md,
+                                                         const uint8_t* in,
+                                                         size_t in_len);
+  static ssl_private_key_result_t PrivateKeySignCompleteCallback(
+      SSL* ssl,
+      uint8_t* out,
+      size_t* out_len,
+      size_t max_out);

[Ryan Sleevi, 2015/06/12 23:37:20]

The growing number of these callbacks makes me wonder why we wouldn't want to
just "struct CallbackHelper; friend struct CallbackHelper;" in the .h file, then
move all of the static callbacks to the .cc and have them reach into the
SSLClientSocketOpenSSL.

This seems like it'd avoid the forward declaration issue entirely, as well as
the need for all these method overloads.

struct CallbackHelper {
  static int PrivateKeyTypeCallback(SSL* ssl) {
    SSLClientSocketOpenSSL* that = ...;
    switch (that->private_key_->GetType()) {
      ...;
    }
  }
};

etc

[davidben, 2015/06/15 21:28:24]

Arguably that's what SSLContext is. I opted not to put them there because
SSLContext mostly forwards to SSLClientSocket private methods, which is
marginally tidier? Nice to only have one component reach into
SSLClientSocketOpenSSL members.

A previous version of this CL actually had those methods non-static and
SSLContext held the static ones, but I ran into visibility issues. (In
retrospect, that was probably because I didn't make
SSLContext::kPrivateKeyMethod public. Oh well.)

On the scale of important things, personally I don't really value "lets us
forward-declare stuff" very highly. C++ is, sadly, not really a language that
lets you hide the internals of a class from your header file because you need to
declare all the private bits. I feel like contortions to allow it just make
things messier because you have to fight against the visibility rules. Should
this header also never switch to ScopedSSL from net/ssl/scoped_openssl_types.h
for the sake of thinning includes?

In fact, the forward-declarations were masking a bug! The reason I needed to
change something in remoting/ was because it was including
ssl_client_socket_openssl.h in NSS ports all this time!

I think a better way to hide OpenSSL headers from consumers would be to use
SSLClientSocket, which is the interface consumers actually want anyway. That
SSLClientSocket{NSS,OpenSSL} have different constructors complicates this
though.

To cite our style guide:
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Forward_Decl...
- When using an ordinary class, relying on a forward declaration is OK, but be
wary of situations where a forward declaration may be insufficient or incorrect;
when in doubt, just #include the appropriate header.
- Do not replace data members with pointers just to avoid an #include

[Ryan Sleevi, 2015/06/15 22:25:22]

https://www.chromium.org/developers/coding-style
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts

[Ryan Sleevi, 2015/06/15 22:35:27]

Sorry, to be explicit: *This* reviewer highly values hiding via forward
declarations, and it's consistent (largely) with Chromium code. Personal views
of C++ not-withstanding, it's also generally held as good practice (Google's
IWYU tool, Lakos' Large Scale C++ Design, Meyers' Effective C++ series, etc).

It certainly offers real compile-time benefits for your coworkers as well :)

[davidben, 2015/06/15 22:37:44]

The Chromium one you cited also says:
I read both this and the google3 one to mean "DO forward-declare when you can,
DON'T contort the code just to forward-declare".

[davidben, 2015/06/15 22:53:36]

Also note, by the way, that this is the entirety of our source tree that
includes this header and would be affected. :-)

https://code.google.com/p/chromium/codesearch#search/&q=%23include.*ssl_clien...

I would argue that the latter is a problem with our "exposed" socket API. 

One option is we move that enum to openssl/base.h and we just include that one.
Part of openssl/base.h's purpose in life is to forward-declare the universe
anyway, so you DON'T have to know that it's

typedef struct evp_pkey_ctx_st EVP_PKEY_CTX; // normal
typedef struct AUTHORITY_KEYID_st AUTHORITY_KEYID; // caps
typedef struct asn1_string_st ASN1_INTEGER; // oh yeah, these are all the same
type
typedef struct env_md_st EVP_MD; // yes, really. env.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/boring...

(This isn't a BoringSSL innovation. Upstream called it ossl_typ.h.)

But even that seems fairly unnecessary.

ssl_client_socket_nss.h also happily includes NSS Headers, incidentally.

[Ryan Sleevi, 2015/06/15 23:02:06]

Except we're not doing that here. And as the C++ Do's and Don'ts points out (and
why it was created) was to show how many places you can and should be forward
declaring.
I guess this relies on your value judgement of "contort the code" with the cost
of exposing your internal details.

The same page (C++'s dos and don'ts) lists 

"Move inner classes into the implementation." and "Move static implementation
details to the implementation whenever possible.", both of which apply here.

+
+  void OnPrivateKeySignComplete(Error error,
+                                const std::vector<uint8_t>& signature);
+
   bool transport_send_busy_;
   bool transport_recv_busy_;
 
   // Buffers which are shared by BoringSSL and SSLClientSocketOpenSSL.
   // GrowableIOBuffer is used to keep ownership and setting offset.
   scoped_refptr<GrowableIOBuffer> send_buffer_;
   scoped_refptr<GrowableIOBuffer> recv_buffer_;
 
   CompletionCallback user_connect_callback_;
   CompletionCallback user_read_callback_;
@@ skipping 88 matching lines @@
   // True if a channel ID was sent.
   bool channel_id_sent_;
   // True if the initial handshake has completed.
   bool handshake_completed_;
   // True if the initial handshake's certificate has been verified.
   bool certificate_verified_;
   // The request handle for |channel_id_service_|.
   ChannelIDService::Request channel_id_request_;
   SSLFailureState ssl_failure_state_;
 
+  scoped_ptr<SSLPrivateKey> private_key_;
+  int signature_result_;
+  std::vector<uint8_t> signature_;
+
   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_