Support for client certs in ssl_server_socket.

Support for client certs in ssl_server_socket.

Allows SSL server sockets (openssl and NSS) to support requesting and
receiving/verifying client auth certificates.

BUG=415663

[dougsteed, 2015-03-09 22:36:32]

dougsteed@chromium.org changed reviewers:
+ davidben@chromium.org, rsleevi@chromium.org

[dougsteed, 2015-03-18 14:02:39]

dougsteed@chromium.org changed required reviewers:
+ davidben@chromium.org

[dougsteed, 2015-03-18 14:02:39]

See crbug/415663.

[Ryan Sleevi, 2015-03-19 04:38:25]

David needs to take a more thorough look.

In particular, it feels like there's several sort of "just get it working" bits
that I believe David has a much cleaner vision for and can elaborate on. Because
nothing in //net (beyond cast) is using client auth, my fear is that if we do
this CL as is, those won't get done, so I'd rather it "done right" now that we
have need to do it at all. In particular, it looks like cleaning up how Cert/Key
binding is done is both needed for unit testing and to make these interfaces
cleaner.

https://codereview.chromium.org/994743003/diff/1/net/cert/mock_cert_verifier.cc
File net/cert/mock_cert_verifier.cc (right):

https://codereview.chromium.org/994743003/diff/1/net/cert/mock_cert_verifier....
net/cert/mock_cert_verifier.cc:47: if (cert) {
Why this change? The API contract of CertVerifier does not allow |cert| to be
NULL.

https://codereview.chromium.org/994743003/diff/1/net/socket/nss_ssl_util.cc
File net/socket/nss_ssl_util.cc (right):

https://codereview.chromium.org/994743003/diff/1/net/socket/nss_ssl_util.cc#n...
net/socket/nss_ssl_util.cc:423: int* ssl_connection_status) {
This method fails to properly wipe |ssl_connection_status| between calls.

It'd be cleaner just to use a stack temp and assign, and then let the compiler
optimize as it's permitted to.

https://codereview.chromium.org/994743003/diff/1/net/socket/nss_ssl_util.h
File net/socket/nss_ssl_util.h (right):

https://codereview.chromium.org/994743003/diff/1/net/socket/nss_ssl_util.h#ne...
net/socket/nss_ssl_util.h:47: void UpdateSSLConnectionStatus(PRFileDesc* nss_fd,
I'm generally uneasy with moving this into a common header; there are //net
exported bits here.

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_client_socket.h
File net/socket/ssl_client_socket.h (right):

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket.h:18: class RSAPrivateKey;
I really don't want to force a dependency on //crypto/RSAPrivateKey. I don't
think it's a good interface fit.

David?

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket.h:242: virtual void
ForceClientCertificateAndKeyForTest(
ForTesting, to get the presubmit

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket.h:243: scoped_refptr<X509Certificate> client_cert,
STYLE: const scoped_refptr<X509Certificate>&

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket.h:244: scoped_ptr<crypto::RSAPrivateKey>
client_private_key) {}
It's not clear to me that this method is needed/appropriate, given that the
SSLConfig contains the client cert.

If you're trying to avoid the OS-key dependency, you should likely work with
David on his proposed refactoring of those bits as appropriate.

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1820: if (client_private_key_.get()) {
STYLE: Don't use .get() for scoped_ptr bool testing. 

if (client_private_key_) {

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1825: }
STYLE: no braces for single-line if
STYLE: nullptr ?

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1830: // net/ client auth API lacking a
private key handle.
STYLE: This comment style is ugly.

If nececessary, force a newline between 1830 and 1831 to stop clang-format from
doing that ugliness.

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket.h
File net/socket/ssl_server_socket.h (right):

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket.h:27: typedef
std::vector<scoped_refptr<X509Certificate>> CertificateList;
ODR VIOLATION: Don't do duplicate typedefs like this.

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket.h:41: virtual void SetAllowClientCert(bool
allow_client_cert) = 0;
The client is not allowed to presumptively send a client cert w/o a request, so
naming wise, this doesn't make sense.

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket.h:47: const CertificateList& client_cert_ca_list) =
0;
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.

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket.h:68: virtual void
SetClientCertVerifier(CertVerifier* client_cert_verifier) = 0;
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]

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
File net/socket/ssl_server_socket_nss.cc (right):

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket_nss.cc:328: bool
SSLServerSocketNSS::GetSSLInfo(SSLInfo* ssl_info) {
I'm vaguely uncomfortable with overloading SSLInfo to represent client cert
details now

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket_nss.cc:332: }
No braces

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket_nss.cc:336: ssl_info->client_cert_sent =
client_cert_.get() ? true : false;
no .get()

ssl_info->client_cert_sent = client_cert_;

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket_nss.cc:354: return true;
Feels pretty unreadable/ wall of text. Break it into logical bits? (eg: line
breaks at 337, 347, 354; 335 should go to 333, with a linebreak between
ExtractClientCert and the two client cert methods)

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket_nss.cc:550: it != client_cert_ca_list_.end(); ++it)
{
for (const auto& certificate : client_cert_ca_list_)
  CERT_AddCertToListTail(certs,
CERT_DupCertificate(certificate->os_cert_handle()));

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket_nss.cc:903: }
no braces

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket_nss.cc:931: CERTCertList* list =
SSL_PeerCertificateChain(nss_fd_);
Use a scoped type helper

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
File net/socket/ssl_server_socket_openssl.cc (right):

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket_openssl.cc:30: // Might want to consider putting
them in a common place.
Yes. Copy pasta bad.

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket_openssl.cc:44: #if OPENSSL_VERSION_NUMBER <
0x1000103fL
Does this matter at all David?

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket_openssl.cc:79: bool GetX509AsDER(X509* cert,
base::StringPiece* sp) {
name better

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket_openssl.cc:389: 
Much more readable construction ;)

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket_openssl.cc:845: for (CertificateList::iterator it =
client_cert_ca_list_.begin();
for (const auto& certificate : client_cert_ca_list_) {
  X509* ca_cert = certificate->os_cert_handle();
  ...
}

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
File net/socket/ssl_server_socket_unittest.cc (right):

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket_unittest.cc:395: }
no braces

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket_unittest.cc:424: const char* kClientCertFileName =
"client_1.pem";
STYLE:

const char kClientCertFilename[] =


But really, move these out of the class and into the unnamed namespace.

https://codereview.chromium.org/994743003/diff/1/net/ssl/ssl_cert_request_info.h
File net/ssl/ssl_cert_request_info.h (right):

https://codereview.chromium.org/994743003/diff/1/net/ssl/ssl_cert_request_inf...
net/ssl/ssl_cert_request_info.h:23: // In TLS 1.1, the CertificateRequest
Aside (Unrelated to this CL, strictly speaking): This should be updated for TLS
1.2

In which SupportedSignatureAlgorithms is part of the CertificateRequest

https://tools.ietf.org/html/rfc5246#section-7.4.4

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

I haven't given anywhere near as close of a look at this as Ryan has, but given
that both halves of this CL (make it possible to drive the client half in tests,
and additional APIs to SSLServerSocket) are both placeholder-type things and
will need considerable work later, it might be best to hold on this until we're
ready for it.

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_client_socket.h
File net/socket/ssl_client_socket.h (right):

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket.h:18: class RSAPrivateKey;
On 2015/03/19 04:38:24, Ryan Sleevi wrote:
> I really don't want to force a dependency on //crypto/RSAPrivateKey. I don't
> think it's a good interface fit.
> 
> David?

Eventually SSLConfig will take both a net::X509Certificate and a <some handle to
a private key>. I hadn't thought too hard about exactly what that handle should
be. crypto::RSAPrivateKey is certainly right out since we do ECDSA as well.
crypto::ScopedEVP_PKEY is a possibility. As is some kind of common base between
crypto::RSAPrivateKey and crypto::ECPrivateKey.

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1819: crypto::ScopedEVP_PKEY privkey;
This entire mess should be EVP_PKEY_dup (which bumps the reference count).

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket.h
File net/socket/ssl_server_socket.h (right):

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket.h:27: typedef
std::vector<scoped_refptr<X509Certificate>> CertificateList;
On 2015/03/19 04:38:24, Ryan Sleevi wrote:
> ODR VIOLATION: Don't do duplicate typedefs like this.

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

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket.h:38: 
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.

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

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket.h
File net/socket/ssl_server_socket.h (right):

https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket...
net/socket/ssl_server_socket.h:27: typedef
std::vector<scoped_refptr<X509Certificate>> CertificateList;
On 2015/03/25 00:05:33, David Benjamin wrote:
> That's actually how we forward-declare that typedef. It drives me crazy too.
:-/

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.

[dougsteed, 2015-11-17 17:38:36]

dougsteed@chromium.org changed reviewers:
- rsleevi@chromium.org

[dougsteed, 2015-11-17 17:38:36]

Ryan Chung will be taking over this CL. The current upload is not intended for
further review by the designated reviewers (and does not yet respond to many of
their prior comments).

[ryanchung, 2015-11-25 22:46:58]

ryanchung@chromium.org changed reviewers:
+ rsleevi@chromium.org

[ryanchung, 2015-11-25 22:47:17]

I couldn't upload a new patch to this issue because I don't own this issue.
Please continue the review on this new code review:
https://codereview.chromium.org/1474983003