52 seconds in Shellbox\Client::computeHmac over 3 calls, I guess all signatures for the remote shellbox calls

I benchmarked the SHA-256 HMAC we're using at 5.2 seconds per gigabyte. If that's too slow, one option is to weaken the hash. On the same test, SHA-1 took 1.8 seconds, and MD5 took 1.3 seconds. Another option is to improve the implementation. PHP has its own implementation of the SHA family hash functions which are not using hardware acceleration. Intel has an article explaining how to use the Intel SHA intrinsics to implement SHA-256, including sample code.

In T292322#7627149, @tstarling wrote:

52 seconds in Shellbox\Client::computeHmac over 3 calls, I guess all signatures for the remote shellbox calls

I benchmarked the SHA-256 HMAC we're using at 5.2 seconds per gigabyte. If that's too slow, one option is to weaken the hash. On the same test, SHA-1 took 1.8 seconds, and MD5 took 1.3 seconds. Another option is to improve the implementation. PHP has its own implementation of the SHA family hash functions which are not using hardware acceleration. Intel has an article explaining how to use the Intel SHA intrinsics to implement SHA-256, including sample code.

First let's see what happens once we've reduced the calls to shellbox. But it could be a good idea to conditionally switch to md5 for request signature when the request content-length is over a certain threshold.

There is openssl_digest() which presumably has hardware acceleration and can do SHA-256 in 2.1 seconds per gigabyte. But its input is a single string, not a stream, so it can't quite work as a drop-in replacement. At least it gives you some idea of what an optimisation patch for PHP could do.

Using a known broken hash like MD5 seems wrong in what's supposed to be a security-sensitive application. Since we are already calculating the SHA-1 hash for img_sha1, can we reuse that and only sign the SHA-1 instead of the entire file? Then the Shellbox server would verify the file matches the provided SHA-1.

(Yes, I know I complained about using broken hashes and then said to use SHA-1, but presumably this would get fixed when we move away from SHA-1 in the image table.)

In T292322#7635623, @Legoktm wrote:

Using a known broken hash like MD5 seems wrong in what's supposed to be a security-sensitive application. Since we are already calculating the SHA-1 hash for img_sha1, can we reuse that and only sign the SHA-1 instead of the entire file? Then the Shellbox server would verify the file matches the provided SHA-1.

(Yes, I know I complained about using broken hashes and then said to use SHA-1, but presumably this would get fixed when we move away from SHA-1 in the image table.)

I like your solution, but I will still argue that I didn't say MD5 lightly. MD5 is surely "broken", meaning it's possible to cause collisions, which means it's easy to solve the problem of guessing a text that gives the same cryptographic hash as yours.

But given in reality I was proposing to do something like:

signature = md5sum( secret + padding + request_body)

the attacker, in order to trick shellbox, would not need to match "signature", but rather to figure out "secret", which isn't how md5 has been broken. So I think it would be an actually valid way to assess the integrity of the request. Let me also note that depending on the length of the secret key, it is probably possible to brute-force guess it, given enough computational power, and yes using a faster hashing algorithm will reduce the computational cost of doing so.

Now, I wasn't aware we already had a sha1 of the file, so I would imagine that we could do hmac(secret, request - binary + sha1(binary)) and it would still be valid and quite more efficient. Of course that will mean that someone who intercepts a request can submit any alternative image which has the same sha1 signature...

In T292322#7636542, @Joe wrote:

But given in reality I was proposing to do something like:

signature = md5sum( secret + padding + request_body)

the attacker, in order to trick shellbox, would not need to match "signature", but rather to figure out "secret", which isn't how md5 has been broken. So I think it would be an actually valid way to assess the integrity of the request. Let me also note that depending on the length of the secret key, it is probably possible to brute-force guess it, given enough computational power, and yes using a faster hashing algorithm will reduce the computational cost of doing so.

It's possible to append to request_body and calculate a new signature without knowing the secret, you only need the previous signature, see https://en.wikipedia.org/wiki/Length_extension_attack . That's why the official HMAC algorithm is md5sum( secret + md5sum( secret + request_body ) ), it obscures the vulnerable hash output from the attacker. It doesn't change your point, but I thought it was worth mentioning.

Change 754578 merged by jenkins-bot:

[mediawiki/extensions/PagedTiffHandler@master] Use getSizeAndMetadata() and general style/maintenance updates

https://gerrit.wikimedia.org/r/754578

To further clarify how I conduct the no-timeout tests:

• sudo puppet agent --disable "T292322 --$USER"

/etc/envoy/envoy.yaml:

• Change the timeout for the local_port_80 listener in envoy to 600s
• Change the access log status filter for shellbox-media to >= 200, instead of >= 500

For php/apache we can use the following small script (to be run as root):

#!/bin/bash
pushd /etc/php/7.2/fpm
perl -i"" -pe 's/^(max_execution_time) = (\d+)$/$1 = 600/' php.ini`
perl -i"" -pe 's/^(request_terminate_timeout) = (\d+)$/$1 = 600/' pool.d/www.conf
popd
perl -i"" -pe 's/^(Timeout) \d+$/$1 600/' /etc/apache2/apache2.conf

Finally, go to /srv/mediawiki/wmf-config/CommonSettings.php and find the part where $wgRequestTimeLimit based on the http host. Add a line at the end of that block saying
$wgRequestTimeLimit = 600

At this point, restart php, apache and envoy.

So apparently my small fixes to PagedTiffHandler shaved off about 100 seconds off of the shellbox-based request, see:

https://performance.wikimedia.org/xhgui/run/view?id=61f8e59ac77d008fdbbbcc0a

This is still not enough though. Also, somehow we're still making what looks like 2 calls to Shellbox from retrievemetadata, for a total of more than 80 seconds spent there.

Of these 80 seconds:

• 55 were spent actually making the requests
• 25 were spent preparing the request
• Of the 55 seconds it took making the requests, about 51 were not spent by the backend elaborating the response, but rather by receiving the request with its 1.8 gb file attached. I'm going to drill through it to see if we might be able to improve on that.

I think this is still way too slow compared to the traditional request time.

I'll update this task later with my findings on the shellbox side once I have them, but I think we're trying to optimize a flawed mechanism - I don't think uploading files larger than a few 100 MBs should be a synchronous process.
The correct fix for this problem, and many others for large files uploaded via upload-via-url, is to make it an acynchronous process, have the jobqueue process the upload and have the upload page poll a backend for completion of the upload.

@tstarling I'm open to ideas on how to optimize the current flow though :)

In T292322#7666754, @Joe wrote:

I think this is still way too slow compared to the traditional request time.

I'll update this task later with my findings on the shellbox side once I have them, but I think we're trying to optimize a flawed mechanism - I don't think uploading files larger than a few 100 MBs should be a synchronous process.
The correct fix for this problem, and many others for large files uploaded via upload-via-url, is to make it an acynchronous process, have the jobqueue process the upload and have the upload page poll a backend for completion of the upload.

Agreed, that's T295007: Upload by URL should use the job queue, possibly chunked with range requests.

So after more rounds of tests:

• Special:UploadWizard still works just fine, given it's an async process it's completely unfazed by the transition to shellbox. It still takes about 1 hour to fully upload a 2 gb file, but that's not significantly longer than without shellbox involved
• upload-by-url didn't work for very large files because it times out already, we're just pushing the limit a bit lower. I don't know if this is acceptable though. The correct solution for this is anyways T295007 (thanks @Legoktm)
• I'll dig a bit more in the performance of shellbox itself as I don't understand why it's taking so long in envoy compared to the time it takes on its backend, at least looking at the envoy numbers. It would look like we are filling some other buffer somewhere, not necessarily in envoy.

After more digging: I have no idea why envoy would report the upstream time spent as 2 seconds, when it really is 20. Looks like a bug there. So: most of the time in a request is actually spent processing it in shellbox as I expected.

Two things we can try to improve:

1. give shellbox-media a bit more CPU per pod, and make the pods QoS instead of burstable
2. make shellbox use an host emptyDir as its temporary file storage (I need to check the code, but I guess that means /tmp as usual for php applications) so that we avoid the penalty of writing a large file to the docker filesystem with copy on write layer and all its indirections.

Change 762768 had a related patch set uploaded (by Giuseppe Lavagetto; author: Giuseppe Lavagetto):

[operations/deployment-charts@master] shellbox-media: use local disk for /tmp

https://gerrit.wikimedia.org/r/762768

In T292322#7710140, @Joe wrote:

After more digging: I have no idea why envoy would report the upstream time spent as 2 seconds, when it really is 20. Looks like a bug there. So: most of the time in a request is actually spent processing it in shellbox as I expected.

Two things we can try to improve:

1. give shellbox-media a bit more CPU per pod, and make the pods QoS instead of burstable

+1 on the more CPU, but as far as QoS, something's missing from the sentence. Do you want QoS Guaranteed or QoS best-effort ?

2. make shellbox use an host emptyDir as its temporary file storage (I need to check the code, but I guess that means /tmp as usual for php applications) so that we avoid the penalty of writing a large file to the docker filesystem with copy on write layer and all its indirections.

That might work, it's one of the caveats of write-heavy apps on devicemapper (or any storage driver for that matter). I 'd be interested to look at numbers pre and post changing this.

Change 762768 merged by jenkins-bot:

[operations/deployment-charts@master] shellbox-media: use local disk for /tmp

https://gerrit.wikimedia.org/r/762768

In T292322#7710927, @akosiaris wrote:

In T292322#7710140, @Joe wrote:

After more digging: I have no idea why envoy would report the upstream time spent as 2 seconds, when it really is 20. Looks like a bug there. So: most of the time in a request is actually spent processing it in shellbox as I expected.

Two things we can try to improve:

1. give shellbox-media a bit more CPU per pod, and make the pods QoS instead of burstable

+1 on the more CPU, but as far as QoS, something's missing from the sentence. Do you want QoS Guaranteed or QoS best-effort ?

D'oh, I forgot to add "guaranteed" of course.

2. make shellbox use an host emptyDir as its temporary file storage (I need to check the code, but I guess that means /tmp as usual for php applications) so that we avoid the penalty of writing a large file to the docker filesystem with copy on write layer and all its indirections.

That might work, it's one of the caveats of write-heavy apps on devicemapper (or any storage driver for that matter). I 'd be interested to look at numbers pre and post changing this.

Testing in a few minutes, actually :)

As I feared, no significant change is seen when using an host-mounted emptyDir in the container. I would assume the shellbox server spends most of its time running the hmac verification of the request, like the client does. We might want to try to convert shellbox to use the sha1 of the file to upload when computing hmac instead of the file contents.

@tstarling @Legoktm do you think we can enable this on commons as well? The only negative effect will be to push the max size of a file we can upload-by-url down from ~ 2 GB to (I guess) ~ 1.3 GB or so, basically making an already broken-by-design system a bit more broken. Else, we might need to work on optimizing shellbox instead.

In T292322#7711900, @Joe wrote:

@tstarling @Legoktm do you think we can enable this on commons as well? The only negative effect will be to push the max size of a file we can upload-by-url down from ~ 2 GB to (I guess) ~ 1.3 GB or so, basically making an already broken-by-design system a bit more broken. Else, we might need to work on optimizing shellbox instead.

Do we have an idea of how many uploads that would affect? I didn't see an easy way to distinguish upload-by-url uploads so I wrote/submitted https://gerrit.wikimedia.org/r/c/mediawiki/core/+/765681/. But I think we just bite the bullet and do it, the technical reasons for security are pretty strong, and unless someone has time to prioritize fixing upload-by-url I don't think it's worth waiting indefinitely for this.

In T292322#7737027, @Legoktm wrote:

In T292322#7711900, @Joe wrote:

@tstarling @Legoktm do you think we can enable this on commons as well? The only negative effect will be to push the max size of a file we can upload-by-url down from ~ 2 GB to (I guess) ~ 1.3 GB or so, basically making an already broken-by-design system a bit more broken. Else, we might need to work on optimizing shellbox instead.

Do we have an idea of how many uploads that would affect? I didn't see an easy way to distinguish upload-by-url uploads so I wrote/submitted https://gerrit.wikimedia.org/r/c/mediawiki/core/+/765681/. But I think we just bite the bullet and do it, the technical reasons for security are pretty strong, and unless someone has time to prioritize fixing upload-by-url I don't think it's worth waiting indefinitely for this.

There is apparently no consensus on proceeding across the technology department, so more discussions will be needed in other avenues. I will just leave this task open in case a consensus is reached. I agree with you that the additional security provided by the switch should in itself justify the move, but management across technology thinks the decision needs to be shared further.

Change 766572 had a related patch set uploaded (by Giuseppe Lavagetto; author: Giuseppe Lavagetto):

[operations/puppet@production] api: remove monitoring from http endpoint

https://gerrit.wikimedia.org/r/766572

Change 766572 merged by Giuseppe Lavagetto:

[operations/puppet@production] api: remove monitoring from http endpoint

https://gerrit.wikimedia.org/r/766572

@tstarling I think we determined that the expensive part of handling large files in shellbox was mostly the download/hash verification of the file received, I don't even think we strictly need PUT support.

In T292322#9614342, @Joe wrote:

@tstarling I think we determined that the expensive part of handling large files in shellbox was mostly the download/hash verification of the file received, I don't even think we strictly need PUT support.

Inverting control of the bulk data transfer allows us to maintain security despite skipping the hash verification.

For the record, the reason we wanted to support large file uploads was not to worsen the performance of upload-by-url, which has since been fixed by making the process asynchronous. Better performance handling large files would be welcome, though.

Change #1067546 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/libs/Shellbox@master] Add remote download/upload support for large file performance

https://gerrit.wikimedia.org/r/1067546

For configuration propagation, it turns out to be better to have the Command act as a factory, so we will have:

$command->inputFile( 'file.midi', $backend->getShellboxInputFile( $command, [ 'src' => $remoteSrc ] ) );

    function getShellboxInputFile( BoxedCommand $command, $params ) {
        return $command->newInputFileFromUrl( $this->getFileHttpUrl( ... ) );
    }

or perhaps winding it back altogether:

$backend->addShellboxInputFile( $command, 'file.midi', [ 'src' => $remoteSrc ] ) );

    function getShellboxInputFile( BoxedCommand $command, $name, $params ) {
        return $command->inputFileFromUrl( $name, $this->getFileHttpUrl( ... ) );
    }

Change #1069023 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/extensions/TimedMediaHandler@master] Allow Shellbox to download input files from Swift remotely

https://gerrit.wikimedia.org/r/1069023

Change #1069025 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] FileRepo: Add support for the new Shellbox large file feature

https://gerrit.wikimedia.org/r/1069025

Change #1067546 merged by jenkins-bot:

[mediawiki/libs/Shellbox@master] Add remote download/upload support for large file performance

https://gerrit.wikimedia.org/r/1067546

Change #1080833 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] Update to Shellbox 4.1.0

https://gerrit.wikimedia.org/r/1080833

Change #1080833 merged by jenkins-bot:

[mediawiki/core@master] Update to Shellbox 4.1.0

https://gerrit.wikimedia.org/r/1080833

Change #1069025 merged by jenkins-bot:

[mediawiki/core@master] FileRepo: Add support for the new Shellbox large file feature

https://gerrit.wikimedia.org/r/1069025

Change #1083954 had a related patch set uploaded (by Reedy; author: Tim Starling):

[mediawiki/core@REL1_43] FileRepo: Add support for the new Shellbox large file feature

https://gerrit.wikimedia.org/r/1083954

Change #1083954 merged by jenkins-bot:

[mediawiki/core@REL1_43] FileRepo: Add support for the new Shellbox large file feature

https://gerrit.wikimedia.org/r/1083954

Support for large file objects support should probably be added to ->quickStore.

Change #1069023 merged by jenkins-bot:

[mediawiki/extensions/TimedMediaHandler@master] Allow Shellbox to download input files from Swift remotely

https://gerrit.wikimedia.org/r/1069023

Change #1101239 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[operations/mediawiki-config@master] Enable canShellboxGetTempUrl

https://gerrit.wikimedia.org/r/1101239

Change #1101944 had a related patch set uploaded (by Scott French; author: Scott French):

[operations/deployment-charts@master] shellbox-video: allow egress to swift

https://gerrit.wikimedia.org/r/1101944

Change #1101944 merged by jenkins-bot:

[operations/deployment-charts@master] shellbox-video: allow egress to swift

https://gerrit.wikimedia.org/r/1101944

For a baseline performance benchmark, I reset the WebM 360P transcode of File:View of the Earth from the ISS (8K).webm

• Previous transcode in May 2022: 16 min 56 s (1016s)
• Job started 09:01:01.796089
• ffmpeg start time: 09:02:40
  • Couldn't find logs, used brute-force method sudo cumin 'wikikube-worker*' 'ps -C ffmpeg -ww -o lstart,args'
• Job completion: 09:24:16.527001
• Shellbox setup time: 98s
• Total time: 1395s

Change #1104398 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[operations/mediawiki-config@master] Enable canShellboxGetTempUrl on testwiki

https://gerrit.wikimedia.org/r/1104398

Change #1104398 merged by jenkins-bot:

[operations/mediawiki-config@master] Enable canShellboxGetTempUrl on testwiki

https://gerrit.wikimedia.org/r/1104398

Mentioned in SAL (#wikimedia-operations) [2024-12-16T23:17:35Z] <tstarling@deploy2002> Started scap sync-world: Backport for [[gerrit:1104398|Enable canShellboxGetTempUrl on testwiki (T292322)]]

Mentioned in SAL (#wikimedia-operations) [2024-12-16T23:23:11Z] <tstarling@deploy2002> tstarling: Backport for [[gerrit:1104398|Enable canShellboxGetTempUrl on testwiki (T292322)]] synced to the testservers (https://wikitech.wikimedia.org/wiki/Mwdebug)

Mentioned in SAL (#wikimedia-operations) [2024-12-16T23:29:36Z] <tstarling@deploy2002> Finished scap sync-world: Backport for [[gerrit:1104398|Enable canShellboxGetTempUrl on testwiki (T292322)]] (duration: 12m 00s)

Change #1104744 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[operations/deployment-charts@master] In shellbox set allowUrlFiles=true

https://gerrit.wikimedia.org/r/1104744

Change #1104744 merged by jenkins-bot:

[operations/deployment-charts@master] In shellbox set allowUrlFiles=true

https://gerrit.wikimedia.org/r/1104744

Mentioned in SAL (#wikimedia-operations) [2024-12-17T01:33:11Z] <swfrench-wmf> deployed shellbox-video to pick up config change for T292322

Change #1104746 had a related patch set uploaded (by Scott French; author: Scott French):

[mediawiki/libs/Shellbox@master] Add wmf-certificates to video variants

https://gerrit.wikimedia.org/r/1104746

Change #1104746 merged by jenkins-bot:

[mediawiki/libs/Shellbox@master] Add wmf-certificates to video variants

https://gerrit.wikimedia.org/r/1104746

Change #1105017 had a related patch set uploaded (by Scott French; author: Scott French):

[operations/deployment-charts@master] shellbox: release image 2024-12-17-061932

https://gerrit.wikimedia.org/r/1105017

Change #1105017 merged by jenkins-bot:

[operations/deployment-charts@master] shellbox: release image 2024-12-17-061932

https://gerrit.wikimedia.org/r/1105017

Mentioned in SAL (#wikimedia-operations) [2024-12-17T16:22:29Z] <swfrench-wmf> deployed shellbox 2024-12-17-061932 for T292322

I confirmed that this is working on testwiki.

Change #1101239 merged by jenkins-bot:

[operations/mediawiki-config@master] Enable canShellboxGetTempUrl everywhere

https://gerrit.wikimedia.org/r/1101239

Mentioned in SAL (#wikimedia-operations) [2025-01-06T03:17:54Z] <tstarling@deploy2002> Started scap sync-world: Backport for [[gerrit:1101239|Enable canShellboxGetTempUrl everywhere (T292322)]]

Mentioned in SAL (#wikimedia-operations) [2025-01-06T03:31:01Z] <tstarling@deploy2002> tstarling: Backport for [[gerrit:1101239|Enable canShellboxGetTempUrl everywhere (T292322)]] synced to the testservers (https://wikitech.wikimedia.org/wiki/Mwdebug)

Mentioned in SAL (#wikimedia-operations) [2025-01-06T03:39:47Z] <tstarling@deploy2002> Finished scap sync-world: Backport for [[gerrit:1101239|Enable canShellboxGetTempUrl everywhere (T292322)]] (duration: 21m 53s)