Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
173853014:
Make OpenSSL UpdateServerCert() OS independent. (Closed)
|
Created:
6 years, 10 months ago by haavardm Modified:
6 years, 9 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMake OpenSSL UpdateServerCert() OS independent.
UpdateServerCert currently creates the server cert chain
directly from the openssl struct X509. This works since
OSCertHandle currently is an OpenSSL X509 struct when
OpenSSL is used on Android and Linux.
This patch makes the UpdateServerCert() OS independent by creating
the X509Certificate from DER data instead of OSCertHandle to make
it compile on the other platforms when USE_OPENSSL is off.
Keep the USE_OPENSSL code to avoid converting back and forth
between X509 and DER twice and OsCertHandle is X509.
I see that there is a DER cache in x509_certificate_openssl.cc
which could have simplified the patch a bit. However, if I understand
a comment correctly, it shouldn't be mixed with certificates that comes
from network, which is the case here. Also, that API is not exposed.
Also remove some unused NSS code from x509_certificate_mac.cc.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257449
Patch Set 1 #
Total comments: 23
Patch Set 2 : Fixes after review comments #
Total comments: 6
Patch Set 3 : New PeerCertificateChain based on Ryan's design #Patch Set 4 : Free Openssl strings #
Total comments: 7
Patch Set 5 : Cleanp and fix id2_x509 conversion test #Patch Set 6 : Added test case for retrieving unverified server cert chain. #
Total comments: 50
Patch Set 7 : More cleanup and fixes #
Total comments: 6
Patch Set 8 : Changed from pointer to scoped_refptr and moved code out of class. #
Total comments: 6
Patch Set 9 : Fixed nits and added comment #Patch Set 10 : Fixed android compile error #
Total comments: 1
Patch Set 11 : Moved FreeX509Stack back inside class using friend to please gcc-4.6 #
Messages
Total messages: 36 (0 generated)
This patch is for the OpenSSL transition. Please review. I didn't reference any bug number for now as I wasn't sure if I should reference the 338885 meta bug and friends or simply create a new blocker bug.
Thanks for taking a stab at this Havard! I think we'll want to continue to parallel the design choices in _nss for the time being, until we deprecate things. I've suggested a few changes on the overall design to that effect. https://codereview.chromium.org/173853014/diff/1/net/cert/x509_certificate_ma... File net/cert/x509_certificate_mac.cc (left): https://codereview.chromium.org/173853014/diff/1/net/cert/x509_certificate_ma... net/cert/x509_certificate_mac.cc:219: }; Unrelated cleanups are best left for a separate CL. https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:921: return server_cert_.get(); So, this is wrong :) Compare with the NSS implementation - the server cert can change in a renegotiation. Note that for NSS, we track _both_ the 'socket' cert (eg: PeerCertificateChain "server_cert_chain") and the OS cert (eg: X509Certificate). We should be adopting the same model here. https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:944: } net/ style: Don't include braces on single-line conditionals (eg: look at line 935-936 or 920-921) https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:948: OPENSSL_free(cert_data); Rather than copying all of these to strings, then constructing string pieces, we should do a single allocation (using the OPENSSL memory functions), create string pieces from them, then free with OPENSSL_free afterwards (using the scoped_ptr<> or the like to set an appropriate free proc) https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:971: DCHECK(server_cert_.get()); This DCHECK is going to have to go when adding !OpenSSL support, as server_cert_ may be NULL now (because the underlying OS can't represent it - eg: Windows and Mac will choke parsing dates too far in the future)
Thanks for the quick review, a couple of questions below. https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:921: return server_cert_.get(); On 2014/02/21 22:34:38, Ryan Sleevi wrote: > So, this is wrong :) > > Compare with the NSS implementation - the server cert can change in a > renegotiation. Ah, ok. I tried to do this fix without changing the existing behavior. > > Note that for NSS, we track _both_ the 'socket' cert (eg: PeerCertificateChain > "server_cert_chain") and the OS cert (eg: X509Certificate). We should be > adopting the same model here. Do you mean I Should try to add a Openssl version of PeerCertificateChain() in this patch or should that be added later? https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:948: OPENSSL_free(cert_data); On 2014/02/21 22:34:38, Ryan Sleevi wrote: > Rather than copying all of these to strings, then constructing string pieces, we > should do a single allocation (using the OPENSSL memory functions), create > string pieces from them, then free with OPENSSL_free afterwards (using the > scoped_ptr<> or the like to set an appropriate free proc) Right. Just to clear up one detail: I assume you mean one OPENSSL memory allocation per certificate der string? https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:966: chain_ref.push_back(base::StringPiece(cert_chain[0])); Ouch, bad typo right there (cert_chain[0]). Will fix if that code is still there after the other fixes is done. Is the missing OpenSSL server socket the reason this was not caught by the unittests? https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:971: DCHECK(server_cert_.get()); On 2014/02/21 22:34:38, Ryan Sleevi wrote: > This DCHECK is going to have to go when adding !OpenSSL support, as server_cert_ > may be NULL now (because the underlying OS can't represent it - eg: Windows and > Mac will choke parsing dates too far in the future) Ok. I'll move it into the USE_OPENSSL code.
https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:921: return server_cert_.get(); On 2014/02/24 18:54:31, Håvard Molland wrote: > On 2014/02/21 22:34:38, Ryan Sleevi wrote: > > So, this is wrong :) > > > > Compare with the NSS implementation - the server cert can change in a > > renegotiation. > > Ah, ok. I tried to do this fix without changing the existing behavior. Yeah, but we can fix this while we're having to (somewhat rewrite) this function > > > > > Note that for NSS, we track _both_ the 'socket' cert (eg: PeerCertificateChain > > "server_cert_chain") and the OS cert (eg: X509Certificate). We should be > > adopting the same model here. > > Do you mean I Should try to add a Openssl version of PeerCertificateChain() in > this patch or should that be added later? > Adding it now. https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:948: OPENSSL_free(cert_data); On 2014/02/24 18:54:31, Håvard Molland wrote: > On 2014/02/21 22:34:38, Ryan Sleevi wrote: > > Rather than copying all of these to strings, then constructing string pieces, > we > > should do a single allocation (using the OPENSSL memory functions), create > > string pieces from them, then free with OPENSSL_free afterwards (using the > > scoped_ptr<> or the like to set an appropriate free proc) > > Right. Just to clear up one detail: I assume you mean one OPENSSL memory > allocation per certificate der string? I'm not sure I grokked your clarification. What I mean is, instead of i2d_X509 (one memory allocation) std::string ctor (one memory allocation) OPENSSL_free (one memory deallocation) base::StringPiece (zero memory allocation) std::string dtor (one memory deallocation) (aka 2 allocations / 2 deallocations per cert) You re-arrange it to i2d_X509 (one memory allocation) base::StringPiece scoped_ptr<> dtor (one memory deallocation / OpenSSL helper) for 1 allocation/cert https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:966: chain_ref.push_back(base::StringPiece(cert_chain[0])); On 2014/02/24 18:54:31, Håvard Molland wrote: > Ouch, bad typo right there (cert_chain[0]). Will fix if that code is still there > after the other fixes is done. Is the missing OpenSSL server socket the reason > this was not caught by the unittests? No. There's no test that covers this. The CertVerifyProcTest.VerifyReturn* tests only test the CertVerifier, and the SSLClientSocketTest.VerifyReturnChainProperlyOrdered only does a mapping on the server cert, not on the additional certs. We don't have any tests that do 1:1 mapping of the server certs to returned, since our cert verifier code has to assume it may only get the server cert in the future. https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:971: DCHECK(server_cert_.get()); On 2014/02/24 18:54:31, Håvard Molland wrote: > On 2014/02/21 22:34:38, Ryan Sleevi wrote: > > This DCHECK is going to have to go when adding !OpenSSL support, as > server_cert_ > > may be NULL now (because the underlying OS can't represent it - eg: Windows > and > > Mac will choke parsing dates too far in the future) > > Ok. I'll move it into the USE_OPENSSL code. > I was saying to remove it entirely.
https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:948: OPENSSL_free(cert_data); On 2014/02/24 19:02:47, Ryan Sleevi wrote: > On 2014/02/24 18:54:31, Håvard Molland wrote: > > On 2014/02/21 22:34:38, Ryan Sleevi wrote: > > > Rather than copying all of these to strings, then constructing string > pieces, > > we > > > should do a single allocation (using the OPENSSL memory functions), create > > > string pieces from them, then free with OPENSSL_free afterwards (using the > > > scoped_ptr<> or the like to set an appropriate free proc) > > > > Right. Just to clear up one detail: I assume you mean one OPENSSL memory > > allocation per certificate der string? > > I'm not sure I grokked your clarification. > > What I mean is, instead of > i2d_X509 (one memory allocation) > std::string ctor (one memory allocation) > OPENSSL_free (one memory deallocation) > base::StringPiece (zero memory allocation) > std::string dtor (one memory deallocation) > > (aka 2 allocations / 2 deallocations per cert) > > You re-arrange it to > > i2d_X509 (one memory allocation) > base::StringPiece > scoped_ptr<> dtor (one memory deallocation / OpenSSL helper) > > for 1 allocation/cert That's how I understood it yes.
Review comments on patch set 1: https://codereview.chromium.org/173853014/diff/1/net/cert/x509_certificate_ma... File net/cert/x509_certificate_mac.cc (right): https://codereview.chromium.org/173853014/diff/1/net/cert/x509_certificate_ma... net/cert/x509_certificate_mac.cc:25: #include "crypto/nss_util.h" This header is NSS-specific and should also be removed. In fact, I believe we can remove it now. I'll look into that. https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:959: std::string(reinterpret_cast<char*>(cert_data), cert_data_length)); If you directly push a base::StringPiece to chain_ref here: chain_ref.push_back(base::StringPiece(reinterpret_cast<char*>(cert_data), cert_data_length)); then after you are done with chain_ref, you can iterate over it and call OPENSSL_free: for (size_t i = 0; i < chain_ref.size();i++) OPENSSL_free(chain_ref[i].data()); You'll also need to do this before the early return on line 956. I don't know how to free the memory more elegantly using scoped_ptr<>.
https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:923: crypto::ScopedOpenSSL<X509, X509_free> cert(SSL_get_peer_certificate(ssl_)); Is this needed? I suddenly realized that cert.get() is the same cert as the first certificate in the stack returned by SSL_get_peer_cert_chain(ssl_). I had a look at the OpenSSL documentation, which seems to say that if SSL_get_peer_cert_chain is used on client side, the first certificate is the server cert. https://www.openssl.org/docs/ssl/SSL_get_peer_cert_chain.html: "If called on the client side, the stack also contains the peer's certificate" https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:959: std::string(reinterpret_cast<char*>(cert_data), cert_data_length)); For that to work one must actually using C-type cast, which seems a bit ugly: OPENSSL_free((unsigned char*)char_ref[i].data()); Another solution is to keep two vectors, one with scoped_ptr<unsigned char, opensslfree> objects, and one with base::StringPiece to keep track of the string length. Or perhaps simply create a new class. On 2014/02/25 01:13:09, wtc wrote: > > If you directly push a base::StringPiece to chain_ref here: > > chain_ref.push_back(base::StringPiece(reinterpret_cast<char*>(cert_data), > cert_data_length)); > > then after you are done with chain_ref, you can iterate over it and call > OPENSSL_free: > > for (size_t i = 0; i < chain_ref.size();i++) > OPENSSL_free(chain_ref[i].data()); > > You'll also need to do this before the early return on line 956. > > I don't know how to free the memory more elegantly using scoped_ptr<>.
https://codereview.chromium.org/173853014/diff/80001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_openssl.cc (left): https://codereview.chromium.org/173853014/diff/80001/net/socket/ssl_client_so... net/socket/ssl_client_socket_openssl.cc:923: crypto::ScopedOpenSSL<X509, X509_free> cert(SSL_get_peer_certificate(ssl_)); I removed this, as it seems that server certificate is the first certificate in the stack returned by by SSL_get_peer_cert_chain(). https://www.openssl.org/docs/ssl/SSL_get_peer_cert_chain.html: "If called on the client side, the stack also contains the peer's certificate. Some basic testing confirmed that SSL_get_peer_certificate(ssl_) == sk_X509_value(SSL_get_peer_cert_chain(ssl_), 0))
On 2014/02/25 19:37:37, Håvard Molland wrote: > https://codereview.chromium.org/173853014/diff/80001/net/socket/ssl_client_so... > File net/socket/ssl_client_socket_openssl.cc (left): > > https://codereview.chromium.org/173853014/diff/80001/net/socket/ssl_client_so... > net/socket/ssl_client_socket_openssl.cc:923: crypto::ScopedOpenSSL<X509, > X509_free> cert(SSL_get_peer_certificate(ssl_)); > I removed this, as it seems that server certificate is the first certificate in > the stack returned by by SSL_get_peer_cert_chain(). > > https://www.openssl.org/docs/ssl/SSL_get_peer_cert_chain.html: > "If called on the client side, the stack also contains the peer's > certificate. > > Some basic testing confirmed that SSL_get_peer_certificate(ssl_) == > sk_X509_value(SSL_get_peer_cert_chain(ssl_), 0)) Yeah, confirmed with the implementation. The exception is SSL2, but we'll never hit that.
https://codereview.chromium.org/173853014/diff/80001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/80001/net/socket/ssl_client_so... net/socket/ssl_client_socket_openssl.cc:401: std::vector<ScopedX509> x509_certs_; Storing a vector of ScopedX509 doesn't work before C++11. Rather than having to worry about this bookkeeping - especially with AsStringPieceVector - why not simplify? void sk_X509_free_helper(STACK_OF(X509)* stack) { sk_X509_pop_free(stack, X509_free); } class SSLClientSocketOpenSSL::PeerCertificateChain { public: explicit PeerCertificateChain(SSL* ssl); ~PeerCertificateChain(); void Reset(SSL* ssl); const scoped_ptr<X509Certificate>& AsNativeChain() const { return native_chain_; } size_t size() const { if (!openssl_chain_.get()) return 0; return sk_X509_num(openssl_chain_.get()); } X509* operator[](size_t index) const { DCHECK_LT(index, size()); return sk_X509_value(openssl_chain_.get(), index); } private: crypto::ScopedOpenSSL<STACK_OF(X509), sk_X509_free_helper> openssl_chain_; scoped_refptr<X509Certificate> native_chain_; }; PeerCertificateChain::PeerCertificateChain(SSL* ssl) { Reset(ssl); } PeerCertificateChain::~PeerCertificateChain() {} void PeerCertificateChain::Reset(SSL* ssl) { openssl_chain_.reset(); native_chain_ = NULL; if (ssl == NULL) return; STACK_OF(X509)* chain = SSL_get_peer_cert_chain(ssl); if (!chain) return; openssl_chain_.reset(sk_X509_dup(chain)); std::vector<base::StringPiece> der_chain; for (int i = 0; i < sk_X509_num(openssl_chain_.get()); ++i) { X509* x = sk_X509_value(openssl_chain_.get(), i); CRYPTO_add(&x->references, 1, CRYPTO_LOCK_X509); unsigned char* cert_data = NULL; int cert_data_length = i2d_X509(x, &cert_data); if (cert_data_length && cert_data) { der_chain.push_back(base::StringPiece(reinterpret_cast<char*>(cert_data), cert_data_length); } } native_chain_ = X509Certificate::CreateFromDERCertChain(der_chain); for (size_t i = 0; i < der_chain.size(); ++i) { OPENSSL_free(der_chain[i].data()); } if (native_chain_.size() != static_cast<size_t>(sk_X509_num(openssl_chain_.get()))) { openssl_chain_.reset(NULL); native_chain_ = NULL; } } This also allows for support for the operator= / Equals support to be added in. https://codereview.chromium.org/173853014/diff/80001/net/socket/ssl_client_so... net/socket/ssl_client_socket_openssl.cc:1014: X509Certificate::CreateFromHandle(sk_X509_value(chain, 0), intermediates); if |chain| is false, or sk_X509_num(chain) is 0, this will crash (eg: compare line 1009)
https://codereview.chromium.org/173853014/diff/80001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/80001/net/socket/ssl_client_so... net/socket/ssl_client_socket_openssl.cc:401: std::vector<ScopedX509> x509_certs_; Seems good. I guess I got a little too focused on getting the scoped_ptr in there :) On 2014/02/25 20:27:09, Ryan Sleevi wrote: > Storing a vector of ScopedX509 doesn't work before C++11. > > Rather than having to worry about this bookkeeping - especially with > AsStringPieceVector - why not simplify? > > void sk_X509_free_helper(STACK_OF(X509)* stack) { > sk_X509_pop_free(stack, X509_free); > } > > class SSLClientSocketOpenSSL::PeerCertificateChain { > public: > explicit PeerCertificateChain(SSL* ssl); > ~PeerCertificateChain(); > > void Reset(SSL* ssl); > const scoped_ptr<X509Certificate>& AsNativeChain() const { > return native_chain_; > } > > size_t size() const { > if (!openssl_chain_.get()) > return 0; > return sk_X509_num(openssl_chain_.get()); > } > > X509* operator[](size_t index) const { > DCHECK_LT(index, size()); > return sk_X509_value(openssl_chain_.get(), index); > } > > private: > crypto::ScopedOpenSSL<STACK_OF(X509), sk_X509_free_helper> openssl_chain_; > scoped_refptr<X509Certificate> native_chain_; > }; > > PeerCertificateChain::PeerCertificateChain(SSL* ssl) { > Reset(ssl); > } > > PeerCertificateChain::~PeerCertificateChain() {} > > void PeerCertificateChain::Reset(SSL* ssl) { > openssl_chain_.reset(); > native_chain_ = NULL; > > if (ssl == NULL) > return; > > STACK_OF(X509)* chain = SSL_get_peer_cert_chain(ssl); > if (!chain) > return; > > openssl_chain_.reset(sk_X509_dup(chain)); > > std::vector<base::StringPiece> der_chain; > for (int i = 0; i < sk_X509_num(openssl_chain_.get()); ++i) { > X509* x = sk_X509_value(openssl_chain_.get(), i); > CRYPTO_add(&x->references, 1, CRYPTO_LOCK_X509); > > unsigned char* cert_data = NULL; > int cert_data_length = i2d_X509(x, &cert_data); > if (cert_data_length && cert_data) { > der_chain.push_back(base::StringPiece(reinterpret_cast<char*>(cert_data), > cert_data_length); > } > } > > native_chain_ = X509Certificate::CreateFromDERCertChain(der_chain); > > for (size_t i = 0; i < der_chain.size(); ++i) { > OPENSSL_free(der_chain[i].data()); > } > > if (native_chain_.size() != > static_cast<size_t>(sk_X509_num(openssl_chain_.get()))) { > openssl_chain_.reset(NULL); > native_chain_ = NULL; > } > } > > > This also allows for support for the operator= / Equals support to be added in.
Did you want to add a test to cover the bug(s) you found? https://codereview.chromium.org/173853014/diff/120001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/120001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:414: OPENSSL_free((void*)der_chain[i].data()); You shouldn't need to cast to void* here. If you do, C++ style. https://codereview.chromium.org/173853014/diff/120001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:419: static_cast<size_t>(sk_X509_num(openssl_chain_.get()))) { Pathological case: What happens when .size() == numeric_limits<size_t>::max() Also, what are you trying to test here? The goal with testing der_chain is just to make sure that all the i2d_X509 succeeded - which should 'never fail'. The conversion to native certs "may" fail - but thats ok.
https://codereview.chromium.org/173853014/diff/120001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/120001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:414: OPENSSL_free((void*)der_chain[i].data()); On 2014/02/27 03:07:29, Ryan Sleevi wrote: > You shouldn't need to cast to void* here. If you do, C++ style. chain[i].data() is 'const char*' while OPENSSL_Free takes non-const 'void*' . I don't see how this can be done without casting, but I'll can use const_cast<char*> instead. (something I do so rarely I frankly forgot it existed) https://codereview.chromium.org/173853014/diff/120001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:419: static_cast<size_t>(sk_X509_num(openssl_chain_.get()))) { On 2014/02/27 03:07:29, Ryan Sleevi wrote: > Pathological case: What happens when .size() == numeric_limits<size_t>::max() I think we would have a crash or freeze long before that happens. But I don't see that +1 makes a difference here since the result is size_t anyway. That is if GetChain().size() existed it would return GetIntermediateCertificates().size() + 1. Will remove anyway, see below > > Also, what are you trying to test here? The goal with testing der_chain is just > to make sure that all the i2d_X509 succeeded - which should 'never fail'. The > conversion to native certs "may" fail - but thats ok. Right. I thought the point was to test the whole conversion chain (X509->der->Os). I forgot that native_chain could 'legally' end up with different chain size after sorting out the real chain. I followed your example which used native_chain. I guess that was a typo, and I'll put in the der_chain instead (which actually has the size() function). That would test that all i2d_X509 are successful.
On 2014/02/27 03:07:29, Ryan Sleevi wrote: > Did you want to add a test to cover the bug(s) you found? Yes, I'll be happy to do that. And I assume you want that too into this CL? (it would have to wait until Monday though)
https://codereview.chromium.org/173853014/diff/120001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/120001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:414: OPENSSL_free((void*)der_chain[i].data()); On 2014/02/27 10:46:28, Håvard Molland wrote: > > I don't see how this can be done without casting, but I'll can use > const_cast<char*> instead. Yes, this requires a cast, and const_cast<char*> is the right cast to use.
Test case for retrieving server chain from SSL stack added. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_unittest.cc:527: class SSLClientSocketCertRequestInfoTest : public SSLClientSocketTest { Moved this class from below into the anonymous namespace https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_unittest.cc:584: } // namespace Moved tests out of the anonymous namespace as it gave trouble for FRIEND_TEST_ALL_PREFIXES
Ping
Patch set 6 LGTM. Please wait for Ryan's approval. https://codereview.chromium.org/173853014/diff/180001/net/socket/socket_test_... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/173853014/diff/180001/net/socket/socket_test_... net/socket/socket_test_util.cc:780: // Returns the certificate chain as presented by server. Nit: omit this comment. We just need to document this method in the .h file of the base class. An implementation of this method only needs to be documented if it does something unusual. https://codereview.chromium.org/173853014/diff/180001/net/socket/socket_test_... File net/socket/socket_test_util.h (right): https://codereview.chromium.org/173853014/diff/180001/net/socket/socket_test_... net/socket/socket_test_util.h:705: virtual const scoped_refptr<X509Certificate> GetUnverifiedServerCertificate() Should this method be public? The base class SSLClientSocket declares this method as private. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:171: virtual const scoped_refptr<X509Certificate> GetUnverifiedServerCertificate() 1. Nit: change "ServerCertificate" to "ServerCertChain" to make it clear the returned X509Certificate object also contains the CA certificates presented by the server. 2. I am not familiar with this aspect of C++, but it seems strange to see a *private* virtual method. I would think this needs to be protected to allow subclasses to override this method. Perhaps the "private" keyword just means only this class can *call* this method, and doesn't care which class can override this method. 3. Nit: in the comment you may want to point out that the cert returned by the GetSSLInfo method is a verified certificate chain, which may be different. This difference may not be obvious to someone unfamiliar with this code. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_nss.h (right): https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.h:84: virtual const scoped_refptr<X509Certificate> GetUnverifiedServerCertificate() 1. This method is listed under the "SSLSocket implementation" section (see line 76), but it is a method of SSLClientSocket. So it is listed under the wrong section. 2. More important: should this method be public? The base class SSLClientSocket declares this method as private. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (left): https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:921: return server_cert_.get(); Please confirm that this "one-time initialization" check is not needed. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:335: return native_chain_; Nit: it's not clear what "native" means here. I would think OpenSSL is more "native" than Chromium's X509Certificate is. Update: I now understand what you meant by "native". Please put the entire PeerCertificateChain class inside #if !defined(USE_OPENSSL). A comment that point out that in this case (USE_OPENSSL is not defined) X509Certificate contains the native OS certificate handles would still be helpful. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:344: // Returns the certificate chain as presented by server. This comment seems wrong; it doesn't seem to describe the [] operator. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:351: static void FreeX509Stack(STACK_OF(X509) * chain) { Nit: delete the space before '*'. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:418: static_cast<size_t>(sk_X509_num(openssl_chain_.get()))) { Just to check my understanding: this can only happen if the check on line 406 inside the first for loop fails, right? Otherwise, |der_chain| and |openssl_chain_| should be of the same size. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:452: server_cert_chain_(new PeerCertificateChain(NULL)), Is it necessary to create an empty PeerCertificateChain object? If not, this initializer can be omitted. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:453: server_cert_(NULL), Nit: we usually don't initialize a scoped_refptr to NULL like this. The scoped_refptr constructor that takes no argument initializes the internal pointer to NULL automatically. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:1023: #ifdef USE_OPENSSL NitL: the Chromium coding style recommends always using the more verbose #if defined(FOO). The reason is that it can be more easily extended with other || defined(BAR) if necessary. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:1026: // increment the reference so sk_X509_free does not need to be called. Nit: I think the "Unlike SSL_get_peer_certificate, " part can be removed from this comment. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:1038: server_cert_ = server_cert_chain_->AsNativeChain(); IMPORTANT: comparing the two implementations of this method, I see one difference: the first implementation sets server_cert_ but does not set server_cert_chain_. Is this a bug? If this is not a bug, it seems to imply that in the second implementation, server_cert_chain_ just needs to be a local variable rather than a class member. If so, this will also solve the problem of the forward declaration of PeerCertificateChain in ssl_client_socket_openssl.h. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:34: class PeerCertificateChain; This class is in the net namespace and is defined in the ssl_client_socket_openssl.cc file. This is uncommon in the Chromium source tree. Usually we would nest this kind of class inside SSLClientSocketOpenSSL. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:64: virtual const scoped_refptr<X509Certificate> GetUnverifiedServerCertificate() Here you list the method under an "SSLClientSocket implementation" section correctly. So the only issue is whether this method should be public or private. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_unittest.cc:576: static bool LogContainsSSLConnectEndEvent( Nit: you can remove "static" because this function is in the anonymous namespace. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_unittest.cc:1753: // Test that the server certificates are properly retrieved from the underlying Nit: I don't understand why it is important to test this explicitly. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_unittest.cc:1756: // The connection does not have to be successful Nit: missing a period (.) at the end of the sentence. Note: Some other comments in this CL are also missing a period (.). To avoid such nits overshadowing the more substantive issues, I won't point out this kind of punctuation issues in the future. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_unittest.cc:1759: // Set up a test server with CERT_CHAIN_WRONG_ROOT. Nit: it seems better to send a valid certificate chain that contains the root certificate unnecessarily. In that case, the verified cert will not include the root certificate. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_unittest.cc:1776: rv = callback.WaitForResult(); Just FYI: there is a callback.GetResult() method that you can use to replace these three lines (1775-1777, and possibly lines 1784-1787). https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_unittest.cc:1807: ASSERT_EQ(3U, server_intermediates.size()); 1. Should we also assert that server_certs.size() is 4u? 2. Nit: move this after the next EXPECT_TRUE, because the next EXPECT_TRUE doesn't use server_intermediates. But it may be better to assert the two sizes together. Up to you. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_unittest.cc:1919: // Verifies the correctness of GetSSLCertRequestInfo. I think this comment should be moved along with the SSLClientSocketCertRequestInfoTest class.
https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:171: virtual const scoped_refptr<X509Certificate> GetUnverifiedServerCertificate() On 2014/03/10 21:45:34, wtc wrote: > > 1. Nit: change "ServerCertificate" to "ServerCertChain" to make it clear the > returned X509Certificate object also contains the CA certificates presented by > the server. > > 2. I am not familiar with this aspect of C++, but it seems strange to see a > *private* virtual method. I would think this needs to be protected to allow > subclasses to override this method. Perhaps the "private" keyword just means > only this class can *call* this method, and doesn't care which class can > override this method. Yeah, seems like this should be protected. > > 3. Nit: in the comment you may want to point out that the cert returned by the > GetSSLInfo method is a verified certificate chain, which may be different. This > difference may not be obvious to someone unfamiliar with this code. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_nss.h (right): https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.h:84: virtual const scoped_refptr<X509Certificate> GetUnverifiedServerCertificate() did you mean for this to be "const scoped_refptr<X509Certificate>&" ? the const does nothing otherwise. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:418: static_cast<size_t>(sk_X509_num(openssl_chain_.get()))) { On 2014/03/10 21:45:34, wtc wrote: > > Just to check my understanding: this can only happen if the check on line 406 > inside the first for loop fails, right? Otherwise, |der_chain| and > |openssl_chain_| should be of the same size. Correct https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:64: virtual const scoped_refptr<X509Certificate> GetUnverifiedServerCertificate() On 2014/03/10 21:45:34, wtc wrote: > > Here you list the method under an "SSLClientSocket implementation" section > correctly. So the only issue is whether this method should be public or private. Yeah, not a fan of exposing this publicly. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_unittest.cc:1753: // Test that the server certificates are properly retrieved from the underlying On 2014/03/10 21:45:34, wtc wrote: > > Nit: I don't understand why it is important to test this explicitly. because we weren't before, and we were buggy :)
Thanks for reviewing. Here's my changes (and I suddenly realized there was a 'done' button). https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:944: } On 2014/02/21 22:34:38, Ryan Sleevi wrote: > net/ style: Don't include braces on single-line conditionals (eg: look at line > 935-936 or 920-921) Done. https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:948: OPENSSL_free(cert_data); On 2014/02/21 22:34:38, Ryan Sleevi wrote: > Rather than copying all of these to strings, then constructing string pieces, we > should do a single allocation (using the OPENSSL memory functions), create > string pieces from them, then free with OPENSSL_free afterwards (using the > scoped_ptr<> or the like to set an appropriate free proc) Done. https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:959: std::string(reinterpret_cast<char*>(cert_data), cert_data_length)); On 2014/02/25 01:13:09, wtc wrote: > > If you directly push a base::StringPiece to chain_ref here: > > chain_ref.push_back(base::StringPiece(reinterpret_cast<char*>(cert_data), > cert_data_length)); > > then after you are done with chain_ref, you can iterate over it and call > OPENSSL_free: > > for (size_t i = 0; i < chain_ref.size();i++) > OPENSSL_free(chain_ref[i].data()); > > You'll also need to do this before the early return on line 956. > > I don't know how to free the memory more elegantly using scoped_ptr<>. Done. https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:966: chain_ref.push_back(base::StringPiece(cert_chain[0])); On 2014/02/24 19:02:47, Ryan Sleevi wrote: > On 2014/02/24 18:54:31, Håvard Molland wrote: > > Ouch, bad typo right there (cert_chain[0]). Will fix if that code is still > there > > after the other fixes is done. Is the missing OpenSSL server socket the reason > > this was not caught by the unittests? > > No. There's no test that covers this. The CertVerifyProcTest.VerifyReturn* tests > only test the CertVerifier, and the > SSLClientSocketTest.VerifyReturnChainProperlyOrdered only does a mapping on the > server cert, not on the additional certs. > > We don't have any tests that do 1:1 mapping of the server certs to returned, > since our cert verifier code has to assume it may only get the server cert in > the future. > Done. https://codereview.chromium.org/173853014/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:971: DCHECK(server_cert_.get()); On 2014/02/24 19:02:47, Ryan Sleevi wrote: > On 2014/02/24 18:54:31, Håvard Molland wrote: > > On 2014/02/21 22:34:38, Ryan Sleevi wrote: > > > This DCHECK is going to have to go when adding !OpenSSL support, as > > server_cert_ > > > may be NULL now (because the underlying OS can't represent it - eg: Windows > > and > > > Mac will choke parsing dates too far in the future) > > > > Ok. I'll move it into the USE_OPENSSL code. > > > > I was saying to remove it entirely. Done. https://codereview.chromium.org/173853014/diff/80001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/80001/net/socket/ssl_client_so... net/socket/ssl_client_socket_openssl.cc:401: std::vector<ScopedX509> x509_certs_; On 2014/02/25 20:53:52, Håvard Molland wrote: > Seems good. I guess I got a little too focused on getting the scoped_ptr in > there :) > > > On 2014/02/25 20:27:09, Ryan Sleevi wrote: > > Storing a vector of ScopedX509 doesn't work before C++11. > > > > Rather than having to worry about this bookkeeping - especially with > > AsStringPieceVector - why not simplify? > > > > void sk_X509_free_helper(STACK_OF(X509)* stack) { > > sk_X509_pop_free(stack, X509_free); > > } > > > > class SSLClientSocketOpenSSL::PeerCertificateChain { > > public: > > explicit PeerCertificateChain(SSL* ssl); > > ~PeerCertificateChain(); > > > > void Reset(SSL* ssl); > > const scoped_ptr<X509Certificate>& AsNativeChain() const { > > return native_chain_; > > } > > > > size_t size() const { > > if (!openssl_chain_.get()) > > return 0; > > return sk_X509_num(openssl_chain_.get()); > > } > > > > X509* operator[](size_t index) const { > > DCHECK_LT(index, size()); > > return sk_X509_value(openssl_chain_.get(), index); > > } > > > > private: > > crypto::ScopedOpenSSL<STACK_OF(X509), sk_X509_free_helper> openssl_chain_; > > scoped_refptr<X509Certificate> native_chain_; > > }; > > > > PeerCertificateChain::PeerCertificateChain(SSL* ssl) { > > Reset(ssl); > > } > > > > PeerCertificateChain::~PeerCertificateChain() {} > > > > void PeerCertificateChain::Reset(SSL* ssl) { > > openssl_chain_.reset(); > > native_chain_ = NULL; > > > > if (ssl == NULL) > > return; > > > > STACK_OF(X509)* chain = SSL_get_peer_cert_chain(ssl); > > if (!chain) > > return; > > > > openssl_chain_.reset(sk_X509_dup(chain)); > > > > std::vector<base::StringPiece> der_chain; > > for (int i = 0; i < sk_X509_num(openssl_chain_.get()); ++i) { > > X509* x = sk_X509_value(openssl_chain_.get(), i); > > CRYPTO_add(&x->references, 1, CRYPTO_LOCK_X509); > > > > unsigned char* cert_data = NULL; > > int cert_data_length = i2d_X509(x, &cert_data); > > if (cert_data_length && cert_data) { > > > der_chain.push_back(base::StringPiece(reinterpret_cast<char*>(cert_data), > > cert_data_length); > > } > > } > > > > native_chain_ = X509Certificate::CreateFromDERCertChain(der_chain); > > > > for (size_t i = 0; i < der_chain.size(); ++i) { > > OPENSSL_free(der_chain[i].data()); > > } > > > > if (native_chain_.size() != > > static_cast<size_t>(sk_X509_num(openssl_chain_.get()))) { > > openssl_chain_.reset(NULL); > > native_chain_ = NULL; > > } > > } > > > > > > This also allows for support for the operator= / Equals support to be added > in. > Done. https://codereview.chromium.org/173853014/diff/80001/net/socket/ssl_client_so... net/socket/ssl_client_socket_openssl.cc:1014: X509Certificate::CreateFromHandle(sk_X509_value(chain, 0), intermediates); On 2014/02/25 20:27:09, Ryan Sleevi wrote: > if |chain| is false, or sk_X509_num(chain) is 0, this will crash (eg: compare > line 1009) Done. https://codereview.chromium.org/173853014/diff/120001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/120001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:414: OPENSSL_free((void*)der_chain[i].data()); On 2014/02/27 17:54:56, wtc wrote: > > On 2014/02/27 10:46:28, Håvard Molland wrote: > > > > I don't see how this can be done without casting, but I'll can use > > const_cast<char*> instead. > > Yes, this requires a cast, and const_cast<char*> is the right cast to use. Done. https://codereview.chromium.org/173853014/diff/120001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:419: static_cast<size_t>(sk_X509_num(openssl_chain_.get()))) { On 2014/02/27 10:46:28, Håvard Molland wrote: > On 2014/02/27 03:07:29, Ryan Sleevi wrote: > > Pathological case: What happens when .size() == numeric_limits<size_t>::max() > I think we would have a crash or freeze long before that happens. But I don't > see that +1 makes a difference here since the result is size_t anyway. That is > if GetChain().size() existed it would return > GetIntermediateCertificates().size() + 1. > > Will remove anyway, see below > > > > > Also, what are you trying to test here? The goal with testing der_chain is > just > > to make sure that all the i2d_X509 succeeded - which should 'never fail'. The > > conversion to native certs "may" fail - but thats ok. > > Right. I thought the point was to test the whole conversion chain > (X509->der->Os). I forgot that native_chain could 'legally' end up with > different chain size after sorting out the real chain. I followed your example > which used native_chain. I guess that was a typo, and I'll put in the der_chain > instead (which actually has the size() function). That would test that all > i2d_X509 are successful. Done. https://codereview.chromium.org/173853014/diff/180001/net/socket/socket_test_... File net/socket/socket_test_util.cc (right): https://codereview.chromium.org/173853014/diff/180001/net/socket/socket_test_... net/socket/socket_test_util.cc:780: // Returns the certificate chain as presented by server. On 2014/03/10 21:45:34, wtc wrote: > > Nit: omit this comment. We just need to document this method in the .h file of > the base class. An implementation of this method only needs to be documented if > it does something unusual. Done. https://codereview.chromium.org/173853014/diff/180001/net/socket/socket_test_... File net/socket/socket_test_util.h (right): https://codereview.chromium.org/173853014/diff/180001/net/socket/socket_test_... net/socket/socket_test_util.h:705: virtual const scoped_refptr<X509Certificate> GetUnverifiedServerCertificate() On 2014/03/10 21:45:34, wtc wrote: > > Should this method be public? The base class SSLClientSocket declares this > method as private. Done. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:171: virtual const scoped_refptr<X509Certificate> GetUnverifiedServerCertificate() On 2014/03/10 21:45:34, wtc wrote: > > 1. Nit: change "ServerCertificate" to "ServerCertChain" to make it clear the > returned X509Certificate object also contains the CA certificates presented by > the server. > > 2. I am not familiar with this aspect of C++, but it seems strange to see a > *private* virtual method. I would think this needs to be protected to allow > subclasses to override this method. Perhaps the "private" keyword just means > only this class can *call* this method, and doesn't care which class can > override this method. It doesn't have to be protected to allow subclasses to override it, only for letting subclasses call it. Letting it be private here and public in the subclass have the affect that you can call the function if you have a instance of subclass. If you only have a instance of this class you cannot call it. The idea was that no code having a SSLClientSocketOpenSSL instance should be able to call GetUnverifiedServerCertificate(), while it could be interesting for internal socket code to call it when having a nss/openssl instance or one of the test ssl socket implementations. I realize it can be a bit confusing though, so I'll make all of them protected instead. > > 3. Nit: in the comment you may want to point out that the cert returned by the > GetSSLInfo method is a verified certificate chain, which may be different. This > difference may not be obvious to someone unfamiliar with this code. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:171: virtual const scoped_refptr<X509Certificate> GetUnverifiedServerCertificate() On 2014/03/10 21:45:34, wtc wrote: > > 1. Nit: change "ServerCertificate" to "ServerCertChain" to make it clear the > returned X509Certificate object also contains the CA certificates presented by > the server. > > 2. I am not familiar with this aspect of C++, but it seems strange to see a > *private* virtual method. I would think this needs to be protected to allow > subclasses to override this method. Perhaps the "private" keyword just means > only this class can *call* this method, and doesn't care which class can > override this method. > > 3. Nit: in the comment you may want to point out that the cert returned by the > GetSSLInfo method is a verified certificate chain, which may be different. This > difference may not be obvious to someone unfamiliar with this code. Done. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_nss.h (right): https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.h:84: virtual const scoped_refptr<X509Certificate> GetUnverifiedServerCertificate() No need for scoped pointer here at all, so I'll use const X509Certificate* instead to allow returning NULL in the mock implementation. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.h:84: virtual const scoped_refptr<X509Certificate> GetUnverifiedServerCertificate() On 2014/03/10 21:45:34, wtc wrote: > > 1. This method is listed under the "SSLSocket implementation" section (see line > 76), but it is a method of SSLClientSocket. So it is listed under the wrong > section. > > 2. More important: should this method be public? The base class SSLClientSocket > declares this method as private. Done. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:335: return native_chain_; For consistency and possible future use of this function, it might be best to leave this as is but rename it to OSCertChain(). See comment at line 1037. On 2014/03/10 21:45:34, wtc wrote: > > Nit: it's not clear what "native" means here. > > I would think OpenSSL is more "native" than Chromium's X509Certificate is. > > Update: I now understand what you meant by "native". Please put the entire > PeerCertificateChain class inside #if !defined(USE_OPENSSL). A comment that > point out that in this case (USE_OPENSSL is not defined) X509Certificate > contains the native OS certificate handles would still be helpful. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:344: // Returns the certificate chain as presented by server. On 2014/03/10 21:45:34, wtc wrote: > > This comment seems wrong; it doesn't seem to describe the [] operator. Done. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:351: static void FreeX509Stack(STACK_OF(X509) * chain) { On 2014/03/10 21:45:34, wtc wrote: > > Nit: delete the space before '*'. Done. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:452: server_cert_chain_(new PeerCertificateChain(NULL)), On 2014/03/10 21:45:34, wtc wrote: > > Is it necessary to create an empty PeerCertificateChain object? > > If not, this initializer can be omitted. That was just to avoid having to check for NULL and create it when used at line 1037. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:453: server_cert_(NULL), On 2014/03/10 21:45:34, wtc wrote: > > Nit: we usually don't initialize a scoped_refptr to NULL like this. The > scoped_refptr constructor that takes no argument initializes the internal > pointer to NULL automatically. Done. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:1023: #ifdef USE_OPENSSL On 2014/03/10 21:45:34, wtc wrote: > > NitL: the Chromium coding style recommends always using the more verbose #if > defined(FOO). The reason is that it can be more easily extended with other || > defined(BAR) if necessary. Done. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:1026: // increment the reference so sk_X509_free does not need to be called. On 2014/03/10 21:45:34, wtc wrote: > > Nit: I think the "Unlike SSL_get_peer_certificate, " part can be removed from > this comment. Done. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:1038: server_cert_ = server_cert_chain_->AsNativeChain(); The difference is that the first implementation does not compile when OSCertHandle is anything else than OpenSSL's X509 structure. This is not portable to mac and windows. The only reason for keeping it is to avoid converting back and forth two times between X509 structure and der when USE_OPENSSL is defined. However, the ifdef was added before adding the server_cert_chain_ logic. You have a good point now that the implementation has evolved. I've removed the first implementation, and instead created a special Reset(SSL* ssl) for USE_OPENSSL to keep this consistent. On 2014/03/10 21:45:34, wtc wrote: > > IMPORTANT: comparing the two implementations of this method, I see one > difference: the first implementation sets server_cert_ but does not set > server_cert_chain_. Is this a bug? > > If this is not a bug, it seems to imply that in the second implementation, > server_cert_chain_ just needs to be a local variable rather than a class member. > If so, this will also solve the problem of the forward declaration of > PeerCertificateChain in ssl_client_socket_openssl.h. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:34: class PeerCertificateChain; On 2014/03/10 21:45:34, wtc wrote: > > This class is in the net namespace and is defined in the > ssl_client_socket_openssl.cc file. This is uncommon in the Chromium source tree. > Usually we would nest this kind of class inside SSLClientSocketOpenSSL. Done. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:64: virtual const scoped_refptr<X509Certificate> GetUnverifiedServerCertificate() On 2014/03/11 00:15:15, Ryan Sleevi wrote: > On 2014/03/10 21:45:34, wtc wrote: > > > > Here you list the method under an "SSLClientSocket implementation" section > > correctly. So the only issue is whether this method should be public or > private. > > Yeah, not a fan of exposing this publicly. Done. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_unittest.cc:1759: // Set up a test server with CERT_CHAIN_WRONG_ROOT. This was to reuse the certs set in BaseTestServer, and avoid having to add a new chain. The test does work (I've tested that it catches the bug I've fixed). Can it stay as is? On 2014/03/10 21:45:34, wtc wrote: > > Nit: it seems better to send a valid certificate chain that contains the root > certificate unnecessarily. In that case, the verified cert will not include the > root certificate. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_unittest.cc:1776: rv = callback.WaitForResult(); Ah, right. I always forget about that one :) On 2014/03/10 21:45:34, wtc wrote: > > Just FYI: there is a callback.GetResult() method that you can use to replace > these three lines (1775-1777, and possibly lines 1784-1787). https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_unittest.cc:1807: ASSERT_EQ(3U, server_intermediates.size()); On 2014/03/10 21:45:34, wtc wrote: > > 1. Should we also assert that server_certs.size() is 4u? > > 2. Nit: move this after the next EXPECT_TRUE, because the next EXPECT_TRUE > doesn't use server_intermediates. But it may be better to assert the two sizes > together. Up to you. Done. https://codereview.chromium.org/173853014/diff/180001/net/socket/ssl_client_s... net/socket/ssl_client_socket_unittest.cc:1919: // Verifies the correctness of GetSSLCertRequestInfo. On 2014/03/10 21:45:34, wtc wrote: > > I think this comment should be moved along with the > SSLClientSocketCertRequestInfoTest class. Done.
https://codereview.chromium.org/173853014/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/173853014/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:158: protected: Do not repeat 'protected' here (it's already set on line 136) https://codereview.chromium.org/173853014/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_nss.h (right): https://codereview.chromium.org/173853014/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.h:111: virtual const X509Certificate* GetUnverifiedServerCertificateChain() const Don't return raw pointers to ref-counted types. It can actually lead to subtle UAF (there in fact was a rather ugly one with an internal function that returned an X509Certificate* - even though it was immediatedly placed into a scoped_refptr) either "const scoped_refptr<X509Certificate>&" (if it's always stored) or "scoped_refptr<X509Certificate>" (if not). X509Certificates themselves are immutable. https://codereview.chromium.org/173853014/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:348: #if defined(USE_OPENSSL) STYLE: Rather than putting this all inline - especially when it can hide whether or not you have a single function or multiple functions covered by the #ifdef, it's better to use the same style in headers - that is, definition and declaration separate. That way you can quickly read the class to understand its interface (and documentation), and then jump to the implementation as needed. Such style also matches the _nss impl
Thanks for quick review. Here's the fixes. https://codereview.chromium.org/173853014/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/173853014/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket.h:158: protected: On 2014/03/12 23:12:03, Ryan Sleevi wrote: > Do not repeat 'protected' here (it's already set on line 136) Done. https://codereview.chromium.org/173853014/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_nss.h (right): https://codereview.chromium.org/173853014/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.h:111: virtual const X509Certificate* GetUnverifiedServerCertificateChain() const On 2014/03/12 23:12:03, Ryan Sleevi wrote: > Don't return raw pointers to ref-counted types. It can actually lead to subtle > UAF (there in fact was a rather ugly one with an internal function that returned > an X509Certificate* - even though it was immediatedly placed into a > scoped_refptr) > > either "const scoped_refptr<X509Certificate>&" (if it's always stored) or > "scoped_refptr<X509Certificate>" (if not). X509Certificates themselves are > immutable. OK. Didn't realize X509Certificate was immutable. My thinking was that one never takes ownership or passes on const pointers unless it is documented. The Mock implementations returns NULL, so I'll change to scoped_refptr<X509Certificate>. https://codereview.chromium.org/173853014/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:348: #if defined(USE_OPENSSL) On 2014/03/12 23:12:03, Ryan Sleevi wrote: > STYLE: Rather than putting this all inline - especially when it can hide whether > or not you have a single function or multiple functions covered by the #ifdef, > it's better to use the same style in headers - that is, definition and > declaration separate. > > That way you can quickly read the class to understand its interface (and > documentation), and then jump to the implementation as needed. > > Such style also matches the _nss impl Done.
LGTM! Sorry for the delays in reviewing. If you ever have a lag of more than 24 hours, feel free to just say "ping" on the thread. Definitely don't want to slow you down. A few nits below on comments. https://codereview.chromium.org/173853014/diff/220001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/220001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:332: void Reset(SSL* ssl); nit: newline after the operator= Let's add a descriptive comment // Resets the PeerCertificateChain to the set of certificates supplied by the peer // of |ssl|, which may be NULL, indicating to empty the store certificates. // Note: If an error occurs, such as being unable to parse the certificates, this // will behave as if Reset(NULL) was called. https://codereview.chromium.org/173853014/diff/220001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:351: crypto::ScopedOpenSSL<STACK_OF(X509), FreeX509Stack> openssl_chain_; nit: newline between 350/351 https://codereview.chromium.org/173853014/diff/220001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:376: // to avoid converting back and forth between der and X509 struct. comment nit: avoiding pronouns is good (see https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... ) // When X509Certificate::OSCertHandle is an X509*, it's possible to use a // short-cut to avoid converting to DER and then re-parsing to an X509* // by just increasing the reference count of the original X509*.
On 2014/03/14 21:06:14, Ryan Sleevi wrote: > LGTM! > > Sorry for the delays in reviewing. If you ever have a lag of more than 24 hours, > feel free to just say "ping" on the thread. Definitely don't want to slow you > down. Thanks! And no worries. Also, the time difference between US and Norway is a contributing factor here. > > A few nits below on comments. > > https://codereview.chromium.org/173853014/diff/220001/net/socket/ssl_client_s... > File net/socket/ssl_client_socket_openssl.cc (right): > > https://codereview.chromium.org/173853014/diff/220001/net/socket/ssl_client_s... > net/socket/ssl_client_socket_openssl.cc:332: void Reset(SSL* ssl); > nit: newline after the operator= > > Let's add a descriptive comment > > // Resets the PeerCertificateChain to the set of certificates supplied by the > peer > // of |ssl|, which may be NULL, indicating to empty the store certificates. > // Note: If an error occurs, such as being unable to parse the certificates, > this > // will behave as if Reset(NULL) was called. > > https://codereview.chromium.org/173853014/diff/220001/net/socket/ssl_client_s... > net/socket/ssl_client_socket_openssl.cc:351: > crypto::ScopedOpenSSL<STACK_OF(X509), FreeX509Stack> openssl_chain_; > nit: newline between 350/351 > > https://codereview.chromium.org/173853014/diff/220001/net/socket/ssl_client_s... > net/socket/ssl_client_socket_openssl.cc:376: // to avoid converting back and > forth between der and X509 struct. > comment nit: avoiding pronouns is good (see > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... > ) > > // When X509Certificate::OSCertHandle is an X509*, it's possible to use a > // short-cut to avoid converting to DER and then re-parsing to an X509* > // by just increasing the reference count of the original X509*.
https://codereview.chromium.org/173853014/diff/220001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/220001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:332: void Reset(SSL* ssl); On 2014/03/14 21:06:15, Ryan Sleevi wrote: > nit: newline after the operator= > > Let's add a descriptive comment > > // Resets the PeerCertificateChain to the set of certificates supplied by the > peer > // of |ssl|, which may be NULL, indicating to empty the store certificates. > // Note: If an error occurs, such as being unable to parse the certificates, > this > // will behave as if Reset(NULL) was called. Done. https://codereview.chromium.org/173853014/diff/220001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:351: crypto::ScopedOpenSSL<STACK_OF(X509), FreeX509Stack> openssl_chain_; On 2014/03/14 21:06:15, Ryan Sleevi wrote: > nit: newline between 350/351 Done. https://codereview.chromium.org/173853014/diff/220001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:376: // to avoid converting back and forth between der and X509 struct. On 2014/03/14 21:06:15, Ryan Sleevi wrote: > comment nit: avoiding pronouns is good (see > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... > ) > > // When X509Certificate::OSCertHandle is an X509*, it's possible to use a > // short-cut to avoid converting to DER and then re-parsing to an X509* > // by just increasing the reference count of the original X509*. Done.
The CQ bit was checked by haavardm@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haavardm@opera.com/173853014/240001
The CQ bit was unchecked by haavardm@opera.com
https://codereview.chromium.org/173853014/diff/260001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/173853014/diff/260001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:245: static void FreeX509Stack(STACK_OF(X509)* cert_chain) { Moved this out of PeerCertificateChain, is it gave compile error on Android: http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/16.... Not sure why this compiled on linux/mac in the first place (c++11 again?). I could have used friend and kept it in the class, but the template arguments of ScopedOpenSSL referring FreeX509Stack made it messy.
The CQ bit was checked by haavardm@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haavardm@opera.com/173853014/260001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
The CQ bit was checked by haavardm@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haavardm@opera.com/173853014/280001
Message was sent while issue was closed.
Change committed as 257449 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
