Closed Bug 1457274 Opened 6 years ago Closed 5 years ago

Upgrade to mitmproxy 4.0.4 in production

Categories

(Testing :: Raptor, enhancement, P1)

Product:
Testing
Component:
Raptor
Version:
Version 3
Type:
enhancement

Tracking

(firefox68 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox68 --- fixed

People

(Reporter: rwood, Assigned: Bebe)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1457274 Upgrade mitmproxy in production
47 bytes, text/x-phabricator-request
Details | Review
raptor.json
7.08 KB, application/json
Details 
In the talos tp6 performance tests we are using mitmproxy 2.X. I'd like to upgrade to the new release 3.X.
Whiteboard: [PI:May]
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.
Whiteboard: [PI:May]
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
: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.
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.
Component: Talos → Raptor
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.
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
Flags: needinfo?(rwood)
Flags: needinfo?(dpalmeiro)
(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!
Flags: needinfo?(rwood) → needinfo?(fstrugariu)
(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.
Flags: needinfo?(dpalmeiro)
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.
Priority: -- → P2

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.

Assignee: nobody → fstrugariu
Status: NEW → ASSIGNED
Priority: P2 → P1

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)

Flags: needinfo?(tarek)
Flags: needinfo?(rwood)
Flags: needinfo?(dave.hunt)

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

Flags: needinfo?(tarek)
Flags: needinfo?(rwood)
Flags: needinfo?(dave.hunt)

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
Attached file raptor.json —

@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

Flags: needinfo?(dave.hunt)

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

Attachment #9051270 - Attachment is obsolete: true
Blocks: 1535597

@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.

Flags: needinfo?(dave.hunt) → needinfo?(jmaher)

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.

Flags: needinfo?(jmaher)
Blocks: 1527620
Summary: Upgrade mitmproxy in production → Upgrade to mitmproxy 4.0.4 in production
Pushed by fstrugariu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4055d335fa50
Upgrade mitmproxy in production r=rwood,tarek

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

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

(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.

status-firefox68: fixed → ---
Flags: needinfo?(fstrugariu)
Target Milestone: mozilla68 → ---

(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

Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Flags: needinfo?(fstrugariu)
Resolution: --- → FIXED
status-firefox68: --- → fixed
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: