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>
+
 #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();

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

SSLClientSocketNSS calls this DoTransportIO, which I think is less-ambiguous
here.

(That is, it's unclear whether the ReadWrite loop being run is against the SSL
socket state machine or against the transport socket state machine, or how this
relates to DoReadLoop/DoWriteLoop)

[davidben, 2015/06/17 20:47:02]

[Terminology: if I ever say "Read" or "Write", I specifically mean
SSLClientSocket::Read and SSLClientSocketWrite, not Read or Write on the
underlying transport socket. That's "transport Read" and "transport Write".]

This is not DoTransportIO. DoTransportIO even exists here. DoTransportIO *only*
pumps the transport socket state machines (both transport Read and transport
Write). It is not safe to call it standalone as it relies on its caller to pump
the DoPayloadRead state machine AND call the Read callback.

DoReadLoop (respectively, DoWriteLoop) pumps the PayloadRead (respectively,
PayloadWrite) state machine together with DoTransportIO. It is not safe to call
it standalone as it relies on its caller to call the Read (respectively, Write)
callback if progress is made.

You can see this come together in OnRecvComplete where we finally bind together
low-level transport read signal, pump PayloadRead and down via DoReadLoop, and
call the read callback.

Likewise, there is the OnSendComplete one which should just call DoWriteLoop
*except for renego*. And so it needs to pump both DoReadLoop and DoWriteLoop in
parallel, which is this method.

OnPrivateKeySignComplete, being another event which may resume a handshake, has
the same property. It can resume EITHER Read or Write and so it needs that same
code. So I factored it into a method.

Really the root problem here is that this state machine is patterned after
another state machine which, if you follow the chain far enough, comes from a
world where we didn't have renego and SSL sockets were actually full-duplex. If
I'm doing my archeology right, it came from the SChannel implementation. Renego
breaks this and APIs with implicit renego (SChannel's renego is, sensibly,
explicit) makes it so that we can't maintain this kind of separation that we do.

A possible solution, if you really don't like RunReadWriteLoops:

OnRecvComplete isn't right either! It actually also needs to call
RunReadWriteLoops! SSL_write can block on read just as easily as SSL_read block
on write. But I believe any case where this can cause problems is also one where
renego doesn't happen in a quiescent state of the upper-level protocol which is
explicitly not supported by BoringSSL.

The situation is if the last thing that happened in the handshake was a read (so
full handshake) and we have both Read and Write in flight. This combination gets
stuck. Specifically:

  1. Consumer calls SSLClientSocket::Read.
  2. HelloRequest comes in over the transport. We enter renego.
  3. Consumer calls SSLClientSocket::Write.
  4. SSL_write blocks on the handshake completing.
  5. The handshake completes and the server Finished comes in over the
transport.
  6. OnSendComplete pumps DoReadLoop which calls SSL_read and completes the
handshake. It then tries to read from the transport again, but no app data has
arrived yet, so the Read callback never signals.
  7. OnSendComplete forgets to pump DoWriteLoop and so SSL_write never notices
that it can send out its payload now. We're now stuck.

However, in this situation, there cannot be an ordering relation steps 2 and 3.
When 2 happens, nothing is signaled outside of SSLClientSocket so, unless the
consumer is interposing on the transport socket, 2 could not have triggered 3
happening. They happened independently and so this order could have happened:

  1. Consumer calls SSLClientSocket::Read.
  2. Consumer calls SSLClientSocket::Write
  3. SSL_write gets partway through dumping data to the transport and has to
pause. BoringSSL's state machine has a half-written record.
  4. HelloRequest comes in over the transport. We enter renego.

This situation is explicitly not supported by BoringSSL. We will abort the
handshake if it happens. This isn't a situation that can occur with the client
auth hack. (The alternative is some very messy and questionable logic in OpenSSL
where it defers renego until both buffers happen it be empty. OpenSSL's state
machine is not capable of dispatching between two subprotocols writing records
at once. It instead has some ad-hoc code for alerts which are otherwise the only
thing that does this.)

So, one thing we could do, to fix the "SSL_write may block on renego" problem is
to check SSL_in_init before calling SSL_write. If yes, we error. Any situation
where this may happen already fails racily. This would mean that the
OnRecvComplete bug above is fixed and OnPrivateKeySignComplete can just call
DoReadLoop + DoReadCallback because only Connect and Read may block on
handshakes.

Confused yet? :-) (I have not been idly crusading against renego and cutting it
down to just the one use case we need. Even what complexity we did have for it
was subtly wrong and not complex enough.)

+
   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);
+
+  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_;