Sign CertificateVerify messages on a background thread.

net/socket/ssl_client_socket_openssl.h

Index: net/socket/ssl_client_socket_openssl.h
diff --git a/net/socket/ssl_client_socket_openssl.h b/net/socket/ssl_client_socket_openssl.h
index fd7a68a3ea29f6e1484ea4168a2923fbd64d87a0..02600a9b2d748322e74704607b7c9bb523ba9337 100644
--- a/net/socket/ssl_client_socket_openssl.h
+++ b/net/socket/ssl_client_socket_openssl.h
@@ -5,7 +5,11 @@
 #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"
@@ -23,24 +27,13 @@
 #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 {
@@ -137,6 +130,11 @@ class SSLClientSocketOpenSSL : public SSLClientSocket {
   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);
@@ -198,6 +196,26 @@ class SSLClientSocketOpenSSL : public SSLClientSocket {
   // 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_;
 
@@ -306,6 +324,10 @@ class SSLClientSocketOpenSSL : public SSLClientSocket {
   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_;