1461608 - Hardening handshake checks for geckodriver

Status: RESOLVED FIXED

(Testing :: geckodriver, enhancement, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Similar to bug 1414882 for the Marionette client we need a handshake for geckodriver which validates that the other end-point is really Marionette. Right now we break out from the connection loop when we are able to connect:

Steps:
1) Run `% ncat -l 127.0.0.1 2828 -k`
2) Run `geckodriver --connect-existing --marionette-port 2828 -vv`
3) Send HTTP request: `curl -d '{"capabilities": {"alwaysMatch": {}}}' http://localhost:4444/session`

In the geckodriver log you will see:

> 1526375752937	webdriver::server	DEBUG	-> POST /session {"capabilities": {"alwaysMatch": {}}}
> 1526375752942	geckodriver::marionette	DEBUG	Waiting 60s to connect to browser on localhost:2828
> 1526375752944	geckodriver::marionette	DEBUG	Connected to Marionette on localhost:2828

We should never get into the connected state because the other end-point is not Marionette and doesn't understand its protocol.

Please also see bug 1461606 about re-using the same port for a multicast address and which implications it will have. We don't guard against this at the moment.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

While working on the Serde implementation I noticed that we actually have a `handshake` check in geckodriver:

https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/testing/geckodriver/src/marionette.rs#1401-1424

But when it fails, geckodriver doesn't shutdown the browser but leaves it running.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I tried my example from comment 0 and actually have seen the following:

> 1535745248602	webdriver::server	DEBUG	-> POST /session {"capabilities": {"alwaysMatch": {}}}
> 1535745248603	geckodriver::marionette	DEBUG	Waiting 60s to connect to browser on 127.0.0.1:2828

Looks like that with the conversion to Serde we no longer connect to a ncat instance! Maybe this fixes issues like we have seen on github like https://github.com/mozilla/geckodriver/issues/979.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

What's left to do here is to also check for the `application_type == gecko`, and close the browser when invalid data is detected.

I got the first check already implemented. Closing the browser I will start working on Monday.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: [geckodriver] Hardening handshake checks for Marionette connections

If the handshake by creating a Marionette connection fails, the
browser process has to be killed.

Also geckodriver has to check for both application type,
and Marionette protocol having valid values.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: [geckodriver] Hardening handshake checks for Marionette connections

If the handshake by creating a Marionette connection fails, the
browser process has to be killed.

Also geckodriver has to check for both application type,
and Marionette protocol having valid values.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: [geckodriver] Reduce stream read timeout for Marionette handshake to 10s

By default the stream read timeout is set to None, which means if
geckodriver successfully connects to a port, which doesn't send any
data, it waits forever.

Given that handshake data should be received immediately reduce the
read timeout on supported platforms to 10s. This would still allow
slower builds (eg. ASAN) to send the Marionette handshake.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08efd8e8cc2a54bc8df9c9389ad2a9611d618f26

Comment 8

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 9006031 [details] [diff] [review]
[geckodriver] Hardening handshake checks for Marionette connections

Review of attachment 9006031 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/geckodriver/src/marionette.rs
@@ +1130,4 @@
>          let resp = self.read_resp()?;
>          let data = serde_json::from_str::<MarionetteHandshake>(&resp)?;
>  
> +        if data.application_type != "gecko".to_owned() {

I think String implements a comparator for &str, right?  Is .to_owned()
necessary here?

Comment 9

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 9006032 [details] [diff] [review]
[geckodriver] Reduce stream read timeout for Marionette handshake to 10s

Review of attachment 9006032 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few minor ideas about logging.  The patch itself is fine.

::: testing/geckodriver/src/marionette.rs
@@ +1125,1 @@
>          let data = self.handshake()?;

This is minor, but:

Can you put “Waiting for handshake” inside ::handshake(), and change
“Connection established on” to “Marionette connection established
on …”?

@@ +1125,5 @@
>          let data = self.handshake()?;
>          self.session.application_type = Some(data.application_type);
>          self.session.protocol = Some(data.protocol);
>  
> +        debug!("Connected to Marionette");

Not sure about the usefulness of this one, seeing as you (correctly)
moved what was here a few lines up.  The first request to start a
Marionette session is bound to come right after this.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andreas Tolfsen ❲:ato❳ from comment #8)
> > +        if data.application_type != "gecko".to_owned() {
> 
> I think String implements a comparator for &str, right?  Is .to_owned()
> necessary here?

I just checked and it's not necessary here. I will remove it. Thanks.

(In reply to Andreas Tolfsen ❲:ato❳ from comment #9)
> >          let data = self.handshake()?;
> 
> This is minor, but:
> 
> Can you put “Waiting for handshake” inside ::handshake(), and change
> “Connection established on” to “Marionette connection established
> on …”?

I'm not happy with the change. Please note that at this stage we don't know if we connected to Marionette, or just some other service running on this port. At earliest after a successful handshake we can be sure that it is Marionette.

Regarding moving the logging to `handshake()` I would keep it as it is, to not clutter the method with logging code.

> > +        debug!("Connected to Marionette");
> 
> Not sure about the usefulness of this one, seeing as you (correctly)
> moved what was here a few lines up.  The first request to start a
> Marionette session is bound to come right after this.

Please see above.

Let me know what you think. Thanks.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Given that Andreas is out today and the remaining question is just a minor thing, I will get my patch landed now. We can do a follow-up if necessary later.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/082161ea398a
[geckodriver] Hardening handshake checks for Marionette connections. r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/492491501281
[geckodriver] Reduce stream read timeout for Marionette handshake to 10s. r=ato

Comment 13

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/082161ea398a
https://hg.mozilla.org/mozilla-central/rev/492491501281