Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1474983003:
Support for client certs in ssl_server_socket. (Closed)
DescriptionSupport for client certs in ssl_server_socket.
Allows SSL server sockets (openssl only) to support requesting and
receiving/verifying client auth certificates.
BUG=415663
Committed: https://crrev.com/987b2ffd6da82a8af2d7dce89db5ec8e372702df
Cr-Commit-Position: refs/heads/master@{#376292}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Comment suggested changes #Patch Set 3 : Rebased #Patch Set 4 : Simplified CertVerifyCallback #
Total comments: 8
Patch Set 5 : Fixed nits on utils #
Total comments: 96
Patch Set 6 : Addresses reviewer comments #
Total comments: 51
Patch Set 7 : Addresses reviewer comments #Patch Set 8 : Minor spacing #Patch Set 9 : Addresses reviewer comments #Patch Set 10 : Rebase only #Patch Set 11 : Rebase only #
Total comments: 99
Patch Set 12 : Addresses reviewer comments #Patch Set 13 : Free results from SSL_load_client_CA_file #
Total comments: 28
Patch Set 14 : Addresses reviewer comments #Patch Set 15 : Rebased #Patch Set 16 : Use new Test SSL private key #Patch Set 17 : Updated browsertest files #
Total comments: 24
Patch Set 18 : Fixed nits #Dependent Patchsets: Messages
Total messages: 36 (10 generated)
Description was changed from ========== 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 ========== to ========== 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 This is a continuation of issue 994743003 due to change of committer. https://codereview.chromium.org/994743003/ ==========
ryanchung@chromium.org changed reviewers: + davidben@chromium.org, dougsteed@chromium.org, rsleevi@chromium.org
This is continuation of issue: https://codereview.chromium.org/994743003/ I couldn't upload a new patch to the existing issue because I don't own this issue.
Description was changed from ========== 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 This is a continuation of issue 994743003 due to change of committer. https://codereview.chromium.org/994743003/ ========== to ========== Support for client certs in ssl_server_socket. Allows SSL server sockets (openssl only) to support requesting and receiving/verifying client auth certificates. BUG=415663 This is a continuation of issue 994743003 due to change of committer. https://codereview.chromium.org/994743003/ ==========
ryanchung@chromium.org changed reviewers: + svaldez@chromium.org
https://codereview.chromium.org/1474983003/diff/1/net/socket/ssl_client_socket.h File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/1474983003/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket.h:189: scoped_ptr<SSLPrivateKey> client_private_key) {} This should no longer be necessary if you rebase. https://codereview.chromium.org/1474983003/diff/1/net/socket/ssl_server_socket.h File net/socket/ssl_server_socket.h (right): https://codereview.chromium.org/1474983003/diff/1/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:30: struct SSLServerSocketContext { SSLClientSocketContext was kind of absurd to begin with and causes problems. We need to fix that one. Don't use it as something to pattern against. If you're going to go add an SSLServerSocketContext now, let's try to get a more sensible API. (Otherwise, we pass ClientCertVerifier into CreateSSLServerSocket and resolve the SSLServerSocketContext mess later as a blocker for adding a session cache.) I was thinking that SSLServerSocketContext is a type you scoped_ptr and it takes SSLServerConfig, ClientCertVerifier, certificate, key, etc. (Or maybe we just put all of them into SSLServerConfig, I dunno.) Then rather than have this global EnableSSLServerSockets and global CreateSSLServerSocket, SSLServerSocketContext has a CreateServerSocket method that takes just the StreamSocket. *Then* we won't have any problems adding a session cache or whatever else because SSLServerSocketContextOpenSSL will just have its own SSL_CTX with its own session cache. https://codereview.chromium.org/1474983003/diff/1/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:46: // If ERR_IO_PENDING is returned, this is considered a verification failure. This isn't even implemented in NSS, no? https://codereview.chromium.org/1474983003/diff/1/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:52: // - crl_set: NULL ?
https://codereview.chromium.org/1474983003/diff/1/net/socket/ssl_client_socket.h File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/1474983003/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket.h:189: scoped_ptr<SSLPrivateKey> client_private_key) {} On 2015/12/01 22:35:17, davidben (behind on reviews) wrote: > This should no longer be necessary if you rebase. Awesome. Done. https://codereview.chromium.org/1474983003/diff/1/net/socket/ssl_server_socket.h File net/socket/ssl_server_socket.h (right): https://codereview.chromium.org/1474983003/diff/1/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:30: struct SSLServerSocketContext { On 2015/12/01 22:35:17, davidben (behind on reviews) wrote: > SSLClientSocketContext was kind of absurd to begin with and causes problems. We > need to fix that one. Don't use it as something to pattern against. If you're > going to go add an SSLServerSocketContext now, let's try to get a more sensible > API. (Otherwise, we pass ClientCertVerifier into CreateSSLServerSocket and > resolve the SSLServerSocketContext mess later as a blocker for adding a session > cache.) > > I was thinking that SSLServerSocketContext is a type you scoped_ptr and it takes > SSLServerConfig, ClientCertVerifier, certificate, key, etc. (Or maybe we just > put all of them into SSLServerConfig, I dunno.) > > Then rather than have this global EnableSSLServerSockets and global > CreateSSLServerSocket, SSLServerSocketContext has a CreateServerSocket method > that takes just the StreamSocket. > > *Then* we won't have any problems adding a session cache or whatever else > because SSLServerSocketContextOpenSSL will just have its own SSL_CTX with its > own session cache. I think putting it with SSLServerConfig seem to make sense... at least for now. https://codereview.chromium.org/1474983003/diff/1/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:46: // If ERR_IO_PENDING is returned, this is considered a verification failure. On 2015/12/01 22:35:17, davidben (behind on reviews) wrote: > This isn't even implemented in NSS, no? Sorry, that was outdated. I believe OpenSSL also requires the certification verification to be completed synchronously. Bug: 347402 Updated comment. https://codereview.chromium.org/1474983003/diff/1/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:52: // - crl_set: NULL On 2015/12/01 22:35:17, davidben (behind on reviews) wrote: > ? Sorry, that was outdated. Fixed.
Could you please take another look at this CL when you get a chance? Thanks!
Minor nits on the utils. On a first pass the client vert verification looks reasonable, but I'll take another look. https://codereview.chromium.org/1474983003/diff/60001/net/ssl/openssl_ssl_uti... File net/ssl/openssl_ssl_util.cc (right): https://codereview.chromium.org/1474983003/diff/60001/net/ssl/openssl_ssl_uti... net/ssl/openssl_ssl_util.cc:262: int GetNetSSLVersion(SSL* ssl) { Can we not use TLS1_1_VERSION, TLS1_2_VERSION in the switch? https://codereview.chromium.org/1474983003/diff/60001/net/ssl/openssl_ssl_uti... net/ssl/openssl_ssl_util.cc:279: #else // !defined(USE_OPENSSL_CERTS) nit: #else //... (2 spaces)
https://codereview.chromium.org/1474983003/diff/60001/net/ssl/openssl_ssl_uti... File net/ssl/openssl_ssl_util.cc (right): https://codereview.chromium.org/1474983003/diff/60001/net/ssl/openssl_ssl_uti... net/ssl/openssl_ssl_util.cc:262: int GetNetSSLVersion(SSL* ssl) { On 2015/12/14 22:05:41, svaldez wrote: > Can we not use TLS1_1_VERSION, TLS1_2_VERSION in the switch? Done. https://codereview.chromium.org/1474983003/diff/60001/net/ssl/openssl_ssl_uti... net/ssl/openssl_ssl_util.cc:279: #else // !defined(USE_OPENSSL_CERTS) On 2015/12/14 22:05:41, svaldez wrote: > nit: #else //... (2 spaces) Done. Every time I run git cl format, it adds the 3rd space in for some reason.
https://codereview.chromium.org/1474983003/diff/60001/net/cert/client_cert_ve... File net/cert/client_cert_verifier.h (right): https://codereview.chromium.org/1474983003/diff/60001/net/cert/client_cert_ve... net/cert/client_cert_verifier.h:16: virtual ~ClientCertVerifier() {} Nit: I'd put a newline here. https://codereview.chromium.org/1474983003/diff/60001/net/cert/mock_client_ce... File net/cert/mock_client_cert_verifier.cc (right): https://codereview.chromium.org/1474983003/diff/60001/net/cert/mock_client_ce... net/cert/mock_client_cert_verifier.cc:26: for (it = rules_.begin(); it != rules_.end(); ++it) { This is a little clearer as: for (const Rule& rule : rules_) { // Check just the server cert. Intermediates will be ignored. if (rule.cert->Equals(cert)) return rule.rv; } // Fall through to the default. return default_result_; https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... File net/socket/ssl_server_socket.h (right): https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket.h:8: #include <vector> Not used? https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:30: bool GetDERFromX509(X509* cert, base::StringPiece* sp) { Use net::x509_util::GetDER from x509_util_openssl.h. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:31: unsigned char* cert_data = NULL; uint8_t https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:65: client_cert = NULL; nullptr https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:302: ssl_server_config_.require_client_cert && client_cert_.get(); Just client_cert_.get() seems right. Alternatively, I might just leave it false since we didn't send a client cert. We received one. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:650: // the cert changed during a renego. Want to file a bug about this? This is sort of a silly situation and seems fixable. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:797: ssl_->verify_mode |= SSL_VERIFY_FAIL_IF_NO_PEER_CERT; Please don't reach into BoringSSL's internal structs. Call the public APIs. https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#C... Also why are these separate from line 701? Seems you could configure it on the SSL_CTX just fine too. (Also I suspect you want SSL_VERIFY_FAIL_IF_NO_PEER_CERT in the require_client_cert case too. Rejecting that was probably an oversight.) https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:815: return; Rather than being a function that caches stuff and has to make sure it's called at the right time, just extract the certificate once when the handshake is done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:818: STACK_OF(X509)* chain = SSL_get_peer_cert_chain(ssl_); NB: This is NOT the verified chain. It is the list of certificates that the client sent you that may or may not be the chain your verifier ended up building. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:826: DCHECK(verifier); This DCHECK is false. You sometimes install NULL ones. I'd replace it with a comment: // If no ClientCertVerifier was supplied, all certificates are accepted. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:833: STACK_OF(X509)* chain = store_ctx->chain; I believe this is always NULL. You want store_ctx->untrusted because you have not verified a chain yet at this point. Also, store_ctx->untrusted already includes the leaf. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:840: X509_STORE_CTX_set_error(store_ctx, X509_V_ERR_CERT_REJECTED); Optional: If you also want to preserve res, OpenSSLPutNetError will make the error code come back out of MapOpenSSLErrorWithDetails. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:123: new DrainableIOBuffer(new StringIOBuffer(std::string("0", 1)), 1)); What's this for? https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:273: class TestSSLPrivateKey : public SSLPrivateKey { (Ooh, thanks! I might pull this into a utility later to try to get our client auth tests going.) https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:290: std::vector<uint8> public_key_info; New code should use uint8_t. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:294: result <<= 1; This also assumes the exponent is small and that the DER wrapping bits aren't far off. It'd be nice if we could do this without reading into these, but: #if defined(USE_OPENSSL) return EVP_PKEY_size(rsa_private_key_->key()); #else NOTIMPLEMENTED(); return 0; #endif https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:324: void CompleteSignDigest(Error err, const std::vector<uint8_t>& signature) {} Unused? https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:324: void CompleteSignDigest(Error err, const std::vector<uint8_t>& signature) {} Nit: newline https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:374: enum ClientCertSupply { Sort of an odd name. Why not just call it ClientCert or ClientCertificate or something? https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:375: kNoneSupplied = 0, This one is never used. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:377: kWrongCertSupplied = 2 enums NAMED_LIKE_MACROS https://www.chromium.org/developers/coding-style#Naming https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:377: kWrongCertSupplied = 2 Why specify values? I don't think you actually care about the values. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:380: enum ClientCertExpect { kNoneExpected = 0, kCertRequired = 2 }; Ditto to both. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:388: client_cert_verifier_->set_default_result(CERT_STATUS_AUTHORITY_INVALID); ERR_CERT_AUTHORITY_INVALID. It looks like the line above had this bug too. set_default_result takes a net error code, not a CertStatus. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:426: void InitializeClientCertsForClient(ClientCertSupply supply) { This all seems like it ought to use a switch-case. It means that when you add a new entry, the compiler will notice. Alternatively, perhaps just have it take two const char*s and pass in the file names? https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:426: void InitializeClientCertsForClient(ClientCertSupply supply) { Calling this method and the one below InitializeBlah is confusing since you have to then call plain Initialize. ConfigureClientCertsForClient? https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:450: if (expect == kCertRequired) { This is weird. There's only two values for expect. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:458: CertVerifyResult ignored; What's this for? https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:471: std::string unneeded; Nit: unneeded -> unused is I think more common? https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:485: std::vector<uint8> key_vector( New code should use uint8_t. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:559: #if defined(USE_OPENSSL) || !defined(NSS_PLATFORM_CLIENT_AUTH) This comment is out of date. I think you just want #if defined(USE_OPENSSL) now. Probably something as simple as // NSS ports don't support client certificates. (The only NSS port remaining is iOS which doesn't do client auth at all.) https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:601: // get SSL_CLIENT_AUTH_CERT_NEEDED. This code path allows us to access the ERR_SSL_CLIENT_AUTH_CERT_NEEDED https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:607: EXPECT_TRUE(server_ret == ERR_IO_PENDING); EXPECT_EQ https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:616: } This whole thing (line 609 down) can be written: EXPECT_EQ(ERR_SSL_CLIENT_AUTH_CERT_NEEDED, connect_callback.GetResult( client_socket_->Connect(connect_callback.callback()))); https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:633: } This should probably be: server_ret = handshake_callback.GetResult(server_ret); EXPECT_TRUE(.....); Why do you get the different error codes? Could you add a comment for why there's two possibilities? https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:655: } Ditto on using GetResult. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:657: server_ret = handshake_callback.WaitForResult(); Ditto on using GetResult. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:663: server_ret == ERR_SSL_SERVER_CERT_CHANGED); It seems this has been fixed, right? https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:665: #endif // defined(USE_OPENSSL) || !defined(NSS_PLATFORM_CLIENT_AUTH) (Don't forget to remove NSS_PLATFORM_CLIENT_AUTH here too.) https://codereview.chromium.org/1474983003/diff/80001/net/ssl/openssl_ssl_util.h File net/ssl/openssl_ssl_util.h (right): https://codereview.chromium.org/1474983003/diff/80001/net/ssl/openssl_ssl_uti... net/ssl/openssl_ssl_util.h:75: using ScopedX509Name = crypto::ScopedOpenSSL<X509_NAME, X509_NAME_free>; Nit: For consistency with other scopers, lets call this ScopedX509_NAME. https://codereview.chromium.org/1474983003/diff/80001/net/ssl/openssl_ssl_uti... net/ssl/openssl_ssl_util.h:78: crypto::ScopedOpenSSL<STACK_OF(X509_NAME), FreeX509NameStack>; I'd put these in net/ssl/scoped_openssl_types.h https://codereview.chromium.org/1474983003/diff/80001/net/ssl/openssl_ssl_uti... net/ssl/openssl_ssl_util.h:81: int EncodeSSLConnectionStatus(int cipher_suite, int compression, int version); This isn't really OpenSSL-specific. It'd go better in net/ssl/ssl_connection_status_flags.h. But even then, there's already SSLConnectionStatusSetCipherSuite and SSLConnectionStatusSetVersion. And TLS compression doesn't exist anymore. You can drop code for that. https://codereview.chromium.org/1474983003/diff/80001/net/ssl/openssl_ssl_uti... net/ssl/openssl_ssl_util.h:85: int GetNetSSLVersion(SSL* ssl); #include <openssl/ssl.h> https://codereview.chromium.org/1474983003/diff/80001/net/ssl/ssl_cert_reques... File net/ssl/ssl_cert_request_info.h (left): https://codereview.chromium.org/1474983003/diff/80001/net/ssl/ssl_cert_reques... net/ssl/ssl_cert_request_info.h:35: // DistinguishedName certificate_authorities<3..2^16-1>; (That's a hilarious typo. :) ) https://codereview.chromium.org/1474983003/diff/80001/net/ssl/ssl_server_conf... File net/ssl/ssl_server_config.h (right): https://codereview.chromium.org/1474983003/diff/80001/net/ssl/ssl_server_conf... net/ssl/ssl_server_config.h:64: // certificates are allowed. Nit: Slightly shorter: // A list of certificates whose names are to be included in the // CertificateRequest handshake message, if client certificates are // required. https://codereview.chromium.org/1474983003/diff/80001/net/ssl/ssl_server_conf... net/ssl/ssl_server_config.h:67: // Indicates that a client certificate is required, and provides the This isn't actually true, no? require_client_cert is that. (Could we perhaps merge the two and have EmbeddedTestServer pass in a dummy CertificateVerifier that just accepts everything?) https://codereview.chromium.org/1474983003/diff/80001/net/ssl/ssl_server_conf... net/ssl/ssl_server_config.h:70: // and must exist at least until the handshake has completed. and must exist [...] -> and must outlive any sockets using this SSLServerConfig. Since the socket makes a copy of the SSLServerConfig, even if it doesn't touch the field before the handshake is done, I think we're much better off with the simpler API contract. For instance, if we supported client-initiated renego (we don't, and aren't going to), this API contract could be wrong. https://codereview.chromium.org/1474983003/diff/80001/net/ssl/ssl_server_conf... net/ssl/ssl_server_config.h:74: // still be allowed (if ssl_server_config.send_client_cert is true), What's send_client_cert? https://codereview.chromium.org/1474983003/diff/80001/net/ssl/ssl_server_conf... net/ssl/ssl_server_config.h:77: // and may be subject to resumption. This API doesn't do session caching right now. https://codereview.chromium.org/1474983003/diff/80001/net/ssl/ssl_server_conf... net/ssl/ssl_server_config.h:79: // synchronously. This note seems unnecessary. If you want to make it asynchronous, file a bug and attach a TODO comment somewhere. https://codereview.chromium.org/1474983003/diff/80001/net/ssl/ssl_server_conf... net/ssl/ssl_server_config.h:82: // - cert: the cert to be verified This seems unnecessary.
https://codereview.chromium.org/1474983003/diff/60001/net/cert/client_cert_ve... File net/cert/client_cert_verifier.h (right): https://codereview.chromium.org/1474983003/diff/60001/net/cert/client_cert_ve... net/cert/client_cert_verifier.h:16: virtual ~ClientCertVerifier() {} On 2015/12/14 23:56:49, davidben wrote: > Nit: I'd put a newline here. Done. https://codereview.chromium.org/1474983003/diff/60001/net/cert/mock_client_ce... File net/cert/mock_client_cert_verifier.cc (right): https://codereview.chromium.org/1474983003/diff/60001/net/cert/mock_client_ce... net/cert/mock_client_cert_verifier.cc:26: for (it = rules_.begin(); it != rules_.end(); ++it) { On 2015/12/14 23:56:49, davidben wrote: > This is a little clearer as: > > for (const Rule& rule : rules_) { > // Check just the server cert. Intermediates will be ignored. > if (rule.cert->Equals(cert)) > return rule.rv; > } > > // Fall through to the default. > return default_result_; Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... File net/socket/ssl_server_socket.h (right): https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket.h:8: #include <vector> On 2015/12/14 23:56:49, davidben wrote: > Not used? Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:30: bool GetDERFromX509(X509* cert, base::StringPiece* sp) { On 2015/12/14 23:56:49, davidben wrote: > Use net::x509_util::GetDER from x509_util_openssl.h. Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:31: unsigned char* cert_data = NULL; On 2015/12/14 23:56:49, davidben wrote: > uint8_t Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:65: client_cert = NULL; On 2015/12/14 23:56:49, davidben wrote: > nullptr Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:302: ssl_server_config_.require_client_cert && client_cert_.get(); On 2015/12/14 23:56:49, davidben wrote: > Just client_cert_.get() seems right. Alternatively, I might just leave it false > since we didn't send a client cert. We received one. Done. Leaving it false. We can know whether we received one by checking client_cert_ . https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:650: // the cert changed during a renego. On 2015/12/14 23:56:49, davidben wrote: > Want to file a bug about this? This is sort of a silly situation and seems > fixable. Done. Created http://crbug.com/570351 https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:797: ssl_->verify_mode |= SSL_VERIFY_FAIL_IF_NO_PEER_CERT; On 2015/12/14 23:56:49, davidben wrote: > Please don't reach into BoringSSL's internal structs. Call the public APIs. > > https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#C... > > Also why are these separate from line 701? Seems you could configure it on the > SSL_CTX just fine too. (Also I suspect you want SSL_VERIFY_FAIL_IF_NO_PEER_CERT > in the require_client_cert case too. Rejecting that was probably an oversight.) Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:815: return; On 2015/12/14 23:56:49, davidben wrote: > Rather than being a function that caches stuff and has to make sure it's called > at the right time, just extract the certificate once when the handshake is done. Done. Will call this function only after handshake. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:826: DCHECK(verifier); On 2015/12/14 23:56:49, davidben wrote: > This DCHECK is false. You sometimes install NULL ones. I'd replace it with a > comment: > > // If no ClientCertVerifier was supplied, all certificates are accepted. Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:833: STACK_OF(X509)* chain = store_ctx->chain; On 2015/12/14 23:56:49, davidben wrote: > I believe this is always NULL. You want store_ctx->untrusted because you have > not verified a chain yet at this point. Also, store_ctx->untrusted already > includes the leaf. Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:123: new DrainableIOBuffer(new StringIOBuffer(std::string("0", 1)), 1)); On 2015/12/14 23:56:50, davidben wrote: > What's this for? This is for the client to be able to disconnect and abort the handshake so that the server socket won't hang waiting for the handshake. DoReadCallback requires data to be not null. Affected test: HandshakeWithClientCertRequiredNotSupplied https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:290: std::vector<uint8> public_key_info; On 2015/12/14 23:56:50, davidben wrote: > New code should use uint8_t. Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:294: result <<= 1; On 2015/12/14 23:56:49, davidben wrote: > This also assumes the exponent is small and that the DER wrapping bits aren't > far off. It'd be nice if we could do this without reading into these, but: > > #if defined(USE_OPENSSL) > return EVP_PKEY_size(rsa_private_key_->key()); > #else > NOTIMPLEMENTED(); > return 0; > #endif Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:324: void CompleteSignDigest(Error err, const std::vector<uint8_t>& signature) {} On 2015/12/14 23:56:50, davidben wrote: > Nit: newline Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:374: enum ClientCertSupply { On 2015/12/14 23:56:50, davidben wrote: > Sort of an odd name. Why not just call it ClientCert or ClientCertificate or > something? Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:375: kNoneSupplied = 0, On 2015/12/14 23:56:50, davidben wrote: > This one is never used. Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:377: kWrongCertSupplied = 2 On 2015/12/14 23:56:50, davidben wrote: > enums NAMED_LIKE_MACROS > > https://www.chromium.org/developers/coding-style#Naming Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:380: enum ClientCertExpect { kNoneExpected = 0, kCertRequired = 2 }; On 2015/12/14 23:56:50, davidben wrote: > Ditto to both. Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:388: client_cert_verifier_->set_default_result(CERT_STATUS_AUTHORITY_INVALID); On 2015/12/14 23:56:50, davidben wrote: > ERR_CERT_AUTHORITY_INVALID. > > It looks like the line above had this bug too. set_default_result takes a net > error code, not a CertStatus. Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:426: void InitializeClientCertsForClient(ClientCertSupply supply) { On 2015/12/14 23:56:50, davidben wrote: > Calling this method and the one below InitializeBlah is confusing since you have > to then call plain Initialize. ConfigureClientCertsForClient? Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:426: void InitializeClientCertsForClient(ClientCertSupply supply) { On 2015/12/14 23:56:50, davidben wrote: > This all seems like it ought to use a switch-case. It means that when you add a > new entry, the compiler will notice. > > Alternatively, perhaps just have it take two const char*s and pass in the file > names? Will take two const char* as parameters. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:450: if (expect == kCertRequired) { On 2015/12/14 23:56:50, davidben wrote: > This is weird. There's only two values for expect. Done. Will use a bool instead. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:458: CertVerifyResult ignored; On 2015/12/14 23:56:50, davidben wrote: > What's this for? Sorry, this isn't used after simplifying the client cert verifier. Removed. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:471: std::string unneeded; On 2015/12/14 23:56:50, davidben wrote: > Nit: unneeded -> unused is I think more common? Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:485: std::vector<uint8> key_vector( On 2015/12/14 23:56:50, davidben wrote: > New code should use uint8_t. Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:559: #if defined(USE_OPENSSL) || !defined(NSS_PLATFORM_CLIENT_AUTH) On 2015/12/14 23:56:50, davidben wrote: > This comment is out of date. I think you just want #if defined(USE_OPENSSL) now. > Probably something as simple as > > // NSS ports don't support client certificates. > > (The only NSS port remaining is iOS which doesn't do client auth at all.) Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:601: // get SSL_CLIENT_AUTH_CERT_NEEDED. This code path allows us to access the On 2015/12/14 23:56:49, davidben wrote: > ERR_SSL_CLIENT_AUTH_CERT_NEEDED Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:607: EXPECT_TRUE(server_ret == ERR_IO_PENDING); On 2015/12/14 23:56:50, davidben wrote: > EXPECT_EQ Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:616: } On 2015/12/14 23:56:50, davidben wrote: > This whole thing (line 609 down) can be written: > > EXPECT_EQ(ERR_SSL_CLIENT_AUTH_CERT_NEEDED, > connect_callback.GetResult( > client_socket_->Connect(connect_callback.callback()))); Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:633: } On 2015/12/14 23:56:50, davidben wrote: > This should probably be: > > server_ret = handshake_callback.GetResult(server_ret); > EXPECT_TRUE(.....); > > Why do you get the different error codes? Could you add a comment for why > there's two possibilities? This was probably due to previous support for both NSS and OpenSSL. I've removed the extra. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:655: } On 2015/12/14 23:56:50, davidben wrote: > Ditto on using GetResult. Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:657: server_ret = handshake_callback.WaitForResult(); On 2015/12/14 23:56:50, davidben wrote: > Ditto on using GetResult. Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:663: server_ret == ERR_SSL_SERVER_CERT_CHANGED); On 2015/12/14 23:56:50, davidben wrote: > It seems this has been fixed, right? Done. https://codereview.chromium.org/1474983003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:665: #endif // defined(USE_OPENSSL) || !defined(NSS_PLATFORM_CLIENT_AUTH) On 2015/12/14 23:56:50, davidben wrote: > (Don't forget to remove NSS_PLATFORM_CLIENT_AUTH here too.) Done. https://codereview.chromium.org/1474983003/diff/80001/net/ssl/openssl_ssl_util.h File net/ssl/openssl_ssl_util.h (right): https://codereview.chromium.org/1474983003/diff/80001/net/ssl/openssl_ssl_uti... net/ssl/openssl_ssl_util.h:75: using ScopedX509Name = crypto::ScopedOpenSSL<X509_NAME, X509_NAME_free>; On 2015/12/14 23:56:50, davidben wrote: > Nit: For consistency with other scopers, lets call this ScopedX509_NAME. Done. https://codereview.chromium.org/1474983003/diff/80001/net/ssl/openssl_ssl_uti... net/ssl/openssl_ssl_util.h:78: crypto::ScopedOpenSSL<STACK_OF(X509_NAME), FreeX509NameStack>; On 2015/12/14 23:56:50, davidben wrote: > I'd put these in net/ssl/scoped_openssl_types.h Done. https://codereview.chromium.org/1474983003/diff/80001/net/ssl/openssl_ssl_uti... net/ssl/openssl_ssl_util.h:81: int EncodeSSLConnectionStatus(int cipher_suite, int compression, int version); On 2015/12/14 23:56:50, davidben wrote: > This isn't really OpenSSL-specific. It'd go better in > net/ssl/ssl_connection_status_flags.h. But even then, there's already > SSLConnectionStatusSetCipherSuite and SSLConnectionStatusSetVersion. > > And TLS compression doesn't exist anymore. You can drop code for that. Done. Dropped. https://codereview.chromium.org/1474983003/diff/80001/net/ssl/openssl_ssl_uti... net/ssl/openssl_ssl_util.h:85: int GetNetSSLVersion(SSL* ssl); On 2015/12/14 23:56:50, davidben wrote: > #include <openssl/ssl.h> Done. https://codereview.chromium.org/1474983003/diff/80001/net/ssl/ssl_server_conf... File net/ssl/ssl_server_config.h (right): https://codereview.chromium.org/1474983003/diff/80001/net/ssl/ssl_server_conf... net/ssl/ssl_server_config.h:64: // certificates are allowed. On 2015/12/14 23:56:51, davidben wrote: > Nit: Slightly shorter: > > // A list of certificates whose names are to be included in the > // CertificateRequest handshake message, if client certificates are > // required. Done. https://codereview.chromium.org/1474983003/diff/80001/net/ssl/ssl_server_conf... net/ssl/ssl_server_config.h:67: // Indicates that a client certificate is required, and provides the On 2015/12/14 23:56:51, davidben wrote: > This isn't actually true, no? require_client_cert is that. (Could we perhaps > merge the two and have EmbeddedTestServer pass in a dummy CertificateVerifier > that just accepts everything?) You're right. The CertVerifyCallback in SSLServerSocketOpenSSL now accepts everything if a verifier is not provided. https://codereview.chromium.org/1474983003/diff/80001/net/ssl/ssl_server_conf... net/ssl/ssl_server_config.h:70: // and must exist at least until the handshake has completed. On 2015/12/14 23:56:51, davidben wrote: > and must exist [...] -> and must outlive any sockets using this SSLServerConfig. > > Since the socket makes a copy of the SSLServerConfig, even if it doesn't touch > the field before the handshake is done, I think we're much better off with the > simpler API contract. > > For instance, if we supported client-initiated renego (we don't, and aren't > going to), this API contract could be wrong. Done. https://codereview.chromium.org/1474983003/diff/80001/net/ssl/ssl_server_conf... net/ssl/ssl_server_config.h:74: // still be allowed (if ssl_server_config.send_client_cert is true), On 2015/12/14 23:56:51, davidben wrote: > What's send_client_cert? Sorry, this is now called require_client_cert. https://codereview.chromium.org/1474983003/diff/80001/net/ssl/ssl_server_conf... net/ssl/ssl_server_config.h:77: // and may be subject to resumption. On 2015/12/14 23:56:51, davidben wrote: > This API doesn't do session caching right now. Done. https://codereview.chromium.org/1474983003/diff/80001/net/ssl/ssl_server_conf... net/ssl/ssl_server_config.h:79: // synchronously. On 2015/12/14 23:56:51, davidben wrote: > This note seems unnecessary. If you want to make it asynchronous, file a bug and > attach a TODO comment somewhere. Done. Removed note. https://codereview.chromium.org/1474983003/diff/80001/net/ssl/ssl_server_conf... net/ssl/ssl_server_config.h:82: // - cert: the cert to be verified On 2015/12/14 23:56:51, davidben wrote: > This seems unnecessary. Done.
https://codereview.chromium.org/1474983003/diff/100001/net/cert/client_cert_v... File net/cert/client_cert_verifier.h (right): https://codereview.chromium.org/1474983003/diff/100001/net/cert/client_cert_v... net/cert/client_cert_verifier.h:10: class BoundNetLog; Unused? https://codereview.chromium.org/1474983003/diff/100001/net/cert/client_cert_v... net/cert/client_cert_verifier.h:20: virtual int Verify(X509Certificate* cert) = 0; DESIGN: Why isn't this interface asynchronous? I think if we're looking to export/support this API (which I'm not really wanting to, but I guess Cast needs it), we shouldn't be doing it blocking. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:279: bool SSLServerSocketOpenSSL::GetSSLInfo(SSLInfo* ssl_info) { I'm pretty sure this is just a wrong interface for server sockets. It doesn't make sense when so much of SSLInfo is client-socket specific. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:283: } nit: We don't do braces for one-line if's in //net nit 2: Extra newline between return false and ExtractClientCert https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:627: ExtractClientCert(); This feels weird, in that it leaves multiple places for client_cert_ to be assigned. Why is that? I would logically expect it to just happen in the handshake. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:687: SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, NULL); Ironically, you should use braces here, even though it's multi-line in the parenthesis. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:783: if (!ssl_server_config_.client_cert_ca_list.empty()) { Could you combine these two conditionals? https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:802: } This feels like a weird API contract; why not just do something like namespace { scoped_refptr<X509Certificate> GetClientCert(SSL* ssl) { ... } } and in DoHandshake something like client_cert_ = GetClientCert(ssl_) https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:819: if (res == OK) { DESIGN: Please handle errors first, which will reduce indentation if (res != OK) { X509_STORE_CTX_set_error(...); return 0; } return 1; https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:127: base::MessageLoop::current()->PostTask( BUG: Use base::ThreadTaskRunnerHandle() over base::MessageLoop::current() Very little (virtually no) new code should be using base::MessageLoop::current() https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:286: kHashes + arraysize(kHashes)); Could do return std::vector<SSLPrivateKey::Hash>(std::begin(kHashes), std::end(kHashes)); https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:551: TestCompletionCallback handshake_callback; You should declare your variables where they're used. TestCompletionCallback handshake_callback; int server_ret = .. ASSERT_TRUE(...); TestCompletionCallback connect_callback; int client_ret = ...; ASSERT_TRUE(...); https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:554: EXPECT_TRUE(server_ret == OK || server_ret == ERR_IO_PENDING); Should be ASSERT https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:557: EXPECT_TRUE(client_ret == OK || client_ret == ERR_IO_PENDING); Should be ASSERT https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:564: } 1) No braces throughout for client certs 2) Just do client_ret = connect_callback.GetResult(client_ret); server_ret = handshake_callback.GetResult(server_ret); ASSERT_EQ(OK, client_ret); ASSERT_EQ(OK, server_ret); The changes to assert above are because you're violating the API contract on line 568 if they failed. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:580: // cert_authorities from the CertificateRequest. Don't use pronouns in comments. https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M // Use the default setting for the client socket, which is to not send // a client certificate. This will cause the client to receive an // ERR_SSL_CLIENT_AUTH_CERT_NEEDED error, and allow for inspecting the // requested cert_authorities from the CertificateRequest sent by the // server. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:583: TestCompletionCallback handshake_callback; Same comments throughout the test. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:585: EXPECT_EQ(server_ret, ERR_IO_PENDING); ASSERT_EQ https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:602: if (server_ret == ERR_IO_PENDING) { This conditional isn't necessary with the above ASSERT change; if you leave it though server_ret = handshake_callback.GetResult(server_ret); EXPECT_EQ(ERR_FAILED, server_ret); https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:604: } No braces https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:616: TestCompletionCallback handshake_callback; Same comments about ordering. https://codereview.chromium.org/1474983003/diff/100001/net/ssl/openssl_ssl_ut... File net/ssl/openssl_ssl_util.h (right): https://codereview.chromium.org/1474983003/diff/100001/net/ssl/openssl_ssl_ut... net/ssl/openssl_ssl_util.h:75: void FreeX509NameStack(STACK_OF(X509_NAME) * ptr); STACK_OF(X509)* ptr); https://codereview.chromium.org/1474983003/diff/100001/net/ssl/ssl_server_con... File net/ssl/ssl_server_config.h (right): https://codereview.chromium.org/1474983003/diff/100001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:65: CertificateList client_cert_ca_list; DESIGN: Why does this take certificates, rather than distinguished names, which would then match both TLS (in implementation) and SSLCertRequestInfo
https://codereview.chromium.org/1474983003/diff/100001/net/cert/client_cert_v... File net/cert/client_cert_verifier.h (right): https://codereview.chromium.org/1474983003/diff/100001/net/cert/client_cert_v... net/cert/client_cert_verifier.h:10: class BoundNetLog; On 2015/12/17 03:47:35, Ryan Sleevi wrote: > Unused? Done. https://codereview.chromium.org/1474983003/diff/100001/net/cert/client_cert_v... net/cert/client_cert_verifier.h:20: virtual int Verify(X509Certificate* cert) = 0; On 2015/12/17 03:47:35, Ryan Sleevi wrote: > DESIGN: Why isn't this interface asynchronous? I think if we're looking to > export/support this API (which I'm not really wanting to, but I guess Cast needs > it), we shouldn't be doing it blocking. I believe OpenSSL only supports synchronous client certificate verification at the moment. http://crbug.com/347402 https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:279: bool SSLServerSocketOpenSSL::GetSSLInfo(SSLInfo* ssl_info) { On 2015/12/17 03:47:36, Ryan Sleevi wrote: > I'm pretty sure this is just a wrong interface for server sockets. It doesn't > make sense when so much of SSLInfo is client-socket specific. Should this be in a new interface like GetSSLServerInfo(SSLServerInfo*) where SSLServerInfo only has the server related info? https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:283: } On 2015/12/17 03:47:35, Ryan Sleevi wrote: > nit: We don't do braces for one-line if's in //net > nit 2: Extra newline between return false and ExtractClientCert Done. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:627: ExtractClientCert(); On 2015/12/17 03:47:35, Ryan Sleevi wrote: > This feels weird, in that it leaves multiple places for client_cert_ to be > assigned. Why is that? I would logically expect it to just happen in the > handshake. This was supposed to be the only place this is called. I missed removing the call in GetSSLInfo(). Sorry. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:687: SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, NULL); On 2015/12/17 03:47:36, Ryan Sleevi wrote: > Ironically, you should use braces here, even though it's multi-line in the > parenthesis. Done. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:783: if (!ssl_server_config_.client_cert_ca_list.empty()) { On 2015/12/17 03:47:36, Ryan Sleevi wrote: > Could you combine these two conditionals? Done. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:802: } On 2015/12/17 03:47:36, Ryan Sleevi wrote: > This feels like a weird API contract; why not just do something like > > namespace { > scoped_refptr<X509Certificate> GetClientCert(SSL* ssl) { > ... > } > } > > and in DoHandshake something like > > client_cert_ = GetClientCert(ssl_) Done. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:819: if (res == OK) { On 2015/12/17 03:47:36, Ryan Sleevi wrote: > DESIGN: Please handle errors first, which will reduce indentation > > if (res != OK) { > X509_STORE_CTX_set_error(...); > return 0; > } > > return 1; Done. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:127: base::MessageLoop::current()->PostTask( On 2015/12/17 03:47:36, Ryan Sleevi wrote: > BUG: Use base::ThreadTaskRunnerHandle() over base::MessageLoop::current() > > Very little (virtually no) new code should be using base::MessageLoop::current() Done. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:286: kHashes + arraysize(kHashes)); On 2015/12/17 03:47:36, Ryan Sleevi wrote: > Could do > > return std::vector<SSLPrivateKey::Hash>(std::begin(kHashes), std::end(kHashes)); Done. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:551: TestCompletionCallback handshake_callback; On 2015/12/17 03:47:36, Ryan Sleevi wrote: > You should declare your variables where they're used. > > TestCompletionCallback handshake_callback; > int server_ret = .. > ASSERT_TRUE(...); > > TestCompletionCallback connect_callback; > int client_ret = ...; > ASSERT_TRUE(...); Done. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:554: EXPECT_TRUE(server_ret == OK || server_ret == ERR_IO_PENDING); On 2015/12/17 03:47:36, Ryan Sleevi wrote: > Should be ASSERT Done. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:557: EXPECT_TRUE(client_ret == OK || client_ret == ERR_IO_PENDING); On 2015/12/17 03:47:36, Ryan Sleevi wrote: > Should be ASSERT Done. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:564: } On 2015/12/17 03:47:36, Ryan Sleevi wrote: > 1) No braces throughout for client certs > 2) Just do > > client_ret = connect_callback.GetResult(client_ret); > server_ret = handshake_callback.GetResult(server_ret); > > ASSERT_EQ(OK, client_ret); > ASSERT_EQ(OK, server_ret); > > The changes to assert above are because you're violating the API contract on > line 568 if they failed. Done. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:580: // cert_authorities from the CertificateRequest. On 2015/12/17 03:47:36, Ryan Sleevi wrote: > Don't use pronouns in comments. > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M > > // Use the default setting for the client socket, which is to not send > // a client certificate. This will cause the client to receive an > // ERR_SSL_CLIENT_AUTH_CERT_NEEDED error, and allow for inspecting the > // requested cert_authorities from the CertificateRequest sent by the > // server. Done. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:583: TestCompletionCallback handshake_callback; On 2015/12/17 03:47:36, Ryan Sleevi wrote: > Same comments throughout the test. Done. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:585: EXPECT_EQ(server_ret, ERR_IO_PENDING); On 2015/12/17 03:47:36, Ryan Sleevi wrote: > ASSERT_EQ Done. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:602: if (server_ret == ERR_IO_PENDING) { On 2015/12/17 03:47:36, Ryan Sleevi wrote: > This conditional isn't necessary with the above ASSERT change; if you leave it > though > > server_ret = handshake_callback.GetResult(server_ret); > EXPECT_EQ(ERR_FAILED, server_ret); Done. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:604: } On 2015/12/17 03:47:36, Ryan Sleevi wrote: > No braces Done. https://codereview.chromium.org/1474983003/diff/100001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:616: TestCompletionCallback handshake_callback; On 2015/12/17 03:47:36, Ryan Sleevi wrote: > Same comments about ordering. Done. https://codereview.chromium.org/1474983003/diff/100001/net/ssl/openssl_ssl_ut... File net/ssl/openssl_ssl_util.h (right): https://codereview.chromium.org/1474983003/diff/100001/net/ssl/openssl_ssl_ut... net/ssl/openssl_ssl_util.h:75: void FreeX509NameStack(STACK_OF(X509_NAME) * ptr); On 2015/12/17 03:47:36, Ryan Sleevi wrote: > STACK_OF(X509)* ptr); Done. https://codereview.chromium.org/1474983003/diff/100001/net/ssl/ssl_server_con... File net/ssl/ssl_server_config.h (right): https://codereview.chromium.org/1474983003/diff/100001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:65: CertificateList client_cert_ca_list; On 2015/12/17 03:47:36, Ryan Sleevi wrote: > DESIGN: Why does this take certificates, rather than distinguished names, which > would then match both TLS (in implementation) and SSLCertRequestInfo A STACK_OF(X509_NAME) needs to be set into ssl_ using SSL_set_client_CA_list(). Storing a list of names in string form will require converting back to X509_NAME. This list is provided by the server code outside of //net. Any suggestions for a clean way to store the list of DN here?
https://codereview.chromium.org/1474983003/diff/100001/net/cert/client_cert_v... File net/cert/client_cert_verifier.h (right): https://codereview.chromium.org/1474983003/diff/100001/net/cert/client_cert_v... net/cert/client_cert_verifier.h:20: virtual int Verify(X509Certificate* cert) = 0; On 2015/12/18 00:00:55, Ryan Chung wrote: > I believe OpenSSL only supports synchronous client certificate verification at > the moment. http://crbug.com/347402 But the Chrome interface should be designed for asynchronicity, and if it gives an asynchronous result, the binding code (to OpenSSL) should then fail. But we shouldn't be writing new //net code that assumes synchronous responses, even if a third-party library demands it. In the worst case, we'd offload the OpenSSL operation to a dedicated thread, and then spin a waitloop on that thread to complete the asynchronous operation (this is how we handled NSS callbacks that demand synchronicity on fundamentally asynchronous events) https://codereview.chromium.org/1474983003/diff/100001/net/ssl/ssl_server_con... File net/ssl/ssl_server_config.h (right): https://codereview.chromium.org/1474983003/diff/100001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:65: CertificateList client_cert_ca_list; On 2015/12/18 00:00:56, Ryan Chung wrote: > On 2015/12/17 03:47:36, Ryan Sleevi wrote: > > DESIGN: Why does this take certificates, rather than distinguished names, > which > > would then match both TLS (in implementation) and SSLCertRequestInfo > > A STACK_OF(X509_NAME) needs to be set into ssl_ using SSL_set_client_CA_list(). > Storing a list of names in string form will require converting back to > X509_NAME. > This list is provided by the server code outside of //net. The client cert code stores it in the DER-encoded form. You can convert to/from the X509_NAME form using i2d_/d2i_X509_NAME.
https://codereview.chromium.org/1474983003/diff/100001/net/cert/client_cert_v... File net/cert/client_cert_verifier.h (right): https://codereview.chromium.org/1474983003/diff/100001/net/cert/client_cert_v... net/cert/client_cert_verifier.h:20: virtual int Verify(X509Certificate* cert) = 0; On 2015/12/18 00:07:09, Ryan Sleevi wrote: > On 2015/12/18 00:00:55, Ryan Chung wrote: > > I believe OpenSSL only supports synchronous client certificate verification at > > the moment. http://crbug.com/347402 > > But the Chrome interface should be designed for asynchronicity, and if it gives > an asynchronous result, the binding code (to OpenSSL) should then fail. > > But we shouldn't be writing new //net code that assumes synchronous responses, > even if a third-party library demands it. In the worst case, we'd offload the > OpenSSL operation to a dedicated thread, and then spin a waitloop on that thread > to complete the asynchronous operation (this is how we handled NSS callbacks > that demand synchronicity on fundamentally asynchronous events) Teaching BoringSSL to be asynchronous with certificate verification should certainly be doable. It's one more interface to change later when we rid ourselves of the legacy X.509 stack, but whatever. It should also work to do the verification after the handshake, like SSLClientSocketOpenSSL does. The two subtleties are: 1. Unless you implement the session cache externally, which seems more trouble than is worth it, the sessions with bad verifications will end up in the session cache. This is actually just fine *as long as you verify on both new handshakes and resumptions*. 2. If you don't like the certificate, you'll decide too late to be able to send an alert to the client. That means that the client won't know to try again. This may not be something that matters to Cast. I also don't actually mind making it synchronous to start with. I'm assuming that Cast's needs for client certificate verification aren't that complex and won't require anything crazy like AIA chasing. And it'd already have to verify CertificateVerify signatures synchronously, so if we're only worried about signature verification, this isn't a big deal.
https://codereview.chromium.org/1474983003/diff/100001/net/cert/client_cert_v... File net/cert/client_cert_verifier.h (right): https://codereview.chromium.org/1474983003/diff/100001/net/cert/client_cert_v... net/cert/client_cert_verifier.h:20: virtual int Verify(X509Certificate* cert) = 0; On 2015/12/18 00:07:09, Ryan Sleevi wrote: > On 2015/12/18 00:00:55, Ryan Chung wrote: > > I believe OpenSSL only supports synchronous client certificate verification at > > the moment. http://crbug.com/347402 > > But the Chrome interface should be designed for asynchronicity, and if it gives > an asynchronous result, the binding code (to OpenSSL) should then fail. I updated the interface. For now, verification will fail if it gives an asynchronous result. https://codereview.chromium.org/1474983003/diff/100001/net/ssl/ssl_server_con... File net/ssl/ssl_server_config.h (right): https://codereview.chromium.org/1474983003/diff/100001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:65: CertificateList client_cert_ca_list; On 2015/12/18 00:07:09, Ryan Sleevi wrote: > On 2015/12/18 00:00:56, Ryan Chung wrote: > > On 2015/12/17 03:47:36, Ryan Sleevi wrote: > > > DESIGN: Why does this take certificates, rather than distinguished names, > > which > > > would then match both TLS (in implementation) and SSLCertRequestInfo > > > > A STACK_OF(X509_NAME) needs to be set into ssl_ using > SSL_set_client_CA_list(). > > Storing a list of names in string form will require converting back to > > X509_NAME. > > This list is provided by the server code outside of //net. > > The client cert code stores it in the DER-encoded form. You can convert to/from > the X509_NAME form using i2d_/d2i_X509_NAME. Done. To populate this list in the unittests, OpenSSL will be used directly to extract the DER-encoded DN out of the certificate because currently there's no easy way to extract that using chromium's API.
Could you take another look at this CL when you get a chance? Thank you.
On 2016/01/21 22:12:48, ryanchung wrote: > Could you take another look at this CL when you get a chance? Thank you. It's in the queue. Our extremely limited bandwidth for SSL-related CLs is hideously over-committed right now and this CL is kind of large.
Thanks! There's a lot of comments, but they're all fairly local. Hopefully this'll be the last or second to last iteration. I'll try to have a better turnaround time for the next roundtrip. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:756: &ssl_info->connection_status); Why did this change? https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:37: if (cert) { Bleh. This probably wants to be documented like: // Creates an X509Certificate out of the concatenation of |cert|, if non-null, with |chain|. Otherwise this check looks like a bug because it is not generally correct to drop the leaf willy-nilly. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:39: return client_cert; If this fails, we should fail the connection, not silently act like there was no certificate. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:43: ScopedX509_STACK openssl_chain(X509_chain_up_ref(chain)); What's the point of this copy? https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:49: return nullptr; Ditto re failing the connection. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:58: STACK_OF(X509)* chain = SSL_get_peer_cert_chain(ssl); Check if cert is NULL and, if so, this needs to return nullptr. See the documentation: https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#S... This is especially important in light of CreateX509Certificate's bizarre behavior around the first parameter being null. I suspect things are just working on accident because X509Certificate::CreateFromDERCertChain is silently doing the right thing on the empty vector. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:692: SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, NULL); NULL -> nullptr https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:695: ssl_server_config_.client_cert_verifier); This probably should also go in the conditional, no? https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:793: reinterpret_cast<const unsigned char*>(authority.c_str()); uint8_t, not unsigned char https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:794: ScopedX509_NAME subj(d2i_X509_NAME(NULL, &name, authority.length())); Nit: NULL -> nullptr https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:794: ScopedX509_NAME subj(d2i_X509_NAME(NULL, &name, authority.length())); This needs error-handling. d2i_X509_NAME parses things, so you must check the return value. Also check for trailing data. Need to check that name == authority.c_str() + authority.length() afterwards. (Yeah, this API is horrible. If it's d2i_FOO, it's one of the legacy bits of OpenSSL we've more-or-less disowned. I'll rewrite this code later to a newer API once it's available.) https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:811: store_ctx, SSL_get_ex_data_X509_STORE_CTX_idx())); You don't seem to be doing anything with this variable. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:814: scoped_ptr<ClientCertVerifier::Request> ignore_async; Very nitpicky nit: declare variables near where they're used, so put it just before the Very call. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:818: int res = verifier->Verify(client_cert.get(), This really needs a comment as to what's going on here in the asynchronous case. Probably also a DCHECK and a bug link. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... File net/socket/ssl_server_socket_openssl.h (right): https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.h:109: void ExtractClientCert(); This function does not seem to exist. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:20: #include <openssl/x509.h> Needs to be guarded by USE_OPENSSL (and ordered after all the other includes since it then depends on build/build_config.h) https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:293: // true for the test keys in use. This comment is no longer accurate. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:295: #if defined(USE_OPENSSL) I would just wrap the whole thing in USE_OPENSSL. This interface will never be consumed by NSS. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:295: #if defined(USE_OPENSSL) #include "build/build_config.h" to condition on USE_OPENSSL. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:305: const SignCallback& callback) override { FYI, this implementation will not work for TLS 1.1 and below. I don't hugely care; hopefully svaldez will have an SSLPrivateKeyOpenSSL very soon for this to use instead and we'll just remove this. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:438: void ConfigureClientCertsForServer(bool cert_expected) { This parameter seems to be always true. (And indeed why would one ever call it with false?) https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:444: #if defined(USE_OPENSSL) This isn't even going to work if we're not USE_OPENSSL, no? I would suggest you wrap the whole method to make sure no one calls it. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:449: .data()); Nit: data -> c_str. In C++11, it doesn't actually matter, but you do want a NUL-terminated string here. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:450: if (cert_names != NULL) { NULL -> nullptr throughout this file. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:450: if (cert_names != NULL) { This should be an ASSERT_TRUE or CHECK or something. If you fail to read the file, it should fail the test. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:452: unsigned char* str = NULL; uint8_t https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:453: int length = i2d_X509_NAME(sk_X509_NAME_value(cert_names, i), &str); This leaks memory. You need to OPENSSL_free(str) afterwards. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:466: X509Certificate* ReadTestCert(const base::StringPiece& name, Should return a scoped_refptr, not a raw pointer. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:467: std::string* cert_der) { Why do you sometimes use ReadTestCert and sometimes ImportCertFromFile? (Couldn't this method be replaced with ImportCertFromFile?) https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:478: crypto::RSAPrivateKey* ReadTestKey(const base::StringPiece& name) { Should return a scoped_ptr, not a raw pointer. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:501: CertificateList trusted_certs_; Unused? https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:519: ASSERT_TRUE(server_ret == OK || server_ret == ERR_IO_PENDING); (It's in the original too, but I wouldn't bother with this line.) https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:523: ASSERT_TRUE(client_ret == OK || client_ret == ERR_IO_PENDING); (Ditto.) https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:549: // NSS ports don't support client certificates Nit: period at end. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:564: ASSERT_TRUE(server_ret == OK || server_ret == ERR_IO_PENDING); (I wouldn't bother with this line. Redundant with line 574.) https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:568: ASSERT_TRUE(client_ret == OK || client_ret == ERR_IO_PENDING); (Ditto.) https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:596: ASSERT_EQ(server_ret, ERR_IO_PENDING); (Strictly speaking, not redundant this time, but I wouldn't bother.) https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:614: EXPECT_EQ(ERR_FAILED, handshake_callback.GetResult(server_ret)); Why is this mapping to such a generic error code? https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:627: EXPECT_EQ(server_ret, ERR_IO_PENDING); (Ditto.) https://codereview.chromium.org/1474983003/diff/200001/net/ssl/openssl_ssl_ut... File net/ssl/openssl_ssl_util.cc (right): https://codereview.chromium.org/1474983003/diff/200001/net/ssl/openssl_ssl_ut... net/ssl/openssl_ssl_util.cc:218: // this SSL connection. Already in the header file. https://codereview.chromium.org/1474983003/diff/200001/net/ssl/openssl_ssl_ut... File net/ssl/openssl_ssl_util.h (right): https://codereview.chromium.org/1474983003/diff/200001/net/ssl/openssl_ssl_ut... net/ssl/openssl_ssl_util.h:77: void FreeX509NameStack(STACK_OF(X509_NAME)* ptr); Why is this declared both here and in scoped_openssl_types.h? https://codereview.chromium.org/1474983003/diff/200001/net/ssl/scoped_openssl... File net/ssl/scoped_openssl_types.h (right): https://codereview.chromium.org/1474983003/diff/200001/net/ssl/scoped_openssl... net/ssl/scoped_openssl_types.h:25: using ScopedX509_STACK = crypto::ScopedOpenSSL<STACK_OF(X509), FreeX509Stack>; This is confusing. The OpenSSL type is not called X509_STACK. I would stick with the original naming. https://codereview.chromium.org/1474983003/diff/200001/net/ssl/scoped_openssl... net/ssl/scoped_openssl_types.h:25: using ScopedX509_STACK = crypto::ScopedOpenSSL<STACK_OF(X509), FreeX509Stack>; #include <openssl/stack.h> https://codereview.chromium.org/1474983003/diff/200001/net/ssl/ssl_server_con... File net/ssl/ssl_server_config.cc (right): https://codereview.chromium.org/1474983003/diff/200001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.cc:17: cert_authorities_(), Not needed. https://codereview.chromium.org/1474983003/diff/200001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.cc:18: client_cert_verifier() {} client_cert_verifier(nullptr) Otherwise I suspect you're still leaving it uninitialized. https://codereview.chromium.org/1474983003/diff/200001/net/url_request/url_re... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1474983003/diff/200001/net/url_request/url_re... net/url_request/url_request_unittest.cc:8291: EXPECT_EQ(0, d.bytes_received()); You change the test from successfully connecting to failing to connect, so you're stressing a bit less of the state machine now. I think we should still have a test which authenticates with no certificate. This is a terrible idea for servers to do, but people do do optional client auth for better or worse. (Also if the test were intended to fail, it needs to assert on the error code and not just check bytes_received.)
Thanks. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:756: &ssl_info->connection_status); On 2016/01/25 20:56:10, davidben (slow) wrote: > Why did this change? You mentioned compression doesn't exists anymore and that there's SSLConnectionStatusSetCipherSuite and SSLConnectionStatusSetVersion so I interpreted that I should remove EncodeSSLConnectionStatus. Please correct me if I interpreted this wrong. Quote: > int EncodeSSLConnectionStatus(int cipher_suite, int compression, int version); > This isn't really OpenSSL-specific. It'd go better in > net/ssl/ssl_connection_status_flags.h. But even then, there's already > SSLConnectionStatusSetCipherSuite and SSLConnectionStatusSetVersion. > > And TLS compression doesn't exist anymore. You can drop code for that. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:37: if (cert) { On 2016/01/25 20:56:10, davidben (slow) wrote: > Bleh. This probably wants to be documented like: > > // Creates an X509Certificate out of the concatenation of |cert|, if non-null, > with |chain|. > > Otherwise this check looks like a bug because it is not generally correct to > drop the leaf willy-nilly. Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:39: return client_cert; On 2016/01/25 20:56:10, davidben (slow) wrote: > If this fails, we should fail the connection, not silently act like there was no > certificate. Ok. I'm adding code to CertVerifyCallback to handle this. But I'm not sure what the appropriate error should be... X509_V_ERR_CERT_REJECTED? https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:43: ScopedX509_STACK openssl_chain(X509_chain_up_ref(chain)); On 2016/01/25 20:56:10, davidben (slow) wrote: > What's the point of this copy? Done. Shouldn't need to copy. Removed. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:49: return nullptr; On 2016/01/25 20:56:10, davidben (slow) wrote: > Ditto re failing the connection. Ok. I'm adding code to CertVerifyCallback to handle this. But I'm not sure what the appropriate error should be... X509_V_ERR_CERT_REJECTED? https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:57: X509* cert = SSL_get_peer_certificate(ssl); I think this was a memory leak. SSL_get_peer_certificate needs the caller to explicitly free the result. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:58: STACK_OF(X509)* chain = SSL_get_peer_cert_chain(ssl); On 2016/01/25 20:56:10, davidben (slow) wrote: > Check if cert is NULL and, if so, this needs to return nullptr. See the > documentation: > > https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#S... > > This is especially important in light of CreateX509Certificate's bizarre > behavior around the first parameter being null. I suspect things are just > working on accident because X509Certificate::CreateFromDERCertChain is silently > doing the right thing on the empty vector. Done. Thanks. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:692: SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, NULL); On 2016/01/25 20:56:10, davidben (slow) wrote: > NULL -> nullptr Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:695: ssl_server_config_.client_cert_verifier); On 2016/01/25 20:56:10, davidben (slow) wrote: > This probably should also go in the conditional, no? Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:793: reinterpret_cast<const unsigned char*>(authority.c_str()); On 2016/01/25 20:56:10, davidben (slow) wrote: > uint8_t, not unsigned char Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:794: ScopedX509_NAME subj(d2i_X509_NAME(NULL, &name, authority.length())); On 2016/01/25 20:56:10, davidben (slow) wrote: > Nit: NULL -> nullptr Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:794: ScopedX509_NAME subj(d2i_X509_NAME(NULL, &name, authority.length())); On 2016/01/25 20:56:10, davidben (slow) wrote: > This needs error-handling. d2i_X509_NAME parses things, so you must check the > return value. Also check for trailing data. Need to check that name == > authority.c_str() + authority.length() afterwards. > > (Yeah, this API is horrible. If it's d2i_FOO, it's one of the legacy bits of > OpenSSL we've more-or-less disowned. I'll rewrite this code later to a newer API > once it's available.) Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:811: store_ctx, SSL_get_ex_data_X509_STORE_CTX_idx())); On 2016/01/25 20:56:10, davidben (slow) wrote: > You don't seem to be doing anything with this variable. Done. Removed. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:814: scoped_ptr<ClientCertVerifier::Request> ignore_async; On 2016/01/25 20:56:10, davidben (slow) wrote: > Very nitpicky nit: declare variables near where they're used, so put it just > before the Very call. Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:818: int res = verifier->Verify(client_cert.get(), On 2016/01/25 20:56:10, davidben (slow) wrote: > This really needs a comment as to what's going on here in the asynchronous case. > Probably also a DCHECK and a bug link. Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... File net/socket/ssl_server_socket_openssl.h (right): https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.h:109: void ExtractClientCert(); On 2016/01/25 20:56:10, davidben (slow) wrote: > This function does not seem to exist. Done. Removed. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:20: #include <openssl/x509.h> On 2016/01/25 20:56:11, davidben (slow) wrote: > Needs to be guarded by USE_OPENSSL (and ordered after all the other includes > since it then depends on build/build_config.h) Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:293: // true for the test keys in use. On 2016/01/25 20:56:11, davidben (slow) wrote: > This comment is no longer accurate. Done. Thx. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:295: #if defined(USE_OPENSSL) On 2016/01/25 20:56:11, davidben (slow) wrote: > #include "build/build_config.h" to condition on USE_OPENSSL. Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:295: #if defined(USE_OPENSSL) On 2016/01/25 20:56:11, davidben (slow) wrote: > I would just wrap the whole thing in USE_OPENSSL. This interface will never be > consumed by NSS. I assume you mean wrapping the whole class. Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:305: const SignCallback& callback) override { On 2016/01/25 20:56:11, davidben (slow) wrote: > FYI, this implementation will not work for TLS 1.1 and below. I don't hugely > care; hopefully svaldez will have an SSLPrivateKeyOpenSSL very soon for this to > use instead and we'll just remove this. Ok. Thanks. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:438: void ConfigureClientCertsForServer(bool cert_expected) { On 2016/01/25 20:56:11, davidben (slow) wrote: > This parameter seems to be always true. (And indeed why would one ever call it > with false?) True. Removed parameter. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:444: #if defined(USE_OPENSSL) On 2016/01/25 20:56:11, davidben (slow) wrote: > This isn't even going to work if we're not USE_OPENSSL, no? I would suggest you > wrap the whole method to make sure no one calls it. Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:449: .data()); On 2016/01/25 20:56:11, davidben (slow) wrote: > Nit: data -> c_str. > In C++11, it doesn't actually matter, but you do want a NUL-terminated string > here. Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:450: if (cert_names != NULL) { On 2016/01/25 20:56:11, davidben (slow) wrote: > NULL -> nullptr throughout this file. Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:450: if (cert_names != NULL) { On 2016/01/25 20:56:11, davidben (slow) wrote: > This should be an ASSERT_TRUE or CHECK or something. If you fail to read the > file, it should fail the test. Done. Good point. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:452: unsigned char* str = NULL; On 2016/01/25 20:56:10, davidben (slow) wrote: > uint8_t Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:453: int length = i2d_X509_NAME(sk_X509_NAME_value(cert_names, i), &str); On 2016/01/25 20:56:11, davidben (slow) wrote: > This leaks memory. You need to OPENSSL_free(str) afterwards. Done. Thanks. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:466: X509Certificate* ReadTestCert(const base::StringPiece& name, On 2016/01/25 20:56:11, davidben (slow) wrote: > Should return a scoped_refptr, not a raw pointer. Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:467: std::string* cert_der) { On 2016/01/25 20:56:11, davidben (slow) wrote: > Why do you sometimes use ReadTestCert and sometimes ImportCertFromFile? > (Couldn't this method be replaced with ImportCertFromFile?) I'll remove this function. I believe it was to also get the DER encoded certificate for SSLConfig::CertAndStatus. But we could get that from GetDEREncoded(). https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:478: crypto::RSAPrivateKey* ReadTestKey(const base::StringPiece& name) { On 2016/01/25 20:56:10, davidben (slow) wrote: > Should return a scoped_ptr, not a raw pointer. Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:501: CertificateList trusted_certs_; On 2016/01/25 20:56:11, davidben (slow) wrote: > Unused? Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:519: ASSERT_TRUE(server_ret == OK || server_ret == ERR_IO_PENDING); On 2016/01/25 20:56:11, davidben (slow) wrote: > (It's in the original too, but I wouldn't bother with this line.) Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:523: ASSERT_TRUE(client_ret == OK || client_ret == ERR_IO_PENDING); On 2016/01/25 20:56:11, davidben (slow) wrote: > (Ditto.) Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:549: // NSS ports don't support client certificates On 2016/01/25 20:56:11, davidben (slow) wrote: > Nit: period at end. Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:564: ASSERT_TRUE(server_ret == OK || server_ret == ERR_IO_PENDING); On 2016/01/25 20:56:11, davidben (slow) wrote: > (I wouldn't bother with this line. Redundant with line 574.) Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:568: ASSERT_TRUE(client_ret == OK || client_ret == ERR_IO_PENDING); On 2016/01/25 20:56:11, davidben (slow) wrote: > (Ditto.) Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:596: ASSERT_EQ(server_ret, ERR_IO_PENDING); On 2016/01/25 20:56:11, davidben (slow) wrote: > (Strictly speaking, not redundant this time, but I wouldn't bother.) Done. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:614: EXPECT_EQ(ERR_FAILED, handshake_callback.GetResult(server_ret)); On 2016/01/25 20:56:11, davidben (slow) wrote: > Why is this mapping to such a generic error code? This is the error code when the connection is closed. In this case, the client closed the connection. I assume the server asks the client for a certificate but the client doesn't even send back an empty certificate and so Disconnect() needs to be called. Is that correct? Otherwise, I'm a little confused because SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT was set on the server socket which means the server socket should error out (when the client connects without a client certificate) without needing the client to disconnect. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:627: EXPECT_EQ(server_ret, ERR_IO_PENDING); On 2016/01/25 20:56:11, davidben (slow) wrote: > (Ditto.) Done. https://codereview.chromium.org/1474983003/diff/200001/net/ssl/openssl_ssl_ut... File net/ssl/openssl_ssl_util.cc (right): https://codereview.chromium.org/1474983003/diff/200001/net/ssl/openssl_ssl_ut... net/ssl/openssl_ssl_util.cc:218: // this SSL connection. On 2016/01/25 20:56:11, davidben (slow) wrote: > Already in the header file. Done. Removed. https://codereview.chromium.org/1474983003/diff/200001/net/ssl/openssl_ssl_ut... File net/ssl/openssl_ssl_util.h (right): https://codereview.chromium.org/1474983003/diff/200001/net/ssl/openssl_ssl_ut... net/ssl/openssl_ssl_util.h:77: void FreeX509NameStack(STACK_OF(X509_NAME)* ptr); On 2016/01/25 20:56:11, davidben (slow) wrote: > Why is this declared both here and in scoped_openssl_types.h? Hmm, I should remove it from here. scoped_openssl_types.h needs to know about the Free functions in openssl_ssl_util.cc but it would create a circular dependancy if I just included openssl_ssl_util.h in scoped_openssl_types.h But then now they'll be declared in scoped_openssl_types.h while being defined in openssl_ssl_util.cc What would you suggest? https://codereview.chromium.org/1474983003/diff/200001/net/ssl/scoped_openssl... File net/ssl/scoped_openssl_types.h (right): https://codereview.chromium.org/1474983003/diff/200001/net/ssl/scoped_openssl... net/ssl/scoped_openssl_types.h:25: using ScopedX509_STACK = crypto::ScopedOpenSSL<STACK_OF(X509), FreeX509Stack>; On 2016/01/25 20:56:11, davidben (slow) wrote: > This is confusing. The OpenSSL type is not called X509_STACK. I would stick with > the original naming. Done. https://codereview.chromium.org/1474983003/diff/200001/net/ssl/scoped_openssl... net/ssl/scoped_openssl_types.h:25: using ScopedX509_STACK = crypto::ScopedOpenSSL<STACK_OF(X509), FreeX509Stack>; On 2016/01/25 20:56:11, davidben (slow) wrote: > #include <openssl/stack.h> Done. https://codereview.chromium.org/1474983003/diff/200001/net/ssl/ssl_server_con... File net/ssl/ssl_server_config.cc (right): https://codereview.chromium.org/1474983003/diff/200001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.cc:17: cert_authorities_(), On 2016/01/25 20:56:12, davidben (slow) wrote: > Not needed. Done. https://codereview.chromium.org/1474983003/diff/200001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.cc:18: client_cert_verifier() {} On 2016/01/25 20:56:12, davidben (slow) wrote: > client_cert_verifier(nullptr) > > Otherwise I suspect you're still leaving it uninitialized. Done. https://codereview.chromium.org/1474983003/diff/200001/net/url_request/url_re... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1474983003/diff/200001/net/url_request/url_re... net/url_request/url_request_unittest.cc:8291: EXPECT_EQ(0, d.bytes_received()); On 2016/01/25 20:56:12, davidben (slow) wrote: > You change the test from successfully connecting to failing to connect, so > you're stressing a bit less of the state machine now. I think we should still > have a test which authenticates with no certificate. This is a terrible idea for > servers to do, but people do do optional client auth for better or worse. > > (Also if the test were intended to fail, it needs to assert on the error code > and not just check bytes_received.) If we want to support optional client certificates, we need to add another flag to SSLServerConfig... something like optional_client_cert which will do the same thing as require_client_cert but doesn't set SSL_VERIFY_FAIL_IF_NO_PEER_CERT. Is this what you have in mind?
Hopefully this is the last batch of comments! (Note: some comments are stapled to a previous patch set.) https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:756: &ssl_info->connection_status); On 2016/01/29 23:22:12, ryanchung wrote: > On 2016/01/25 20:56:10, davidben (slow) wrote: > > Why did this change? > > You mentioned compression doesn't exists anymore and that there's > SSLConnectionStatusSetCipherSuite and SSLConnectionStatusSetVersion so I > interpreted that I should remove EncodeSSLConnectionStatus. Please correct me if > I interpreted this wrong. > > Quote: > > int EncodeSSLConnectionStatus(int cipher_suite, int compression, int version); > > This isn't really OpenSSL-specific. It'd go better in > > net/ssl/ssl_connection_status_flags.h. But even then, there's already > > SSLConnectionStatusSetCipherSuite and SSLConnectionStatusSetVersion. > > > > And TLS compression doesn't exist anymore. You can drop code for that. Derp! Sorry, my bad. I wasn't paying attention and completely forgot EncodeSSLConnectionStatus was part of this file. https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1474983003/diff/200001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:614: EXPECT_EQ(ERR_FAILED, handshake_callback.GetResult(server_ret)); On 2016/01/29 23:22:13, ryanchung wrote: > On 2016/01/25 20:56:11, davidben (slow) wrote: > > Why is this mapping to such a generic error code? > > This is the error code when the connection is closed. In this case, the client > closed the connection. Oh, right. I haven't ported the better error handling from SSLClientSocket over to SSLServerSocket, so it loses all the transport errors. > I assume the server asks the client for a certificate but the client doesn't > even send back an empty certificate and so Disconnect() needs to be called. Is > that correct? Yeah, that's right. We probably should Disconnect() it ourselves or something. We kinda just let the socket sit there and usually a higher-level part of the stack destroys the socket which causes it to close (or maybe reset?) on it. > Otherwise, I'm a little confused because SSL_VERIFY_PEER | > SSL_VERIFY_FAIL_IF_NO_PEER_CERT was set on the server socket which means the > server socket should error out (when the client connects without a client > certificate) without needing the client to disconnect. https://codereview.chromium.org/1474983003/diff/200001/net/ssl/openssl_ssl_ut... File net/ssl/openssl_ssl_util.h (right): https://codereview.chromium.org/1474983003/diff/200001/net/ssl/openssl_ssl_ut... net/ssl/openssl_ssl_util.h:77: void FreeX509NameStack(STACK_OF(X509_NAME)* ptr); On 2016/01/29 23:22:14, ryanchung wrote: > On 2016/01/25 20:56:11, davidben (slow) wrote: > > Why is this declared both here and in scoped_openssl_types.h? > > Hmm, I should remove it from here. > scoped_openssl_types.h needs to know about the Free functions in > openssl_ssl_util.cc but it would create a circular dependancy if I just included > openssl_ssl_util.h in scoped_openssl_types.h > > But then now they'll be declared in scoped_openssl_types.h while being defined > in openssl_ssl_util.cc > What would you suggest? Hrm. I'd probably either add scoped_openssl_types.cc or maybe just define it in the header as an inline function. As functions go, they're pretty tiny. https://codereview.chromium.org/1474983003/diff/200001/net/url_request/url_re... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1474983003/diff/200001/net/url_request/url_re... net/url_request/url_request_unittest.cc:8291: EXPECT_EQ(0, d.bytes_received()); On 2016/01/29 23:22:14, ryanchung wrote: > On 2016/01/25 20:56:12, davidben (slow) wrote: > > You change the test from successfully connecting to failing to connect, so > > you're stressing a bit less of the state machine now. I think we should still > > have a test which authenticates with no certificate. This is a terrible idea > for > > servers to do, but people do do optional client auth for better or worse. > > > > (Also if the test were intended to fail, it needs to assert on the error code > > and not just check bytes_received.) > > If we want to support optional client certificates, we need to add another flag > to SSLServerConfig... something like optional_client_cert which will do the same > thing as require_client_cert but doesn't set SSL_VERIFY_FAIL_IF_NO_PEER_CERT. Is > this what you have in mind? > Oh hrmf, did I ask you do remove that from a previous version? Sorry! :-( Hard to remember global requirements. We probably do want to be able to test that we don't screw this up. ERR_SSL_PROTOCOL_ERROR could also mean "you spoke the protocol wrong and messed something up". Maybe an enum? That's what Go's TLS library does: https://golang.org/pkg/crypto/tls/#ClientAuthType enum ClientCertType { NO_CLIENT_CERT, OPTIONAL_CLIENT_CERT, REQUIRE_CLIENT_CERT, }; ClientCertType client_cert; Or a pair of booleans works too. I don't have hugely strong opinions. svaldez, do you care? I think my default would be to go with an enum, but if anyone else cares, go with that. https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:47: if (x509_util::GetDER(x, &der_cert)) { Style nit: typically errors paths are the early return when possible. So something like: if (!x509_util::GetDER(x, &der_cert)) return nullptr; der_chain.push_back(der_cert); The idea is to keep the main code indented normally and have error paths out of the way. https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:60: if (!cert.get()) Nit: .get() isn't needed here. (You need it for scoped_refptr, but not scoped_ptr. It's weird.) https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:62: // The caller does not take ownership of SSL_get_peer_cert_chain's results Nit: period at end. Also, yeesh! I had not realized those two APIs had completely different ownerships. Yay OpenSSL? https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:65: } So the issue is that GetClientCert doesn't let the caller tell if we're returning null because ssl has no cert (ok), or because ssl's cert can't be parsed (bad). One option is to just inline this into the call site. It's pretty short anyway. ScopedX509 cert(SSL_get_peer_certificate(ssl_)); if (cert) { client_cert_ = CreateX509Certificate(cert.get(), SSL_get_peer_cert_chain(ssl_)); if (!client_cert.get()) return ERR_SSL_CLIENT_AUTH_CERT_BAD_FORMAT; } https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:801: if (subj && name == name_start + authority.length()) I'd probably suggest: if (!subj || name != name_start + authority.length()) return ERR_UNEXPECTED; so it's not silent. https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:820: if (sk_X509_num(chain) > 0 && client_cert == nullptr) { Does this need an X509_STORE_CTX_set_error call? If this codepath is hit, we want to reject the certificate too. https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:830: DCHECK(res != ERR_IO_PENDING); Nit: DCHECK_NE https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... File net/socket/ssl_server_socket_unittest.cc (left): https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:119: Stray backspace? (Unless you intentionally removed that line. I don't personally care much one way or another.) https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:22: #endif Move this after line 67. The problem is ssl_server_socket.h might not have included build/build_config.h and so badness ensues. (The style guide says has an exception to include order about conditional includes for this reason.) https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:131: new DrainableIOBuffer(new StringIOBuffer(std::string("0", 1)), 1)); We chatted over IM, but rather than push fake data_ here, you want the pending read callback to get an ERR_CONNECTION_CLOSED or some such. I *think* it'd be sufficient to remove this line and make DoReadCallback say something like: if (read_callback_.is_null()) return; if (closed_) { base::ResetAndReturn(&read_callback_).Run(ERR_CONNECTION_CLOSED); return; } if (data_.empty()) return; // Existing logic. But you'll want to check me on that. https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:282: class TestSSLPrivateKey : public SSLPrivateKey { If you rebase, you can replace this class with a call to this function: https://chromium.googlesource.com/chromium/src/+/master/net/ssl/test_ssl_priv... (The key() method on RSAPrivateKey returns an EVP_PKEY. You can use ScopedEVP_PKEY(EVP_PKEY_up_ref(key->key())) to take a reference possibly we should have made that take a bare pointer, I dunno...) https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:443: cert_names = SSL_load_client_CA_file(GetTestCertsDirectory() ScopedX509NameStack cert_name(SSL_load_.....); https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:447: ASSERT_TRUE(cert_names != nullptr); Nit: This can just be ASSERT_TRUE(cert_names); https://codereview.chromium.org/1474983003/diff/240001/net/ssl/openssl_ssl_ut... File net/ssl/openssl_ssl_util.cc (left): https://codereview.chromium.org/1474983003/diff/240001/net/ssl/openssl_ssl_ut... net/ssl/openssl_ssl_util.cc:161: Guessing you accidentally hit backspace somewhere? :-) (Unless you actively wanted to remove that line? I don't actually care either way.)
https://codereview.chromium.org/1474983003/diff/200001/net/ssl/openssl_ssl_ut... File net/ssl/openssl_ssl_util.h (right): https://codereview.chromium.org/1474983003/diff/200001/net/ssl/openssl_ssl_ut... net/ssl/openssl_ssl_util.h:77: void FreeX509NameStack(STACK_OF(X509_NAME)* ptr); On 2016/02/04 00:40:11, davidben (catching up) wrote: > On 2016/01/29 23:22:14, ryanchung wrote: > > On 2016/01/25 20:56:11, davidben (slow) wrote: > > > Why is this declared both here and in scoped_openssl_types.h? > > > > Hmm, I should remove it from here. > > scoped_openssl_types.h needs to know about the Free functions in > > openssl_ssl_util.cc but it would create a circular dependancy if I just > included > > openssl_ssl_util.h in scoped_openssl_types.h > > > > But then now they'll be declared in scoped_openssl_types.h while being defined > > in openssl_ssl_util.cc > > What would you suggest? > > Hrm. I'd probably either add scoped_openssl_types.cc or maybe just define it in > the header as an inline function. As functions go, they're pretty tiny. Done. Inlined. https://codereview.chromium.org/1474983003/diff/200001/net/url_request/url_re... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1474983003/diff/200001/net/url_request/url_re... net/url_request/url_request_unittest.cc:8291: EXPECT_EQ(0, d.bytes_received()); On 2016/02/04 00:40:11, davidben (catching up) wrote: > On 2016/01/29 23:22:14, ryanchung wrote: > > On 2016/01/25 20:56:12, davidben (slow) wrote: > > > You change the test from successfully connecting to failing to connect, so > > > you're stressing a bit less of the state machine now. I think we should > still > > > have a test which authenticates with no certificate. This is a terrible idea > > for > > > servers to do, but people do do optional client auth for better or worse. > > > > > > (Also if the test were intended to fail, it needs to assert on the error > code > > > and not just check bytes_received.) > > > > If we want to support optional client certificates, we need to add another > flag > > to SSLServerConfig... something like optional_client_cert which will do the > same > > thing as require_client_cert but doesn't set SSL_VERIFY_FAIL_IF_NO_PEER_CERT. > Is > > this what you have in mind? > > > > Oh hrmf, did I ask you do remove that from a previous version? Sorry! :-( Hard > to remember global requirements. > > We probably do want to be able to test that we don't screw this up. > ERR_SSL_PROTOCOL_ERROR could also mean "you spoke the protocol wrong and messed > something up". Maybe an enum? That's what Go's TLS library does: > https://golang.org/pkg/crypto/tls/#ClientAuthType > > enum ClientCertType { > NO_CLIENT_CERT, > OPTIONAL_CLIENT_CERT, > REQUIRE_CLIENT_CERT, > }; > > ClientCertType client_cert; > > Or a pair of booleans works too. I don't have hugely strong opinions. svaldez, > do you care? I think my default would be to go with an enum, but if anyone else > cares, go with that. Ok, using a enum. I changed the test back to what it was originally. https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:47: if (x509_util::GetDER(x, &der_cert)) { On 2016/02/04 00:40:11, davidben (catching up) wrote: > Style nit: typically errors paths are the early return when possible. So > something like: > > if (!x509_util::GetDER(x, &der_cert)) > return nullptr; > der_chain.push_back(der_cert); > > The idea is to keep the main code indented normally and have error paths out of > the way. Done. https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:60: if (!cert.get()) On 2016/02/04 00:40:11, davidben (catching up) wrote: > Nit: .get() isn't needed here. > > (You need it for scoped_refptr, but not scoped_ptr. It's weird.) Done. https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:62: // The caller does not take ownership of SSL_get_peer_cert_chain's results On 2016/02/04 00:40:11, davidben (catching up) wrote: > Nit: period at end. > > Also, yeesh! I had not realized those two APIs had completely different > ownerships. Yay OpenSSL? Done. https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:65: } On 2016/02/04 00:40:11, davidben (catching up) wrote: > So the issue is that GetClientCert doesn't let the caller tell if we're > returning null because ssl has no cert (ok), or because ssl's cert can't be > parsed (bad). > > One option is to just inline this into the call site. It's pretty short anyway. > > ScopedX509 cert(SSL_get_peer_certificate(ssl_)); > if (cert) { > client_cert_ = CreateX509Certificate(cert.get(), > SSL_get_peer_cert_chain(ssl_)); > if (!client_cert.get()) > return ERR_SSL_CLIENT_AUTH_CERT_BAD_FORMAT; > } Done. https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:801: if (subj && name == name_start + authority.length()) On 2016/02/04 00:40:11, davidben (catching up) wrote: > I'd probably suggest: > > if (!subj || name != name_start + authority.length()) > return ERR_UNEXPECTED; > > so it's not silent. Done. https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:820: if (sk_X509_num(chain) > 0 && client_cert == nullptr) { On 2016/02/04 00:40:11, davidben (catching up) wrote: > Does this need an X509_STORE_CTX_set_error call? If this codepath is hit, we > want to reject the certificate too. Done. It seems like the most appropriate error would still be X509_V_ERR_CERT_REJECTED? There doesn't seem to be a generic certificate error code. https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:830: DCHECK(res != ERR_IO_PENDING); On 2016/02/04 00:40:11, davidben (catching up) wrote: > Nit: DCHECK_NE Done. https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... File net/socket/ssl_server_socket_unittest.cc (left): https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:119: On 2016/02/04 00:40:11, davidben (catching up) wrote: > Stray backspace? > > (Unless you intentionally removed that line. I don't personally care much one > way or another.) Done. Unintentional. https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:22: #endif On 2016/02/04 00:40:11, davidben (catching up) wrote: > Move this after line 67. The problem is ssl_server_socket.h might not have > included build/build_config.h and so badness ensues. (The style guide says has > an exception to include order about conditional includes for this reason.) Done. https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:131: new DrainableIOBuffer(new StringIOBuffer(std::string("0", 1)), 1)); On 2016/02/04 00:40:11, davidben (catching up) wrote: > We chatted over IM, but rather than push fake data_ here, you want the pending > read callback to get an ERR_CONNECTION_CLOSED or some such. I *think* it'd be > sufficient to remove this line and make DoReadCallback say something like: > > if (read_callback_.is_null()) > return; > > if (closed_) { > base::ResetAndReturn(&read_callback_).Run(ERR_CONNECTION_CLOSED); > return; > } > > if (data_.empty()) > return; > // Existing logic. > > But you'll want to check me on that. Done. That works. https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:282: class TestSSLPrivateKey : public SSLPrivateKey { On 2016/02/04 00:40:11, davidben (catching up) wrote: > If you rebase, you can replace this class with a call to this function: > https://chromium.googlesource.com/chromium/src/+/master/net/ssl/test_ssl_priv... > > (The key() method on RSAPrivateKey returns an EVP_PKEY. You can use > ScopedEVP_PKEY(EVP_PKEY_up_ref(key->key())) to take a reference possibly we > should have made that take a bare pointer, I dunno...) Done. https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:443: cert_names = SSL_load_client_CA_file(GetTestCertsDirectory() On 2016/02/04 00:40:11, davidben (catching up) wrote: > ScopedX509NameStack cert_name(SSL_load_.....); Done. https://codereview.chromium.org/1474983003/diff/240001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:447: ASSERT_TRUE(cert_names != nullptr); On 2016/02/04 00:40:11, davidben (catching up) wrote: > Nit: This can just be ASSERT_TRUE(cert_names); Done. https://codereview.chromium.org/1474983003/diff/240001/net/ssl/openssl_ssl_ut... File net/ssl/openssl_ssl_util.cc (left): https://codereview.chromium.org/1474983003/diff/240001/net/ssl/openssl_ssl_ut... net/ssl/openssl_ssl_util.cc:161: On 2016/02/04 00:40:12, davidben (catching up) wrote: > Guessing you accidentally hit backspace somewhere? :-) > > (Unless you actively wanted to remove that line? I don't actually care either > way.) Done. Probably a trigger-happy backspace.
lgtm! Thanks so much for your patience and I'm so so sorry about how long that took. This was way too long on our end. https://codereview.chromium.org/1474983003/diff/310001/net/cert/mock_client_c... File net/cert/mock_client_cert_verifier.cc (right): https://codereview.chromium.org/1474983003/diff/310001/net/cert/mock_client_c... net/cert/mock_client_cert_verifier.cc:27: RuleList::const_iterator it; Unused variable? https://codereview.chromium.org/1474983003/diff/310001/net/socket/ssl_server_... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1474983003/diff/310001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:704: // NO_CLIENT_CERT Nit: I would instead do: case SSLServerConfig::ClientCertType::NO_CLIENT_CERT: without a default case. The reason is the compiler will notice if we add a case and forget to handle it. https://codereview.chromium.org/1474983003/diff/310001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:802: SSLServerConfig::ClientCertType::OPTIONAL_CLIENT_CERT) && Nit/optional: Maybe ssl_server_config_.client_cert_type != ...::NO_CLIENT_CERT? Probably any other value we add will also interpret this field. https://codereview.chromium.org/1474983003/diff/310001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:829: if (sk_X509_num(chain) > 0 && client_cert == nullptr) { I think probably this should just be if (!client_cert.get()) { without the sk_X509_num check. I don't believe this is possible but, if it were, we would want to reject it and having ClientCertVerifier account for the null case seems kind of weird. https://codereview.chromium.org/1474983003/diff/310001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:839: base::Bind(&DoNothingOnCompletion), &ignore_async); Optional: I think you can just write CompletionCallback() rather than Bind an explicit no-op function. We're assuming this always completes synchronously anyway and because you destroy the Request immediately, we should be guaranteed the callback is never called. (//net async functions typically do *not* call the callback in the synchronous case, only the asynchronous case.) https://codereview.chromium.org/1474983003/diff/310001/net/socket/ssl_server_... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1474983003/diff/310001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:388: if (cert_file_name && private_key_file_name) { No need for the null checks here. I think you could write this as just: client_ssl_config_.send_client_cert = true; client_ssl_config_.client_cert = ImportCertFromFile(...); ASSERT_TRUE(client_ssl_config_.client_cert); scoped_ptr<crypto::RSAPrivateKey> key = ReadTestKey(private_key_file_name); ASSERT_TRUE(key); client_ssl_config_.client_private_key = WrapOpenSSLPrivateKey(crypto::ScopedEVP_PKEY(EVP_PKEY_up_ref(key->key()))); (I don't think it hugely matters, but it's probably good to explicitly ASSERT_TRUE rather than crash?) https://codereview.chromium.org/1474983003/diff/310001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:432: key_string.length())); [Ah, our constant switching for char and uint8_t. We really need to stop doing that in our APIs so this isn't needed everywhere. :-/ ] https://codereview.chromium.org/1474983003/diff/310001/net/ssl/openssl_ssl_ut... File net/ssl/openssl_ssl_util.cc (right): https://codereview.chromium.org/1474983003/diff/310001/net/ssl/openssl_ssl_ut... net/ssl/openssl_ssl_util.cc:241: return ScopedX509Stack(); Nit: We can write nullptr instead of ScopedFoo() these days. https://codereview.chromium.org/1474983003/diff/310001/net/ssl/openssl_ssl_ut... net/ssl/openssl_ssl_util.cc:245: } Nit: newline https://codereview.chromium.org/1474983003/diff/310001/net/ssl/ssl_server_con... File net/ssl/ssl_server_config.h (right): https://codereview.chromium.org/1474983003/diff/310001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:64: // Set the requirement for client certificates during handshake Nit: period at end, "Set" -> "Sets" https://codereview.chromium.org/1474983003/diff/310001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:72: // Provides the CertificateVerifier that is to be used to verify Nit: CertificateVerifier -> ClientCertVerifier https://codereview.chromium.org/1474983003/diff/310001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:76: // This field is meaningful only if client certificates are required. Nit: are required -> are requested? Or maybe "if client_cert_type is set" https://codereview.chromium.org/1474983003/diff/310001/net/url_request/url_re... File net/url_request/url_request_unittest.cc (left): https://codereview.chromium.org/1474983003/diff/310001/net/url_request/url_re... net/url_request/url_request_unittest.cc:8400: ssl_config.require_client_cert = true; (Oh, pfft. Did we call this require_client_cert but actually have it mean request_client_cert? Whoops.)
Thanks! https://codereview.chromium.org/1474983003/diff/310001/net/cert/mock_client_c... File net/cert/mock_client_cert_verifier.cc (right): https://codereview.chromium.org/1474983003/diff/310001/net/cert/mock_client_c... net/cert/mock_client_cert_verifier.cc:27: RuleList::const_iterator it; On 2016/02/17 22:46:03, davidben wrote: > Unused variable? Done. https://codereview.chromium.org/1474983003/diff/310001/net/socket/ssl_server_... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1474983003/diff/310001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:704: // NO_CLIENT_CERT On 2016/02/17 22:46:03, davidben wrote: > Nit: I would instead do: > > case SSLServerConfig::ClientCertType::NO_CLIENT_CERT: > > without a default case. The reason is the compiler will notice if we add a case > and forget to handle it. Done. https://codereview.chromium.org/1474983003/diff/310001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:802: SSLServerConfig::ClientCertType::OPTIONAL_CLIENT_CERT) && On 2016/02/17 22:46:03, davidben wrote: > Nit/optional: Maybe ssl_server_config_.client_cert_type != ...::NO_CLIENT_CERT? > Probably any other value we add will also interpret this field. Done. https://codereview.chromium.org/1474983003/diff/310001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:829: if (sk_X509_num(chain) > 0 && client_cert == nullptr) { On 2016/02/17 22:46:03, davidben wrote: > I think probably this should just be > > if (!client_cert.get()) { > > without the sk_X509_num check. I don't believe this is possible but, if it were, > we would want to reject it and having ClientCertVerifier account for the null > case seems kind of weird. Done. https://codereview.chromium.org/1474983003/diff/310001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:839: base::Bind(&DoNothingOnCompletion), &ignore_async); On 2016/02/17 22:46:03, davidben wrote: > Optional: I think you can just write CompletionCallback() rather than Bind an > explicit no-op function. We're assuming this always completes synchronously > anyway and because you destroy the Request immediately, we should be guaranteed > the callback is never called. > > (//net async functions typically do *not* call the callback in the synchronous > case, only the asynchronous case.) Done. https://codereview.chromium.org/1474983003/diff/310001/net/socket/ssl_server_... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1474983003/diff/310001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:388: if (cert_file_name && private_key_file_name) { On 2016/02/17 22:46:03, davidben wrote: > No need for the null checks here. I think you could write this as just: > > client_ssl_config_.send_client_cert = true; > client_ssl_config_.client_cert = ImportCertFromFile(...); > ASSERT_TRUE(client_ssl_config_.client_cert); > scoped_ptr<crypto::RSAPrivateKey> key = ReadTestKey(private_key_file_name); > ASSERT_TRUE(key); > client_ssl_config_.client_private_key = > WrapOpenSSLPrivateKey(crypto::ScopedEVP_PKEY(EVP_PKEY_up_ref(key->key()))); > > (I don't think it hugely matters, but it's probably good to explicitly > ASSERT_TRUE rather than crash?) Done. https://codereview.chromium.org/1474983003/diff/310001/net/ssl/openssl_ssl_ut... File net/ssl/openssl_ssl_util.cc (right): https://codereview.chromium.org/1474983003/diff/310001/net/ssl/openssl_ssl_ut... net/ssl/openssl_ssl_util.cc:241: return ScopedX509Stack(); On 2016/02/17 22:46:03, davidben wrote: > Nit: We can write nullptr instead of ScopedFoo() these days. Done. https://codereview.chromium.org/1474983003/diff/310001/net/ssl/openssl_ssl_ut... net/ssl/openssl_ssl_util.cc:245: } On 2016/02/17 22:46:03, davidben wrote: > Nit: newline Done. https://codereview.chromium.org/1474983003/diff/310001/net/ssl/ssl_server_con... File net/ssl/ssl_server_config.h (right): https://codereview.chromium.org/1474983003/diff/310001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:64: // Set the requirement for client certificates during handshake On 2016/02/17 22:46:03, davidben wrote: > Nit: period at end, "Set" -> "Sets" Done. https://codereview.chromium.org/1474983003/diff/310001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:72: // Provides the CertificateVerifier that is to be used to verify On 2016/02/17 22:46:03, davidben wrote: > Nit: CertificateVerifier -> ClientCertVerifier Done. https://codereview.chromium.org/1474983003/diff/310001/net/ssl/ssl_server_con... net/ssl/ssl_server_config.h:76: // This field is meaningful only if client certificates are required. On 2016/02/17 22:46:03, davidben wrote: > Nit: are required -> are requested? Or maybe "if client_cert_type is set" Done.
Description was changed from ========== Support for client certs in ssl_server_socket. Allows SSL server sockets (openssl only) to support requesting and receiving/verifying client auth certificates. BUG=415663 This is a continuation of issue 994743003 due to change of committer. https://codereview.chromium.org/994743003/ ========== to ========== Support for client certs in ssl_server_socket. Allows SSL server sockets (openssl only) to support requesting and receiving/verifying client auth certificates. BUG=415663 ==========
ryanchung@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Hi Devlin, could you please review chrome/browser/extensions/ Thanks!
extensions lgtm
The CQ bit was checked by ryanchung@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1474983003/#ps330001 (title: "Fixed nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1474983003/330001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1474983003/330001
Message was sent while issue was closed.
Description was changed from ========== Support for client certs in ssl_server_socket. Allows SSL server sockets (openssl only) to support requesting and receiving/verifying client auth certificates. BUG=415663 ========== to ========== Support for client certs in ssl_server_socket. Allows SSL server sockets (openssl only) to support requesting and receiving/verifying client auth certificates. BUG=415663 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:330001)
Message was sent while issue was closed.
Description was changed from ========== Support for client certs in ssl_server_socket. Allows SSL server sockets (openssl only) to support requesting and receiving/verifying client auth certificates. BUG=415663 ========== to ========== Support for client certs in ssl_server_socket. Allows SSL server sockets (openssl only) to support requesting and receiving/verifying client auth certificates. BUG=415663 Committed: https://crrev.com/987b2ffd6da82a8af2d7dce89db5ec8e372702df Cr-Commit-Position: refs/heads/master@{#376292} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/987b2ffd6da82a8af2d7dce89db5ec8e372702df Cr-Commit-Position: refs/heads/master@{#376292} |
