Closed Bug 1461608 Opened Last year Closed Last year

Hardening handshake checks for geckodriver

Categories

(Testing :: geckodriver, enhancement, P1)

Version 3
enhancement

Tracking

(firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.
Blocks: 1466818
No longer blocks: 1441204
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.
Summary: Add handshake to geckodriver → Hardening handshake checks for geckodriver
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.
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.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P3 → P1
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.
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.
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 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?
Attachment #9006031 - Flags: review?(ato) → review+
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.
Attachment #9006032 - Flags: review?(ato) → review+
(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.
Flags: needinfo?(ato)
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.
Pushed by hskupin@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/082161ea398a
https://hg.mozilla.org/mozilla-central/rev/492491501281
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Flags: needinfo?(ato)
You need to log in before you can comment on or make changes to this bug.