Support for client certs in ssl_server_socket.

net/socket/ssl_server_socket.h

Index: net/socket/ssl_server_socket.h
diff --git a/net/socket/ssl_server_socket.h b/net/socket/ssl_server_socket.h
index 88f7f94143956764cb67b32dc2e7667136b38b9f..708065d4bf68dd51f4ef9eb35b5b8107a3723e65 100644
--- a/net/socket/ssl_server_socket.h
+++ b/net/socket/ssl_server_socket.h
@@ -5,12 +5,15 @@
 #ifndef NET_SOCKET_SSL_SERVER_SOCKET_H_
 #define NET_SOCKET_SSL_SERVER_SOCKET_H_
 
+#include <vector>
+
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
 #include "net/base/completion_callback.h"
 #include "net/base/net_export.h"
 #include "net/socket/ssl_socket.h"
 #include "net/socket/stream_socket.h"
+#include "net/ssl/ssl_client_cert_type.h"
 
 namespace crypto {
 class RSAPrivateKey;
@@ -18,8 +21,10 @@ class RSAPrivateKey;
 
 namespace net {
 
+class CertVerifier;
 struct SSLConfig;
 class X509Certificate;
+typedef std::vector<scoped_refptr<X509Certificate>> CertificateList;

[Ryan Sleevi, 2015/03/19 04:38:24]

ODR VIOLATION: Don't do duplicate typedefs like this.

[davidben, 2015/03/25 00:05:33]

That's actually how we forward-declare that typedef. It drives me crazy too. :-/

[Ryan Sleevi, 2015/03/25 00:14:08]

Any place that is doing that is buggy; this isn't forward declaring, this is
duplicate definitions, and can/will cause issue if ever both headers are
present.

This is one type that you can't forward declare.

 
 class SSLServerSocket : public SSLSocket {
  public:
@@ -30,6 +35,37 @@ class SSLServerSocket : public SSLSocket {
   // completion then the callback will be silently, as for other StreamSocket
   // calls.
   virtual int Handshake(const CompletionCallback& callback) = 0;
+

[davidben, 2015/03/25 00:05:33]

These APIs are very dangerous because of the global server session cache. If you
handshake *any* SSLServerSocket without, say, calling SetClientCertVerifier,
then your session cache will contain valid sessions that weren't authenticated.
Those sessions will be happily resumed and this API won't know any better.

It's likely we should hold on officially supporting too much crazy stuff in
SSLServerSocket before we can ditch the NSS halves of it. It's really a bad API
in general because of how global everything is. Right now, I can't see any
workable API outside of something like global, and that's kind of crazy.

+  // Indicates whether a client certificate is to be allowed by the upcoming
+  // Handshake.
+  virtual void SetAllowClientCert(bool allow_client_cert) = 0;

[Ryan Sleevi, 2015/03/19 04:38:24]

The client is not allowed to presumptively send a client cert w/o a request, so
naming wise, this doesn't make sense.

+
+  // Provides the list of certificates whose names are to be included in the
+  // CertificateRequest handshake message. Calling this function is only useful
+  // if certificates are allowed.
+  virtual void SetClientCertCAList(
+      const CertificateList& client_cert_ca_list) = 0;

[Ryan Sleevi, 2015/03/19 04:38:24]

I'd prefer this actually be part of the construction requirements or in the call
to Handshake.

In particular, I'm not keen to introduce 3 new methods that "Must" be called
before Handshake.

+
+  // Indicates that a client certificate is not only allowed but required, and
+  // provides the CertificateVerifier that is to be used to verify it during the
+  // handshake. The |client_cert_verifier| continues to be owned by the caller,
+  // and must exist at least until the handshake has completed.
+  // This function is meaningful only if client certificates are allowed.
+  // NOTES:
+  // 1. If no CertificateVerifier is provided, then a client certificate may
+  // still be allowed (if ssl_config.send_client_cert is true), but in that case
+  // verification must be done after the handshake has completed, by which time
+  // the session will have been cached, and may be subject to resumption.
+  // 2. The |client_cert_verifier| must provide its response synchronously, and
+  // blocks the IO thread while it runs. This results from a limitation of NSS.
+  // If ERR_IO_PENDING is returned, this is considered a verification failure.
+  // 3. For verifying a client certificate, the CertVerifier::Verify method
+  // will be called with input parameters as follows:
+  //    - cert: the cert to be verified
+  //    - hostname: empty string
+  //    - flags: 0
+  //    - crl_set: NULL
+  virtual void SetClientCertVerifier(CertVerifier* client_cert_verifier) = 0;

[Ryan Sleevi, 2015/03/19 04:38:24]

From a design level, I don't think I'm comfortable with this re-use of
CertVerifier.

CertVerifier is, from a design perspective, scoped to SSL server certs, and I
don't want to introduce an API that says it's acceptable for client certs, at
least not without means to distinguish intent.

I'd prefer for your use case you make this a new, explicit interface,
"ClientCertVerifier", with a Verify() that is minimally sufficient for your
needs. As we refine the PKI code, I think we can revisit how "Verify for purpose
X" should be best expressed at an API level, but I do think it's a bad thing to
conflate the two with semantically and syntactically equivalent overloaded
calls.

[Yes, I'm intentionally not suggesting renaming CertVerifier ->
ServerCertVerifier, nor renaming Verify() to VerifyServer() and adding a
VerifyClient(); I think there's a number of approaches to possible designs here,
and I'm trying to avoid having to weigh all of those considerations as a blocker
for this CL; thus the easiest and most expedient solution is to introduce the
additional class, which will see virtually no use in Chrome, and then revisit
the desired long-term design as time permits]

 };
 
 // Configures the underlying SSL library for the use of SSL server sockets.