Re-add compression to BwdServer

[pbiggar]

This adds gzip (and also brotli!) to the BwdServer backend. That was pretty trivial.

However, testing it was not easy. I tried some different strategies. The first was just to add the zipped binary data that we expect. However, F# and OCaml produced different binary data (expected, since gzip can run at different settings), but that made testing hard.

So then I thought let's just decompress the data and check it's the expected output (which means we're putting the expected output in the test instead of the raw bytes). However, I ran into some problems decoding the bytes. The bytes are a set of transfer-encoding: chunked bytes. This means each chunk is has a header like this: 7\r\n and then 7 bytes (or whatever). It's supposed to end in 0\r\n but asp.net doesn't provide the final chunk header. Instead, I think we're supposed to use the closing of the connection to interpret that the stream is done.

This seems to work in chrome, but doesn't work with the library I added. I was on the way to changing the test to try fetching the body using the .NET HttpClient library (only in the event of content-encoding: gzip+transfer-encoding:chunked). I started refactoring to allow that.

Another option might be to just add the 0\r\n at the end of the bytes if they aren't already there. I suspect that would allow the current solution to work.

The tests are in a half-finished state, and do not represent the right input or output. The long tests are supposed to have enough output to trigger the nginx gzip (which kicks in at 1024 bytes). I don't know when the asp.net gzip kicks in.

Note that the nginx we use doesn't support brotli, so that might complicate testing (asp.net does). Perhaps we only support gzip for now and enable brotli later? Or just do an FSHARPONLY test.

[StachuDotNet]

Allow running the Expecto --debug flag without triggering lldb

Can you explain this a bit more? I'm not super familiar with lldb and why we'd want to avoid it.

[pbiggar]

Allow running the Expecto --debug flag without triggering lldb

Can you explain this a bit more? I'm not super familiar with lldb and why we'd want to avoid it.

Expecto has its own --debug flag that we may want, probably more commonly than lldb, which is more useful for low-level memory issues. But --debug called lldb, when I really just wanted it to be passed through as a flag to expecto.

[pbiggar]

I've gotten stuck here, and having a few issues:

issue one

If I run

./scripts/run-fsharp-tests --filter-test-case request-header-accept-encoding

with this
diff.txt, I get a failure about 70% of the time:

[10:47:00 ERR] tests.BwdServer.httptestfiles.Httpfiles: request-header-accept-encoding-many.test errored in 00:00:00.6600000 <Expecto>
Expecto.AssertException: (FSharp as string).
expected:
("HTTP/1.1 200 OK",
 [("Access-Control-Allow-Origin", "*"); ("Content-Encoding", "gzip");
  ("Content-Type", "text/plain; charset=utf-8");
  ("Date", "xxx, xx xxx xxxx xx:xx:xx xxx"); ("Server", "darklang");
  ("Strict-Transport-Security", "max-age=31536000; includeSubDomains; preload");
  ("Transfer-Encoding", "chunked"); ("Vary", "Accept-Encoding");
  ("x-darklang-execution-id", "0123456789")], "
"2019-09-07T22:44:25Z"")
  actual:
("HTTP/1.1 200 OK",
 [("Access-Control-Allow-Origin", "*"); ("Content-Encoding", "gzip");
  ("Content-Type", "text/plain; charset=utf-8");
  ("Date", "xxx, xx xxx xxxx xx:xx:xx xxx"); ("Server", "darklang");
  ("Strict-Transport-Security", "max-age=31536000; includeSubDomains; preload");
  ("Transfer-Encoding", "chunked"); ("Vary", "Accept-Encoding");
  ("x-darklang-execution-id", "0123456789")], "")

It's always with that test, never any other test. And it doesn't fail if I run just the request-header-accept-encoding-star.test test.

I did some debugging on this and I determined that this is happening between the dark server sending a response and the test suite receiving it. So probably in the compression middleware?

What's happening AFAICT is that the response is being truncated. It starts the same as expected but then truncated unexpectedly after a number of bytes (possibly the first transfer-encoding chunk?)

It may be necessary to report it upstream and wait til it gets fixed.

issue 2

I'm a little sketch about compressing every bwdserver response. Maybe I'm wrong about that, I guess it just feels wrong to compress everything.

To handle this I started Compression.fs, which is a custom ICompressionProvider that check the response ContentLength before compressing. The rule seems to work, but the compression doesn't though. It's possible that we also need to inherit from the middleware, but I couldn't see what to do here.

issue 3

I think we need more tests, probably at least one that actually does compression for each of the request-header-accept-encoding-*.test files. And of course the current tests should pass.

[StachuDotNet]

Still processing this, but I found this to be a worthwhile read on the topic of "compressing every bwdserver response" - mostly, the last bit.

[StachuDotNet]

here's a quick dump of public F# repos using AddResponseCompression - maybe there's something to learn here (e.g. an example of testing working)

For example, web frameworks like Saturn and Falco are public, and potentially have tests around this.

Edit: worth a shot, but reviewed this and didn't find anything worthwhile.

[pbiggar]

We might also say that we don't want https for bwdserver because of BREACH: https://www.breachattack.com/

My read is that compressing before encrypting will allow attackers to guess some contents in some cases. Since users will have programs that support those use cases (such as csrf and cookies), and won't be able to turn off compression, perhaps we shouldn't compress at all.

[pbiggar]

https://security.stackexchange.com/questions/222677/how-are-websites-actually-mititating-breach-https-compression

What sticks out the most is that both attacker-supplied data and the client secret need to be in the same response. Furthermore, the attacker needs to be able to cause the client to receive a lot of responses, and all of them need to contain the modified input from the attacker.

Of course, this might be up to the user.

[StachuDotNet]

perhaps we shouldn't compress at all.

At the very least, I'm not convinced that compression is a blocker for go-live of BwdServer, given the risks to weigh there.

[StachuDotNet]

Regarding

What's happening AFAICT is that the response is being truncated.

By what - our testing set-up, or something earlier? (conclusion might not be there yet, suspicion fine)

edit: nevermind, saw some details in your earlier comments.

[StachuDotNet]

Somehow, ./scripts/run-fsharp-tests --filter-test-case request-header-accept-encoding ran fine for me 10x in a row, after applying your diff locally (and making no other changes). Not sure what to make of that

[StachuDotNet]

Regarding "I'm a little sketch about compressing every bwdserver response," using CompressionLevel.Fastest universally would remove my worry there, beyond the noted security risks. I don't fully comprehend those security risks yet.

[pbiggar]

Regarding "I'm a little sketch about compressing every bwdserver response," using CompressionLevel.Fastest universally would remove my worry there, beyond the noted security risks. I don't fully comprehend those security risks yet.

Pretty sure that's the default. It's more that it doesn't add much to a 5 bytes response.

[StachuDotNet]

@pbiggar I'm curious if you've had any thoughts on this recently?

[pbiggar]

Yeah, I had a think about the security implications. From reading around, the general consensus is that compression is important and BREACH requires enough convergence of circumstances that it's ok to require customers to provide the mitigations should they choose (such as adding a random amount of whitespace to their JSON responses, and being careful where they put CSRFs and user replaying data the user provides). In the future, we could choose to add various mitigations for customers if we wanted.

So let's enable it for now, at Fastest.

[pbiggar]

This could be taken on by a motivated contributor. There have been a lot of changes since then so it will probably be easier to port the functionality than to merge/rebase. The only thing remaining to make this PR work was testing, and we've since added testing features to allow us to test using binary files, so this should be easy now.