1457274 - Upgrade to mitmproxy 4.0.4 in production

Status: RESOLVED FIXED

(Testing :: Raptor, enhancement, P1)

Description

[Robert Wood [:rwood]]

In the talos tp6 performance tests we are using mitmproxy 2.X. I'd like to upgrade to the new release 3.X.

Comment 1

[Robert Wood [:rwood]]

I did a try push for tp6 with the new mitmproxy release 3.x (uploaded to tooltool in Bug 1457271) and it doesn't work. After some investigation, I found out it is because the new mitmproxy release made a change which is not backwards compatible. Mimdump now doesn't allow you to pass in command line arguments to custom playback scripts. We are using a custom playback script [1].

Apparently to get around this, we will need to set some custom mitmproxy server settings to replace our command line args in the custom script, or something to that effect. Mitmproxy has not updated their online documentation yet as how to do that. So I'm postponing this upgrade for now.

Comment 2

[Robert Wood [:rwood]]

[1] https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/testing/talos/talos/mitmproxy/alternate-server-replay.py

Comment 3

[Simon Fraser [:sfraser] ⌚️GMT]

Could we revisit this now that the 4.x series is out? I can help if needed.

There are security alerts for mitmproxy and its dependency, cryptography:

/Users/sfraser/hg.m.o/mozilla-unified/testing/raptor/raptor/playback/mitmproxy_requirements.txt
  FAIL mitmproxy - mitmproxy installed:2.0.2 affected:<4.0.3 description:mitmproxy before 4.0.3 does not protect mitmweb against DNS rebinding.
  FAIL mitmproxy - mitmproxy installed:2.0.2 affected:<4.0.4 description:mitmproxy before 4.0.4 does not protect mitmweb against DNS rebinding.
  FAIL /Users/sfraser/hg.m.o/mozilla-unified/testing/raptor/raptor/playback/mitmproxy_requirements.txt - mitmproxy

/Users/sfraser/hg.m.o/mozilla-unified/testing/talos/talos/mitmproxy/mitmproxy_requirements.txt
  FAIL cryptography - cryptography installed:2.1.4 affected:>=1.9.0,<2.3 description:python-cryptography versions >=1.9.0 and <2.3 did not enforce a minimum tag length for finalize_with_tag API. If a user did not validate the input length prior to passing it to finalize_with_tag an attacker could craft an invalid payload with a shortened tag (e.g. 1 byte) such that they would have a 1 in 256 chance of passing the MAC check. GCM tag forgeries can cause key leakage.
  FAIL mitmproxy - mitmproxy installed:2.0.2 affected:<4.0.3 description:mitmproxy before 4.0.3 does not protect mitmweb against DNS rebinding.
  FAIL mitmproxy - mitmproxy installed:2.0.2 affected:<4.0.4 description:mitmproxy before 4.0.4 does not protect mitmweb against DNS rebinding.
  FAIL /Users/sfraser/hg.m.o/mozilla-unified/testing/talos/talos/mitmproxy/mitmproxy_requirements.txt - mitmproxy cryptography

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

:sfraser, you are welcome to work on this- but I do not see us making this a priority until Q1 next year- if this was estimated to be 1 day of work, then we could squeeze it in- right now we have 125% scheduled workload to finish some new test development.

Comment 5

[Robert Wood [:rwood]]

Moving this to testing ==> raptor since we will soon be turning off talos tp6 in favour of raptor tp6. Note: This is not a high priority, as it will take a bit of time - as upgrading Mitmproxy will require a work-around so we can still use our custom playback script (comment 1); testing the new Mitmproxy on all platforms; and ensuring the existing tp6/gdocs recordings can be played back with the new version.

Comment 6

[Robert Wood [:rwood]]

From IRC:

<denispal> rwood:  btw, I got full page load replays to work with the latest mitmproxy.  You're right that they don't let you override the playback script any more, but I just modified the one in the source directly instead to do the same thing the one in raptor does (issue 404 on kill requests).  
<denispal> and if we inject some deterministic JS (thanks ehsan for the tip!) , we can record and playback pretty much any website.

Comment 7

[Florin Strugariu [:Bebe]]

I did a little hacking today on this also:
Managed to successfully modify the replay script to run and generate the desired content:

https://gist.github.com/bebef1987/a7782ca7c3c6076e9af765ad0d252476 

:rwood :denispal can you take a look

Comment 8

[Robert Wood [:rwood]]

(In reply to Florin Strugariu [:Bebe] from comment #7)
> I did a little hacking today on this also:
> Managed to successfully modify the replay script to run and generate the
> desired content:
> 
> https://gist.github.com/bebef1987/a7782ca7c3c6076e9af765ad0d252476 
> 
> :rwood :denispal can you take a look

Cool thanks. What is the mitmdump command line you use to playback a recording with the alternate-server-replay.py script you modified?

Also can you please provide us a diff of the alternate-server-replay.py script instead of just the new one (I am not familiar with this script at all so a diff would be helpful). Thanks!

Comment 9

[Denis Palmeiro [:denispal]]

(In reply to Florin Strugariu [:Bebe] from comment #7)
> I did a little hacking today on this also:
> Managed to successfully modify the replay script to run and generate the
> desired content:
> 
> https://gist.github.com/bebef1987/a7782ca7c3c6076e9af765ad0d252476 
> 
> :rwood :denispal can you take a look

Is this for the latest mitmproxy?  All I did was basically apply the 404 response changes in alternate-server-replay.py to the actual replay script here:  https://github.com/mitmproxy/mitmproxy/blob/master/mitmproxy/addons/serverplayback.py#L191.

Comment 10

[Florin Strugariu [:Bebe]]

I have multiple versions in work: 
https://github.com/bebef1987/mitmproxy-server-replay
I think this is the best option:
https://github.com/bebef1987/mitmproxy-server-replay/blob/master/alternate-server-replay-4.0.4-best-match.py

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

It looks like we can just create our own server playback addon to respond with a 404. I've tested this locally by just copying https://github.com/mitmproxy/mitmproxy/blob/889987aa0a7f4852758ed09f70fe5d30f733a6d3/mitmproxy/addons/serverplayback.py locally and modifying it to respond with 404. You can then run using the latest mitmproxy version by adding -s to the command line and the path to the modified script. Let's see if we can proceed with this. I would also suggest opening an issue against mitmproxy to see if the maintainers would support the idea of a --server-replay-404-extra command line option to avoid us having to maintain a fork of this addon.

Comment 12

[Robert Wood [:rwood]]

As requested :bebe I uploaded 'mitmproxy-4.0.4-linux.tar.gz' to tooltool, for your testing on Linux64. I'll email you the manifest.

Comment 13

[Florin Strugariu [:Bebe]]

Attachment: Bug 1457274 Upgrade mitmproxy in production

Comment 14

[Florin Strugariu [:Bebe]]

Try builds:
4.0.4 replay script https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e2e18d78f313d0afe939fbf5d7737d285502ea2
best match script for 4.0.4 https://treeherder.mozilla.org/#/jobs?repo=try&revision=fac10855cbf065689339042629433c32dc58f23c

Comment 15

[Florin Strugariu [:Bebe]]

Latest try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f83a98bad4fc265dd6f4abba858c48ba7b246a5f
I think this is the final form of this patch

From my dissolution with Dave we decided to activate 4.0.4 to only one test run (tp6-8)

Comment 16

[Dave Hunt [:davehunt] [he/him] ⌚BST]

See phabricator review. My main concern is the performance impact that upgrading mitmproxy appears to have.

Comment 17

[Florin Strugariu [:Bebe]]

I ran a test with the same website using a 2.0.2 recording and a 4.0.4 recording

ebay.com    2.0.2    4.0.4

dcf         385.5    401.5
fcp         290.0    294.5
fnbpaint    274.0    272.5
ttfi        393.5    438.5
loadtime    449.0    461.5

total       352.12   365.5

Comment 18

[Florin Strugariu [:Bebe]]

Attachment: raptor.json

Comment 19

[Florin Strugariu [:Bebe]]

@davehunt To have a better view on this we should run in parallel these tests and see how it works on treeherder

I can make some recordings and have them run in parallel

Comment 20

[Florin Strugariu [:Bebe]]

here you can find 4.0.4 recording for tp6-8 tests
https://drive.google.com/open?id=1JuDbJuD65CNWZZ2j0zIQMoioOxhgI2SL

Comment 21

[Florin Strugariu [:Bebe]]

Attachment: Bug 1457274 Upgrade mitmproxy in production

Comment 22

[Dave Hunt [:davehunt] [he/him] ⌚BST]

@davehunt To have a better view on this we should run in parallel these tests and see how it works on treeherder

I can make some recordings and have them run in parallel

If mitmproxy 4 is able to run using mitmproxy 2 recordings, producing identical visual results (verified by enabling screenshots on a try run) and the performance hit is not addressed by fresh recordings, then it would make sense to only change one thing at a time.

Is there a significant change to the standard deviation between the results for the two mitmproxy versions? Are we seeing a similar shift in the measurements for Chrome? If the change is consistent between Firefox and Chrome, and the noise has not increased, then we can probably accept the "regression" for all tests.

:jmaher I'm keen to hear your thoughts on this.

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

in my mind changing infrastructure or tooling should have little concern- I think to your point of calling out similar shifts in both firefox and chrome and measuring the noise, that would be of most concern. Even recording a fresh copy of a website in mitmproxy 2 vs 4 seems problematic as they might have different ways to record- I guess one way to validate the recordings is record a website in mitmproxy4 from it being replayed in mitmproxy2 :)

I understand fresh copies of the websites would be more desired, so maybe just ensuring the data looks to be the same pattern for all data points in a page, the noise level over time and how it stacks up on different browsers.

Comment 24

[Florin Strugariu [:Bebe]]

try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=55045973f85dba08e86433974e2804fb9f242e9f

Comment 25

[Florin Strugariu [:Bebe]]

try build with standard deviation
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e580184b392f032b5332fb5681d3354dce66d5a4&selectedJob=236124827

Comment 26

[Florin Strugariu [:Bebe]]

Doc that contains load times and stddev
https://docs.google.com/spreadsheets/d/1xznuAYMfUByYq98UKsZG15Vsa2qWagMqM2kQPiOKt-w/edit#gid=0

Comment 27

[Florin Strugariu [:Bebe]]

Try build:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&revision=05e056c3fe9c703e855fb089ccc72ce97532754e

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4055d335fa50
Upgrade mitmproxy in production r=rwood,tarek

Comment 29

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/4055d335fa50

Comment 30

[Florin Strugariu [:Bebe]]

Let's keep this open until we make the final decisions on mitmproxy 4.0.4

Comment 31

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

(In reply to Florin Strugariu [:Bebe] from comment #30)

Let's keep this open until we make the final decisions on mitmproxy 4.0.4

What does the final decision is about? Enabling it by default? If yes, mind filing a new bug for doing that? I better helps keeping track of landed commits. This one here was huge.

Comment 32

[Florin Strugariu [:Bebe]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #31)

(In reply to Florin Strugariu [:Bebe] from comment #30)

Let's keep this open until we make the final decisions on mitmproxy 4.0.4

What does the final decision is about? Enabling it by default? If yes, mind filing a new bug for doing that? I better helps keeping track of landed commits. This one here was huge.

You are right. We will track the stability of this in bug 1540627