Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1178193002:
Sign CertificateVerify messages on a background thread. (Closed)
DescriptionSign CertificateVerify messages on a background thread.
In the general case, client certificates may involve accessing a smart
card (slow). Now that BoringSSL supports asynchronous certificate signing
operations in the SSL layer, use it on Mac and Windows ports. This will
avoid janking the IO thread in this (extremely uncommon) use case. Moreover,
some Windows smartcard drivers will show UI which, as IPC traffic between
UI thread and GPU process increases, causes deadlocks.
This introduces an SSLPrivateKey interface on the Chromium side which
implements an asynchronous signing operation. We then add a
ThreadedSSLPrivateKey implementation which pushes otherwise synchronous
signatures to a base::TaskRunner and implement Windows and Mac bridges to it.
These SSLPrivateKey implementations are still vended from an X509Certificate
for now, but https://crbug.com/394131 will track moving it up the stack for
testability (at which point it'll also gain a Copy method).
Android will be switched to use it in a follow-up. (The Android codepath
goes through this OpenSSLClientKeyStore hack so doing it separately
is easier.)
BUG=493575, 347404
Committed: https://crrev.com/1d48952e7ac455a52ffbe6597b58a571faab42e6
Cr-Commit-Position: refs/heads/master@{#337079}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 38
Patch Set 4 : rsleevi comments #
Total comments: 14
Patch Set 5 : sleevi comments, no static privates #Patch Set 6 : rebase #Patch Set 7 : additional comments #Patch Set 8 : gn issue #Patch Set 9 : rebase (try jobs can finally run) #Patch Set 10 : fix net_nacl #
Total comments: 37
Patch Set 11 : rsleevi comments #Patch Set 12 : rebase #
Total comments: 1
Patch Set 13 : copyright, remove enum runtime check #Patch Set 14 : fix build #Patch Set 15 : mac fix, edge case from downstream cl #Patch Set 16 : more mac build fix, this is blind while mac checkout syncs #
Messages
Total messages: 29 (5 generated)
davidben@chromium.org changed reviewers: + rsleevi@chromium.org
Sending this out for initial review. Try bots are red, but that's expected. This won't work without the corresponding BoringSSL patch here. The patch may still get tweaked later if the API changes, but I don't expect that to cause ripples anywhere but SSLClientSocketOpenSSL. https://boringssl-review.googlesource.com/#/c/5049/ (And the ones above and below it to add some utility functions: https://boringssl-review.googlesource.com/#/c/4940/ https://boringssl-review.googlesource.com/#/c/5080/ ) Scenarios tested: - P-256, P-384 certificates with CNG and Mac, against TLS 1.0 through 1.2. - RSA certificates with CNG, CAPI, and Mac, against TLS 1.0 through 1.2. - RSA certificate with CNG, CAPI, and Mac against a TLS 1.2 server which requested certs on renego. - UI was still responsive while a Keychain permission prompt was open. - Closing the tab while that prompt was open didn't explode. - Changing a return OK to return ERR_FAILED routed up the stack correctly. Design notes: Contrary to previous plans, this does NOT extend RSA_METHOD/ECDSA_METHOD and does NOT use EVP. We tried this and this is not really feasible. This would have to plumb in at the RSA and EC_KEY layer. There's no such thing as an EVP_PKEY method table. It internally has some, but they can't be reasonably exposed and are somewhat weird. There's a very strong assumption that all EVP_PKEY_RSAs are backed by RSA*, etc. (EVP_PKEY isn't an interface, it's a discriminated union.) This means we'd continue to have weird things like https://crbug.com/478917 and even our lowest-level crypto layers would have an asynchronous interface. This gets to be kind of much. Adam and I also weren't terribly thrilled about having to fight with RSA_METHOD again. It also fails to give an abstraction anyway. BoringSSL's asynchronous model isn't an async "perform operation, call this callback when it's done". It's a non-blocking "perform this operation, tell me if I need to try again later". The key distinction is that the consumer is responsible for re-running the state machine later. That means that passing a hypothetical async EVP_PKEY into a hypothetical, I dunno, OpenSSL-based SSH implementation *will not work*, even after you modify this SSH implementation's state machine. You still need to route the callback from the SSLPrivateKey::Sign call back up to the SSH state machine driver. The same way how, if you pass a non-blocking BIO* to an SSL*, you still need to know the BIO's implementation details to extract the underlying fd to listen on in select(). We can't use BIO as the asynchronous interface because non-blocking-style APIs don't know about event loops or TaskRunner or whathaveyou. Sadly, we're stuck with both this and RSA_METHOD because Conscrypt's use of RSA_METHOD is very... impressive... I'll have to understand JCA more to disentangle that one. Oh well. We can live with both around. https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:9: #include <openssl/ssl.h> base.h's main purpose in life is to forward-declare just about everything. But ssl.h needs to be included because you can't forward-declare enums. https://codereview.chromium.org/1178193002/diff/40001/net/ssl/ssl_platform_key.h File net/ssl/ssl_platform_key.h (right): https://codereview.chromium.org/1178193002/diff/40001/net/ssl/ssl_platform_ke... net/ssl/ssl_platform_key.h:6: #define NET_SSL_SSL_PLATFORM_KEY_H_ I renamed these from openssl_foo because there's nothing OpenSSL-specific about the interface anymore. The implementations on Mac and Windows (slightly) depend on BoringSSL, but that's fine because we can assume those ports use OpenSSL at this point. (I can easily make the NSS implementation not depend on BoringSSL.) I was a little sloppy about not including threaded_ssl_private_key.cc and ssl_platform_key_nss.cc on build configurations that don't need it, but things should build and the static linker will drop it all. I can put the exclusions back if it really matters. https://codereview.chromium.org/1178193002/diff/40001/remoting/protocol/ssl_h... File remoting/protocol/ssl_hmac_channel_authenticator.cc (right): https://codereview.chromium.org/1178193002/diff/40001/remoting/protocol/ssl_h... remoting/protocol/ssl_hmac_channel_authenticator.cc:31: #endif (See line 155. Needed because we were including ssl_client_socket_openssl.h on NSS ports. It was only working because forward declarations mask #include bugs.)
I haven't really closely dug into the implementation; I mostly focused on scanning the headers to see the design approach, and trying to offer feedback on that early. https://codereview.chromium.org/1178193002/diff/40001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1178193002/diff/40001/net/BUILD.gn#newcode250 net/BUILD.gn:250: "ssl/ssl_client_session_cache_openssl.h", no files to add? https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:969: // in parallel. ? https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:2013: return EVP_PKEY_NONE; remove the default, and move the return outside the switch. If you add any new types to the return of GetType(), clang will start yelling at compile-time. Better than a NOTREACHED. https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:214: size_t max_out); The growing number of these callbacks makes me wonder why we wouldn't want to just "struct CallbackHelper; friend struct CallbackHelper;" in the .h file, then move all of the static callbacks to the .cc and have them reach into the SSLClientSocketOpenSSL. This seems like it'd avoid the forward declaration issue entirely, as well as the need for all these method overloads. struct CallbackHelper { static int PrivateKeyTypeCallback(SSL* ssl) { SSLClientSocketOpenSSL* that = ...; switch (that->private_key_->GetType()) { ...; } } }; etc https://codereview.chromium.org/1178193002/diff/40001/net/ssl/ssl_private_key.h File net/ssl/ssl_private_key.h (right): https://codereview.chromium.org/1178193002/diff/40001/net/ssl/ssl_private_key... net/ssl/ssl_private_key.h:19: class SSLPrivateKey { It's unclear why you split this and the threaded implementation apart? Could you explain more the thinking/reasoning behind that? It may be fine, but it just seems like non-trivial abstraction. https://codereview.chromium.org/1178193002/diff/40001/net/ssl/ssl_private_key... net/ssl/ssl_private_key.h:21: using SignCallback = base::Callback<void(Error, const std::vector<uint8_t>&)>; Blergh; I generally hate typedefs like these; I find it makes things less readable rather than more. It's not strictly needed, is it? https://codereview.chromium.org/1178193002/diff/40001/net/ssl/ssl_private_key... net/ssl/ssl_private_key.h:42: virtual Type GetType() = 0; :/ This seems to violate http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Run-Time_Type... and turns this into a form of poor-man's RTTI. Having only looked at this interface first, I'm not sure what concrete suggestions to make. https://codereview.chromium.org/1178193002/diff/40001/net/ssl/ssl_private_key... net/ssl/ssl_private_key.h:49: virtual size_t GetMaxSignatureLength() = 0; I suppose this API works because we assume asymetric keys will always go up to at least the key size (possibly +1) ? https://codereview.chromium.org/1178193002/diff/40001/net/ssl/threaded_ssl_pr... File net/ssl/threaded_ssl_private_key.cc (right): https://codereview.chromium.org/1178193002/diff/40001/net/ssl/threaded_ssl_pr... net/ssl/threaded_ssl_private_key.cc:59: base::ThreadTaskRunnerHandle::Get())); An alternative way to do this namespace { void DoCallback(const SSLPrivateKey::SignCallback& callback, std::vector<uint8_t>* signature, Error result) { callback.Run(error, *signature); }; } // namespace std::vector<uint8_t>* signature = new std::vector<uint8_t>; task_runner_->PostTaskAndReplyWithResult( FROM_HERE, base::Bind(Core::SignDigest, core_, hash, input.as_string(), signature), base::Bind(DoCallback, callback, base::Owned(signature))); The benefit to the above approach is it eliminates the need to have DoCallback & DoSign - you can reduce that to one (if we had bound placeholders, you could reorder the args via bind) https://codereview.chromium.org/1178193002/diff/40001/net/ssl/threaded_ssl_pr... File net/ssl/threaded_ssl_private_key.h (right): https://codereview.chromium.org/1178193002/diff/40001/net/ssl/threaded_ssl_pr... net/ssl/threaded_ssl_private_key.h:30: class Core : public base::RefCountedThreadSafe<Core> { The naming of Core is a little weird here. Aren't these things normally called Delegates? And is it necessary to expose Ref-Counting as part of the public interface? I pretty much NACK anything new doing this unless there's a strong reason. It seems like the threading core should be an internal implementation detail, rather than the public API contract. Finally, does the Core even need to be subclassable? Could it just be embodied in platform-specific implementations? Why or why not? https://codereview.chromium.org/1178193002/diff/40001/net/ssl/threaded_ssl_pr... net/ssl/threaded_ssl_private_key.h:67: const std::vector<uint8_t>& signature); There's no need to make this a private class member; You could have the static helper in your .cc, pass it a WeakPtr to the ThreadedSSLPrivateKey, and have the helper check that this runs. Mostly, I'm trying to avoid exposing the implementation details in headers; this works, but exposes how publicly.
https://codereview.chromium.org/1178193002/diff/40001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1178193002/diff/40001/net/BUILD.gn#newcode250 net/BUILD.gn:250: "ssl/ssl_client_session_cache_openssl.h", On 2015/06/12 23:37:19, Ryan Sleevi wrote: > no files to add? The new interfaces aren't OpenSSL-specific (hence the name), so I figured it wasn't worth the strangeness of having non-OpenSSL files being excluded from the various OpenSSL bits. Instead its: - SSLClientSocketOpenSSL depends on the SSLPrivateKey interface always. - SSLPrivateKey isn't OpenSSL-specific, neither is ThreadedSSLPrivateKey. They happen not to be used in the remaining NSS ports, but hopefully the static linker will make them fall away? - ThreadedSSLPrivateKey isn't specific to OpenSSL - SSLPlatformKey{Mac,Win} implement a non-OpenSSL-specific interface. They depend very slightly on OpenSSL as an implementation detail, but that's a fine assumption today. Conceivably they could have a little #ifdef if we still cared. - SSLPlatformKeyNSS can probably be implemented without any OpenSSL dependency to avoid making life complicated should we stick with this static linker route. I can put those back though. I think it's a question of which is the least of the three evils: - Name these things with "openssl" in the name even though it's not OpenSSL-specific and we hope to surface SSLPrivateKey in //net's public interface someday. - Name them as-is, but add build exclusions for them, making them use_openssl build-exclusions for non-OpenSSL-named files. With the understanding that we'll have to take ssl_private_key.h out of the exclusions later when we surface SSLPrivateKey via //net's public interface because switching iOS to BoringSSL isn't a blocker for that refactor. - Name them as-is and don't include build exclusions. Each file's inclusion is locally understandable, but we somewhat prefer the static linker to drop things on the floor. https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:969: // in parallel. On 2015/06/12 23:37:20, Ryan Sleevi wrote: > ? Oops. Removed. Remnant of when I was toying with adding a separate HandshakeState, but that's a dud anyway because renego doesn't go through HandshakeState. I later determined that this didn't matter, although it is a very confusing and slightly broken part of our current state machine. -- TANGENTIAL STUFF FOLLOWS -- Our state machine is patterned after SSLClientSocketNSS's which is patterned after the SChannel implementation. From what I can tell, SChannel's API style is very different from NSS and OpenSSL, so we can reasonably have separate states for every kind of event we wait on. Notably it doesn't own the transport and just consumes data that you give it and tells you what to write. That is NOT true of these state machines. It *almost* is, but due to the write buffer, we can have a transport Write in flight while another asynchronous operation is being waited on, like private key operation or channel ID lookup. That means that OnSendComplete might get called while we're actually waiting for a different event like cert verify or channel ID lookup. This turns out to be fine, though sketchy, because either we're in STATE_HANDSHAKE and we pump the handshake state machine (it'll notice that whatever operation it was waiting on isn't done yet) or we're not and it'll pump the read loop. Which is either correct or horribly wrong because Connect isn't done. But that's fine because then there won't be any read payload so we don't do anything. Incidentally, Adam and I intend that BoringSSL will have a lower-level buffer-free API with in-place encryptions and decryptions (and the consumer explicitly switches to pumping renego). After toying with this state machine, I'm now thinking I want to move Chromium to it too when the time comes. We can then keep track of: - ConnectState = what is Connect() waiting on - ReadState = what is Read() waiting on - WriteState = what is Write() waiting on - HandshakeState = what is the current handshake waiting on. And then each of the three higher-level states will have an option for "I am waiting on the handshake" so we know which loops to wake up when the current handshake completes. Also we can finally fix all the craziness around write errors. (If we want to. We might be getting some perf win by starting to listen for Read() before Write() actually completes?? But if that's true, presumably we'd want this optimization elsewhere too like in HttpBasicStream.) https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:2013: return EVP_PKEY_NONE; On 2015/06/12 23:37:19, Ryan Sleevi wrote: > remove the default, and move the return outside the switch. > > If you add any new types to the return of GetType(), clang will start yelling at > compile-time. Better than a NOTREACHED. Done. https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:214: size_t max_out); Arguably that's what SSLContext is. I opted not to put them there because SSLContext mostly forwards to SSLClientSocket private methods, which is marginally tidier? Nice to only have one component reach into SSLClientSocketOpenSSL members. A previous version of this CL actually had those methods non-static and SSLContext held the static ones, but I ran into visibility issues. (In retrospect, that was probably because I didn't make SSLContext::kPrivateKeyMethod public. Oh well.) On the scale of important things, personally I don't really value "lets us forward-declare stuff" very highly. C++ is, sadly, not really a language that lets you hide the internals of a class from your header file because you need to declare all the private bits. I feel like contortions to allow it just make things messier because you have to fight against the visibility rules. Should this header also never switch to ScopedSSL from net/ssl/scoped_openssl_types.h for the sake of thinning includes? In fact, the forward-declarations were masking a bug! The reason I needed to change something in remoting/ was because it was including ssl_client_socket_openssl.h in NSS ports all this time! I think a better way to hide OpenSSL headers from consumers would be to use SSLClientSocket, which is the interface consumers actually want anyway. That SSLClientSocket{NSS,OpenSSL} have different constructors complicates this though. To cite our style guide: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Forward_Decl... - When using an ordinary class, relying on a forward declaration is OK, but be wary of situations where a forward declaration may be insufficient or incorrect; when in doubt, just #include the appropriate header. - Do not replace data members with pointers just to avoid an #include https://codereview.chromium.org/1178193002/diff/40001/net/ssl/ssl_private_key.h File net/ssl/ssl_private_key.h (right): https://codereview.chromium.org/1178193002/diff/40001/net/ssl/ssl_private_key... net/ssl/ssl_private_key.h:19: class SSLPrivateKey { On 2015/06/12 23:37:20, Ryan Sleevi wrote: > It's unclear why you split this and the threaded implementation apart? Could you > explain more the thinking/reasoning behind that? It may be fine, but it just > seems like non-trivial abstraction. SSLClientSocketOpenSSL doesn't care that the operation is on a background thread, just that it's asynchronous. I'm assuming the CHAPS code would eventually just talk to chapsd and we'd have no need to burn a thread for it. https://codereview.chromium.org/1178193002/diff/40001/net/ssl/ssl_private_key... net/ssl/ssl_private_key.h:21: using SignCallback = base::Callback<void(Error, const std::vector<uint8_t>&)>; On 2015/06/12 23:37:20, Ryan Sleevi wrote: > Blergh; I generally hate typedefs like these; I find it makes things less > readable rather than more. It's not strictly needed, is it? It's used a medium-ish amount of times between this header and it being mentioned a couple more times in threaded_ssl_private_key.{cc,h}. But the full callback is actually kinda long, so we have to wrap things awkwardly: void SignDigest( Hash hash, const base::StringPiece& input, const base::Callback<void(Error, const std::vector<uint8_t>&)>& callback) override; virtual void SignDigest( Hash hash, const base::StringPiece& input, const base::Callback<void(Error, const std::vector<uint8_t>&)>& callback) = 0; I think I mildly prefer having the callback name. There's only the one operation, so I don't think it'd be that confusing. But I don't insist on it. https://codereview.chromium.org/1178193002/diff/40001/net/ssl/ssl_private_key... net/ssl/ssl_private_key.h:42: virtual Type GetType() = 0; On 2015/06/12 23:37:20, Ryan Sleevi wrote: > :/ > > This seems to violate > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Run-Time_Type... > and turns this into a form of poor-man's RTTI. Having only looked at this > interface first, I'm not sure what concrete suggestions to make. Per out-of-band discussion, added a comment to GetType explaining why this is needed. https://codereview.chromium.org/1178193002/diff/40001/net/ssl/ssl_private_key... net/ssl/ssl_private_key.h:49: virtual size_t GetMaxSignatureLength() = 0; On 2015/06/12 23:37:20, Ryan Sleevi wrote: > I suppose this API works because we assume asymetric keys will always go up to > at least the key size (possibly +1) ? I'm not sure what you mean. It's the caller's job to figure out the maximum. ECDSA keys are NOT sized up to the key size (which isn't really a well-defined notion) and rather the maximum size of some DER thing. OpenSSL generally expects to size buffers itself and wants to know the size synchronously. I only need an upper bound that's tight enough to size a buffer sanely. https://codereview.chromium.org/1178193002/diff/40001/net/ssl/threaded_ssl_pr... File net/ssl/threaded_ssl_private_key.cc (right): https://codereview.chromium.org/1178193002/diff/40001/net/ssl/threaded_ssl_pr... net/ssl/threaded_ssl_private_key.cc:59: base::ThreadTaskRunnerHandle::Get())); On 2015/06/12 23:37:20, Ryan Sleevi wrote: > An alternative way to do this > > namespace { > > void DoCallback(const SSLPrivateKey::SignCallback& callback, > std::vector<uint8_t>* signature, Error result) { > callback.Run(error, *signature); > }; > > } // namespace > > std::vector<uint8_t>* signature = new std::vector<uint8_t>; > task_runner_->PostTaskAndReplyWithResult( > FROM_HERE, > base::Bind(Core::SignDigest, core_, hash, input.as_string(), signature), > base::Bind(DoCallback, callback, base::Owned(signature))); > > > The benefit to the above approach is it eliminates the need to have DoCallback & > DoSign - you can reduce that to one (if we had bound placeholders, you could > reorder the args via bind) I don't believe it quite avoids the need for DoCallback; the callback should not run if you destroy SSLPrivateKey. It somewhat avoids DoSign, though the Core/Delegate split brings it back somewhat. The base::Unretained + base::Owned pair kind of scares me somewhat, but okay. Anyway, done. https://codereview.chromium.org/1178193002/diff/40001/net/ssl/threaded_ssl_pr... File net/ssl/threaded_ssl_private_key.h (right): https://codereview.chromium.org/1178193002/diff/40001/net/ssl/threaded_ssl_pr... net/ssl/threaded_ssl_private_key.h:30: class Core : public base::RefCountedThreadSafe<Core> { On 2015/06/12 23:37:20, Ryan Sleevi wrote: > The naming of Core is a little weird here. Aren't these things normally called > Delegates? Done. > And is it necessary to expose Ref-Counting as part of the public interface? I > pretty much NACK anything new doing this unless there's a strong reason. It > seems like the threading core should be an internal implementation detail, > rather than the public API contract. Per out-of-band discussion, Core is now internal and ref-counted, hiding the ref-counts from Delegate. > Finally, does the Core even need to be subclassable? Could it just be embodied > in platform-specific implementations? Why or why not? This is mostly so we don't have to repeat the threaded logic in all the implementations, since we have two Windows ones, a Mac one. We'll have an Android and NSS one. Maybe we'll need a CHAPS one later. I dunno. https://codereview.chromium.org/1178193002/diff/40001/net/ssl/threaded_ssl_pr... net/ssl/threaded_ssl_private_key.h:67: const std::vector<uint8_t>& signature); On 2015/06/12 23:37:20, Ryan Sleevi wrote: > There's no need to make this a private class member; > > You could have the static helper in your .cc, pass it a WeakPtr to the > ThreadedSSLPrivateKey, and have the helper check that this runs. > > Mostly, I'm trying to avoid exposing the implementation details in headers; this > works, but exposes how publicly. Now it needs to be. If we split up Delegate and Core, it needs to have access to the class's private bits.
https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:214: size_t max_out); On 2015/06/15 21:28:24, David Benjamin wrote: > To cite our style guide: > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Forward_Decl... > - When using an ordinary class, relying on a forward declaration is OK, but be > wary of situations where a forward declaration may be insufficient or incorrect; > when in doubt, just #include the appropriate header. > - Do not replace data members with pointers just to avoid an #include https://www.chromium.org/developers/coding-style https://www.chromium.org/developers/coding-style/cpp-dos-and-donts
https://codereview.chromium.org/1178193002/diff/40001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1178193002/diff/40001/net/BUILD.gn#newcode250 net/BUILD.gn:250: "ssl/ssl_client_session_cache_openssl.h", On 2015/06/15 21:28:24, David Benjamin wrote: > - Name them as-is and don't include build exclusions. Each file's inclusion is > locally understandable, but we somewhat prefer the static linker to drop things > on the floor. I'm least keen on this solution, and I don't think it encourages good build-file health. If files aren't used, we should just explicitly exclude them. So the SSLPrivateKey, the SSLPlatformKey, and the ThreadedSSLPrivateKey all get excluded from use_nss builds unless/until we switch NSS to use them. So that's your option #2, and is the better route IMO. https://codereview.chromium.org/1178193002/diff/40001/net/log/net_log_event_t... File net/log/net_log_event_type_list.h (right): https://codereview.chromium.org/1178193002/diff/40001/net/log/net_log_event_t... net/log/net_log_event_type_list.h:457: EVENT_TYPE(SSL_PRIVATE_KEY_OPERATION) It's not quite clear to me why this added event? Just for diagnostics, right? https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:214: size_t max_out); On 2015/06/15 22:25:22, Ryan Sleevi wrote: > https://www.chromium.org/developers/coding-style > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts Sorry, to be explicit: *This* reviewer highly values hiding via forward declarations, and it's consistent (largely) with Chromium code. Personal views of C++ not-withstanding, it's also generally held as good practice (Google's IWYU tool, Lakos' Large Scale C++ Design, Meyers' Effective C++ series, etc). It certainly offers real compile-time benefits for your coworkers as well :) https://codereview.chromium.org/1178193002/diff/40001/net/ssl/ssl_platform_key.h File net/ssl/ssl_platform_key.h (right): https://codereview.chromium.org/1178193002/diff/40001/net/ssl/ssl_platform_ke... net/ssl/ssl_platform_key.h:6: #define NET_SSL_SSL_PLATFORM_KEY_H_ On 2015/06/12 21:39:01, David Benjamin wrote: > I can put the exclusions back if it really matters. Yes, please do. https://codereview.chromium.org/1178193002/diff/40001/net/ssl/ssl_private_key.h File net/ssl/ssl_private_key.h (right): https://codereview.chromium.org/1178193002/diff/40001/net/ssl/ssl_private_key... net/ssl/ssl_private_key.h:21: using SignCallback = base::Callback<void(Error, const std::vector<uint8_t>&)>; On 2015/06/15 21:28:25, David Benjamin wrote: > I think I mildly prefer having the callback name. There's only the one > operation, so I don't think it'd be that confusing. But I don't insist on it. I guess it depends on whether or not you're using the enum class for the ability to forward declare (based on your previous reply to forward declarations, I suspect it's not likely, in which case, an enum class isn't terribly useful but I'm ambivalent). The typedef would also prevent any form of forward declaration
https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:214: size_t max_out); On 2015/06/15 22:35:27, Ryan Sleevi wrote: > On 2015/06/15 22:25:22, Ryan Sleevi wrote: > > https://www.chromium.org/developers/coding-style > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts > > Sorry, to be explicit: *This* reviewer highly values hiding via forward > declarations, and it's consistent (largely) with Chromium code. Personal views > of C++ not-withstanding, it's also generally held as good practice (Google's > IWYU tool, Lakos' Large Scale C++ Design, Meyers' Effective C++ series, etc). > > It certainly offers real compile-time benefits for your coworkers as well :) The Chromium one you cited also says: > If it would otherwise make sense to use a type as a member by-value, don't convert it to a pointer just to be able to forward-declare the type. I read both this and the google3 one to mean "DO forward-declare when you can, DON'T contort the code just to forward-declare".
https://codereview.chromium.org/1178193002/diff/40001/net/ssl/ssl_private_key.h File net/ssl/ssl_private_key.h (right): https://codereview.chromium.org/1178193002/diff/40001/net/ssl/ssl_private_key... net/ssl/ssl_private_key.h:21: using SignCallback = base::Callback<void(Error, const std::vector<uint8_t>&)>; On 2015/06/15 22:35:27, Ryan Sleevi wrote: > On 2015/06/15 21:28:25, David Benjamin wrote: > > I think I mildly prefer having the callback name. There's only the one > > operation, so I don't think it'd be that confusing. But I don't insist on it. > > I guess it depends on whether or not you're using the enum class for the ability > to forward declare (based on your previous reply to forward declarations, I > suspect it's not likely, in which case, an enum class isn't terribly useful but > I'm ambivalent). The typedef would also prevent any form of forward declaration Hrm? I'm not sure I follow. Is there any consumer of SignedCallback that doesn't also implement SSLPrivateKey? For those, we need to include ssl_private_key.h regardless. (I used enum class for the static checking on switch-case. That didn't really work, but I figured the namespacing and losing implicit conversions was marginally better. And then you pointed out the MSVC thing, so now we have the static checking too.) The problematic enum in ssl_client_socket_openssl.h is enum ssl_private_key_result_t which can't be an enum class because that's from C.
Last email I promise. :-) https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:214: size_t max_out); On 2015/06/15 22:37:44, David Benjamin wrote: > On 2015/06/15 22:35:27, Ryan Sleevi wrote: > > On 2015/06/15 22:25:22, Ryan Sleevi wrote: > > > https://www.chromium.org/developers/coding-style > > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts > > > > Sorry, to be explicit: *This* reviewer highly values hiding via forward > > declarations, and it's consistent (largely) with Chromium code. Personal views > > of C++ not-withstanding, it's also generally held as good practice (Google's > > IWYU tool, Lakos' Large Scale C++ Design, Meyers' Effective C++ series, etc). > > > > It certainly offers real compile-time benefits for your coworkers as well :) > > The Chromium one you cited also says: > > > If it would otherwise make sense to use a type as a member by-value, don't > convert it to a pointer just to be able to forward-declare the type. > > I read both this and the google3 one to mean "DO forward-declare when you can, > DON'T contort the code just to forward-declare". Also note, by the way, that this is the entirety of our source tree that includes this header and would be affected. :-) https://code.google.com/p/chromium/codesearch#search/&q=%23include.*ssl_clien... I would argue that the latter is a problem with our "exposed" socket API. One option is we move that enum to openssl/base.h and we just include that one. Part of openssl/base.h's purpose in life is to forward-declare the universe anyway, so you DON'T have to know that it's typedef struct evp_pkey_ctx_st EVP_PKEY_CTX; // normal typedef struct AUTHORITY_KEYID_st AUTHORITY_KEYID; // caps typedef struct asn1_string_st ASN1_INTEGER; // oh yeah, these are all the same type typedef struct env_md_st EVP_MD; // yes, really. env. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/boring... (This isn't a BoringSSL innovation. Upstream called it ossl_typ.h.) But even that seems fairly unnecessary. ssl_client_socket_nss.h also happily includes NSS Headers, incidentally.
https://codereview.chromium.org/1178193002/diff/60001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1178193002/diff/60001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:1302: // renegotiation handshake is in progress. I find this comment less clear then the original (ambiguity as to what the "Read" state machine is; the original comment was clear it was about DoPayloadRead) https://codereview.chromium.org/1178193002/diff/60001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:1760: base::WorkerPool::GetTaskRunner(true /* task_is_slow */)); Per our chat, doesn't this need to be explicitly supplied? https://codereview.chromium.org/1178193002/diff/60001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1178193002/diff/60001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:136: void RunReadWriteLoops(); SSLClientSocketNSS calls this DoTransportIO, which I think is less-ambiguous here. (That is, it's unclear whether the ReadWrite loop being run is against the SSL socket state machine or against the transport socket state machine, or how this relates to DoReadLoop/DoWriteLoop) https://codereview.chromium.org/1178193002/diff/60001/net/ssl/ssl_platform_key.h File net/ssl/ssl_platform_key.h (right): https://codereview.chromium.org/1178193002/diff/60001/net/ssl/ssl_platform_ke... net/ssl/ssl_platform_key.h:24: const X509Certificate* certificate, Don't make this const. That's not the API contract afforded by Windows (for example). That is, even if it's logically const, it's not physically const, so better to express that via the API. https://codereview.chromium.org/1178193002/diff/60001/net/ssl/ssl_platform_ke... File net/ssl/ssl_platform_key_mac.cc (right): https://codereview.chromium.org/1178193002/diff/60001/net/ssl/ssl_platform_ke... net/ssl/ssl_platform_key_mac.cc:70: base::AutoLock lock(crypto::GetMacSecurityServicesLock()); BUG: Not sure why this lock wasn't extended to cover line 80, but it should have been. https://codereview.chromium.org/1178193002/diff/60001/net/ssl/ssl_platform_ke... net/ssl/ssl_platform_key_mac.cc:103: DCHECK_EQ(CSSM_ALGID_ECDSA, cssm_key_->KeyHeader.AlgorithmId); This just strikes me as dangerous :/ switch (cssm_key_->KeyHeader.AlgorithmId) { case CSSM_ALGID_RSA: return SSLPrivateKey::Type::RSA; case CSSM_ALGID_ECDSA: return SSLPrivateKey::Type::ECDSA; default: NOTREACHED(); return static_cast<SSLPrivateKey::Type>(-1); } I guess I say "danger" because these DCHECKs are no-ops in release mode, and just programmer guards. The 'actual' guard is line 237-238, but that's subtle and not directly connected to this code. Alternatively, you're duplicating the logic in line 93/94 in this DCHECK, and it seems better just to explicitly handle the cases you care about. Just unqualified elses make me extremely nervous (ditto line 113, where, for example, you don't DCHECK like you do here on 103) https://codereview.chromium.org/1178193002/diff/60001/net/ssl/threaded_ssl_pr... File net/ssl/threaded_ssl_private_key.h (right): https://codereview.chromium.org/1178193002/diff/60001/net/ssl/threaded_ssl_pr... net/ssl/threaded_ssl_private_key.h:65: Error error); nit: I'm still unclear why this needs to be a private member. This doesn't access any private instance data - seems perfectly fine to keep isolated to the .cc You're just doing it for the WeakFactory/WeakPtr, AFAICT, and that seems fine to express by just passing a WeakPtr<ThreadedSSLPrivateKey> as the first arg to the static DoCallback with the check to is_valid() [or, more aptly, to bool equivalency DoCallback(const base::WeakPtr<ThreadedSSLPrivateKey>& private_key, ...) { if (!private_key) return; callback.Run(...); } [This one I'm less worried about forward declaring, but it does seem unnecessary for the cancellation semantics, strictly speaking]
https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1178193002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:214: size_t max_out); On 2015/06/15 22:37:44, David Benjamin wrote: > The Chromium one you cited also says: > > > If it would otherwise make sense to use a type as a member by-value, don't > convert it to a pointer just to be able to forward-declare the type. Except we're not doing that here. And as the C++ Do's and Don'ts points out (and why it was created) was to show how many places you can and should be forward declaring. > I read both this and the google3 one to mean "DO forward-declare when you can, > DON'T contort the code just to forward-declare". I guess this relies on your value judgement of "contort the code" with the cost of exposing your internal details. The same page (C++'s dos and don'ts) lists "Move inner classes into the implementation." and "Move static implementation details to the implementation whenever possible.", both of which apply here. https://codereview.chromium.org/1178193002/diff/40001/net/ssl/ssl_private_key.h File net/ssl/ssl_private_key.h (right): https://codereview.chromium.org/1178193002/diff/40001/net/ssl/ssl_private_key... net/ssl/ssl_private_key.h:21: using SignCallback = base::Callback<void(Error, const std::vector<uint8_t>&)>; On 2015/06/15 22:41:20, David Benjamin wrote: > Hrm? I'm not sure I follow. Is there any consumer of SignedCallback that doesn't > also implement SSLPrivateKey? For those, we need to include ssl_private_key.h > regardless. Well, strictly speaking, callbacks are meant to break the dependency graph of needing to explicitly know some type (commonly, the Delegate* foo pattern). A callback can be specified in the form of a general signature, and then base::Bind() does the type-erasure for it. I guess the previous comment wasn't clearer that this is "OK here, for several reasons, but generally an anti-pattern" (as somewhat discussed in the chat). > > (I used enum class for the static checking on switch-case. That didn't really > work, but I figured the namespacing and losing implicit conversions was > marginally better. And then you pointed out the MSVC thing, so now we have the > static checking too.) You don't need the enum class for that though. That's provided by the compiler regardless for enums.
So, I sorted out the visibility problems that kept me from putting the private key hooks into the SSLContext and did that, so now there's no more private statics. However, it still doesn't avoid the ssl.h include. I made this version to see what you think: this is consistent with all of the other SSLContext hooks which always have a static that forwards to a non-static method on SSLClientSocketOpenSSL itself. This is how all of our other callbacks work, so I think the consistency is valuable. (If you really want, I can put the enum in openssl/base.h, so the specifically only include the BoringSSL forward-decl header.) https://codereview.chromium.org/1178193002/diff/60001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1178193002/diff/60001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:1302: // renegotiation handshake is in progress. On 2015/06/15 22:55:22, Ryan Sleevi wrote: > I find this comment less clear then the original (ambiguity as to what the > "Read" state machine is; the original comment was clear it was about > DoPayloadRead) The problem is the original comment is wrong. We need to run the *entire* Read state machine, including DoReadCallback. See long reply to your other comment. https://codereview.chromium.org/1178193002/diff/60001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:1760: base::WorkerPool::GetTaskRunner(true /* task_is_slow */)); On 2015/06/15 22:55:22, Ryan Sleevi wrote: > Per our chat, doesn't this need to be explicitly supplied? Done. https://codereview.chromium.org/1178193002/diff/60001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1178193002/diff/60001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:136: void RunReadWriteLoops(); On 2015/06/15 22:55:22, Ryan Sleevi wrote: > SSLClientSocketNSS calls this DoTransportIO, which I think is less-ambiguous > here. > > (That is, it's unclear whether the ReadWrite loop being run is against the SSL > socket state machine or against the transport socket state machine, or how this > relates to DoReadLoop/DoWriteLoop) [Terminology: if I ever say "Read" or "Write", I specifically mean SSLClientSocket::Read and SSLClientSocketWrite, not Read or Write on the underlying transport socket. That's "transport Read" and "transport Write".] This is not DoTransportIO. DoTransportIO even exists here. DoTransportIO *only* pumps the transport socket state machines (both transport Read and transport Write). It is not safe to call it standalone as it relies on its caller to pump the DoPayloadRead state machine AND call the Read callback. DoReadLoop (respectively, DoWriteLoop) pumps the PayloadRead (respectively, PayloadWrite) state machine together with DoTransportIO. It is not safe to call it standalone as it relies on its caller to call the Read (respectively, Write) callback if progress is made. You can see this come together in OnRecvComplete where we finally bind together low-level transport read signal, pump PayloadRead and down via DoReadLoop, and call the read callback. Likewise, there is the OnSendComplete one which should just call DoWriteLoop *except for renego*. And so it needs to pump both DoReadLoop and DoWriteLoop in parallel, which is this method. OnPrivateKeySignComplete, being another event which may resume a handshake, has the same property. It can resume EITHER Read or Write and so it needs that same code. So I factored it into a method. Really the root problem here is that this state machine is patterned after another state machine which, if you follow the chain far enough, comes from a world where we didn't have renego and SSL sockets were actually full-duplex. If I'm doing my archeology right, it came from the SChannel implementation. Renego breaks this and APIs with implicit renego (SChannel's renego is, sensibly, explicit) makes it so that we can't maintain this kind of separation that we do. A possible solution, if you really don't like RunReadWriteLoops: OnRecvComplete isn't right either! It actually also needs to call RunReadWriteLoops! SSL_write can block on read just as easily as SSL_read block on write. But I believe any case where this can cause problems is also one where renego doesn't happen in a quiescent state of the upper-level protocol which is explicitly not supported by BoringSSL. The situation is if the last thing that happened in the handshake was a read (so full handshake) and we have both Read and Write in flight. This combination gets stuck. Specifically: 1. Consumer calls SSLClientSocket::Read. 2. HelloRequest comes in over the transport. We enter renego. 3. Consumer calls SSLClientSocket::Write. 4. SSL_write blocks on the handshake completing. 5. The handshake completes and the server Finished comes in over the transport. 6. OnSendComplete pumps DoReadLoop which calls SSL_read and completes the handshake. It then tries to read from the transport again, but no app data has arrived yet, so the Read callback never signals. 7. OnSendComplete forgets to pump DoWriteLoop and so SSL_write never notices that it can send out its payload now. We're now stuck. However, in this situation, there cannot be an ordering relation steps 2 and 3. When 2 happens, nothing is signaled outside of SSLClientSocket so, unless the consumer is interposing on the transport socket, 2 could not have triggered 3 happening. They happened independently and so this order could have happened: 1. Consumer calls SSLClientSocket::Read. 2. Consumer calls SSLClientSocket::Write 3. SSL_write gets partway through dumping data to the transport and has to pause. BoringSSL's state machine has a half-written record. 4. HelloRequest comes in over the transport. We enter renego. This situation is explicitly not supported by BoringSSL. We will abort the handshake if it happens. This isn't a situation that can occur with the client auth hack. (The alternative is some very messy and questionable logic in OpenSSL where it defers renego until both buffers happen it be empty. OpenSSL's state machine is not capable of dispatching between two subprotocols writing records at once. It instead has some ad-hoc code for alerts which are otherwise the only thing that does this.) So, one thing we could do, to fix the "SSL_write may block on renego" problem is to check SSL_in_init before calling SSL_write. If yes, we error. Any situation where this may happen already fails racily. This would mean that the OnRecvComplete bug above is fixed and OnPrivateKeySignComplete can just call DoReadLoop + DoReadCallback because only Connect and Read may block on handshakes. Confused yet? :-) (I have not been idly crusading against renego and cutting it down to just the one use case we need. Even what complexity we did have for it was subtly wrong and not complex enough.) https://codereview.chromium.org/1178193002/diff/60001/net/ssl/ssl_platform_key.h File net/ssl/ssl_platform_key.h (right): https://codereview.chromium.org/1178193002/diff/60001/net/ssl/ssl_platform_ke... net/ssl/ssl_platform_key.h:24: const X509Certificate* certificate, On 2015/06/15 22:55:23, Ryan Sleevi wrote: > Don't make this const. That's not the API contract afforded by Windows (for > example). That is, even if it's logically const, it's not physically const, so > better to express that via the API. (That just came from openssl_platform_key.h.) Fixed. https://codereview.chromium.org/1178193002/diff/60001/net/ssl/ssl_platform_ke... File net/ssl/ssl_platform_key_mac.cc (right): https://codereview.chromium.org/1178193002/diff/60001/net/ssl/ssl_platform_ke... net/ssl/ssl_platform_key_mac.cc:70: base::AutoLock lock(crypto::GetMacSecurityServicesLock()); On 2015/06/15 22:55:23, Ryan Sleevi wrote: > BUG: Not sure why this lock wasn't extended to cover line 80, but it should have > been. It came from here: https://chromium.googlesource.com/chromium/src/+/0f931925f7c0b966c2eb83321886... And was added in: https://chromium.googlesource.com/chromium/src/+/b5c40cd46e1225a737b2397f5985... We don't seem to ever had had any trouble with SecIdentityCopyPrivateKey. I presume because the implementation is literally *privateKeyRef = identity->_privateKey; CFRetain(*privateKeyRef); return 0; https://codereview.chromium.org/1178193002/diff/60001/net/ssl/ssl_platform_ke... net/ssl/ssl_platform_key_mac.cc:103: DCHECK_EQ(CSSM_ALGID_ECDSA, cssm_key_->KeyHeader.AlgorithmId); On 2015/06/15 22:55:23, Ryan Sleevi wrote: > This just strikes me as dangerous :/ > > switch (cssm_key_->KeyHeader.AlgorithmId) { > case CSSM_ALGID_RSA: > return SSLPrivateKey::Type::RSA; > case CSSM_ALGID_ECDSA: > return SSLPrivateKey::Type::ECDSA; > default: > NOTREACHED(); > return static_cast<SSLPrivateKey::Type>(-1); > } > > I guess I say "danger" because these DCHECKs are no-ops in release mode, and > just programmer guards. The 'actual' guard is line 237-238, but that's subtle > and not directly connected to this code. > > Alternatively, you're duplicating the logic in line 93/94 in this DCHECK, and it > seems better just to explicitly handle the cases you care about. > > Just unqualified elses make me extremely nervous (ditto line 113, where, for > example, you don't DCHECK like you do here on 103) I'm somewhat unhappy about the static_cast<EnumClass>(-1) since enum classes are specifically supposed to get rid of the implicit version of that cast. Violates the promise that we actually have an enumeration here and not a funny int. What do you think about making it a CHECK? Alternatively, rather than querying KeyHeader.AlgorithmId, add an explicit SSLPrivateKey::Type type_ member and pass it into the constructor. Waste of an int since it's redundant, but meh. https://codereview.chromium.org/1178193002/diff/60001/net/ssl/threaded_ssl_pr... File net/ssl/threaded_ssl_private_key.h (right): https://codereview.chromium.org/1178193002/diff/60001/net/ssl/threaded_ssl_pr... net/ssl/threaded_ssl_private_key.h:65: Error error); On 2015/06/15 22:55:23, Ryan Sleevi wrote: > nit: I'm still unclear why this needs to be a private member. This doesn't > access any private instance data - seems perfectly fine to keep isolated to the > .cc > > You're just doing it for the WeakFactory/WeakPtr, AFAICT, and that seems fine to > express by just passing a WeakPtr<ThreadedSSLPrivateKey> as the first arg to the > static DoCallback with the check to is_valid() [or, more aptly, to bool > equivalency > > DoCallback(const base::WeakPtr<ThreadedSSLPrivateKey>& private_key, > ...) { > if (!private_key) > return; > callback.Run(...); > } > > [This one I'm less worried about forward declaring, but it does seem unnecessary > for the cancellation semantics, strictly speaking] Done.
Sorry about that. Missed these comments somehow. Build exclusions restored. https://codereview.chromium.org/1178193002/diff/40001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1178193002/diff/40001/net/BUILD.gn#newcode250 net/BUILD.gn:250: "ssl/ssl_client_session_cache_openssl.h", On 2015/06/15 22:35:27, Ryan Sleevi wrote: > On 2015/06/15 21:28:24, David Benjamin wrote: > > - Name them as-is and don't include build exclusions. Each file's inclusion is > > locally understandable, but we somewhat prefer the static linker to drop > things > > on the floor. > > I'm least keen on this solution, and I don't think it encourages good build-file > health. > > If files aren't used, we should just explicitly exclude them. So the > SSLPrivateKey, the SSLPlatformKey, and the ThreadedSSLPrivateKey all get > excluded from use_nss builds unless/until we switch NSS to use them. > > So that's your option #2, and is the better route IMO. Done. https://codereview.chromium.org/1178193002/diff/40001/net/log/net_log_event_t... File net/log/net_log_event_type_list.h (right): https://codereview.chromium.org/1178193002/diff/40001/net/log/net_log_event_t... net/log/net_log_event_type_list.h:457: EVENT_TYPE(SSL_PRIVATE_KEY_OPERATION) On 2015/06/15 22:35:27, Ryan Sleevi wrote: > It's not quite clear to me why this added event? Just for diagnostics, right? Yeah. This is a new asynchronous event that we can wait on. I expect we'll have some problem or other and so we'll want to know if that's what we're stuck on. For instance, if we get another deadlock for some dumb reason, Chrome may no longer be as visibly affected. Just connections that use client auth. net-internals will tell us what the connections are blocked on.
(As before, try jobs are expected to be almost all red. I'm just interested in the NSS ports.)
Implementation LGTM, although there's two nits that I think we want to hash out. - The commenting/naming for RunReadWriteLoops is still confusing. I totally appreciate the detail went into this CL, but I still find it a bit confusing as to what's going on or the conditions. I don't think we have to stuff it all in the header - I think it's fine to explain a bit in the .cc some of the complexity (which, admittedly, the implementation itself isn't complex, it's the *why* that's complex). For example, with my "I'm not stupid, I was just distracted" reviewing hat on (e.g. when I reviewed this during the codesigning WG, before taking time to separately re-review it), my confusion was "Well, why keep DoReadLoop/DoWriteLoop around at all then?". - The other nit was around the MaxSignatureLength and making the unit explicit https://codereview.chromium.org/1178193002/diff/180001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1178193002/diff/180001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:164: *hash = SSLPrivateKey::Hash::SHA224; There are no plans to ever support this, right? Pretty sure we agreed 224 was crap that wasn't implemented and with no plans to implement (and aligns with other platforms) https://codereview.chromium.org/1178193002/diff/180001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:192: base::SequencedTaskRunner* task_runner() { return task_runner_.get(); } DANGER: There's a slight 'danger' in this API shape because with other places, the scoped_refptr<> backing the raw pointer could go out of scope in between the full statements. While it's obviously not the case here, in general, it's desirable to return it using a scoped container. You can avoid any added AddRef costs by using .Pass() with the thing this is passing to (This is also why dcheng@ removed a lot of the "implicit" casts) https://codereview.chromium.org/1178193002/diff/180001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:1836: // client auth on NaCl altogether. Does this TODO really belong here, or on line 1820? https://codereview.chromium.org/1178193002/diff/180001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2135: LOG(ERROR) << "Buffer too small"; Is the message necessary? I generally grump about production LOG() messages and wanting to understand what diagnostic purpose they serve, and whether it belongs elsewhere (such as the net_log_) https://codereview.chromium.org/1178193002/diff/180001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1178193002/diff/180001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.h:130: // which point both state machines will block on the new handshake. I still find this comment confusing :( It's certainly not calling DoReadLoop/DoWriteLoop, and so the relation here to what it's doing is still somewhat ambiguous. I'm torn on suggesting naming, and I'm not terribly sure I like mine, but I'm curious if you can find a different way to word this and a different way to write this comment to be more descriptive. I'm totally fine with moving some of the more detailed (tricky) concepts to the documentation in the .cc, in line with the style guide. PumpReadWriteEvents? Pump*? https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... File net/ssl/ssl_platform_key.h (right): https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. Date https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key.h:10: #include "net/ssl/ssl_private_key.h" Can forward declare https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... File net/ssl/ssl_platform_key_mac.cc (right): https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key_mac.cc:145: cssm_signature.InitializeInto()) != CSSM_OK) { I'm a little nervous with this call; it'd require more digging in to in the CSSM API, but my fear (and suspicion, from what I've seen from the other Apple security APIs) is that it may drop an invalid handle but return a negative result. When we get a negative result, we shouldn't free the (invalid) handle. Same around line 135-136. [The same has come up w/ some MSFT APIs] https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key_mac.cc:170: break; Ditto re: no support https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key_mac.cc:185: return ERR_SSL_CLIENT_AUTH_SIGNATURE_FAILED; Do we want a way to smuggle error details back up? https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... File net/ssl/ssl_platform_key_win.cc (right): https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key_win.cc:199: BCRYPT_PKCS1_PADDING_INFO rsa_padding_info; = {0}; ? https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key_win.cc:261: if (!sig->r || !sig->s) Confirmed that ECDSA_sig_free does the right thing (that is, it handles either of these being null) https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_private_key.h File net/ssl/ssl_private_key.h (right): https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_private_ke... net/ssl/ssl_private_key.h:21: using SignCallback = base::Callback<void(Error, const std::vector<uint8_t>&)>; need stdint.h https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_private_ke... net/ssl/ssl_private_key.h:54: virtual size_t GetMaxSignatureLength() = 0; While reviewing this, I still found it confusing that this type isn't explicit (bits vs bytes) Can you canonicalize what unit this is in, and enshrine it in the name? GetMaxSignatureLengthInBytes() ? GetMaxSignatureLengthBytes? https://codereview.chromium.org/1178193002/diff/180001/net/ssl/threaded_ssl_p... File net/ssl/threaded_ssl_private_key.h (right): https://codereview.chromium.org/1178193002/diff/180001/net/ssl/threaded_ssl_p... net/ssl/threaded_ssl_private_key.h:40: std::vector<uint8_t>* signature) = 0; All of this should be documented as to what the expectations are of implementers. I'm totally cool with just saying something to the effect of // These methods behave the same as those of of the same name on SSLPrivateKey, but must // be callable on any thread. (Or something, if you have a preference) https://codereview.chromium.org/1178193002/diff/180001/net/ssl/threaded_ssl_p... net/ssl/threaded_ssl_private_key.h:47: const scoped_refptr<base::TaskRunner>& task_runner); Is the API contract TaskRunner or SequencedTaskRunner? Does it make a difference if multiple invocations were executing in parallel? (My gut is that we either want SequencedTaskRunner or SingleThreadTaskRunner as the API contract for these; really, the strong requirement comes from the concrete implementations, but since under the Liskov Substitution Principle you really don't want concrete impls requiring stronger pre-conditions than their base class, we should virally propagate this to the base class as the strong API guarantee)
https://codereview.chromium.org/1178193002/diff/180001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1178193002/diff/180001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:164: *hash = SSLPrivateKey::Hash::SHA224; On 2015/06/23 15:27:28, Ryan Sleevi wrote: > There are no plans to ever support this, right? Pretty sure we agreed 224 was > crap that wasn't implemented and with no plans to implement (and aligns with > other platforms) Oh hrm. Yes, you're right. In fact ssl_platform_key_win.cc is missing that hash function. And I guess it didn't notice because we're not (yet) building clang on Windows. Hrmf. Pruned it down to match and added an explicit check since we're not going to get any compile-time checks for it. https://codereview.chromium.org/1178193002/diff/180001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:192: base::SequencedTaskRunner* task_runner() { return task_runner_.get(); } On 2015/06/23 15:27:28, Ryan Sleevi wrote: > DANGER: There's a slight 'danger' in this API shape because with other places, > the scoped_refptr<> backing the raw pointer could go out of scope in between the > full statements. > > While it's obviously not the case here, in general, it's desirable to return it > using a scoped container. You can avoid any added AddRef costs by using .Pass() > with the thing this is passing to > > (This is also why dcheng@ removed a lot of the "implicit" casts) Pass() doesn't work unless we pass parameters in as plain scoped_refptr<T> rather than const scoped_refptr<T>&. (Which I'm actually all for. I think const scoped_refptr<T>& is kinda weird. Though I do understand the reasons for it. Pass() doesn't quiiiite capture all the cases where you might want to avoid the ref-bouncing.) Anyway. I did something. Is this what you meant? https://codereview.chromium.org/1178193002/diff/180001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:1836: // client auth on NaCl altogether. On 2015/06/23 15:27:28, Ryan Sleevi wrote: > Does this TODO really belong here, or on line 1820? Done. https://codereview.chromium.org/1178193002/diff/180001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:2135: LOG(ERROR) << "Buffer too small"; On 2015/06/23 15:27:28, Ryan Sleevi wrote: > Is the message necessary? I generally grump about production LOG() messages and > wanting to understand what diagnostic purpose they serve, and whether it belongs > elsewhere (such as the net_log_) Removed. https://codereview.chromium.org/1178193002/diff/180001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1178193002/diff/180001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.h:130: // which point both state machines will block on the new handshake. On 2015/06/23 15:27:28, Ryan Sleevi wrote: > I still find this comment confusing :( It's certainly not calling > DoReadLoop/DoWriteLoop, and so the relation here to what it's doing is still > somewhat ambiguous. > > I'm torn on suggesting naming, and I'm not terribly sure I like mine, but I'm > curious if you can find a different way to word this and a different way to > write this comment to be more descriptive. I'm totally fine with moving some of > the more detailed (tricky) concepts to the documentation in the .cc, in line > with the style guide. > > PumpReadWriteEvents? Pump*? PumpReadWriteEvents SGTM. What do you think of this comment? Left the mention of renego in the cc file at the call sites. https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... File net/ssl/ssl_platform_key.h (right): https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/06/23 15:27:28, Ryan Sleevi wrote: > Date Done. (Originally originally this was a copy of openssl_platform_key.h, but git thinks it's a new file, so whatever. It basically all changed.) https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key.h:10: #include "net/ssl/ssl_private_key.h" On 2015/06/23 15:27:29, Ryan Sleevi wrote: > Can forward declare Done. https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... File net/ssl/ssl_platform_key_mac.cc (right): https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key_mac.cc:145: cssm_signature.InitializeInto()) != CSSM_OK) { On 2015/06/23 15:27:29, Ryan Sleevi wrote: > I'm a little nervous with this call; it'd require more digging in to in the CSSM > API, but my fear (and suspicion, from what I've seen from the other Apple > security APIs) is that it may drop an invalid handle but return a negative > result. When we get a negative result, we shouldn't free the (invalid) handle. > > Same around line 135-136. > > > [The same has come up w/ some MSFT APIs] (This just came from the previous version of the file.) Got rid of the CSSM InitializeInto. The others I've left alone since it comes from scoped_cftyperef. So if Apple code is bad about it for Core Foundation types, we'll need to address that more pervasively. I'm a little more optimistic about that one since they tend to prefer automatic reference counting and stuff these days. https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key_mac.cc:170: break; On 2015/06/23 15:27:29, Ryan Sleevi wrote: > Ditto re: no support Done. https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key_mac.cc:185: return ERR_SSL_CLIENT_AUTH_SIGNATURE_FAILED; On 2015/06/23 15:27:29, Ryan Sleevi wrote: > Do we want a way to smuggle error details back up? Dunno. We could... I could add a string to the interface or new error codes or whatever else. Any opinions? I mostly left it at an error code since that's what //net did and... didn't end up using any error codes but ERR_SSL_CLIENT_AUTH_SIGNATURE_FAILED... https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... File net/ssl/ssl_platform_key_win.cc (right): https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key_win.cc:199: BCRYPT_PKCS1_PADDING_INFO rsa_padding_info; On 2015/06/23 15:27:29, Ryan Sleevi wrote: > = {0}; ? Done. https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key_win.cc:261: if (!sig->r || !sig->s) On 2015/06/23 15:27:29, Ryan Sleevi wrote: > Confirmed that ECDSA_sig_free does the right thing (that is, it handles either > of these being null) Yeah, BoringSSL free functions should all check for NULL pervasively for consistency with upstream and C free. Plus it would be bad manners if ECDSA_SIG_free(ECDSA_SIG_new()) didn't work. :-) Although... actually there's a different bug here. sig->r and sig->s are already initialized, so this leaks. Hrmf. https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_private_key.h File net/ssl/ssl_private_key.h (right): https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_private_ke... net/ssl/ssl_private_key.h:21: using SignCallback = base::Callback<void(Error, const std::vector<uint8_t>&)>; On 2015/06/23 15:27:29, Ryan Sleevi wrote: > need stdint.h Done. https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_private_ke... net/ssl/ssl_private_key.h:54: virtual size_t GetMaxSignatureLength() = 0; On 2015/06/23 15:27:29, Ryan Sleevi wrote: > While reviewing this, I still found it confusing that this type isn't explicit > (bits vs bytes) > > Can you canonicalize what unit this is in, and enshrine it in the name? > > GetMaxSignatureLengthInBytes() ? GetMaxSignatureLengthBytes? Done, though I think a comment is sufficient for signatures. Nothing, not even X.509 when you factor in how every profile works, uses bits for signatures. It's always bytes. Plus the type is std::vector<uint8_t>, not std::vector<bool>. https://codereview.chromium.org/1178193002/diff/180001/net/ssl/threaded_ssl_p... File net/ssl/threaded_ssl_private_key.h (right): https://codereview.chromium.org/1178193002/diff/180001/net/ssl/threaded_ssl_p... net/ssl/threaded_ssl_private_key.h:40: std::vector<uint8_t>* signature) = 0; Done. Though I slightly tweaked the "callback on any thread" rule to mention it's always called on the TaskRunner. https://codereview.chromium.org/1178193002/diff/180001/net/ssl/threaded_ssl_p... net/ssl/threaded_ssl_private_key.h:47: const scoped_refptr<base::TaskRunner>& task_runner); On 2015/06/23 15:27:29, Ryan Sleevi wrote: > Is the API contract TaskRunner or SequencedTaskRunner? Does it make a difference > if multiple invocations were executing in parallel? > > (My gut is that we either want SequencedTaskRunner or SingleThreadTaskRunner as > the API contract for these; really, the strong requirement comes from the > concrete implementations, but since under the Liskov Substitution Principle you > really don't want concrete impls requiring stronger pre-conditions than their > base class, we should virally propagate this to the base class as the strong API > guarantee) > Hrm. I think I would say that ThreadedSSLPrivateKey should take a base::TaskRunner because it internally doesn't care. Instead it promises that SignDigest will be called on the TaskRunner. Then ssl_platform_key.h takes a SingleThreadTaskRunner parameter, passes it to ThreadedSSLPrivateKey along with the delegate that is potentially buggy. I definitely agree that ssl_platform_key.h should take a SingleThreadTaskRunner because that's where the single-threaded constraint is coming from, so done on that. I don't think this new one breaks Liskov? (If you disagree, I'm happy to bleed the SingleThreadTaskRunner further though.)
https://codereview.chromium.org/1178193002/diff/180001/net/ssl/threaded_ssl_p... File net/ssl/threaded_ssl_private_key.h (right): https://codereview.chromium.org/1178193002/diff/180001/net/ssl/threaded_ssl_p... net/ssl/threaded_ssl_private_key.h:47: const scoped_refptr<base::TaskRunner>& task_runner); On 2015/06/24 21:43:13, David Benjamin wrote: > On 2015/06/23 15:27:29, Ryan Sleevi wrote: > > Is the API contract TaskRunner or SequencedTaskRunner? Does it make a > difference > > if multiple invocations were executing in parallel? > > > > (My gut is that we either want SequencedTaskRunner or SingleThreadTaskRunner > as > > the API contract for these; really, the strong requirement comes from the > > concrete implementations, but since under the Liskov Substitution Principle > you > > really don't want concrete impls requiring stronger pre-conditions than their > > base class, we should virally propagate this to the base class as the strong > API > > guarantee) > > > > Hrm. I think I would say that ThreadedSSLPrivateKey should take a > base::TaskRunner because it internally doesn't care. Instead it promises that > SignDigest will be called on the TaskRunner. > > Then ssl_platform_key.h takes a SingleThreadTaskRunner parameter, passes it to > ThreadedSSLPrivateKey along with the delegate that is potentially buggy. I > definitely agree that ssl_platform_key.h should take a SingleThreadTaskRunner > because that's where the single-threaded constraint is coming from, so done on > that. > > I don't think this new one breaks Liskov? (If you disagree, I'm happy to bleed > the SingleThreadTaskRunner further though.) (Er, s/SingleThread/Sequenced/. The WorkerPool vends out Sequenced. It's going to be SingleThreaded anyway but it didn't seem worth caring too much.)
https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... File net/ssl/ssl_platform_key_mac.cc (right): https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key_mac.cc:185: return ERR_SSL_CLIENT_AUTH_SIGNATURE_FAILED; On 2015/06/24 21:43:12, David Benjamin wrote: > On 2015/06/23 15:27:29, Ryan Sleevi wrote: > > Do we want a way to smuggle error details back up? > > Dunno. We could... I could add a string to the interface or new error codes or > whatever else. Any opinions? I mostly left it at an error code since that's what > //net did and... didn't end up using any error codes but > ERR_SSL_CLIENT_AUTH_SIGNATURE_FAILED... (Not sure where this "We could... I could" typo came from. I think I started typing one sentence, got distracted, and typed a different one. I'm really bad at typing. It was probably meant to be "We could... add a string to the interface or new error codes or whatever else" with the "..." because adding a string seems silly.)
davidben@chromium.org changed reviewers: + sergeyu@chromium.org
(Ryan, am I still waiting for a final stamp from you, since the last set of comments was still decently large and there were a few high-level questions?) +sergeyu for remoting https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... File net/ssl/ssl_platform_key.h (right): https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. Updated the cc files also to 2015 since that should be consistent w.r.t. whether it's a new file. https://codereview.chromium.org/1178193002/diff/220001/net/ssl/ssl_platform_k... File net/ssl/ssl_platform_key_win.cc (right): https://codereview.chromium.org/1178193002/diff/220001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key_win.cc:116: // switches are checked statically, this can be downgraded to a DCHECK. Replaced with a DCHECK_NE, since it seems clang/win is moving along.
Sorry, still LGTM. Should ahve been more explicit about that :) https://codereview.chromium.org/1178193002/diff/180001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1178193002/diff/180001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.cc:192: base::SequencedTaskRunner* task_runner() { return task_runner_.get(); } On 2015/06/24 21:43:12, David Benjamin wrote: > On 2015/06/23 15:27:28, Ryan Sleevi wrote: > > DANGER: There's a slight 'danger' in this API shape because with other places, > > the scoped_refptr<> backing the raw pointer could go out of scope in between > the > > full statements. > > > > While it's obviously not the case here, in general, it's desirable to return > it > > using a scoped container. You can avoid any added AddRef costs by using > .Pass() > > with the thing this is passing to > > > > (This is also why dcheng@ removed a lot of the "implicit" casts) > > Pass() doesn't work unless we pass parameters in as plain scoped_refptr<T> > rather than const scoped_refptr<T>&. (Which I'm actually all for. I think const > scoped_refptr<T>& is kinda weird. Though I do understand the reasons for it. > Pass() doesn't quiiiite capture all the cases where you might want to avoid the > ref-bouncing.) > > Anyway. I did something. Is this what you meant? Yes :) https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_private_key.h File net/ssl/ssl_private_key.h (right): https://codereview.chromium.org/1178193002/diff/180001/net/ssl/ssl_private_ke... net/ssl/ssl_private_key.h:54: virtual size_t GetMaxSignatureLength() = 0; On 2015/06/24 21:43:12, David Benjamin wrote: > On 2015/06/23 15:27:29, Ryan Sleevi wrote: > > While reviewing this, I still found it confusing that this type isn't explicit > > (bits vs bytes) > > > > Can you canonicalize what unit this is in, and enshrine it in the name? > > > > GetMaxSignatureLengthInBytes() ? GetMaxSignatureLengthBytes? > > Done, though I think a comment is sufficient for signatures. Nothing, not even > X.509 when you factor in how every profile works, uses bits for signatures. It's > always bytes. Plus the type is std::vector<uint8_t>, not std::vector<bool>. If the suggestion is that if you wanted bits, you'd use std::vector<bool> - no one does that :) They'd use std::vector<uint8_t> with the bit length (same as the C APIs that take a "const char*/unsigned char*" and an int for bit length. [There's special hell with bitsets as bools anyways; I think it was More Effective C++ that covered this, although it was somewhere in that series]
davidben@chromium.org changed reviewers: + garykac@chromium.org
+garykac for remoting. (Seems sergeyu is OOO.)
lgtm
The CQ bit was checked by davidben@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/1178193002/#ps290001 (title: "more mac build fix, this is blind while mac checkout syncs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1178193002/290001
Message was sent while issue was closed.
Committed patchset #16 (id:290001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/1d48952e7ac455a52ffbe6597b58a571faab42e6 Cr-Commit-Position: refs/heads/master@{#337079} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
