Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1696563002:
Blimp: add support for SSL connections. (Closed)
|
Created:
4 years, 10 months ago by Kevin M Modified:
4 years, 9 months ago CC:
anandc+watch-blimp_chromium.org, cbentzel+watch_chromium.org, chromium-reviews, dtrainor+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, Matt Welsh, nyquist+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBlimp: add support for SSL connections.
This CL allows the Blimp client to establish TLS-protected channels with the backend engine. The authenticity of the engine is validated by checking if its cert is an exact match of a certificate provided separately by the Assigner API.
* Create new Blimp SSL transport class: SSLClientTransport.
* Create custom CertValidator for checking an exact cert match against the SSL peer's cert
* Integrate SSLClientTransport with BlimpClientSession.
* Assignment: add certificate field.
* AssignmentSource: add certificate file reading; PEM file parsing;
X509 certificate parsing.
* Created new DEPS entries as appropriate.
R=wez@chromium.org
CC=rsleevi@chromium.org
BUG=585279, 589202
Committed: https://crrev.com/c80f5095f045ad1712f1f1075a44547a561f774a
Cr-Commit-Position: refs/heads/master@{#378839}
Committed: https://crrev.com/05e3f96385c7a9808dec06f13419d7bb1996ec45
Cr-Commit-Position: refs/heads/master@{#379081}
Patch Set 1 #
Total comments: 78
Patch Set 2 : Address wez feedback #Patch Set 3 : Removed extra deps from Dockerfile #
Total comments: 30
Patch Set 4 : Address wez feedback #
Total comments: 48
Patch Set 5 : Rebase against dtrainor's Assigner changes #Patch Set 6 : Address rsleevi feedback #Patch Set 7 : Address remaining rsleevi feedback items #
Total comments: 43
Patch Set 8 : Feedback on patch 7 comments #Patch Set 9 : scoped_refptr& => std::move() #Patch Set 10 : std::move'd another scoped_refptr. #
Total comments: 26
Patch Set 11 : Feedback for rsleevi #26 #
Total comments: 2
Patch Set 12 : Add safe_json JNI registry entries & BUILD deps #Patch Set 13 : Updated "running.md" #
Total comments: 47
Patch Set 14 : rsleevi feedback #40 #
Total comments: 6
Patch Set 15 : wez/bauerb feedback #
Total comments: 42
Patch Set 16 : wez feedback #48 #
Total comments: 32
Patch Set 17 : wez feedback #52 #Patch Set 18 : Rebase. #Patch Set 19 : data_deps => data #
Total comments: 6
Patch Set 20 : rsleevi changes #
Total comments: 1
Patch Set 21 : Reland (safe_json issue fixed). #Messages
Total messages: 76 (18 generated)
Some initial comments; haven't got to the SSLClientTransport itself yet. https://codereview.chromium.org/1696563002/diff/1/blimp/client/app/blimp_clie... File blimp/client/app/blimp_client_switches.h (right): https://codereview.chromium.org/1696563002/diff/1/blimp/client/app/blimp_clie... blimp/client/app/blimp_client_switches.h:17: // --blimplet-cert-path=/home/blimp/certs/cert.pem Presumably this must only be specified in conjunction w/ the --blimplet-ssl-port? Can we rename this --engine-cert-path or just --engine-cert? https://codereview.chromium.org/1696563002/diff/1/blimp/client/app/blimp_clie... blimp/client/app/blimp_client_switches.h:30: // --blimplet-ssl-port=25466. typo: encrypted https://codereview.chromium.org/1696563002/diff/1/blimp/client/app/blimp_clie... blimp/client/app/blimp_client_switches.h:31: extern const char kBlimpletSSLPort[]; Could we instead have a --engine-url parameter of the form scheme://<host>:<port> rather than have all these parameters? Or could we have a single parameter --test-mode which causes any subsequent parameters on the command-line to be treated as host, port and cert-file? (Having so many verbose-named parameters seems overkill and not very convenient for development/testing) https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:26: const uint16_t kNoPort = 0; nit: kPortNotSet or kInvalidPort? https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:49: uint port_64t; nit: This needs to be explicitly initialized. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:60: uint16_t GetBlimpletSSLPort() { This is basically identical to the TCP port one above - why not have a more generic GetUint16Parameter(const& switch_name) and have the CHECK include the name? https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:62: switches::kBlimpletSSLPort)) { GetSwitchValueASCII returns an empty string if the switch isn't present, so you don't really need to explicitly check for the switch beforehand - just early-exit if it's empty. (The existing e.g. IPAddress switch code could be simplified similarly) https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:66: uint port_64t; nit: This needs to be explicitly initialized. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:69: CHECK(false) << "Invalid SSL port " << port_str; nit: Save the result of StringToUint || IsValue.. into a bool and then CHECK(bool). Also note that range-checking the value isn't sufficient, since 0 is not a valid port number. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:77: base::FilePath GetBlimpletCertPath() { This function is redundant; GetSwitchValuePath() already returns an empty FilePath() if the switch is missing. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:90: Assignment::Assignment() {} See above re inline init; you need to make sure to initialize the port numbers, otherwise naive instantiation of an Assignment instance has it in a weird state. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:99: io_thread_.StartWithOptions(options); Cleaner to avoid creating this thread until you actually need it? https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:104: void AssignmentSource::ReadFromDisk( As noted elsewhere, this function can be a static that you Bind() any necessary data into. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:112: return; ICK! This sort of trampolining is safe while the calling object owns the Thread, but will break the moment some bright spark switches it to use some other IO thread that is owned externally, or re-orders this object's members so that the Thread member isn't deleted (and therefore joined) before all the others! https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:117: LOG(ERROR) << "Couldn't read cert from file: " << path.LossyDisplayName(); Why LOG(ERROR) when we CHECK for everything else? https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:120: main_task_runner_->PostTask(FROM_HERE, base::Bind(callback, output)); This doesn't match the API comment. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:127: if (!cert_str.empty()) { Surely you can't reach here if it's empty, so this should be a DCHECK? https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:128: scoped_refptr<net::X509Certificate> cert; Doesn't look like you use this? https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:130: int num_certs = 0; Rather than counting the certs, you can surely just CHECK() that there is no assignment.cert before you CreateFromBytes - the developer can go and check their PEM file and see how many certs they accidentally provided! https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:147: main_task_runner_->PostTask(FROM_HERE, base::Bind(callback, assignment)); You're already on the main task runner, with a clean stack - why are you posting a task to run this? https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:154: if (GetBlimpletTCPPort() > 0) { This if() is always true, since the Get*Port() method returns a uint16_t. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:166: base::Unretained(this), assignment, callback)); See notes above re combining file-read and cert-parse into a single static function using PostTaskAndReply. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:168: main_task_runner_->PostTask(FROM_HERE, base::Bind(callback, assignment)); Assert that the SSL port is kNoPort in this case? https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... File blimp/client/session/assignment_source.h (right): https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.h:30: ~Assignment(); IIUC you only need these because the struct is no longer purely POD? https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.h:34: int16_t ssl_port; Can you inline initialize these? These need to be uint16_t, otherwise they can't hold the full potential port range; your command-line parsing code uses uint16_t internally, so you're implicitly reinterpreting when you assign from one to the other - I'm a little surprising the compiler didn't complain! https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.h:41: class BLIMP_CLIENT_EXPORT AssignmentSource { It seems strange to be bundling the dev-mode file-based assignment fetch into this class - I'd assumed we'd have two discrete implementations, one for dev-mode and one for production. It looks like you're planning on having this class implement both, though? https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.h:50: virtual ~AssignmentSource(); Why is this dtor virtual? https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.h:58: // Sends the contents to |callback| on the IO thread. This comment looks to be incorrect? As far as I can tell there is no reason for either of these functions to even exist in this class, so having them here just increases the risk of future code changes breaking things, with no net benefit. You're just trying to turn a GetAssignment call into a file-read on the IO-thread followed by a decode of the PEM file (does that also have to be on the IO thread?), so all you need is for GetAssignment to PostTask a static function with all the necessary data, including the calling task-runner and callback, bound to it. Better still, you could use PostTaskAndReply to do some of the thread-hopping for you. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.h:59: void ReadFromDisk(base::FilePath path, |path| should be const& here https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.h:64: void ParseCertForAssignment(Assignment assignment, nit: It's really strange to be passing Assignment by-value; it'll end up being copied (expensive, since it's non-POD) and then be mutable by the recipient. As noted, though, it doesn't look like you actually need this method at all? https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/blimp_... File blimp/client/session/blimp_client_session.cc (right): https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/blimp_... blimp/client/session/blimp_client_session.cc:101: DLOG(FATAL) << "SSL port provided but no cert was found."; This check should be at the command-line parsing layer, not here. You could DCHECK() that it's set here, if you feel paranoid. https://codereview.chromium.org/1696563002/diff/1/blimp/engine/engine-manifes... File blimp/engine/engine-manifest.txt (right): https://codereview.chromium.org/1696563002/diff/1/blimp/engine/engine-manifes... blimp/engine/engine-manifest.txt:9: ./libosmesa.so Why so many new deps in this file? Did you remember to remove the unnecessary ones as per the comment above? (Which, incidentally, we should automate ;) https://codereview.chromium.org/1696563002/diff/1/blimp/net/blimp_cert_verifi... File blimp/net/blimp_cert_verifier.h (right): https://codereview.chromium.org/1696563002/diff/1/blimp/net/blimp_cert_verifi... blimp/net/blimp_cert_verifier.h:17: // Rejects all other certificates. Suggest: "Rejects all certificates other than the |engine_cert| specified at construction time."
https://codereview.chromium.org/1696563002/diff/1/blimp/net/blimp_cert_verifi... File blimp/net/blimp_cert_verifier.h (right): https://codereview.chromium.org/1696563002/diff/1/blimp/net/blimp_cert_verifi... blimp/net/blimp_cert_verifier.h:18: class BLIMP_NET_EXPORT BlimpCertVerifier : public net::CertVerifier { Is there anything actually Blimp specific about this verifier? In general if a class has nothing Blimp-specific, let's not prefix it with Blimp - that way it's more obvious that it could be considered for re-use elsewhere. https://codereview.chromium.org/1696563002/diff/1/blimp/net/ssl_client_transp... File blimp/net/ssl_client_transport.h (right): https://codereview.chromium.org/1696563002/diff/1/blimp/net/ssl_client_transp... blimp/net/ssl_client_transport.h:33: class BLIMP_NET_EXPORT SSLClientTransport : public BlimpTransport { TCP connection logic already exists in TCPClientTransport, so you could build SSLClientTransport as a subclass: - move OnTCPConnectComplete to a protected member, OnConnectComplete. - have a new OnTCPConnectComplete that is a protected virtual. - have TCPClientTransport::OnTCPConnectComplete simple call OnConnectComplete. - have SSLClientTransport override OnTCPConnectComplete by wrapping |socket_| w/ a new SSLStreamSocket and kicking off the SSL handshake. - on SSL handshake completion call down to OnConnectComplete as before. https://codereview.chromium.org/1696563002/diff/1/blimp/net/ssl_client_transp... blimp/net/ssl_client_transport.h:40: scoped_refptr<net::X509Certificate> assigned_cert, nit: Why is "assigned" relevant here? At this level all the transport cares about is that it is the certificate that the peer must present. https://codereview.chromium.org/1696563002/diff/1/blimp/net/ssl_client_transp... blimp/net/ssl_client_transport.h:53: void SetClientSocketFactoryForTest(net::ClientSocketFactory* factory); Does this need to be private? I thought we had a presubmit check that prevents non-test code from calling *ForTest methods, so we can leave it public? https://codereview.chromium.org/1696563002/diff/1/blimp/net/ssl_client_transp... blimp/net/ssl_client_transport.h:62: net::NetLog* net_log_; nit: Suggest inline init this to nullptr, and socket_factory_ below. https://codereview.chromium.org/1696563002/diff/1/blimp/net/ssl_client_transp... blimp/net/ssl_client_transport.h:66: scoped_ptr<BlimpCertVerifier> cert_verifier_; nit: Looks like this could be a direct member rather than allocating separately on heap.
https://codereview.chromium.org/1696563002/diff/1/blimp/client/app/blimp_clie... File blimp/client/app/blimp_client_switches.h (right): https://codereview.chromium.org/1696563002/diff/1/blimp/client/app/blimp_clie... blimp/client/app/blimp_client_switches.h:17: // --blimplet-cert-path=/home/blimp/certs/cert.pem On 2016/02/12 02:31:13, Wez wrote: > Presumably this must only be specified in conjunction w/ the > --blimplet-ssl-port? > > Can we rename this --engine-cert-path or just --engine-cert? This is consistent with the engine's use of a "-path" suffix to denote parameters that indirectly supply values via filepaths. https://codereview.chromium.org/1696563002/diff/1/blimp/client/app/blimp_clie... blimp/client/app/blimp_client_switches.h:30: // --blimplet-ssl-port=25466. On 2016/02/12 02:31:13, Wez wrote: > typo: encrypted Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/app/blimp_clie... blimp/client/app/blimp_client_switches.h:31: extern const char kBlimpletSSLPort[]; On 2016/02/12 02:31:12, Wez wrote: > Could we instead have a --engine-url parameter of the form > scheme://<host>:<port> rather than have all these parameters? > > Or could we have a single parameter --test-mode which causes any subsequent > parameters on the command-line to be treated as host, port and cert-file? > > (Having so many verbose-named parameters seems overkill and not very convenient > for development/testing) Would adding support for URLs imply that we should also support hostname resolution, though? https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:26: const uint16_t kNoPort = 0; On 2016/02/12 02:31:13, Wez wrote: > nit: kPortNotSet or kInvalidPort? Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:49: uint port_64t; On 2016/02/12 02:31:13, Wez wrote: > nit: This needs to be explicitly initialized. Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:60: uint16_t GetBlimpletSSLPort() { On 2016/02/12 02:31:13, Wez wrote: > This is basically identical to the TCP port one above - why not have a more > generic GetUint16Parameter(const& switch_name) and have the CHECK include the > name? Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:62: switches::kBlimpletSSLPort)) { On 2016/02/12 02:31:13, Wez wrote: > GetSwitchValueASCII returns an empty string if the switch isn't present, so you > don't really need to explicitly check for the switch beforehand - just > early-exit if it's empty. > > (The existing e.g. IPAddress switch code could be simplified similarly) Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:66: uint port_64t; On 2016/02/12 02:31:13, Wez wrote: > nit: This needs to be explicitly initialized. Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:69: CHECK(false) << "Invalid SSL port " << port_str; On 2016/02/12 02:31:13, Wez wrote: > nit: Save the result of StringToUint || IsValue.. into a bool and then > CHECK(bool). > > Also note that range-checking the value isn't sufficient, since 0 is not a valid > port number. Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:77: base::FilePath GetBlimpletCertPath() { On 2016/02/12 02:31:13, Wez wrote: > This function is redundant; GetSwitchValuePath() already returns an empty > FilePath() if the switch is missing. Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:90: Assignment::Assignment() {} On 2016/02/12 02:31:13, Wez wrote: > See above re inline init; you need to make sure to initialize the port numbers, > otherwise naive instantiation of an Assignment instance has it in a weird state. Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:99: io_thread_.StartWithOptions(options); On 2016/02/12 02:31:13, Wez wrote: > Cleaner to avoid creating this thread until you actually need it? Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:104: void AssignmentSource::ReadFromDisk( On 2016/02/12 02:31:13, Wez wrote: > As noted elsewhere, this function can be a static that you Bind() any necessary > data into. Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:112: return; On 2016/02/12 02:31:13, Wez wrote: > ICK! This sort of trampolining is safe while the calling object owns the > Thread, but will break the moment some bright spark switches it to use some > other IO thread that is owned externally, or re-orders this object's members so > that the Thread member isn't deleted (and therefore joined) before all the > others! Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:117: LOG(ERROR) << "Couldn't read cert from file: " << path.LossyDisplayName(); On 2016/02/12 02:31:13, Wez wrote: > Why LOG(ERROR) when we CHECK for everything else? Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:120: main_task_runner_->PostTask(FROM_HERE, base::Bind(callback, output)); On 2016/02/12 02:31:13, Wez wrote: > This doesn't match the API comment. Acknowledged. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:127: if (!cert_str.empty()) { On 2016/02/12 02:31:13, Wez wrote: > Surely you can't reach here if it's empty, so this should be a DCHECK? Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:128: scoped_refptr<net::X509Certificate> cert; On 2016/02/12 02:31:13, Wez wrote: > Doesn't look like you use this? Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:130: int num_certs = 0; On 2016/02/12 02:31:13, Wez wrote: > Rather than counting the certs, you can surely just CHECK() that there is no > assignment.cert before you CreateFromBytes - the developer can go and check > their PEM file and see how many certs they accidentally provided! Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:147: main_task_runner_->PostTask(FROM_HERE, base::Bind(callback, assignment)); On 2016/02/12 02:31:13, Wez wrote: > You're already on the main task runner, with a clean stack - why are you posting > a task to run this? This line is from Tommy's initial impl, I was thinking he might have had a good reason. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:154: if (GetBlimpletTCPPort() > 0) { On 2016/02/12 02:31:13, Wez wrote: > This if() is always true, since the Get*Port() method returns a uint16_t. Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:166: base::Unretained(this), assignment, callback)); On 2016/02/12 02:31:13, Wez wrote: > See notes above re combining file-read and cert-parse into a single static > function using PostTaskAndReply. Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.cc:168: main_task_runner_->PostTask(FROM_HERE, base::Bind(callback, assignment)); On 2016/02/12 02:31:13, Wez wrote: > Assert that the SSL port is kNoPort in this case? Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... File blimp/client/session/assignment_source.h (right): https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.h:30: ~Assignment(); On 2016/02/12 02:31:14, Wez wrote: > IIUC you only need these because the struct is no longer purely POD? Correct https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.h:34: int16_t ssl_port; vOn 2016/02/12 02:31:13, Wez wrote: > Can you inline initialize these? Done > These need to be uint16_t, otherwise they can't hold the full potential port > range; your command-line parsing code uses uint16_t internally, so you're > implicitly reinterpreting when you assign from one to the other - I'm a little > surprising the compiler didn't complain! Hmm, yes - why no warning? <_< Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.h:41: class BLIMP_CLIENT_EXPORT AssignmentSource { On 2016/02/12 02:31:13, Wez wrote: > It seems strange to be bundling the dev-mode file-based assignment fetch into > this class - I'd assumed we'd have two discrete implementations, one for > dev-mode and one for production. It looks like you're planning on having this > class implement both, though? I think the boundary between "dev mode" and "production" is blurry - we might want to run a release APK on an local engine instance, for example. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.h:50: virtual ~AssignmentSource(); On 2016/02/12 02:31:13, Wez wrote: > Why is this dtor virtual? Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.h:58: // Sends the contents to |callback| on the IO thread. On 2016/02/12 02:31:13, Wez wrote: > This comment looks to be incorrect? > > As far as I can tell there is no reason for either of these functions to even > exist in this class, so having them here just increases the risk of future code > changes breaking things, with no net benefit. > > You're just trying to turn a GetAssignment call into a file-read on the > IO-thread followed by a decode of the PEM file (does that also have to be on the > IO thread?), so all you need is for GetAssignment to PostTask a static function > with all the necessary data, including the calling task-runner and callback, > bound to it. > > Better still, you could use PostTaskAndReply to do some of the thread-hopping > for you. Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.h:59: void ReadFromDisk(base::FilePath path, On 2016/02/12 02:31:13, Wez wrote: > |path| should be const& here Done. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/assign... blimp/client/session/assignment_source.h:64: void ParseCertForAssignment(Assignment assignment, On 2016/02/12 02:31:14, Wez wrote: > nit: It's really strange to be passing Assignment by-value; it'll end up being > copied (expensive, since it's non-POD) and then be mutable by the recipient. > > As noted, though, it doesn't look like you actually need this method at all? Passing around Assignments as scoped_ptr now. https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/blimp_... File blimp/client/session/blimp_client_session.cc (right): https://codereview.chromium.org/1696563002/diff/1/blimp/client/session/blimp_... blimp/client/session/blimp_client_session.cc:101: DLOG(FATAL) << "SSL port provided but no cert was found."; On 2016/02/12 02:31:14, Wez wrote: > This check should be at the command-line parsing layer, not here. You could > DCHECK() that it's set here, if you feel paranoid. Done. https://codereview.chromium.org/1696563002/diff/1/blimp/engine/engine-manifes... File blimp/engine/engine-manifest.txt (right): https://codereview.chromium.org/1696563002/diff/1/blimp/engine/engine-manifes... blimp/engine/engine-manifest.txt:9: ./libosmesa.so On 2016/02/12 02:31:14, Wez wrote: > Why so many new deps in this file? Did you remember to remove the unnecessary > ones as per the comment above? (Which, incidentally, we should automate ;) I did - and Nick's comments on that CL indicate that the tool generates a lot of extra entries that's only conditionally needed (e.g. for certain command line configs.) I'll look into helping them out with that, since IMO tool output is much less useful if there's manual assembly required. ;) https://codereview.chromium.org/1696563002/diff/1/blimp/net/blimp_cert_verifi... File blimp/net/blimp_cert_verifier.h (right): https://codereview.chromium.org/1696563002/diff/1/blimp/net/blimp_cert_verifi... blimp/net/blimp_cert_verifier.h:17: // Rejects all other certificates. On 2016/02/12 02:31:14, Wez wrote: > Suggest: "Rejects all certificates other than the |engine_cert| specified at > construction time." Done. https://codereview.chromium.org/1696563002/diff/1/blimp/net/blimp_cert_verifi... blimp/net/blimp_cert_verifier.h:18: class BLIMP_NET_EXPORT BlimpCertVerifier : public net::CertVerifier { On 2016/02/12 21:42:38, Wez wrote: > Is there anything actually Blimp specific about this verifier? > > In general if a class has nothing Blimp-specific, let's not prefix it with Blimp > - that way it's more obvious that it could be considered for re-use elsewhere. Done. https://codereview.chromium.org/1696563002/diff/1/blimp/net/ssl_client_transp... File blimp/net/ssl_client_transport.h (right): https://codereview.chromium.org/1696563002/diff/1/blimp/net/ssl_client_transp... blimp/net/ssl_client_transport.h:33: class BLIMP_NET_EXPORT SSLClientTransport : public BlimpTransport { On 2016/02/12 21:42:38, Wez wrote: > TCP connection logic already exists in TCPClientTransport, so you could build > SSLClientTransport as a subclass: > > - move OnTCPConnectComplete to a protected member, OnConnectComplete. > - have a new OnTCPConnectComplete that is a protected virtual. > - have TCPClientTransport::OnTCPConnectComplete simple call OnConnectComplete. > - have SSLClientTransport override OnTCPConnectComplete by wrapping |socket_| w/ > a new SSLStreamSocket and kicking off the SSL handshake. > - on SSL handshake completion call down to OnConnectComplete as before. Done. https://codereview.chromium.org/1696563002/diff/1/blimp/net/ssl_client_transp... blimp/net/ssl_client_transport.h:40: scoped_refptr<net::X509Certificate> assigned_cert, On 2016/02/12 21:42:38, Wez wrote: > nit: Why is "assigned" relevant here? At this level all the transport cares > about is that it is the certificate that the peer must present. Done. https://codereview.chromium.org/1696563002/diff/1/blimp/net/ssl_client_transp... blimp/net/ssl_client_transport.h:53: void SetClientSocketFactoryForTest(net::ClientSocketFactory* factory); On 2016/02/12 21:42:38, Wez wrote: > Does this need to be private? I thought we had a presubmit check that prevents > non-test code from calling *ForTest methods, so we can leave it public? Done. https://codereview.chromium.org/1696563002/diff/1/blimp/net/ssl_client_transp... blimp/net/ssl_client_transport.h:62: net::NetLog* net_log_; On 2016/02/12 21:42:38, Wez wrote: > nit: Suggest inline init this to nullptr, and socket_factory_ below. Done. https://codereview.chromium.org/1696563002/diff/1/blimp/net/ssl_client_transp... blimp/net/ssl_client_transport.h:66: scoped_ptr<BlimpCertVerifier> cert_verifier_; On 2016/02/12 21:42:38, Wez wrote: > nit: Looks like this could be a direct member rather than allocating separately > on heap. Done.
kmarshall@chromium.org changed reviewers: + pauljensen@chromium.org
+pauljensen@chromium.org for DEPS
kmarshall@chromium.org changed reviewers: + rsleevi@chromium.org
+rsleevi for custom CertVerifier (see exact_match_cert_verifier.(cc|h))
Please write up more in the bug report or provide a design doc. This is security-critical code and I want to ensure it gets adequate review and documentation.
To be clearer: Your CL description reads what's changing, but doesn't explain why it's changing. Neither does the bug. My goal is not just to make sure your code is mechnically correct, but that it's logically correct - that it helps you achieve the thing you set out to do, that you have tests for the cases where you don't expect, and that the design and approach is sound with your long term goals. To that end, we rely on CL descriptions and bugs to capture that; this is especially important for future readers of the code, who will only read the commit message in trying to figure things out.
kmarshall@chromium.org changed reviewers: + gcasto@chromium.org
I added more context to the bug: crbug.com/585279 Thanks.
kmarshall@chromium.org changed reviewers: - gcasto@chromium.org, pauljensen@chromium.org
-pauljensen because rsleevi is a net/ OWNER; don't need a separate DEPS reviewer
https://codereview.chromium.org/1696563002/diff/40001/blimp/client/app/blimp_... File blimp/client/app/blimp_client_switches.h (right): https://codereview.chromium.org/1696563002/diff/40001/blimp/client/app/blimp_... blimp/client/app/blimp_client_switches.h:13: // The path to the Blimplet's PEM-encoded X509 certificate. s/Blimplet/Engine throughout the comments in this header. https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.cc:33: switches::kEngineHost); This is super verbose; can't you just say: string host = GetSwitchValueASCII(...); if (host.empty()) host = kDefaultBlimpletIPAddress; https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.cc:39: CHECK(false) << "Invalid BlimpletAssignment host " << host; Do you mean CHECK(ip_address.AssignFromIPLiteral(...)) << ... here? If you _do_ ever find yourself doing CHECK(false), use LOG(FATAL) instead. https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.cc:44: // Returns true if the parameter was provided. This doesn't make clear what it does if the parameter isn't provided. Wouldn't it be cleaner to use the invalid zero value to indicate that no port number was provided? https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.cc:46: // within the range [1, 65535] Suggest: "CHECK()s that |param| decodes to a valid IP port-number." https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.cc:58: CHECK(is_valid) << "Invalid range for parameter " << param; nit: Misleading; may also not be a number. nit: Why use IsValueInRangeForNumericType, rather than comparing against 65535 explicitly, since that is the maximum valid IP port? https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.cc:59: *output = base::checked_cast<uint16_t>(param_parsed); No need to use checked_cast<> here, since |param_parsed| can never be out-of-range at this point. https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.cc:76: DCHECK(assignment); nit: This code dereferences |assignment| directly, so there isn't really any need to DCHECK it here. https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.cc:82: CHECK(!assignment->cert.get()); nit: No need for .get() when testing a scoped[ref]ptr. Suggest adding a message to the CHECK(...) << "Multiple CERTIFICATE entries provided." https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.cc:108: options.message_loop_type = base::MessageLoop::TYPE_IO; nit: Why not use the type+size constructor with the "use default" size of zero? https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.cc:132: base::Passed(std::move(assignment)), callback)); Eeek; you'll need to take the bare pointer to |cert_str| before the PostTaskAndReply() call - the compiler is at liberty to evaluated parameters in whatever order makes most sense for the platform, so you must never a.get() and std::move(a) in the same parameter sequence. https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... File blimp/client/session/assignment_source.h (right): https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.h:57: // Gets the task runner for reading files on the IO thread. Suggest "Returns the TaskRunner for |io_thread_|, creating the thread if necessary." https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/bl... File blimp/client/session/blimp_client_session.cc (right): https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/bl... blimp/client/session/blimp_client_session.cc:98: CreateAddressList(assignment.ip_addresses, assignment.ssl_port), nit: Could the Assignment just contain a net::AddressList directly? https://codereview.chromium.org/1696563002/diff/40001/blimp/net/exact_match_c... File blimp/net/exact_match_cert_verifier.h (right): https://codereview.chromium.org/1696563002/diff/40001/blimp/net/exact_match_c... blimp/net/exact_match_cert_verifier.h:23: scoped_refptr<net::X509Certificate> assigned_cert); nit: Rename this to match the comment? https://codereview.chromium.org/1696563002/diff/40001/blimp/net/ssl_client_tr... File blimp/net/ssl_client_transport.h (right): https://codereview.chromium.org/1696563002/diff/40001/blimp/net/ssl_client_tr... blimp/net/ssl_client_transport.h:33: // Creates and connects SSL socket connections to Blimplets. s/Blimplets/Engines
https://codereview.chromium.org/1696563002/diff/40001/blimp/client/app/blimp_... File blimp/client/app/blimp_client_switches.h (right): https://codereview.chromium.org/1696563002/diff/40001/blimp/client/app/blimp_... blimp/client/app/blimp_client_switches.h:13: // The path to the Blimplet's PEM-encoded X509 certificate. On 2016/02/18 00:40:50, Wez wrote: > s/Blimplet/Engine throughout the comments in this header. Done. https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.cc:33: switches::kEngineHost); On 2016/02/18 00:40:50, Wez wrote: > This is super verbose; can't you just say: > > string host = GetSwitchValueASCII(...); > if (host.empty()) > host = kDefaultBlimpletIPAddress; Done. https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.cc:39: CHECK(false) << "Invalid BlimpletAssignment host " << host; On 2016/02/18 00:40:50, Wez wrote: > Do you mean CHECK(ip_address.AssignFromIPLiteral(...)) << ... here? > > If you _do_ ever find yourself doing CHECK(false), use LOG(FATAL) instead. Done. https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.cc:44: // Returns true if the parameter was provided. On 2016/02/18 00:40:50, Wez wrote: > This doesn't make clear what it does if the parameter isn't provided. > > Wouldn't it be cleaner to use the invalid zero value to indicate that no port > number was provided? Done. https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.cc:46: // within the range [1, 65535] On 2016/02/18 00:40:50, Wez wrote: > Suggest: "CHECK()s that |param| decodes to a valid IP port-number." Done. https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.cc:58: CHECK(is_valid) << "Invalid range for parameter " << param; On 2016/02/18 00:40:50, Wez wrote: > nit: Misleading; may also not be a number. > > nit: Why use IsValueInRangeForNumericType, rather than comparing against 65535 > explicitly, since that is the maximum valid IP port? Done. https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.cc:59: *output = base::checked_cast<uint16_t>(param_parsed); On 2016/02/18 00:40:50, Wez wrote: > No need to use checked_cast<> here, since |param_parsed| can never be > out-of-range at this point. Done. https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.cc:76: DCHECK(assignment); On 2016/02/18 00:40:50, Wez wrote: > nit: This code dereferences |assignment| directly, so there isn't really any > need to DCHECK it here. Done. https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.cc:82: CHECK(!assignment->cert.get()); On 2016/02/18 00:40:50, Wez wrote: > nit: No need for .get() when testing a scoped[ref]ptr. > > Suggest adding a message to the CHECK(...) << "Multiple CERTIFICATE entries > provided." Done. https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.cc:108: options.message_loop_type = base::MessageLoop::TYPE_IO; On 2016/02/18 00:40:50, Wez wrote: > nit: Why not use the type+size constructor with the "use default" size of zero? Done. https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.cc:132: base::Passed(std::move(assignment)), callback)); On 2016/02/18 00:40:50, Wez wrote: > Eeek; you'll need to take the bare pointer to |cert_str| before the > PostTaskAndReply() call - the compiler is at liberty to evaluated parameters in > whatever order makes most sense for the platform, so you must never a.get() and > std::move(a) in the same parameter sequence. Good catch, thanks! Done https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... File blimp/client/session/assignment_source.h (right): https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/as... blimp/client/session/assignment_source.h:57: // Gets the task runner for reading files on the IO thread. On 2016/02/18 00:40:50, Wez wrote: > Suggest "Returns the TaskRunner for |io_thread_|, creating the thread if > necessary." Done. https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/bl... File blimp/client/session/blimp_client_session.cc (right): https://codereview.chromium.org/1696563002/diff/40001/blimp/client/session/bl... blimp/client/session/blimp_client_session.cc:98: CreateAddressList(assignment.ip_addresses, assignment.ssl_port), On 2016/02/18 00:40:50, Wez wrote: > nit: Could the Assignment just contain a net::AddressList directly? AddressList is a list of IPEndpoints, which are address/port pairs. I would like to keep the list of addresses port-agnostic, so that they can be used across multiple transport types/ports. https://codereview.chromium.org/1696563002/diff/40001/blimp/net/exact_match_c... File blimp/net/exact_match_cert_verifier.h (right): https://codereview.chromium.org/1696563002/diff/40001/blimp/net/exact_match_c... blimp/net/exact_match_cert_verifier.h:23: scoped_refptr<net::X509Certificate> assigned_cert); On 2016/02/18 00:40:50, Wez wrote: > nit: Rename this to match the comment? Done. https://codereview.chromium.org/1696563002/diff/40001/blimp/net/ssl_client_tr... File blimp/net/ssl_client_transport.h (right): https://codereview.chromium.org/1696563002/diff/40001/blimp/net/ssl_client_tr... blimp/net/ssl_client_transport.h:33: // Creates and connects SSL socket connections to Blimplets. On 2016/02/18 00:40:50, Wez wrote: > s/Blimplets/Engines Done, with singular "Engine" (the instance is bound to a single endpoint)
To the extent I can review this, I've done my best. Whenever someone introduces a new socket abstraction, I weep inside a little, knowing that it will almost certainly cause pain, but such is life. I would strongly discourage the SetSocket/TakeSocket approach, from our lessons learned in //net, but that's up to you and wez@ to work out and maintain. I believe it violates some of the Chrome design principles (like single responsibility) and makes it much harder to reason about, but that's just my $.02. I would also encourage you that, as you think about your networking stack, you avoid patterns like those we saw in libjingle and cast - that is, excessive use of currying methods/callbacks and of trying to design connection oriented methods as linear flows. The Chrome //net code uses state machines (expressed via the DoLoop pattern throughout), as it makes it significantly easier to reason about, document, and debug the code flow. For example, "if I had a pony", your TLS socket would follow a similar pattern as //net's ConnectJob interface, which abstract out different (asynchronous) steps that need to happen, which range from things like DNS resolution (in the case of TCP sockets) to more complex cases (TLS first needing a TCP connection), to greatly complex case (HTTP proxies needing a TCP[+SSL] connection, writing an HTTP request, and then draining / piping the response). You'll find your ability to move and iterate quickly will be greatly enhanced if you put the design-work in beforehand to accomodate such work. This is especially true if your Blimp client will need to handle proxies (and I'm sure it eventually will, if it's meant to be a side-by-side execution environment) https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:18: #include "net/cert/pem_tokenizer.h" DESIGN: This is not intended to be used outside of //net Please use the supported //net interface of X509Certificate::CreateFromBytes(..., ..., X509Certificate::FORMAT_PEM_SEQUENCE); (The existing uses are bugs that should be fixed - filed https://bugs.chromium.org/p/chromium/issues/detail?id=588298 for that ) https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:36: CHECK(ip_address.AssignFromIPLiteral(host)) DESIGN: It seems counter to Chromium practices to CHECK() on user-input. https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:52: uint param_parsed = 0; BUG: C++ does not define "uint". Please use one of the standard types from the language or stddef.h. In this case, your API contract is "unsigned". https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:55: CHECK(is_valid) << "Invalid range for parameter " << param; DESIGN: Please use //base/numerics/safe_conversions.h checked_cast for converting "unsigned" to uint16_t https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:75: net::PEMTokenizer pem_tokenizer(*cert_str, {"CERTIFICATE"}); STYLE: Uniform initialization syntax is explicitly forbidden in Chromium - see https://chromium-cpp.appspot.com/#core-blacklist https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:98: scoped_refptr<base::SingleThreadTaskRunner> DESIGN: Your API contract does not require the use of a SingleThreadTaskRunner; you could return a better type, such as a SequencedTaskRunner or, ideally, a TaskRunner, which accomplishes your goal/use case. https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:124: std::string* cert_str_ptr = cert_str.get(); PostTaskAndReplyWithResult explicitly exists to avoid needing this level of heap allocation gymanstics Change your signature to std::string ReadFromDisk(const base::FilePath& path) to accomodate this. https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:125: GetIOTaskRunner()->PostTaskAndReply( BUG/DESIGN: This seems improperly named, or at least, likely to confuse any Chromie that works on this, as the IO thread is *not* allowed to do file access (that's handled by a file thread). Nor is it necessary for your use case to use a MessageLoop::TYPE_IO file thread for this, IIRC ( I believe all our FILE threads are just normal MessageLoops) https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:128: base::Passed(std::move(assignment)), callback)); DESIGN: While this is only one level, I find myself frequently discouraging people from this pattern, because it makes it difficult to follow the code flow and execution state machine, and makes it easy to introduce bugs later on, as time has repeatedly borne out. Our encouragement from //net is to consider explicit state machines to make this clear - but this is up to you and your team, who will ultimately maintain this. https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... File blimp/client/session/assignment_source.h (right): https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.h:12: #include "base/files/file_path.h" Unused https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.h:16: #include "net/base/hash_value.h" Unused https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.h:17: #include "net/cert/x509_certificate.h" Can forward declare https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.h:32: std::vector<net::IPAddress> ip_addresses; STYLE: Document this. It took me a while of reading the code to figure out how this is used, and I'm not entirely sure it's correct/desirable. That is, you're seemingly using this "as if" it was resolved - that is, addresses have a non-deterministic order in which they'll be attempted. Further, you only ever use one IP address - it seems like it'd make your later construction of the AddressList easier if you just stored a single net::IPAddress here (and then later constructed the AddressList using https://code.google.com/p/chromium/codesearch#chromium/src/net/base/address_l... ) Alternatively, if you really do want that behaviour, then it seems better to store both the TCP addresses and the SSL addresses themselves in individual net::AddressLists here (which encompass both the net::IPAddress and the port) https://codereview.chromium.org/1696563002/diff/60001/blimp/net/exact_match_c... File blimp/net/exact_match_cert_verifier.cc (right): https://codereview.chromium.org/1696563002/diff/60001/blimp/net/exact_match_c... blimp/net/exact_match_cert_verifier.cc:8: #include "blimp/net/exact_match_cert_verifier.h" STYLE: This should be at the top of the file, before any other includes Please see https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes https://codereview.chromium.org/1696563002/diff/60001/blimp/net/exact_match_c... blimp/net/exact_match_cert_verifier.cc:18: : engine_cert_(engine_cert) {} STYLE: You're passing a scoped_refptr<> as non-const, so you need to std::move() this to avoid unnecessary refcount churn. https://codereview.chromium.org/1696563002/diff/60001/blimp/net/exact_match_c... blimp/net/exact_match_cert_verifier.cc:33: if (!cert->Equals(engine_cert_.get())) { BUG: You return an error code but fail to set the CertStatus of |verify_result| https://codereview.chromium.org/1696563002/diff/60001/blimp/net/exact_match_c... blimp/net/exact_match_cert_verifier.cc:37: verify_result->verified_cert = cert; POTENTIAL BUG: You fail to fill in public_key_hashes to match verified_cert. Downstream consumers of CertVerifier generally expect this to be filled (the exception is unit tests) https://codereview.chromium.org/1696563002/diff/60001/blimp/net/exact_match_c... File blimp/net/exact_match_cert_verifier.h (right): https://codereview.chromium.org/1696563002/diff/60001/blimp/net/exact_match_c... blimp/net/exact_match_cert_verifier.h:36: const net::BoundNetLog&) override; STYLE: https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D... "Parameter names may be omitted only if the parameter is unused and its purpose is obvious." I do not believe these parameters meet that criteria. https://codereview.chromium.org/1696563002/diff/60001/blimp/net/ssl_client_tr... File blimp/net/ssl_client_transport.cc (right): https://codereview.chromium.org/1696563002/diff/60001/blimp/net/ssl_client_tr... blimp/net/ssl_client_transport.cc:25: : TCPClientTransport(addresses, net_log), cert_verifier_(cert) {} STYLE: You're passing a scoped_refptr<> as a non-const, so you need to std::move() this to avoid unnecessary refcount churn. https://codereview.chromium.org/1696563002/diff/60001/blimp/net/ssl_client_tr... blimp/net/ssl_client_transport.cc:58: net::HostPortPair::FromIPEndPoint(connected_endpoint); DESIGN: In production code, this would be categorically unsafe for real SSL. Though you're not attempting to properly implement TLS at this time, your API should explicitly take the desired hostname (or IP address, in string-form), and pass that through the appropriate layers, as this is necessary for TLS connections. I have trouble l-g-t-m'ing this change, knowing that the API encourages unsafe practices in its present form. https://codereview.chromium.org/1696563002/diff/60001/blimp/net/ssl_client_tr... File blimp/net/ssl_client_transport_unittest.cc (right): https://codereview.chromium.org/1696563002/diff/60001/blimp/net/ssl_client_tr... blimp/net/ssl_client_transport_unittest.cc:20: const char kIPV4Address[] = "192.168.0.1"; Please use a loopback address (127.0.0.x) to avoid any improper behaviour.
Description was changed from ========== Blimp: add support for SSL connections. * Create new Blimp SSL transport class: SSLClientTransport. * Create custom CertValidator for checking an exact cert match against the SSL peer's cert * Integrate SSLClientTransport with BlimpClientSession. * Assignment: add certificate field. * AssignmentSource: add certificate file reading; PEM file parsing; X509 certificate parsing. * Created new DEPS entries as appropriate. R=wez@chromium.org CC=rsleevi@chromium.org BUG=585279 ========== to ========== Blimp: add support for SSL connections. This CL allows the Blimp client to establish TLS-protected channels with the backend engine. The authenticity of the engine is validated by checking if its cert is an exact match of a certificate provided separately by the Assigner API. * Create new Blimp SSL transport class: SSLClientTransport. * Create custom CertValidator for checking an exact cert match against the SSL peer's cert * Integrate SSLClientTransport with BlimpClientSession. * Assignment: add certificate field. * AssignmentSource: add certificate file reading; PEM file parsing; X509 certificate parsing. * Created new DEPS entries as appropriate. R=wez@chromium.org CC=rsleevi@chromium.org BUG=585279 ==========
"I would strongly discourage the SetSocket/TakeSocket approach, from our lessons learned in //net, but that's up to you and wez@ to work out and maintain. I believe it violates some of the Chrome design principles (like single responsibility) and makes it much harder to reason about, but that's just my $.02." We had gone back and forth between the two approaches of reusing TCPClientTransport and duplicating the TCP connection logic portions. I'll do that in a followup patch on the same CL, but in the meantime you can take a look at the feedback items that were addressed. "For example, "if I had a pony", your TLS socket would follow a similar pattern as //net's ConnectJob interface..." That's great to know about, and it seems like something we should do in the near future. That big of a change would blow up the scope of this bug, so I created a bug for it (https://bugs.chromium.org/p/chromium/issues/detail?id=588860) and added it as a dependency to our main cleanup bug. https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:36: CHECK(ip_address.AssignFromIPLiteral(host)) On 2016/02/19 22:56:08, Ryan Sleevi wrote: > DESIGN: It seems counter to Chromium practices to CHECK() on user-input. Do command line parameters count as user input, as is the case here? https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:52: uint param_parsed = 0; On 2016/02/19 22:56:07, Ryan Sleevi wrote: > BUG: C++ does not define "uint". Please use one of the standard types from the > language or stddef.h. > > In this case, your API contract is "unsigned". Done. https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:55: CHECK(is_valid) << "Invalid range for parameter " << param; On 2016/02/19 22:56:08, Ryan Sleevi wrote: > DESIGN: Please use //base/numerics/safe_conversions.h checked_cast for > converting "unsigned" to uint16_t Done. Should ParseHostAndPort be made to return |port| as an uint16_t* instead of an int*? https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:75: net::PEMTokenizer pem_tokenizer(*cert_str, {"CERTIFICATE"}); On 2016/02/19 22:56:07, Ryan Sleevi wrote: > STYLE: Uniform initialization syntax is explicitly forbidden in Chromium - see > https://chromium-cpp.appspot.com/#core-blacklist Done. (FYI, other PEMTokenizer clients do the same thing and should be clened up - though, they shouldn't be using that class in the first place?) https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:98: scoped_refptr<base::SingleThreadTaskRunner> On 2016/02/19 22:56:08, Ryan Sleevi wrote: > DESIGN: Your API contract does not require the use of a SingleThreadTaskRunner; > you could return a better type, such as a SequencedTaskRunner or, ideally, a > TaskRunner, which accomplishes your goal/use case. The URLRequestContextGetter (now integrated on trunk, see the rebased file) needs a SingleThreadTaskRunner. I'll store it in a field as a plain ol' TaskRunner for PostBackAndReplyWithResult. https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:124: std::string* cert_str_ptr = cert_str.get(); On 2016/02/19 22:56:08, Ryan Sleevi wrote: > PostTaskAndReplyWithResult explicitly exists to avoid needing this level of heap > allocation gymanstics > > Change your signature to > > std::string ReadFromDisk(const base::FilePath& path) to accomodate this. Thanks, done. I switched GetCustomAssignment to use PostTaskAndReplyWithResult, and it's a nice improvement. https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:125: GetIOTaskRunner()->PostTaskAndReply( On 2016/02/19 22:56:07, Ryan Sleevi wrote: > BUG/DESIGN: This seems improperly named, or at least, likely to confuse any > Chromie that works on this, as the IO thread is *not* allowed to do file access > (that's handled by a file thread). > > Nor is it necessary for your use case to use a MessageLoop::TYPE_IO file thread > for this, IIRC ( I believe all our FILE threads are just normal MessageLoops) Question: how do we create a FILE thread using base::Thread? The client doesn't depend on content/ so we don't have access to the usual BrowserThreads. https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:128: base::Passed(std::move(assignment)), callback)); On 2016/02/19 22:56:07, Ryan Sleevi wrote: > DESIGN: While this is only one level, I find myself frequently discouraging > people from this pattern, because it makes it difficult to follow the code flow > and execution state machine, and makes it easy to introduce bugs later on, as > time has repeatedly borne out. > > Our encouragement from //net is to consider explicit state machines to make this > clear - but this is up to you and your team, who will ultimately maintain this. Ack. https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... File blimp/client/session/assignment_source.h (right): https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.h:12: #include "base/files/file_path.h" On 2016/02/19 22:56:08, Ryan Sleevi wrote: > Unused Done. https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.h:16: #include "net/base/hash_value.h" On 2016/02/19 22:56:08, Ryan Sleevi wrote: > Unused Done. https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.h:17: #include "net/cert/x509_certificate.h" On 2016/02/19 22:56:08, Ryan Sleevi wrote: > Can forward declare Done. https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.h:32: std::vector<net::IPAddress> ip_addresses; On 2016/02/19 22:56:08, Ryan Sleevi wrote: > STYLE: Document this. It took me a while of reading the code to figure out how > this is used, and I'm not entirely sure it's correct/desirable. > > That is, you're seemingly using this "as if" it was resolved - that is, > addresses have a non-deterministic order in which they'll be attempted. Further, > you only ever use one IP address - it seems like it'd make your later > construction of the AddressList easier if you just stored a single > net::IPAddress here (and then later constructed the AddressList using > https://code.google.com/p/chromium/codesearch#chromium/src/net/base/address_l... > ) > > Alternatively, if you really do want that behaviour, then it seems better to > store both the TCP addresses and the SSL addresses themselves in individual > net::AddressLists here (which encompass both the net::IPAddress and the port) Agreed that multiple addresses contribute to ambiguity. I'll switch to one IPEndpoint host+port pair with a protocol enum, so it's 100% clear what the endpoint should be. In the future we might want to support multiple connection types (e.g. favor QUIC connections first, then fallback to TLS) but we can add that capability pretty easily. No sense in overdesigning up front. https://codereview.chromium.org/1696563002/diff/60001/blimp/net/exact_match_c... File blimp/net/exact_match_cert_verifier.cc (right): https://codereview.chromium.org/1696563002/diff/60001/blimp/net/exact_match_c... blimp/net/exact_match_cert_verifier.cc:8: #include "blimp/net/exact_match_cert_verifier.h" On 2016/02/19 22:56:08, Ryan Sleevi wrote: > STYLE: This should be at the top of the file, before any other includes > > Please see > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes Done. https://codereview.chromium.org/1696563002/diff/60001/blimp/net/exact_match_c... blimp/net/exact_match_cert_verifier.cc:18: : engine_cert_(engine_cert) {} On 2016/02/19 22:56:08, Ryan Sleevi wrote: > STYLE: You're passing a scoped_refptr<> as non-const, so you need to std::move() > this to avoid unnecessary refcount churn. Done. I converted this parameter into a const reference. https://codereview.chromium.org/1696563002/diff/60001/blimp/net/exact_match_c... blimp/net/exact_match_cert_verifier.cc:33: if (!cert->Equals(engine_cert_.get())) { On 2016/02/19 22:56:08, Ryan Sleevi wrote: > BUG: You return an error code but fail to set the CertStatus of |verify_result| Done. https://codereview.chromium.org/1696563002/diff/60001/blimp/net/exact_match_c... blimp/net/exact_match_cert_verifier.cc:37: verify_result->verified_cert = cert; On 2016/02/19 22:56:08, Ryan Sleevi wrote: > POTENTIAL BUG: You fail to fill in public_key_hashes to match verified_cert. > Downstream consumers of CertVerifier generally expect this to be filled (the > exception is unit tests) Done. https://codereview.chromium.org/1696563002/diff/60001/blimp/net/exact_match_c... File blimp/net/exact_match_cert_verifier.h (right): https://codereview.chromium.org/1696563002/diff/60001/blimp/net/exact_match_c... blimp/net/exact_match_cert_verifier.h:36: const net::BoundNetLog&) override; On 2016/02/19 22:56:08, Ryan Sleevi wrote: > STYLE: > https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D... > > "Parameter names may be omitted only if the parameter is unused and its purpose > is obvious." > > I do not believe these parameters meet that criteria. > Done. https://codereview.chromium.org/1696563002/diff/60001/blimp/net/ssl_client_tr... File blimp/net/ssl_client_transport.cc (right): https://codereview.chromium.org/1696563002/diff/60001/blimp/net/ssl_client_tr... blimp/net/ssl_client_transport.cc:25: : TCPClientTransport(addresses, net_log), cert_verifier_(cert) {} On 2016/02/19 22:56:08, Ryan Sleevi wrote: > STYLE: You're passing a scoped_refptr<> as a non-const, so you need to > std::move() this to avoid unnecessary refcount churn. Thanks, but I'll just make this const& for consistency. https://codereview.chromium.org/1696563002/diff/60001/blimp/net/ssl_client_tr... blimp/net/ssl_client_transport.cc:58: net::HostPortPair::FromIPEndPoint(connected_endpoint); On 2016/02/19 22:56:08, Ryan Sleevi wrote: > DESIGN: In production code, this would be categorically unsafe for real SSL. > > Though you're not attempting to properly implement TLS at this time, your API > should explicitly take the desired hostname (or IP address, in string-form), and > pass that through the appropriate layers, as this is necessary for TLS > connections. > > I have trouble l-g-t-m'ing this change, knowing that the API encourages unsafe > practices in its present form. Done - the original address is used here. I can also check if the connected socket's peer address matches, too, but I'm not sure if that would be adding any security. https://codereview.chromium.org/1696563002/diff/60001/blimp/net/ssl_client_tr... File blimp/net/ssl_client_transport_unittest.cc (right): https://codereview.chromium.org/1696563002/diff/60001/blimp/net/ssl_client_tr... blimp/net/ssl_client_transport_unittest.cc:20: const char kIPV4Address[] = "192.168.0.1"; On 2016/02/19 22:56:08, Ryan Sleevi wrote: > Please use a loopback address (127.0.0.x) to avoid any improper behaviour. Done.
I started reviewing, but then saw multiple things you indicated as completed which were not done. I presume you uploaded the wrong version? Please let me know when you're ready for me to re-review, as I am assuming the present version is not yet complete.
Apologies, one of the changes was made in a parallel Vim buffer that I didn't save. Everything should be there now. PTAL. https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:18: #include "net/cert/pem_tokenizer.h" On 2016/02/19 22:56:08, Ryan Sleevi wrote: > DESIGN: This is not intended to be used outside of //net > > Please use the supported //net interface of > X509Certificate::CreateFromBytes(..., ..., > X509Certificate::FORMAT_PEM_SEQUENCE); > > (The existing uses are bugs that should be fixed - filed > https://bugs.chromium.org/p/chromium/issues/detail?id=588298 for that ) Done. https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:75: net::PEMTokenizer pem_tokenizer(*cert_str, {"CERTIFICATE"}); On 2016/02/22 22:53:31, Kevin M wrote: > On 2016/02/19 22:56:07, Ryan Sleevi wrote: > > STYLE: Uniform initialization syntax is explicitly forbidden in Chromium - see > > https://chromium-cpp.appspot.com/#core-blacklist > > Done. (FYI, other PEMTokenizer clients do the same thing and should be clened up > - though, they shouldn't be using that class in the first place?) PEMTokenizer was removed and replaced with CreateCertificateListFromBytes.
https://codereview.chromium.org/1696563002/diff/120001/blimp/client/DEPS File blimp/client/DEPS (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/client/DEPS#newc... blimp/client/DEPS:11: "+net/cert", Not needed given line 9 https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:138: DCHECK(io_task_runner_->RunsTasksOnCurrentThread()); It makes me deeply nervous to have an object that lives on multiple threads, as that's a ripe source of bugs. Also, the use of base::Unretained(this) [lines 131-134] are definitely a warning sign that "Here is code we can no longer reason about" From examining your interface visibility, it does not seem like this needs to be a member method - that is, it seems (and perhaps I missed something) that this could be entirely accomplished within an unnamed namespace in your .cc file - which not only hides your abstraction more, but helps reason about the classes. Separately, I would question if 133-134 should be using base::Unretained(this) - my instinct is htat you should be using WeakPtrs to handle the AssignmentSource going away mid-operation. https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:153: CHECK(port > 0 && port < 65535) << "Invalid IP port number: " << port; Why do you check this, given url.is_valid()? https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:157: if (url.has_scheme()) { Why do you check this, given line 145? https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:191: net::IPEndPoint(ip_address, base::checked_cast<uint16_t>(port)); Why do you do this, given line 153? https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:204: .Run(AssignmentSource::RESULT_OK, *custom_assignment); The API contract for using a heap-allocated custom_assignment seems... weird. Is that just to curry whether or not a custom assignment was used? It feels weird when it gets to line 239 & friends. https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:289: scoped_ptr<base::Value> json = base::JSONReader::Read(response); SECURITY: In Chrome code, we never process JSON in a trusted process, we use the utility process for this. This is true regardless of it coming from a 'trusted source' https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:323: if (!base::IsValueInRangeForNumericType<uint16_t>(port)) { Why do you soft-fail these errors, but CHECK on the others? (Line 191 for example) https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:334: base::ResetAndReturn(&callback_) The heavy use of base::ResetAndReturn(...) makes me think that callback_ can invalidate the present object. Is that the case? If so, this does seem like a rather dangerous API. https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:338: scoped_refptr<net::X509Certificate> cert = std::move(cert_list[0]); Why do you std::move() here, only to copy-assign on line 346? https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... File blimp/client/session/assignment_source.h (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.h:68: class FileReader { What's the envisioned usage here for this? Judging by the ForTesting, couldn't that also be accomplished via a Test Cert? That is, it's unclear why this interface is needed for testing. https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... File blimp/client/session/assignment_source_unittest.cc (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:62: class MockFileReader : public FileReader { If this is the whole reason for abstracting the FileReader interface, I would just suggest your test use the filesystem directly, which is a more realistic test anyways. https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:82: CHECK_EQ(1u, cert_list.size()); Seems like this is duplicating logic/preconditions from your other code (e.g. "not ideal") https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/b... File blimp/client/session/blimp_client_session.cc (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/b... blimp/client/session/blimp_client_session.cc:89: } break; Did you git cl format this? I have never seen this style used in Chromium code. https://codereview.chromium.org/1696563002/diff/120001/blimp/engine/BUILD.gn File blimp/engine/BUILD.gn (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/engine/BUILD.gn#... blimp/engine/BUILD.gn:255: "//sandbox/linux:chrome_sandbox", Unrelated? https://codereview.chromium.org/1696563002/diff/120001/blimp/net/DEPS File blimp/net/DEPS (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/net/DEPS#newcode6 blimp/net/DEPS:6: "+net/ssl", At this point, does it just make sense to use +net? I don't think we're saving ourselves any layering at this point. https://codereview.chromium.org/1696563002/diff/120001/blimp/net/exact_match_... File blimp/net/exact_match_cert_verifier.cc (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/net/exact_match_... blimp/net/exact_match_cert_verifier.cc:39: verify_result->verified_cert = cert; Line 39 should be before line 34. https://codereview.chromium.org/1696563002/diff/120001/blimp/net/ssl_client_t... File blimp/net/ssl_client_transport_unittest.cc (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/net/ssl_client_t... blimp/net/ssl_client_transport_unittest.cc:26: CHECK(parsed_address.AssignFromIPLiteral(address)); You should NEVER CHECK in unit tests. https://codereview.chromium.org/1696563002/diff/120001/blimp/net/ssl_client_t... blimp/net/ssl_client_transport_unittest.cc:28: } Change kIPV4Address to const uint8_t kIPV4Address[] = { 127, 0, 0, 1 }; and you can just do "net::IPEndPoint(kIPV4Address, port)" (IPAddress is implicitly constructible from const uint8_t of fixed sizes)
https://codereview.chromium.org/1696563002/diff/120001/blimp/client/DEPS File blimp/client/DEPS (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/client/DEPS#newc... blimp/client/DEPS:11: "+net/cert", On 2016/02/23 00:49:44, Ryan Sleevi wrote: > Not needed given line 9 Done. https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:138: DCHECK(io_task_runner_->RunsTasksOnCurrentThread()); On 2016/02/23 00:49:45, Ryan Sleevi wrote: > It makes me deeply nervous to have an object that lives on multiple threads, as > that's a ripe source of bugs. Also, the use of base::Unretained(this) [lines > 131-134] are definitely a warning sign that "Here is code we can no longer > reason about" > > From examining your interface visibility, it does not seem like this needs to be > a member method - that is, it seems (and perhaps I missed something) that this > could be entirely accomplished within an unnamed namespace in your .cc file - > which not only hides your abstraction more, but helps reason about the classes. > > Separately, I would question if 133-134 should be using base::Unretained(this) - > my instinct is htat you should be using WeakPtrs to handle the AssignmentSource > going away mid-operation. Made it an anonymous namespaced function and added WeakPtrs. https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:153: CHECK(port > 0 && port < 65535) << "Invalid IP port number: " << port; On 2016/02/23 00:49:45, Ryan Sleevi wrote: > Why do you check this, given url.is_valid()? Done. https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:157: if (url.has_scheme()) { On 2016/02/23 00:49:45, Ryan Sleevi wrote: > Why do you check this, given line 145? Done. https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:191: net::IPEndPoint(ip_address, base::checked_cast<uint16_t>(port)); On 2016/02/23 00:49:45, Ryan Sleevi wrote: > Why do you do this, given line 153? In the previous patch you recommended using checked_cast to downcast an int to an uint16_t. IPEndPoint takes a uint16_t in its ctor. https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:204: .Run(AssignmentSource::RESULT_OK, *custom_assignment); On 2016/02/23 00:49:45, Ryan Sleevi wrote: > The API contract for using a heap-allocated custom_assignment seems... weird. Is > that just to curry whether or not a custom assignment was used? It feels weird > when it gets to line 239 & friends. Done. https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:289: scoped_ptr<base::Value> json = base::JSONReader::Read(response); On 2016/02/23 00:49:45, Ryan Sleevi wrote: > SECURITY: In Chrome code, we never process JSON in a trusted process, we use the > utility process for this. This is true regardless of it coming from a 'trusted > source' Interesting. Is there a Codesearch link or a doc that I can use as a reference for doing that? +dtrainor@chromium.org FYI https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:323: if (!base::IsValueInRangeForNumericType<uint16_t>(port)) { On 2016/02/23 00:49:45, Ryan Sleevi wrote: > Why do you soft-fail these errors, but CHECK on the others? (Line 191 for > example) This code handles responses received over the network so errors should be handled gracefully. Lines 191 et al parse command line arguments that are provided by developers, so abrupt termination w/a stack trace is a nice thing to have. https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:334: base::ResetAndReturn(&callback_) On 2016/02/23 00:49:44, Ryan Sleevi wrote: > The heavy use of base::ResetAndReturn(...) makes me think that callback_ can > invalidate the present object. Is that the case? If so, this does seem like a > rather dangerous API. No, it's a safety practice my team has been using to turn quietly buggy callback reuse into segfaults. https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:338: scoped_refptr<net::X509Certificate> cert = std::move(cert_list[0]); On 2016/02/23 00:49:45, Ryan Sleevi wrote: > Why do you std::move() here, only to copy-assign on line 346? :P Good point. https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... File blimp/client/session/assignment_source.h (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.h:68: class FileReader { On 2016/02/23 00:49:45, Ryan Sleevi wrote: > What's the envisioned usage here for this? Judging by the ForTesting, couldn't > that also be accomplished via a Test Cert? > > That is, it's unclear why this interface is needed for testing. This provides us a mockable hook for file access, to verify that the code is interpreting the command line parameter and reading the PEM cert from disk correctly. I removed it and wrote the PEM cert to a ScopedTempDir instead. https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... File blimp/client/session/assignment_source_unittest.cc (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:62: class MockFileReader : public FileReader { On 2016/02/23 00:49:45, Ryan Sleevi wrote: > If this is the whole reason for abstracting the FileReader interface, I would > just suggest your test use the filesystem directly, which is a more realistic > test anyways. Done. https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:82: CHECK_EQ(1u, cert_list.size()); On 2016/02/23 00:49:45, Ryan Sleevi wrote: > Seems like this is duplicating logic/preconditions from your other code (e.g. > "not ideal") This seems pretty trivial to me, and PEM certificate lists seem like a good way to inline certs in test code? https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/b... File blimp/client/session/blimp_client_session.cc (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/b... blimp/client/session/blimp_client_session.cc:89: } break; On 2016/02/23 00:49:45, Ryan Sleevi wrote: > Did you git cl format this? I have never seen this style used in Chromium code. Yes, this is output from git cl format. Removed the braces (had a variable declaration previously, no longer do.) https://codereview.chromium.org/1696563002/diff/120001/blimp/engine/BUILD.gn File blimp/engine/BUILD.gn (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/engine/BUILD.gn#... blimp/engine/BUILD.gn:255: "//sandbox/linux:chrome_sandbox", On 2016/02/23 00:49:45, Ryan Sleevi wrote: > Unrelated? Done. https://codereview.chromium.org/1696563002/diff/120001/blimp/net/DEPS File blimp/net/DEPS (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/net/DEPS#newcode6 blimp/net/DEPS:6: "+net/ssl", On 2016/02/23 00:49:45, Ryan Sleevi wrote: > At this point, does it just make sense to use +net? I don't think we're saving > ourselves any layering at this point. Done. https://codereview.chromium.org/1696563002/diff/120001/blimp/net/exact_match_... File blimp/net/exact_match_cert_verifier.cc (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/net/exact_match_... blimp/net/exact_match_cert_verifier.cc:39: verify_result->verified_cert = cert; On 2016/02/23 00:49:45, Ryan Sleevi wrote: > Line 39 should be before line 34. Done. https://codereview.chromium.org/1696563002/diff/120001/blimp/net/ssl_client_t... File blimp/net/ssl_client_transport_unittest.cc (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/net/ssl_client_t... blimp/net/ssl_client_transport_unittest.cc:26: CHECK(parsed_address.AssignFromIPLiteral(address)); On 2016/02/23 00:49:45, Ryan Sleevi wrote: > You should NEVER CHECK in unit tests. Done. https://codereview.chromium.org/1696563002/diff/120001/blimp/net/ssl_client_t... blimp/net/ssl_client_transport_unittest.cc:28: } On 2016/02/23 00:49:45, Ryan Sleevi wrote: > Change kIPV4Address to > > const uint8_t kIPV4Address[] = { 127, 0, 0, 1 }; > > and you can just do > > "net::IPEndPoint(kIPV4Address, port)" (IPAddress is implicitly constructible > from const uint8_t of fixed sizes) Done. Neat.
https://codereview.chromium.org/1696563002/diff/60001/blimp/net/exact_match_c... File blimp/net/exact_match_cert_verifier.cc (right): https://codereview.chromium.org/1696563002/diff/60001/blimp/net/exact_match_c... blimp/net/exact_match_cert_verifier.cc:18: : engine_cert_(engine_cert) {} On 2016/02/22 22:53:31, Kevin M wrote: > On 2016/02/19 22:56:08, Ryan Sleevi wrote: > > STYLE: You're passing a scoped_refptr<> as non-const, so you need to > std::move() > > this to avoid unnecessary refcount churn. > > Done. I converted this parameter into a const reference. https://groups.google.com/a/chromium.org/d/msg/chromium-dev/TlL1D-Djta0/hDMBe... the std::move path is the recommended path :)
https://codereview.chromium.org/1696563002/diff/60001/blimp/net/exact_match_c... File blimp/net/exact_match_cert_verifier.cc (right): https://codereview.chromium.org/1696563002/diff/60001/blimp/net/exact_match_c... blimp/net/exact_match_cert_verifier.cc:18: : engine_cert_(engine_cert) {} On 2016/02/23 02:26:26, Ryan Sleevi wrote: > On 2016/02/22 22:53:31, Kevin M wrote: > > On 2016/02/19 22:56:08, Ryan Sleevi wrote: > > > STYLE: You're passing a scoped_refptr<> as non-const, so you need to > > std::move() > > > this to avoid unnecessary refcount churn. > > > > Done. I converted this parameter into a const reference. > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/TlL1D-Djta0/hDMBe... > > the std::move path is the recommended path :) Done.
https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:289: scoped_ptr<base::Value> json = base::JSONReader::Read(response); On 2016/02/23 01:58:25, Kevin M wrote: > On 2016/02/23 00:49:45, Ryan Sleevi wrote: > > SECURITY: In Chrome code, we never process JSON in a trusted process, we use > the > > utility process for this. This is true regardless of it coming from a 'trusted > > source' > > Interesting. Is there a Codesearch link or a doc that I can use as a reference > for doing that? > > mailto:+dtrainor@chromium.org FYI Filed bug https://bugs.chromium.org/p/chromium/issues/detail?id=589202 to track this.
https://codereview.chromium.org/1696563002/diff/180001/blimp/client/app/blimp... File blimp/client/app/blimp_client_switches.h (right): https://codereview.chromium.org/1696563002/diff/180001/blimp/client/app/blimp... blimp/client/app/blimp_client_switches.h:21: // --engine-endpoint="tcp:127.0.0.1:25467". Valid schemes are "ssl", DOCUMENTATION: So it looks like you're intending to use this URL style, but you've constructed a URL in the form we explicitly discourage (because it fails to abide by the generic syntax) Perhaps you meant tcp://127.0.0.1:25467, which would make more sense (the :// is a special indicator of the relative syntax). This would also clarify what I originally was going to ask, which is how to handle tcp://[::1]:25467 (that is, are IPv6 literals in URL form or not) https://codereview.chromium.org/1696563002/diff/180001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/180001/blimp/client/session/a... blimp/client/session/assignment_source.cc:149: } Is it a bug to supply kEngineCertPath without supplying a scheme of SSL/QUIC? https://codereview.chromium.org/1696563002/diff/180001/blimp/client/session/a... blimp/client/session/assignment_source.cc:174: io_task_runner_(std::move(io_task_runner)), CODE DESIGN: OK, so this is one of those times where it's rather subtle and can be surprising, because you're invalidating |io_task_runner| via the std::move(), but you're using it on 173, which is fine for initializer lists but not fine for argument lists (due to order of execution of full statements). The TL;DR: is this is technically correct, but somewhat eye catching and subtle and has to be reasoned about. One way to restructure this so it's "more" obvious : io_task_runner_(std::move(io_task_runner)), url_request_context_(new ...(io_task_runner_)) ... At least there it's clearer that every subsequent use uses the member variable, rather than the (potentially invalidated due to move) function argument, although it's equally subtle in noting the _ or not. It's fine if you chose to leave this as is, but it was worth highlighting here. https://codereview.chromium.org/1696563002/diff/180001/blimp/client/session/a... File blimp/client/session/assignment_source_unittest.cc (right): https://codereview.chromium.org/1696563002/diff/180001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:59: (!assignment.cert || arg.cert->Equals(assignment.cert.get())); BUG: Is this right? What if arg->cert != NULL, but assignment.cert == null. Don't you need an additional check return ... arg.client_token == .. && ((!arg.cert && !assignment.cert) || (arg.cert && assignment.cert && arg.cert->Equals(assignment.cert.get()))); https://codereview.chromium.org/1696563002/diff/180001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:64: EXPECT_TRUE(ip_address.AssignFromIPLiteral(ip)); BUG/DESIGN: This does nothing to stop your test early, because it's in a subroutine. Just wanting to make sure that was expected. https://codereview.chromium.org/1696563002/diff/180001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:79: assignment.ip_endpoint = BuildIPEndPoint("100.150.200.250", 500); TEST DESIGN: You should not use real IPs in tests. Please use one of the reserved host ranges. FURTHER TEST DESIGN: Should these be named constants? https://codereview.chromium.org/1696563002/diff/180001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:112: arraysize(kCertificatePEMEncoded)); DESIGN: This seems wasteful/inefficient for a unit test. Is there a reason you can't simply check in the file and load during your unit tests? //net does this extensively with //net/data and //net/base/test_data_directory.h https://codereview.chromium.org/1696563002/diff/180001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:182: net::TestURLFetcherFactory factory_; FWIW my gut is that using EmbeddedTestServer might be easier for this than what you're doing with TestURLFetcher, but that's more for future work/experimentation. https://codereview.chromium.org/1696563002/diff/180001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:198: switches::kEngineEndpoint, "tcp:100.150.200.250:500"); Same remarks about constants, but also about URL scheme syntax. https://codereview.chromium.org/1696563002/diff/180001/blimp/net/exact_match_... File blimp/net/exact_match_cert_verifier.h (right): https://codereview.chromium.org/1696563002/diff/180001/blimp/net/exact_match_... blimp/net/exact_match_cert_verifier.h:15: // Checks if the peer certificate is an exact match to the X509 certificate s/X509/X.509/ https://codereview.chromium.org/1696563002/diff/180001/blimp/net/exact_match_... blimp/net/exact_match_cert_verifier.h:16: // supplied out-of-band by the Blimp Assigner. This seems overly specific documentation, but perhaps I'm not familiar with terminology. It would seem that "Blimp Assigner" refers to your "AssignmentSource", which seems like a comment layering violation (talking about who uses this, rather than how to use it), but perhaps that's another layer. It may help to rephrase this comment to explain what the class does, not how it's meant to be used. https://codereview.chromium.org/1696563002/diff/180001/blimp/net/ssl_client_t... File blimp/net/ssl_client_transport.cc (right): https://codereview.chromium.org/1696563002/diff/180001/blimp/net/ssl_client_t... blimp/net/ssl_client_transport.cc:31: const std::string SSLClientTransport::GetName() const { random API nit: 1) Any reason this is returning std::string rather than char* 2) The "const std::string" is forcing a copy any time someone does std::string ... = GetName() - is that intentional? It seems rather useless to const-qualify the return value since you're not returning a const-ref https://codereview.chromium.org/1696563002/diff/180001/blimp/net/ssl_client_t... blimp/net/ssl_client_transport.cc:38: scoped_ptr<net::StreamSocket> tcp_socket = TCPClientTransport::TakeSocket(); Acknowledged that you're going to investigate re-designing, but it's something like this that highlights the layering challenges - you could concievably be building a TLS socket atop an HTTP proxy (or an HTTP/2 proxy's STREAM abstraction), hence it's not always guaranteed that the underlying implementation will be a TCPClientTransport :)
dtrainor@chromium.org changed reviewers: + dtrainor@chromium.org
https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:289: scoped_ptr<base::Value> json = base::JSONReader::Read(response); On 2016/02/23 20:38:15, Kevin M wrote: > On 2016/02/23 01:58:25, Kevin M wrote: > > On 2016/02/23 00:49:45, Ryan Sleevi wrote: > > > SECURITY: In Chrome code, we never process JSON in a trusted process, we use > > the > > > utility process for this. This is true regardless of it coming from a > 'trusted > > > source' > > > > Interesting. Is there a Codesearch link or a doc that I can use as a reference > > for doing that? > > > > mailto:+dtrainor@chromium.org FYI > > Filed bug https://bugs.chromium.org/p/chromium/issues/detail?id=589202 to track > this. Hmm it looks like there are multiple instances of this happening in the browser process currently (GaiaAuthFetcher? I tracked it back to the AccountFetcherService which looks like it's running in the Browser process, but I'm not 100% sure). Should we update json_reader.h to say something along these lines in the header?
Description was changed from ========== Blimp: add support for SSL connections. This CL allows the Blimp client to establish TLS-protected channels with the backend engine. The authenticity of the engine is validated by checking if its cert is an exact match of a certificate provided separately by the Assigner API. * Create new Blimp SSL transport class: SSLClientTransport. * Create custom CertValidator for checking an exact cert match against the SSL peer's cert * Integrate SSLClientTransport with BlimpClientSession. * Assignment: add certificate field. * AssignmentSource: add certificate file reading; PEM file parsing; X509 certificate parsing. * Created new DEPS entries as appropriate. R=wez@chromium.org CC=rsleevi@chromium.org BUG=585279 ========== to ========== Blimp: add support for SSL connections. This CL allows the Blimp client to establish TLS-protected channels with the backend engine. The authenticity of the engine is validated by checking if its cert is an exact match of a certificate provided separately by the Assigner API. * Create new Blimp SSL transport class: SSLClientTransport. * Create custom CertValidator for checking an exact cert match against the SSL peer's cert * Integrate SSLClientTransport with BlimpClientSession. * Assignment: add certificate field. * AssignmentSource: add certificate file reading; PEM file parsing; X509 certificate parsing. * Created new DEPS entries as appropriate. R=wez@chromium.org CC=rsleevi@chromium.org BUG=585279,589202 ==========
https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:289: scoped_ptr<base::Value> json = base::JSONReader::Read(response); On 2016/02/23 21:34:29, David Trainor wrote: > On 2016/02/23 20:38:15, Kevin M wrote: > > On 2016/02/23 01:58:25, Kevin M wrote: > > > On 2016/02/23 00:49:45, Ryan Sleevi wrote: > > > > SECURITY: In Chrome code, we never process JSON in a trusted process, we > use > > > the > > > > utility process for this. This is true regardless of it coming from a > > 'trusted > > > > source' > > > > > > Interesting. Is there a Codesearch link or a doc that I can use as a > reference > > > for doing that? > > > > > > mailto:+dtrainor@chromium.org FYI > > > > Filed bug https://bugs.chromium.org/p/chromium/issues/detail?id=589202 to > track > > this. > > Hmm it looks like there are multiple instances of this happening in the browser > process currently (GaiaAuthFetcher? I tracked it back to the > AccountFetcherService which looks like it's running in the Browser process, but > I'm not 100% sure). > > Should we update json_reader.h to say something along these lines in the header? Issue is resolved. I modified the JSON parsing code to use safe_json instead.
On 2016/02/23 22:01:50, Kevin M wrote: > https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... > File blimp/client/session/assignment_source.cc (right): > > https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... > blimp/client/session/assignment_source.cc:289: scoped_ptr<base::Value> json = > base::JSONReader::Read(response); > On 2016/02/23 21:34:29, David Trainor wrote: > > On 2016/02/23 20:38:15, Kevin M wrote: > > > On 2016/02/23 01:58:25, Kevin M wrote: > > > > On 2016/02/23 00:49:45, Ryan Sleevi wrote: > > > > > SECURITY: In Chrome code, we never process JSON in a trusted process, we > > use > > > > the > > > > > utility process for this. This is true regardless of it coming from a > > > 'trusted > > > > > source' > > > > > > > > Interesting. Is there a Codesearch link or a doc that I can use as a > > reference > > > > for doing that? > > > > > > > > mailto:+dtrainor@chromium.org FYI > > > > > > Filed bug https://bugs.chromium.org/p/chromium/issues/detail?id=589202 to > > track > > > this. > > > > Hmm it looks like there are multiple instances of this happening in the > browser > > process currently (GaiaAuthFetcher? I tracked it back to the > > AccountFetcherService which looks like it's running in the Browser process, > but > > I'm not 100% sure). > > > > Should we update json_reader.h to say something along these lines in the > header? > > Issue is resolved. I modified the JSON parsing code to use safe_json instead. Awesome thanks Kevin! Sorry about that!
https://codereview.chromium.org/1696563002/diff/180001/blimp/client/app/blimp... File blimp/client/app/blimp_client_switches.h (right): https://codereview.chromium.org/1696563002/diff/180001/blimp/client/app/blimp... blimp/client/app/blimp_client_switches.h:21: // --engine-endpoint="tcp:127.0.0.1:25467". Valid schemes are "ssl", On 2016/02/23 21:01:31, Ryan Sleevi wrote: > DOCUMENTATION: So it looks like you're intending to use this URL style, but > you've constructed a URL in the form we explicitly discourage (because it fails > to abide by the generic syntax) > > Perhaps you meant tcp://127.0.0.1:25467, which would make more sense (the :// is > a special indicator of the relative syntax). This would also clarify what I > originally was going to ask, which is how to handle tcp://[::1]:25467 (that is, > are IPv6 literals in URL form or not) Done. https://codereview.chromium.org/1696563002/diff/180001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/180001/blimp/client/session/a... blimp/client/session/assignment_source.cc:149: } On 2016/02/23 21:01:31, Ryan Sleevi wrote: > Is it a bug to supply kEngineCertPath without supplying a scheme of SSL/QUIC? That's a harmless flag combination, so not really. https://codereview.chromium.org/1696563002/diff/180001/blimp/client/session/a... blimp/client/session/assignment_source.cc:174: io_task_runner_(std::move(io_task_runner)), On 2016/02/23 21:01:31, Ryan Sleevi wrote: > CODE DESIGN: OK, so this is one of those times where it's rather subtle and can > be surprising, because you're invalidating |io_task_runner| via the std::move(), > but you're using it on 173, which is fine for initializer lists but not fine for > argument lists (due to order of execution of full statements). > > The TL;DR: is this is technically correct, but somewhat eye catching and subtle > and has to be reasoned about. > > One way to restructure this so it's "more" obvious > > : io_task_runner_(std::move(io_task_runner)), > url_request_context_(new ...(io_task_runner_)) > ... > > At least there it's clearer that every subsequent use uses the member variable, > rather than the (potentially invalidated due to move) function argument, > although it's equally subtle in noting the _ or not. > > It's fine if you chose to leave this as is, but it was worth highlighting here. Reordered fields & initialization list based on recommendation. I have to change the class field to a SingleThreadTaskRunner because that's what the ContextGetter depends on. https://codereview.chromium.org/1696563002/diff/180001/blimp/client/session/a... File blimp/client/session/assignment_source_unittest.cc (right): https://codereview.chromium.org/1696563002/diff/180001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:59: (!assignment.cert || arg.cert->Equals(assignment.cert.get())); On 2016/02/23 21:01:31, Ryan Sleevi wrote: > BUG: Is this right? What if arg->cert != NULL, but assignment.cert == null. > Don't you need an additional check > > return ... > arg.client_token == .. && > ((!arg.cert && !assignment.cert) || > (arg.cert && assignment.cert && arg.cert->Equals(assignment.cert.get()))); Done. https://codereview.chromium.org/1696563002/diff/180001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:64: EXPECT_TRUE(ip_address.AssignFromIPLiteral(ip)); On 2016/02/23 21:01:31, Ryan Sleevi wrote: > BUG/DESIGN: This does nothing to stop your test early, because it's in a > subroutine. Just wanting to make sure that was expected. Helper fn removed. https://codereview.chromium.org/1696563002/diff/180001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:79: assignment.ip_endpoint = BuildIPEndPoint("100.150.200.250", 500); On 2016/02/23 21:01:31, Ryan Sleevi wrote: > TEST DESIGN: You should not use real IPs in tests. Please use one of the > reserved host ranges. > > FURTHER TEST DESIGN: Should these be named constants? Done, done, also switched to the uint8_t[4] IPEndPoint constructor. https://codereview.chromium.org/1696563002/diff/180001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:112: arraysize(kCertificatePEMEncoded)); On 2016/02/23 21:01:31, Ryan Sleevi wrote: > DESIGN: This seems wasteful/inefficient for a unit test. Is there a reason you > can't simply check in the file and load during your unit tests? > > //net does this extensively with //net/data and //net/base/test_data_directory.h Done, created a PEM file for unittest.selfsigned.der. https://codereview.chromium.org/1696563002/diff/180001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:182: net::TestURLFetcherFactory factory_; On 2016/02/23 21:01:31, Ryan Sleevi wrote: > FWIW my gut is that using EmbeddedTestServer might be easier for this than what > you're doing with TestURLFetcher, but that's more for future > work/experimentation. Acknowledged. https://codereview.chromium.org/1696563002/diff/180001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:198: switches::kEngineEndpoint, "tcp:100.150.200.250:500"); On 2016/02/23 21:01:31, Ryan Sleevi wrote: > Same remarks about constants, but also about URL scheme syntax. Done. https://codereview.chromium.org/1696563002/diff/180001/blimp/net/exact_match_... File blimp/net/exact_match_cert_verifier.h (right): https://codereview.chromium.org/1696563002/diff/180001/blimp/net/exact_match_... blimp/net/exact_match_cert_verifier.h:15: // Checks if the peer certificate is an exact match to the X509 certificate On 2016/02/23 21:01:31, Ryan Sleevi wrote: > s/X509/X.509/ Done. https://codereview.chromium.org/1696563002/diff/180001/blimp/net/exact_match_... blimp/net/exact_match_cert_verifier.h:16: // supplied out-of-band by the Blimp Assigner. On 2016/02/23 21:01:31, Ryan Sleevi wrote: > This seems overly specific documentation, but perhaps I'm not familiar with > terminology. It would seem that "Blimp Assigner" refers to your > "AssignmentSource", which seems like a comment layering violation (talking about > who uses this, rather than how to use it), but perhaps that's another layer. > > It may help to rephrase this comment to explain what the class does, not how > it's meant to be used. Done. https://codereview.chromium.org/1696563002/diff/180001/blimp/net/ssl_client_t... File blimp/net/ssl_client_transport.cc (right): https://codereview.chromium.org/1696563002/diff/180001/blimp/net/ssl_client_t... blimp/net/ssl_client_transport.cc:31: const std::string SSLClientTransport::GetName() const { On 2016/02/23 21:01:32, Ryan Sleevi wrote: > random API nit: > 1) Any reason this is returning std::string rather than char* > 2) The "const std::string" is forcing a copy any time someone does std::string > ... = GetName() - is that intentional? It seems rather useless to const-qualify > the return value since you're not returning a const-ref No reason, returning a const char* is fine. (Done; all BlimpTransport subclasses are changed accordingly.) https://codereview.chromium.org/1696563002/diff/180001/blimp/net/ssl_client_t... blimp/net/ssl_client_transport.cc:38: scoped_ptr<net::StreamSocket> tcp_socket = TCPClientTransport::TakeSocket(); On 2016/02/23 21:01:32, Ryan Sleevi wrote: > Acknowledged that you're going to investigate re-designing, but it's something > like this that highlights the layering challenges - you could concievably be > building a TLS socket atop an HTTP proxy (or an HTTP/2 proxy's STREAM > abstraction), hence it's not always guaranteed that the underlying > implementation will be a TCPClientTransport :) Acknowledged.
kmarshall@chromium.org changed reviewers: + rsesek@chromium.org
R += rsesek@chromium.org for components/safe_json DEPS
Friendly ping. We are hoping to be able to test with this by EOW.
https://codereview.chromium.org/1696563002/diff/200001/blimp/client/app/blimp... File blimp/client/app/blimp_client_switches.h (right): https://codereview.chromium.org/1696563002/diff/200001/blimp/client/app/blimp... blimp/client/app/blimp_client_switches.h:21: // --engine-endpoint="tcp://127.0.0.1:25467". Valid schemes are "ssl", Ah I like this better! Can you also update blimp/docs/running.md if you're adding the '//' to the endpoint format?
https://codereview.chromium.org/1696563002/diff/200001/blimp/client/app/blimp... File blimp/client/app/blimp_client_switches.h (right): https://codereview.chromium.org/1696563002/diff/200001/blimp/client/app/blimp... blimp/client/app/blimp_client_switches.h:21: // --engine-endpoint="tcp://127.0.0.1:25467". Valid schemes are "ssl", On 2016/02/25 00:05:01, David Trainor wrote: > Ah I like this better! Can you also update blimp/docs/running.md if you're > adding the '//' to the endpoint format? Done.
https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.cc:113: std::string path = url.path(); Ideally you would check .is_empty() / is_valid() / has_scheme() before this Having just reviewed the Headless code, I suspect that in retrospect, trying to fit this into a GURL (or GURL-shaped string) may not be ideal; it seems like expressing this as three explicit flags avoids any ambiguity (protocol, host, port) may be clearer, at least for your use case. FWIW, there is url::AddStandardScheme() to register your schemes, but that has global side-effects https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... File blimp/client/session/assignment_source.h (right): https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:53: bool is_null() const; DESIGN NIT: I would have suggested this be called "IsValid()" (with the inverted logic); null on a concrete object just feels like it 'reads' weird. https://codereview.chromium.org/1696563002/diff/240001/blimp/net/exact_match_... File blimp/net/exact_match_cert_verifier.cc (right): https://codereview.chromium.org/1696563002/diff/240001/blimp/net/exact_match_... blimp/net/exact_match_cert_verifier.cc:35: if (!cert->Equals(engine_cert_.get())) { One thing worth noting: X509Certificate::Equals only compares the leaf cert, not any intermediates. In thinking through this, I suspect you want to change line 39 to verify_result->verified_cert = engine_cert_; That way you're always saying you verified against the canonical config and not arbitrary (unrelated) intermediates. (hopefully that makes sense) https://codereview.chromium.org/1696563002/diff/240001/blimp/net/exact_match_... blimp/net/exact_match_cert_verifier.cc:49: verify_result->public_key_hashes.push_back(net::HashValue(sha256_hash)); FWIW, since |cert->os_cert_handle()| will always be the same as |engine_cert_|, you could totes precompute these and store them next to engine_cert_ e.g. in the ctor engine_cert_hashes_ = ...; and then here verify_result->public_key_hashes = engine_cert_hashes_; https://codereview.chromium.org/1696563002/diff/240001/net/data/ssl/certifica... File net/data/ssl/certificates/unittest.selfsigned.pem (right): https://codereview.chromium.org/1696563002/diff/240001/net/data/ssl/certifica... net/data/ssl/certificates/unittest.selfsigned.pem:1: -----BEGIN CERTIFICATE----- Please don't add this file here; add it next to your unittests If you do need it to be here, you need to update the README, you should include the openssl x509 -text output (see the other certs), and you should include steps on how to regenerate it and how it should be used. Also, it should be renamed to something meaningful :)
https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.cc:113: std::string path = url.path(); On 2016/02/25 22:16:25, Ryan Sleevi wrote: > Ideally you would check .is_empty() / is_valid() / has_scheme() before this > > Having just reviewed the Headless code, I suspect that in retrospect, trying to > fit this into a GURL (or GURL-shaped string) may not be ideal; it seems like > expressing this as three explicit flags avoids any ambiguity (protocol, host, > port) may be clearer, at least for your use case. > > FWIW, there is url::AddStandardScheme() to register your schemes, but that has > global side-effects Done. https://codereview.chromium.org/1696563002/diff/240001/blimp/net/exact_match_... File blimp/net/exact_match_cert_verifier.cc (right): https://codereview.chromium.org/1696563002/diff/240001/blimp/net/exact_match_... blimp/net/exact_match_cert_verifier.cc:35: if (!cert->Equals(engine_cert_.get())) { On 2016/02/25 22:16:25, Ryan Sleevi wrote: > One thing worth noting: X509Certificate::Equals only compares the leaf cert, not > any intermediates. > > In thinking through this, I suspect you want to change line 39 to > > verify_result->verified_cert = engine_cert_; > > That way you're always saying you verified against the canonical config and not > arbitrary (unrelated) intermediates. > > (hopefully that makes sense) Yep, that makes sense. Done. https://codereview.chromium.org/1696563002/diff/240001/blimp/net/exact_match_... blimp/net/exact_match_cert_verifier.cc:49: verify_result->public_key_hashes.push_back(net::HashValue(sha256_hash)); On 2016/02/25 22:16:25, Ryan Sleevi wrote: > FWIW, since |cert->os_cert_handle()| will always be the same as |engine_cert_|, > you could totes precompute these and store them next to engine_cert_ > > e.g. in the ctor > > engine_cert_hashes_ = ...; > > and then here > > verify_result->public_key_hashes = engine_cert_hashes_; Done. https://codereview.chromium.org/1696563002/diff/240001/net/data/ssl/certifica... File net/data/ssl/certificates/unittest.selfsigned.pem (right): https://codereview.chromium.org/1696563002/diff/240001/net/data/ssl/certifica... net/data/ssl/certificates/unittest.selfsigned.pem:1: -----BEGIN CERTIFICATE----- On 2016/02/25 22:16:25, Ryan Sleevi wrote: > Please don't add this file here; add it next to your unittests > > If you do need it to be here, you need to update the README, you should include > the openssl x509 -text output (see the other certs), and you should include > steps on how to regenerate it and how it should be used. > > Also, it should be renamed to something meaningful :) Eh, I'll just move it into the test dir. Done.
kmarshall@chromium.org changed reviewers: + bauerb@chromium.org
R+=bauerb for safe_json DEPS
https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:125: GetIOTaskRunner()->PostTaskAndReply( On 2016/02/22 22:53:31, Kevin M wrote: > On 2016/02/19 22:56:07, Ryan Sleevi wrote: > > BUG/DESIGN: This seems improperly named, or at least, likely to confuse any > > Chromie that works on this, as the IO thread is *not* allowed to do file > access > > (that's handled by a file thread). > > > > Nor is it necessary for your use case to use a MessageLoop::TYPE_IO file > thread > > for this, IIRC ( I believe all our FILE threads are just normal MessageLoops) > > Question: how do we create a FILE thread using base::Thread? > > The client doesn't depend on content/ so we don't have access to the usual > BrowserThreads. +1 on Ryan's comment; it was very confusing to me to see references to "the IO thread" everywhere. You don't need to use the FILE thread if you can't depend on content/ (in fact, even if you could, you should probably use BrowserThread::GetBlockingPool() instead), just don't refer to the thread as the IO thread or task runner, and remove the MessageLoop::TYPE_IO. https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:289: scoped_ptr<base::Value> json = base::JSONReader::Read(response); On 2016/02/23 00:49:45, Ryan Sleevi wrote: > SECURITY: In Chrome code, we never process JSON in a trusted process, we use the > utility process for this. This is true regardless of it coming from a 'trusted > source' So, uh, I hate to be the bearer of bad news here, but we do parse JSON from a trusted source in the browser in a _lot_ of places. So far the guidance I've received from the security team has been that we only need to parse _untrusted_ JSON out-of-process. https://codereview.chromium.org/1696563002/diff/260001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/260001/blimp/client/session/a... blimp/client/session/assignment_source.cc:120: CHECK(ip_str.empty() == port_str.empty() && You mention in the review that failing with an error message is useful for developers, but is CHECKing really what you want? For example, a crash due to a CHECK will produce a crash report that is uploaded to our servers, which does not seem quite right for something that could legitimately happen due to a user error. It also means you can't actually test the behavior if invalid switches are passed in (unless you use death tests, but those are kinda yuck IMO). https://codereview.chromium.org/1696563002/diff/260001/blimp/client/session/a... blimp/client/session/assignment_source.cc:191: url_request_context_(new SimpleURLRequestContextGetter(io_task_runner_)), Actually, I'm still confused. You use this thread for the network request, but also to synchronously read files? Wouldn't that block any network communication? Or is just that you don't care because those things happen sequentially? (TBH, I would still use two different threads here and set ThreadRestrictions::SetIOAllowed() on the network thread, just to make it clear in the future which thread should be used for what, but it's up to you, I guess. Blimp is crazy, I tell ya! 😉) https://codereview.chromium.org/1696563002/diff/260001/blimp/client/session/a... File blimp/client/session/assignment_source_unittest.cc (right): https://codereview.chromium.org/1696563002/diff/260001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:152: // MessageLoop is required by TestingJsonParser's self-deletion logic. Ooh... actually, I should probably modify TestingJsonParser to use SequencedTaskRunnerHandle instead. Can you add a TODO(bauerb) to switch back to TestSimpleTaskRunner once that happens? (Assuming that that's what you'd prefer all other things being equal.)
wez@chromium.org changed reviewers: - bauerb@chromium.org
https://codereview.chromium.org/1696563002/diff/240001/blimp/client/app/blimp... File blimp/client/app/blimp_client_switches.h (right): https://codereview.chromium.org/1696563002/diff/240001/blimp/client/app/blimp... blimp/client/app/blimp_client_switches.h:22: // "tcp", and "quic". "quic" is not a valid scheme, surely - we don't have a QUIC transport implemented. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.cc:103: // Must be called on the IO thread. nit: s/the/an https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.cc:104: nit: Remove this blank line https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.cc:105: Assignment GetCustomAssignment() { So should this be called GetAssignmentFromCommandLine()? https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.cc:198: PostTaskAndReplyWithResult( OT: I <3 PostTaskAndReplyWithResult - so handy! https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.cc:293: weak_factory_.GetWeakPtr()), nit: This line-wrap looks weird - I'd expect the base::Binds to be aligned - did you apply git cl format? https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... File blimp/client/session/assignment_source.h (right): https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:45: QUIC = 3, We don't have a QUIC transport, so let's remove this. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:53: bool is_null() const; nit: is_valid() seems a more appropriate name for this - having one of those fields unspecified does not seem conceptually null-like. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:55: // The transport protocol used to contact the engine. nit: This is a somewhat tautologous and ambiguous comment - at a first glance I can't tell whether you mean it identifies the _type_ of transport protocol (which seems to be the case) or it is the actual implementation of a transport protocol. Suggest: Specifies the transport to use to connect to the engine. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:58: // The IP address of the engine. nit: This comment also seems tautologous, especially if you rename ip_endpoint to something more descriptive, e.g. engine_endpoint Suggest: Specifies the IP address & port on which to reach the engine. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:61: // The token used to authenticate the client to the engine. Suggest simply: Used to authenticate to the specified engine. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:62: std::string client_token; Also suggest renaming this to auth_token. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:65: // |cert| exactly, otherwise the connection will be dropped. The suggestions I've given above switch the sense of the comments to describe how the field is intended to be used, rather than just describing what it contains (although the two are often pretty similar!) - I'd suggest doing the same here, e.g. "Specifies the X509 certificate that the engine must report. ..." https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:94: scoped_refptr<base::SingleThreadTaskRunner> io_task_runner); nit: Document why the |io_task_runner| is needed, e.g. to use for File I/O? Or URL fetching, etc? https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:105: // Called when GetCustomAssignment() has completed. GetCustomAssignment() isn't declared here, so it doesn't make sense to refer to it in documenting a public API. Why is this API, which seems to be a completion-handler function really, public? What is a "custom assignment"? https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:117: void AssignerJSONParseError(const std::string& error); These names don't scan - do you mean OnJsonParsed() and OnJsonParseError(), perhaps? https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:120: // the request has completed (whether it be a success or failure). nit: (And I realise this was already here) This comment describes _what_ you're doing, but not _why_ - the only reason I can see offhand is to so that we can DCHECK that GetAssignment isn't called twice? https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:126: base::WeakPtrFactory<AssignmentSource> weak_factory_; nit: Suggest separating the weak_factory_ from the rest by a blank-line since it is Special. :P
https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:125: GetIOTaskRunner()->PostTaskAndReply( On 2016/02/26 16:26:30, Bernhard Bauer wrote: > On 2016/02/22 22:53:31, Kevin M wrote: > > On 2016/02/19 22:56:07, Ryan Sleevi wrote: > > > BUG/DESIGN: This seems improperly named, or at least, likely to confuse any > > > Chromie that works on this, as the IO thread is *not* allowed to do file > > access > > > (that's handled by a file thread). > > > > > > Nor is it necessary for your use case to use a MessageLoop::TYPE_IO file > > thread > > > for this, IIRC ( I believe all our FILE threads are just normal > MessageLoops) > > > > Question: how do we create a FILE thread using base::Thread? > > > > The client doesn't depend on content/ so we don't have access to the usual > > BrowserThreads. > > +1 on Ryan's comment; it was very confusing to me to see references to "the IO > thread" everywhere. > > You don't need to use the FILE thread if you can't depend on content/ (in fact, > even if you could, you should probably use BrowserThread::GetBlockingPool() > instead), just don't refer to the thread as the IO thread or task runner, and > remove the MessageLoop::TYPE_IO. In the latest patch, you'll notice that we take a task runner from the object owner for network and file tasks. That would be considered an IO thread, wouldn't it? Re: using BrowserThread::GetBlockingPool, we have a "-content" DEPS exclusion rule on the Android client to keep us honest about not pulling in content/. https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/120001/blimp/client/session/a... blimp/client/session/assignment_source.cc:289: scoped_ptr<base::Value> json = base::JSONReader::Read(response); On 2016/02/26 16:26:30, Bernhard Bauer wrote: > On 2016/02/23 00:49:45, Ryan Sleevi wrote: > > SECURITY: In Chrome code, we never process JSON in a trusted process, we use > the > > utility process for this. This is true regardless of it coming from a 'trusted > > source' > > So, uh, I hate to be the bearer of bad news here, but we do parse JSON from a > trusted source in the browser in a _lot_ of places. So far the guidance I've > received from the security team has been that we only need to parse _untrusted_ > JSON out-of-process. Is output from googleapis.com, as in this case, considered a "trusted source"? https://codereview.chromium.org/1696563002/diff/240001/blimp/client/app/blimp... File blimp/client/app/blimp_client_switches.h (right): https://codereview.chromium.org/1696563002/diff/240001/blimp/client/app/blimp... blimp/client/app/blimp_client_switches.h:22: // "tcp", and "quic". On 2016/02/26 18:32:08, Wez wrote: > "quic" is not a valid scheme, surely - we don't have a QUIC transport > implemented. Done. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.cc:103: // Must be called on the IO thread. On 2016/02/26 18:32:09, Wez wrote: > nit: s/the/an Done. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.cc:104: On 2016/02/26 18:32:09, Wez wrote: > nit: Remove this blank line Done. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.cc:105: Assignment GetCustomAssignment() { On 2016/02/26 18:32:09, Wez wrote: > So should this be called GetAssignmentFromCommandLine()? Done. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.cc:293: weak_factory_.GetWeakPtr()), On 2016/02/26 18:32:09, Wez wrote: > nit: This line-wrap looks weird - I'd expect the base::Binds to be aligned - did > you apply git cl format? Yes, this is formatted. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... File blimp/client/session/assignment_source.h (right): https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:45: QUIC = 3, On 2016/02/26 18:32:09, Wez wrote: > We don't have a QUIC transport, so let's remove this. Done. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:53: bool is_null() const; On 2016/02/26 18:32:09, Wez wrote: > nit: is_valid() seems a more appropriate name for this - having one of those > fields unspecified does not seem conceptually null-like. Done. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:55: // The transport protocol used to contact the engine. On 2016/02/26 18:32:09, Wez wrote: > nit: This is a somewhat tautologous and ambiguous comment - at a first glance I > can't tell whether you mean it identifies the _type_ of transport protocol > (which seems to be the case) or it is the actual implementation of a transport > protocol. > > Suggest: Specifies the transport to use to connect to the engine. Done. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:58: // The IP address of the engine. On 2016/02/26 18:32:09, Wez wrote: > nit: This comment also seems tautologous, especially if you rename ip_endpoint > to something more descriptive, e.g. engine_endpoint > > Suggest: Specifies the IP address & port on which to reach the engine. Done. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:61: // The token used to authenticate the client to the engine. On 2016/02/26 18:32:09, Wez wrote: > Suggest simply: Used to authenticate to the specified engine. Done. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:62: std::string client_token; On 2016/02/26 18:32:09, Wez wrote: > Also suggest renaming this to auth_token. That would make this inconsistent with the Assigner API's nomenclature. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:65: // |cert| exactly, otherwise the connection will be dropped. On 2016/02/26 18:32:09, Wez wrote: > The suggestions I've given above switch the sense of the comments to describe > how the field is intended to be used, rather than just describing what it > contains (although the two are often pretty similar!) - I'd suggest doing the > same here, e.g. "Specifies the X509 certificate that the engine must report. > ..." Done. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:94: scoped_refptr<base::SingleThreadTaskRunner> io_task_runner); On 2016/02/26 18:32:09, Wez wrote: > nit: Document why the |io_task_runner| is needed, e.g. to use for File I/O? Or > URL fetching, etc? Done. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:105: // Called when GetCustomAssignment() has completed. On 2016/02/26 18:32:09, Wez wrote: > GetCustomAssignment() isn't declared here, so it doesn't make sense to refer to > it in documenting a public API. > > Why is this API, which seems to be a completion-handler function really, public? > > What is a "custom assignment"? Doesn't need to be public. "custom assignment" means "command line parsed assignment". Changed the name and visibility. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:120: // the request has completed (whether it be a success or failure). On 2016/02/26 18:32:09, Wez wrote: > nit: (And I realise this was already here) This comment describes _what_ you're > doing, but not _why_ - the only reason I can see offhand is to so that we can > DCHECK that GetAssignment isn't called twice? Mostly because URLFetcher takes a delegate object for completion, not a base::Callback, so this is how we stash the callback for later invocation. Added the "why". https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:126: base::WeakPtrFactory<AssignmentSource> weak_factory_; On 2016/02/26 18:32:09, Wez wrote: > nit: Suggest separating the weak_factory_ from the rest by a blank-line since it > is Special. :P Done. https://codereview.chromium.org/1696563002/diff/260001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/260001/blimp/client/session/a... blimp/client/session/assignment_source.cc:120: CHECK(ip_str.empty() == port_str.empty() && On 2016/02/26 16:26:30, Bernhard Bauer wrote: > You mention in the review that failing with an error message is useful for > developers, but is CHECKing really what you want? For example, a crash due to a > CHECK will produce a crash report that is uploaded to our servers, which does > not seem quite right for something that could legitimately happen due to a user > error. It also means you can't actually test the behavior if invalid switches > are passed in (unless you use death tests, but those are kinda yuck IMO). Good point about the reporting noise. I'll switch to DCHECKs/DLOGs with graceful fallback behavior for release builds. https://codereview.chromium.org/1696563002/diff/260001/blimp/client/session/a... blimp/client/session/assignment_source.cc:191: url_request_context_(new SimpleURLRequestContextGetter(io_task_runner_)), On 2016/02/26 16:26:30, Bernhard Bauer wrote: > Actually, I'm still confused. You use this thread for the network request, but > also to synchronously read files? Wouldn't that block any network communication? > Or is just that you don't care because those things happen sequentially? > > (TBH, I would still use two different threads here and set > ThreadRestrictions::SetIOAllowed() on the network thread, just to make it clear > in the future which thread should be used for what, but it's up to you, I guess. > Blimp is crazy, I tell ya! 😉) I added a separate task runner for file access. The operations are executed sequentially, and the file accesses happen once on application start, so it's OK that they point to the same task runner refptr for the time being, but this allows for the caller to run the operations on separate threads if needed. https://codereview.chromium.org/1696563002/diff/260001/blimp/client/session/a... File blimp/client/session/assignment_source_unittest.cc (right): https://codereview.chromium.org/1696563002/diff/260001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:152: // MessageLoop is required by TestingJsonParser's self-deletion logic. On 2016/02/26 16:26:30, Bernhard Bauer wrote: > Ooh... actually, I should probably modify TestingJsonParser to use > SequencedTaskRunnerHandle instead. Can you add a TODO(bauerb) to switch back to > TestSimpleTaskRunner once that happens? (Assuming that that's what you'd prefer > all other things being equal.) Done.
https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/60001/blimp/client/session/as... blimp/client/session/assignment_source.cc:36: CHECK(ip_address.AssignFromIPLiteral(host)) On 2016/02/22 at 22:53:31, Kevin M wrote: > On 2016/02/19 22:56:08, Ryan Sleevi wrote: > > DESIGN: It seems counter to Chromium practices to CHECK() on user-input. > > Do command line parameters count as user input, as is the case here? For code which users will invoke via the command-line, yes. :) In this case it's a developer-facing command-line interface, so we should go with whatever mechanism is easiest for a developer to work with. https://codereview.chromium.org/1696563002/diff/240001/blimp/client/app/blimp... File blimp/client/app/blimp_client_switches.h (right): https://codereview.chromium.org/1696563002/diff/240001/blimp/client/app/blimp... blimp/client/app/blimp_client_switches.h:22: // "tcp", and "quic". On 2016/02/26 19:57:22, Kevin M wrote: > On 2016/02/26 18:32:08, Wez wrote: > > "quic" is not a valid scheme, surely - we don't have a QUIC transport > > implemented. > > Done. What got done? Looks like it's still here? https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... File blimp/client/session/assignment_source.h (right): https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:62: std::string client_token; On 2016/02/26 19:57:23, Kevin M wrote: > On 2016/02/26 18:32:09, Wez wrote: > > Also suggest renaming this to auth_token. > > That would make this inconsistent with the Assigner API's nomenclature. Yes, but we're already inconsistent with the names of the other fields (engine_endpoint vs host_and_port, cert vs certificate), so I'm not sure that's a concern. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:26: #include "net/base/url_util.h" Do you need this include any more? https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:50: // documentation in blimp_client_switches.cc for details. This comment is now out-of-date. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:52: const char kTCPTransport[] = "tcp"; nit: Suggest kSslTransportValue and kTcpTransportValue, since these are potential values of the --engine-transport parameter. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:102: // Must be called on an IO thread. nit: Suggest being explicit as to why e.g. "Must be called on a thread suitable for file-IO." https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:112: switches::kEngineTransport); Don't bother pulling these out of the command-line until you are ready to parse them, below - in general you want to declare local variables as close to their first use as possible. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:115: return Assignment(); This early-exit and the subsequent test to see that everything's either set or un-set is equivalent to decoding each field individually into an Assignment and then returning it only if it IsValid(). I'd therefore suggest creating an empty Assignment here, and parsing each non-empty command-line parameter into it, below, and finally checking IsValid() before returning it. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:119: ip_str.empty() != transport_str.empty()) { nit: If you keep the early-exit, above, then this becomes a check that they are all not-empty(), but see my comment above. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:127: uint16_t port; nit: Always initialize PoD types, even if they're about to be overwritten by some function call out-parameter. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:130: DLOG(FATAL) << "--engine-port must be a value between 0 and 65535."; It's not valid for the port # to be zero, so this comment, and the range-check, are not correct. Also consider writing & using a trivial helper IsValidTcpPortNumber() or something. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:180: net::IPEndPoint(ip_address, base::checked_cast<uint16_t>(port)); You have already checked_cast<> when assigning to |port|, above. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:213: } Follow-up: This looks dubious: 1. the caller should know whether they requested an assignment, so know not to call again until they see that earlier request responded to. 2. If there is a concern about requests stalling, and therefore needing to be cancelled then I'd suggest an explicit CancelGetAssignment() method. 3. With this code as-is, the |weak_factory_| is not being invalidated, so an earlier assignment request will still complete and we'll try to call |callback_| twice, once for the old and once for the new request. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:219: base::Bind(&GetAssignmentFromCommandLine), Wouldn't it be cleaner to only GetAssignmentFromCommandLine() if there is an --engine-host parameter? (The function itself can check for the other parameters and barf if necessary), and otherwise move on to calling GetAssignmentFromNetwork() directly? Alternatively keep a call to GetAssignmentFromCommandLine here, but push the PostTaskAndReplyWithResult() call inside it, so that it's just the actual file-IO we do on the file thread. Or is there other code in there that needs the file-IO thread? https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:227: // If GetAssignmentFromCommandLine succeeded, then return the custom nit: Not "custom" assignment any more. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:236: // Call out to the network for a real assignment. Build the network request nit: Suggest keeping the network-assignment code as a separate GetAssignmentFromNetwork(), that can be called directly from GetAssignment if no --engine-cert was provided. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... File blimp/client/session/assignment_source.h (right): https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.h:97: scoped_refptr<base::SingleThreadTaskRunner> file_task_runner); nit: Any reason not to pass these const&? https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.h:110: void OnURLFetchComplete(const net::URLFetcher* source) override; nit: Looks like this is also an implementation detail, so it can be private as well (though the interface inheritance needs to stay public). https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.h:117: void OnJSONParseError(const std::string& error); nit: Prefer to capitalize acronyms as single words. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... File blimp/client/session/assignment_source_unittest.cc (right): https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:40: const char kTestIP[] = "127.0.0.1"; nit: Give this and the two below more helpful names, e.g. kTestIpAddressString, kTcpTransportName
PTAL https://codereview.chromium.org/1696563002/diff/240001/blimp/client/app/blimp... File blimp/client/app/blimp_client_switches.h (right): https://codereview.chromium.org/1696563002/diff/240001/blimp/client/app/blimp... blimp/client/app/blimp_client_switches.h:22: // "tcp", and "quic". On 2016/03/01 00:23:55, Wez wrote: > On 2016/02/26 19:57:22, Kevin M wrote: > > On 2016/02/26 18:32:08, Wez wrote: > > > "quic" is not a valid scheme, surely - we don't have a QUIC transport > > > implemented. > > > > Done. > > What got done? Looks like it's still here? Oops, made most of the change, didn't remove the old stuff. PTAL https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... File blimp/client/session/assignment_source.h (right): https://codereview.chromium.org/1696563002/diff/240001/blimp/client/session/a... blimp/client/session/assignment_source.h:62: std::string client_token; On 2016/03/01 00:23:55, Wez wrote: > On 2016/02/26 19:57:23, Kevin M wrote: > > On 2016/02/26 18:32:09, Wez wrote: > > > Also suggest renaming this to auth_token. > > > > That would make this inconsistent with the Assigner API's nomenclature. > > Yes, but we're already inconsistent with the names of the other fields > (engine_endpoint vs host_and_port, cert vs certificate), so I'm not sure that's > a concern. It's not just the Assigner response; "client_token" is used all over the codebase. I'd have to touch a lot of files to incorporate this change and keep it consistent across all of blimp/'s source files, which would dilute the reviewing focus of this CL. I'd prefer to do that in its own CL. Filed bug 591074. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:26: #include "net/base/url_util.h" On 2016/03/01 00:23:55, Wez wrote: > Do you need this include any more? Done. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:50: // documentation in blimp_client_switches.cc for details. On 2016/03/01 00:23:56, Wez wrote: > This comment is now out-of-date. Done. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:52: const char kTCPTransport[] = "tcp"; On 2016/03/01 00:23:55, Wez wrote: > nit: Suggest kSslTransportValue and kTcpTransportValue, since these are > potential values of the --engine-transport parameter. Done. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:102: // Must be called on an IO thread. On 2016/03/01 00:23:55, Wez wrote: > nit: Suggest being explicit as to why e.g. "Must be called on a thread suitable > for file-IO." Done. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:102: // Must be called on an IO thread. On 2016/03/01 00:23:55, Wez wrote: > nit: Suggest being explicit as to why e.g. "Must be called on a thread suitable > for file-IO." Done. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:112: switches::kEngineTransport); On 2016/03/01 00:23:55, Wez wrote: > Don't bother pulling these out of the command-line until you are ready to parse > them, below - in general you want to declare local variables as close to their > first use as possible. Done. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:115: return Assignment(); On 2016/03/01 00:23:55, Wez wrote: > This early-exit and the subsequent test to see that everything's either set or > un-set is equivalent to decoding each field individually into an Assignment and > then returning it only if it IsValid(). > > I'd therefore suggest creating an empty Assignment here, and parsing each > non-empty command-line parameter into it, below, and finally checking IsValid() > before returning it. Done. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:119: ip_str.empty() != transport_str.empty()) { On 2016/03/01 00:23:56, Wez wrote: > nit: If you keep the early-exit, above, then this becomes a check that they are > all not-empty(), but see my comment above. Done. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:127: uint16_t port; On 2016/03/01 00:23:55, Wez wrote: > nit: Always initialize PoD types, even if they're about to be overwritten by > some function call out-parameter. Done. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:130: DLOG(FATAL) << "--engine-port must be a value between 0 and 65535."; On 2016/03/01 00:23:55, Wez wrote: > It's not valid for the port # to be zero, so this comment, and the range-check, > are not correct. Also consider writing & using a trivial helper > IsValidTcpPortNumber() or something. Done. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:130: DLOG(FATAL) << "--engine-port must be a value between 0 and 65535."; On 2016/03/01 00:23:55, Wez wrote: > It's not valid for the port # to be zero, so this comment, and the range-check, > are not correct. Also consider writing & using a trivial helper > IsValidTcpPortNumber() or something. Done. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:180: net::IPEndPoint(ip_address, base::checked_cast<uint16_t>(port)); On 2016/03/01 00:23:56, Wez wrote: > You have already checked_cast<> when assigning to |port|, above. Done. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:213: } On 2016/03/01 00:23:55, Wez wrote: > Follow-up: This looks dubious: > > 1. the caller should know whether they requested an assignment, so know not to > call again until they see that earlier request responded to. > > 2. If there is a concern about requests stalling, and therefore needing to be > cancelled then I'd suggest an explicit CancelGetAssignment() method. > > 3. With this code as-is, the |weak_factory_| is not being invalidated, so an > earlier assignment request will still complete and we'll try to call |callback_| > twice, once for the old and once for the new request. Hey +dtrainor@chromium.org, is there a specific reason why this code is the way it is? Because I'd rather just have dupe callbacks be a DCHECKable error condition instead of something we handle. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... File blimp/client/session/assignment_source.h (right): https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.h:97: scoped_refptr<base::SingleThreadTaskRunner> file_task_runner); On 2016/03/01 00:23:56, Wez wrote: > nit: Any reason not to pass these const&? Done. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.h:110: void OnURLFetchComplete(const net::URLFetcher* source) override; On 2016/03/01 00:23:56, Wez wrote: > nit: Looks like this is also an implementation detail, so it can be private as > well (though the interface inheritance needs to stay public). Done. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.h:110: void OnURLFetchComplete(const net::URLFetcher* source) override; On 2016/03/01 00:23:56, Wez wrote: > nit: Looks like this is also an implementation detail, so it can be private as > well (though the interface inheritance needs to stay public). Done. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.h:117: void OnJSONParseError(const std::string& error); On 2016/03/01 00:23:56, Wez wrote: > nit: Prefer to capitalize acronyms as single words. Done. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... File blimp/client/session/assignment_source_unittest.cc (right): https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:40: const char kTestIP[] = "127.0.0.1"; On 2016/03/01 00:23:56, Wez wrote: > nit: Give this and the two below more helpful names, e.g. kTestIpAddressString, > kTcpTransportName Done. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:40: const char kTestIP[] = "127.0.0.1"; On 2016/03/01 00:23:56, Wez wrote: > nit: Give this and the two below more helpful names, e.g. kTestIpAddressString, > kTcpTransportName Done.
bauerb/rsleevi, PTAL
On 2016/02/26 00:30:31, Kevin M wrote: > R+=bauerb for safe_json DEPS FYI I've been watching this review, but it doesn't seem ready, which is why I was holding off on approving your DEPS. Generally DEPS (and other OWNERS) approvals come at the end, after most major review comments have been addressed.
LGTM w/ some minor nits (and a couple of questions just for my eng-lightenment). Kudos for the addtional unit-test cases, BTW, they seem pretty comprehensive (famous last words, I know ;) https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:213: } On 2016/03/01 18:23:17, Kevin M wrote: > On 2016/03/01 00:23:55, Wez wrote: > > Follow-up: This looks dubious: > > > > 1. the caller should know whether they requested an assignment, so know not to > > call again until they see that earlier request responded to. > > > > 2. If there is a concern about requests stalling, and therefore needing to be > > cancelled then I'd suggest an explicit CancelGetAssignment() method. > > > > 3. With this code as-is, the |weak_factory_| is not being invalidated, so an > > earlier assignment request will still complete and we'll try to call > |callback_| > > twice, once for the old and once for the new request. > > Hey mailto:+dtrainor@chromium.org, is there a specific reason why this code is the way > it is? Because I'd rather just have dupe callbacks be a DCHECKable error > condition instead of something we handle. Acknowledged. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:227: // If GetAssignmentFromCommandLine succeeded, then return the custom On 2016/03/01 00:23:56, Wez wrote: > nit: Not "custom" assignment any more. nit: Ping! https://codereview.chromium.org/1696563002/diff/300001/blimp/client/BUILD.gn File blimp/client/BUILD.gn (right): https://codereview.chromium.org/1696563002/diff/300001/blimp/client/BUILD.gn#... blimp/client/BUILD.gn:72: data += [ "session/test_selfsigned_cert.pem" ] Is this sufficient to get the correct test-isolate dependency on the PEM file? https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source.cc:127: assignment.engine_endpoint = net::IPEndPoint(ip_address, port); nit: I think you can put the checked_cast for port in-line here, rather than pulling it out explicitly to a local variable above. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source.cc:166: if (!assignment.IsValid()) { nit: Do you want to DLOG(FATAL) if it's not valid, as you do for other errors? https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source.cc:170: assignment.client_token = kDummyClientToken; nit: Suggest moving this up to the top, immediately after the Assignment is created. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source.cc:183: (transport_protocol != TransportProtocol::SSL || cert); nit: This is getting complex enough to break down into a set if (...) return false; statements followed by a return true, IMO. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source.cc:223: // If no assignment was passed via the comamnd line, then fall back on typo: command https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... File blimp/client/session/assignment_source_unittest.cc (right): https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:59: const std::string& expected_cert_str) { nit: AFAICT all the calls to this API get passed the result of BuildValidAssignment(), and cert_pem_, so you could make this a zero-parameter method of the test base. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:78: PathService::Get(base::DIR_SOURCE_ROOT, &src_root); nit: ASSERT_TRUE this as well. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:83: net::X509Certificate::FORMAT_PEM_CERT_SEQUENCE)[0]); nit: ASSERT_TRUE that |cert_| is not empty? https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:149: Assignment BuildValidAssignment(); nit: Suggest renaming this to BuildSslAssignment(), since that's what it does, for clarity. :) https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:159: base::ScopedTempDir temp_dir_; nit: Doesn't look like this is used any more? https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:172: safe_json::TestingJsonParser::ScopedFactoryOverride json_parsing_factory_; nit: One-line comment to explain this? https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:240: BuildResponseFromAssignment(assignment, cert_pem_), "UserAuthT0kenz", nit: Move this token value into a constant? https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:372: dict.SetInteger("port", 500); nit: You could just call BuildResponse...() and then Remove("certificate", nullptr) here, and in the other "missing field" tests, which would make them easier to grok. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/b... File blimp/client/session/blimp_client_session.cc (right): https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/b... blimp/client/session/blimp_client_session.cc:94: default: nit: Handle UNKNOWN explicitly here, rather than via default - that way the compilation will fail if a new type is added w/out updating this. https://codereview.chromium.org/1696563002/diff/300001/blimp/net/tcp_client_t... File blimp/net/tcp_client_transport.cc (right): https://codereview.chromium.org/1696563002/diff/300001/blimp/net/tcp_client_t... blimp/net/tcp_client_transport.cc:76: void TCPClientTransport::SetSocket(scoped_ptr<net::StreamSocket> socket) { nit: Is it ever valid for caller to SetSocket(nullptr)? DCHECK here if not. :)
https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:219: base::Bind(&GetAssignmentFromCommandLine), On 2016/03/01 00:23:55, Wez wrote: > Wouldn't it be cleaner to only GetAssignmentFromCommandLine() if there is an > --engine-host parameter? (The function itself can check for the other parameters > and barf if necessary), and otherwise move on to calling > GetAssignmentFromNetwork() directly? > > Alternatively keep a call to GetAssignmentFromCommandLine here, but push the > PostTaskAndReplyWithResult() call inside it, so that it's just the actual > file-IO we do on the file thread. Or is there other code in there that needs the > file-IO thread? (Done on previous patch) https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:227: // If GetAssignmentFromCommandLine succeeded, then return the custom On 2016/03/02 02:26:44, Wez wrote: > On 2016/03/01 00:23:56, Wez wrote: > > nit: Not "custom" assignment any more. > > nit: Ping! Done. https://codereview.chromium.org/1696563002/diff/280001/blimp/client/session/a... blimp/client/session/assignment_source.cc:236: // Call out to the network for a real assignment. Build the network request On 2016/03/01 00:23:55, Wez wrote: > nit: Suggest keeping the network-assignment code as a separate > GetAssignmentFromNetwork(), that can be called directly from GetAssignment if no > --engine-cert was provided. Done. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/BUILD.gn File blimp/client/BUILD.gn (right): https://codereview.chromium.org/1696563002/diff/300001/blimp/client/BUILD.gn#... blimp/client/BUILD.gn:72: data += [ "session/test_selfsigned_cert.pem" ] On 2016/03/02 02:26:44, Wez wrote: > Is this sufficient to get the correct test-isolate dependency on the PEM file? I *think* so? remoting_unittests uses "data" for its cert file deps. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source.cc:127: assignment.engine_endpoint = net::IPEndPoint(ip_address, port); On 2016/03/02 02:26:44, Wez wrote: > nit: I think you can put the checked_cast for port in-line here, rather than > pulling it out explicitly to a local variable above. Done. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source.cc:166: if (!assignment.IsValid()) { On 2016/03/02 02:26:44, Wez wrote: > nit: Do you want to DLOG(FATAL) if it's not valid, as you do for other errors? Done. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source.cc:170: assignment.client_token = kDummyClientToken; On 2016/03/02 02:26:45, Wez wrote: > nit: Suggest moving this up to the top, immediately after the Assignment is > created. Done. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source.cc:183: (transport_protocol != TransportProtocol::SSL || cert); On 2016/03/02 02:26:45, Wez wrote: > nit: This is getting complex enough to break down into a set if (...) return > false; statements followed by a return true, IMO. Done. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source.cc:223: // If no assignment was passed via the comamnd line, then fall back on On 2016/03/02 02:26:45, Wez wrote: > typo: command Done. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... File blimp/client/session/assignment_source_unittest.cc (right): https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:59: const std::string& expected_cert_str) { On 2016/03/02 02:26:45, Wez wrote: > nit: AFAICT all the calls to this API get passed the result of > BuildValidAssignment(), and cert_pem_, so you could make this a zero-parameter > method of the test base. Good suggestion, done. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:78: PathService::Get(base::DIR_SOURCE_ROOT, &src_root); On 2016/03/02 02:26:45, Wez wrote: > nit: ASSERT_TRUE this as well. Done. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:83: net::X509Certificate::FORMAT_PEM_CERT_SEQUENCE)[0]); On 2016/03/02 02:26:45, Wez wrote: > nit: ASSERT_TRUE that |cert_| is not empty? Done. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:149: Assignment BuildValidAssignment(); On 2016/03/02 02:26:45, Wez wrote: > nit: Suggest renaming this to BuildSslAssignment(), since that's what it does, > for clarity. :) Done. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:159: base::ScopedTempDir temp_dir_; On 2016/03/02 02:26:45, Wez wrote: > nit: Doesn't look like this is used any more? Done. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:172: safe_json::TestingJsonParser::ScopedFactoryOverride json_parsing_factory_; On 2016/03/02 02:26:45, Wez wrote: > nit: One-line comment to explain this? Done. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:240: BuildResponseFromAssignment(assignment, cert_pem_), "UserAuthT0kenz", On 2016/03/02 02:26:45, Wez wrote: > nit: Move this token value into a constant? Done. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:372: dict.SetInteger("port", 500); On 2016/03/02 02:26:45, Wez wrote: > nit: You could just call BuildResponse...() and then Remove("certificate", > nullptr) here, and in the other "missing field" tests, which would make them > easier to grok. Done. https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/b... File blimp/client/session/blimp_client_session.cc (right): https://codereview.chromium.org/1696563002/diff/300001/blimp/client/session/b... blimp/client/session/blimp_client_session.cc:94: default: On 2016/03/02 02:26:45, Wez wrote: > nit: Handle UNKNOWN explicitly here, rather than via default - that way the > compilation will fail if a new type is added w/out updating this. Done. https://codereview.chromium.org/1696563002/diff/300001/blimp/net/tcp_client_t... File blimp/net/tcp_client_transport.cc (right): https://codereview.chromium.org/1696563002/diff/300001/blimp/net/tcp_client_t... blimp/net/tcp_client_transport.cc:76: void TCPClientTransport::SetSocket(scoped_ptr<net::StreamSocket> socket) { On 2016/03/02 02:26:45, Wez wrote: > nit: Is it ever valid for caller to SetSocket(nullptr)? DCHECK here if not. :) Done.
This was a *long* review, but I'm happy with how it turned out, and happy with the follow-up work to explore (the socket connection logic being distinct from the socket abstraction logic) ... DEPS and //net LGTM :) https://codereview.chromium.org/1696563002/diff/360001/blimp/client/session/a... File blimp/client/session/assignment_source_unittest.cc (right): https://codereview.chromium.org/1696563002/diff/360001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:76: net::X509Certificate::FORMAT_PEM_CERT_SEQUENCE)[0]); FWIW, this will crash if cert_pem_.data() is invalid (because it will be .empty()) CertificateList certs = net::X509Certificate::CreateCertificateListFromBytes(...); ASSERT_FALSE(certs.empty()); cert_ = std::move(certs[0]); ASSERT_TRUE(cert_); https://codereview.chromium.org/1696563002/diff/360001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:349: response->SetString("host", "happywhales"); Love the name, but make it happywhales.test (It's RFC 6761 reserved)
safe_json LGTM https://codereview.chromium.org/1696563002/diff/360001/blimp/client/session/DEPS File blimp/client/session/DEPS (right): https://codereview.chromium.org/1696563002/diff/360001/blimp/client/session/D... blimp/client/session/DEPS:2: "+components/safe_json", This is now covered by the parent directory DEPS.
Phew!! Thanks for the insightful comments, everyone. https://codereview.chromium.org/1696563002/diff/360001/blimp/client/session/DEPS File blimp/client/session/DEPS (right): https://codereview.chromium.org/1696563002/diff/360001/blimp/client/session/D... blimp/client/session/DEPS:2: "+components/safe_json", On 2016/03/02 19:22:09, Robert Sesek wrote: > This is now covered by the parent directory DEPS. Done. https://codereview.chromium.org/1696563002/diff/360001/blimp/client/session/a... File blimp/client/session/assignment_source_unittest.cc (right): https://codereview.chromium.org/1696563002/diff/360001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:76: net::X509Certificate::FORMAT_PEM_CERT_SEQUENCE)[0]); On 2016/03/02 19:18:14, Ryan Sleevi wrote: > FWIW, this will crash if cert_pem_.data() is invalid (because it will be > .empty()) > > CertificateList certs = > net::X509Certificate::CreateCertificateListFromBytes(...); > ASSERT_FALSE(certs.empty()); > cert_ = std::move(certs[0]); > ASSERT_TRUE(cert_); Done. https://codereview.chromium.org/1696563002/diff/360001/blimp/client/session/a... blimp/client/session/assignment_source_unittest.cc:349: response->SetString("host", "happywhales"); On 2016/03/02 19:18:14, Ryan Sleevi wrote: > Love the name, but make it happywhales.test (It's RFC 6761 reserved) TIL the .test TLD!
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org, rsesek@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/1696563002/#ps380001 (title: "rsleevi changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1696563002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1696563002/380001
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Blimp: add support for SSL connections. This CL allows the Blimp client to establish TLS-protected channels with the backend engine. The authenticity of the engine is validated by checking if its cert is an exact match of a certificate provided separately by the Assigner API. * Create new Blimp SSL transport class: SSLClientTransport. * Create custom CertValidator for checking an exact cert match against the SSL peer's cert * Integrate SSLClientTransport with BlimpClientSession. * Assignment: add certificate field. * AssignmentSource: add certificate file reading; PEM file parsing; X509 certificate parsing. * Created new DEPS entries as appropriate. R=wez@chromium.org CC=rsleevi@chromium.org BUG=585279,589202 ========== to ========== Blimp: add support for SSL connections. This CL allows the Blimp client to establish TLS-protected channels with the backend engine. The authenticity of the engine is validated by checking if its cert is an exact match of a certificate provided separately by the Assigner API. * Create new Blimp SSL transport class: SSLClientTransport. * Create custom CertValidator for checking an exact cert match against the SSL peer's cert * Integrate SSLClientTransport with BlimpClientSession. * Assignment: add certificate field. * AssignmentSource: add certificate file reading; PEM file parsing; X509 certificate parsing. * Created new DEPS entries as appropriate. R=wez@chromium.org CC=rsleevi@chromium.org BUG=585279,589202 Committed: https://crrev.com/c80f5095f045ad1712f1f1075a44547a561f774a Cr-Commit-Position: refs/heads/master@{#378839} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/c80f5095f045ad1712f1f1075a44547a561f774a Cr-Commit-Position: refs/heads/master@{#378839}
Message was sent while issue was closed.
This broke blimp_unittests_apk target on a bunch of (non-main waterfall, non-cq) bots: https://build.chromium.org/p/chromium.android/builders/Android%20arm%20Builde... Missing dependency somewhere I think
Message was sent while issue was closed.
boliu@chromium.org changed reviewers: + boliu@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1696563002/diff/380001/blimp/client/BUILD.gn File blimp/client/BUILD.gn (right): https://codereview.chromium.org/1696563002/diff/380001/blimp/client/BUILD.gn#... blimp/client/BUILD.gn:30: "//components/safe_json", This transitively pulls in //content/public/browser, but not its dependencies it appears (content/renderer, content/gpu from error it seems). I don't really see an obvious way to fix this, so going to revert. Sorry
Message was sent while issue was closed.
A revert of this CL (patchset #20 id:380001) has been created in https://codereview.chromium.org/1757153002/ by boliu@chromium.org. The reason for reverting is: Broke blimp_unittests_apk building on android: FAILED: python "/b/build/slave/android_build/build/src/build/toolchain/gcc_solink_wrapper.py" --readelf="../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-readelf" --nm="../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-nm" --strip=../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-strip --sofile="./lib.unstripped/libblimp_client_android.so" --tocfile="./libblimp_client_android.so.TOC" --output="./libblimp_client_android.so" -- ../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++ -shared -Wl,--fatal-warnings -fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro -Wl,-z,defs -fuse-ld=gold -Wl,--no-undefined -Wl,--exclude-libs=libgcc.a -Wl,--exclude-libs=libc++_static.a -Wl,--exclude-libs=libvpx_assembly_arm.a -Wl,--icf=all -Wl,--warn-shared-textrel -Wl,-O1 -Wl,--as-needed -nostdlib -Wl,--warn-shared-textrel --sysroot=../../third_party/android_tools/ndk/platforms/android-16/arch-arm -Wl,--version-script=/b/build/slave/android_build/build/src/build/android/android_no_jni_exports.lst -L../../third_party/android_tools/ndk/sources/cxx-stl/llvm-libc++/libs/armeabi-v7a -o "./lib.unstripped/libblimp_client_android.so" -Wl,-soname="libblimp_client_android.so" @"./libblimp_client_android.so.rsp" ../../content/browser/android/in_process/synchronous_compositor_factory_impl.cc:125: error: undefined reference to 'content::SynchronousCompositorExternalBeginFrameSource::SynchronousCompositorExternalBeginFrameSource(int, content::SynchronousCompositorRegistry*)' ../../content/browser/android/in_process/synchronous_compositor_factory_impl.cc:113: error: undefined reference to 'content::SynchronousCompositorOutputSurface::SynchronousCompositorOutputSurface(scoped_refptr<cc::ContextProvider> const&, scoped_refptr<cc::ContextProvider> const&, int, content::SynchronousCompositorRegistry*, scoped_refptr<content::FrameSwapMessageQueue>)' ../../base/memory/ref_counted.h:193: error: undefined reference to 'content::FrameSwapMessageQueue::~FrameSwapMessageQueue()' ../../content/browser/android/in_process/synchronous_compositor_factory_impl.cc:93: error: undefined reference to 'content::SynchronousCompositorFactory::SetInstance(content::SynchronousCompositorFactory*)' ../../content/browser/android/in_process/synchronous_compositor_factory_impl.cc:197: error: undefined reference to 'content::StreamTextureFactorySynchronousImpl::Create(base::Callback<scoped_refptr<content::StreamTextureFactorySynchronousImpl::ContextProvider> ()> const&)' ../../content/browser/android/in_process/synchronous_compositor_factory_impl.cc:272: error: undefined reference to 'content::RenderThreadImpl::SetStreamTextureFactory(scoped_refptr<content::StreamTextureFactory>)' ../../content/browser/android/in_process/synchronous_compositor_impl.cc:102: error: undefined reference to 'content::SynchronousCompositorOutputSurface::SetTreeActivationCallback(base::Callback<void ()> const&)' ../../content/browser/android/in_process/synchronous_compositor_impl.cc:125: error: undefined reference to 'content::SynchronousCompositorOutputSurface::SetSyncClient(content::SynchronousCompositorOutputSurfaceClient*)' ../../content/browser/android/in_process/synchronous_compositor_impl.cc:126: error: undefined reference to 'content::SynchronousCompositorExternalBeginFrameSource::SetClient(content::SynchronousCompositorExternalBeginFrameSourceClient*)' ../../content/browser/android/in_process/synchronous_compositor_impl.cc:135: error: undefined reference to 'content::SynchronousCompositorOutputSurface::SetTreeActivationCallback(base::Callback<void ()> const&)' ../../content/browser/android/in_process/synchronous_compositor_impl.cc:141: error: undefined reference to 'content::SynchronousCompositorExternalBeginFrameSource::SetClient(content::SynchronousCompositorExternalBeginFrameSourceClient*)' ../../content/browser/android/in_process/synchronous_compositor_impl.cc:142: error: undefined reference to 'content::SynchronousCompositorOutputSurface::SetSyncClient(content::SynchronousCompositorOutputSurfaceClient*)' ../../content/browser/android/in_process/synchronous_compositor_impl.cc:178: error: undefined reference to 'content::SynchronousCompositorOutputSurface::ReturnResources(cc::CompositorFrameAck const&)' ../../content/browser/android/in_process/synchronous_compositor_impl.cc:215: error: undefined reference to 'content::SynchronousCompositorOutputSurface::SetMemoryPolicy(unsigned int)' ../../content/browser/android/in_process/synchronous_compositor_impl.cc:275: error: undefined reference to 'content::SynchronousCompositorExternalBeginFrameSource::BeginFrame(cc::BeginFrameArgs const&)' ../../content/browser/android/in_process/synchronous_compositor_impl.cc:320: error: undefined reference to 'content::SynchronousCompositorOutputSurface::GetMessagesToDeliver(std::__1::vector<scoped_ptr<IPC::Message, std::__1::default_delete<IPC::Message> >, std::__1::allocator<scoped_ptr<IPC::Message, std::__1::default_delete<IPC::Message> > > >*)' ../../content/browser/android/in_process/synchronous_compositor_impl.cc:167: error: undefined reference to 'content::SynchronousCompositorOutputSurface::DemandDrawHw(gfx::Size const&, gfx::Transform const&, gfx::Rect const&, gfx::Rect const&, gfx::Rect const&, gfx::Transform const&)' ../../content/browser/android/in_process/synchronous_compositor_impl.cc:187: error: undefined reference to 'content::SynchronousCompositorOutputSurface::DemandDrawSw(SkCanvas*)' ../../content/browser/android/in_process/synchronous_compositor_renderer_statics.cc:12: error: undefined reference to 'content::SynchronousCompositorProxy::SetSkCanvasForDraw(SkCanvas*)' ../../content/browser/android/synchronous_compositor_base.cc:32: error: undefined reference to 'content::InProcessGpuThread::InProcessGpuThread(content::InProcessChildThreadParams const&, gpu::GpuPreferences const&, gpu::SyncPointManager*)' collect2: error: ld returned 1 exit status.
Message was sent while issue was closed.
Thanks boliu for reverting the breakage, which only occurs on static Android builds. I put a fix for SafeJSON out for review: https://codereview.chromium.org/1761943002/
Message was sent while issue was closed.
Description was changed from ========== Blimp: add support for SSL connections. This CL allows the Blimp client to establish TLS-protected channels with the backend engine. The authenticity of the engine is validated by checking if its cert is an exact match of a certificate provided separately by the Assigner API. * Create new Blimp SSL transport class: SSLClientTransport. * Create custom CertValidator for checking an exact cert match against the SSL peer's cert * Integrate SSLClientTransport with BlimpClientSession. * Assignment: add certificate field. * AssignmentSource: add certificate file reading; PEM file parsing; X509 certificate parsing. * Created new DEPS entries as appropriate. R=wez@chromium.org CC=rsleevi@chromium.org BUG=585279,589202 Committed: https://crrev.com/c80f5095f045ad1712f1f1075a44547a561f774a Cr-Commit-Position: refs/heads/master@{#378839} ========== to ========== Blimp: add support for SSL connections. This CL allows the Blimp client to establish TLS-protected channels with the backend engine. The authenticity of the engine is validated by checking if its cert is an exact match of a certificate provided separately by the Assigner API. * Create new Blimp SSL transport class: SSLClientTransport. * Create custom CertValidator for checking an exact cert match against the SSL peer's cert * Integrate SSLClientTransport with BlimpClientSession. * Assignment: add certificate field. * AssignmentSource: add certificate file reading; PEM file parsing; X509 certificate parsing. * Created new DEPS entries as appropriate. R=wez@chromium.org CC=rsleevi@chromium.org BUG=585279,589202 Committed: https://crrev.com/c80f5095f045ad1712f1f1075a44547a561f774a Cr-Commit-Position: refs/heads/master@{#378839} ==========
The CQ bit was checked by kmarshall@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1696563002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1696563002/400001
Message was sent while issue was closed.
Description was changed from ========== Blimp: add support for SSL connections. This CL allows the Blimp client to establish TLS-protected channels with the backend engine. The authenticity of the engine is validated by checking if its cert is an exact match of a certificate provided separately by the Assigner API. * Create new Blimp SSL transport class: SSLClientTransport. * Create custom CertValidator for checking an exact cert match against the SSL peer's cert * Integrate SSLClientTransport with BlimpClientSession. * Assignment: add certificate field. * AssignmentSource: add certificate file reading; PEM file parsing; X509 certificate parsing. * Created new DEPS entries as appropriate. R=wez@chromium.org CC=rsleevi@chromium.org BUG=585279,589202 Committed: https://crrev.com/c80f5095f045ad1712f1f1075a44547a561f774a Cr-Commit-Position: refs/heads/master@{#378839} ========== to ========== Blimp: add support for SSL connections. This CL allows the Blimp client to establish TLS-protected channels with the backend engine. The authenticity of the engine is validated by checking if its cert is an exact match of a certificate provided separately by the Assigner API. * Create new Blimp SSL transport class: SSLClientTransport. * Create custom CertValidator for checking an exact cert match against the SSL peer's cert * Integrate SSLClientTransport with BlimpClientSession. * Assignment: add certificate field. * AssignmentSource: add certificate file reading; PEM file parsing; X509 certificate parsing. * Created new DEPS entries as appropriate. R=wez@chromium.org CC=rsleevi@chromium.org BUG=585279,589202 Committed: https://crrev.com/c80f5095f045ad1712f1f1075a44547a561f774a Cr-Commit-Position: refs/heads/master@{#378839} ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== Blimp: add support for SSL connections. This CL allows the Blimp client to establish TLS-protected channels with the backend engine. The authenticity of the engine is validated by checking if its cert is an exact match of a certificate provided separately by the Assigner API. * Create new Blimp SSL transport class: SSLClientTransport. * Create custom CertValidator for checking an exact cert match against the SSL peer's cert * Integrate SSLClientTransport with BlimpClientSession. * Assignment: add certificate field. * AssignmentSource: add certificate file reading; PEM file parsing; X509 certificate parsing. * Created new DEPS entries as appropriate. R=wez@chromium.org CC=rsleevi@chromium.org BUG=585279,589202 Committed: https://crrev.com/c80f5095f045ad1712f1f1075a44547a561f774a Cr-Commit-Position: refs/heads/master@{#378839} ========== to ========== Blimp: add support for SSL connections. This CL allows the Blimp client to establish TLS-protected channels with the backend engine. The authenticity of the engine is validated by checking if its cert is an exact match of a certificate provided separately by the Assigner API. * Create new Blimp SSL transport class: SSLClientTransport. * Create custom CertValidator for checking an exact cert match against the SSL peer's cert * Integrate SSLClientTransport with BlimpClientSession. * Assignment: add certificate field. * AssignmentSource: add certificate file reading; PEM file parsing; X509 certificate parsing. * Created new DEPS entries as appropriate. R=wez@chromium.org CC=rsleevi@chromium.org BUG=585279,589202 Committed: https://crrev.com/c80f5095f045ad1712f1f1075a44547a561f774a Cr-Commit-Position: refs/heads/master@{#378839} Committed: https://crrev.com/05e3f96385c7a9808dec06f13419d7bb1996ec45 Cr-Commit-Position: refs/heads/master@{#379081} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/05e3f96385c7a9808dec06f13419d7bb1996ec45 Cr-Commit-Position: refs/heads/master@{#379081}
Message was sent while issue was closed.
On 2016/02/26 18:32:07, Wez wrote: > mailto:wez@chromium.org changed reviewers: > - mailto:bauerb@chromium.org Um... Wez, you removed me as a reviewer without putting me on CC, so I did not see any of the following discussion. Could you... not do that next time pls? 😃
Message was sent while issue was closed.
Um... Nope, don't think so. Looks like you got dropped in-between comments #44 and #46? On 15 March 2016 at 03:09, <bauerb@chromium.org> wrote: > On 2016/02/26 18:32:07, Wez wrote: > > mailto:wez@chromium.org changed reviewers: > > - mailto:bauerb@chromium.org > > Um... Wez, you removed me as a reviewer without putting me on CC, so I did > not > see any of the following discussion. Could you... not do that next time > pls? 😃 > > https://codereview.chromium.org/1696563002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
