Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-add compression to BwdServer #3664

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft

Re-add compression to BwdServer #3664

wants to merge 15 commits into from

Conversation

pbiggar
Copy link
Member

@pbiggar pbiggar commented Apr 8, 2022

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
Copy link
Member

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
Copy link
Member Author

pbiggar commented Apr 8, 2022

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 pbiggar changed the title Re add compression Re-add compression Apr 9, 2022
@StachuDotNet StachuDotNet requested review from StachuDotNet and removed request for StachuDotNet April 11, 2022 14:21
@pbiggar pbiggar force-pushed the paul/re-add-compression branch 2 times, most recently from 6b78517 to c26c52f Compare April 27, 2022 16:50
@pbiggar
Copy link
Member Author

pbiggar commented Apr 28, 2022

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
Copy link
Member

StachuDotNet commented Apr 28, 2022

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
Copy link
Member

StachuDotNet commented Apr 28, 2022

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
Copy link
Member Author

pbiggar commented Apr 28, 2022

We might also say that we don't want https for bwdserver because of BREACH: http://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
Copy link
Member Author

pbiggar commented Apr 28, 2022

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
Copy link
Member

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
Copy link
Member

StachuDotNet commented Apr 28, 2022

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
Copy link
Member

StachuDotNet commented Apr 28, 2022

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
Copy link
Member

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
Copy link
Member Author

pbiggar commented Apr 29, 2022

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
Copy link
Member

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

@pbiggar
Copy link
Member Author

pbiggar commented May 8, 2022

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 pbiggar added the temporarily-abandoned We'll come back to this, but it isn't a high priority right now. label Dec 1, 2022
@pbiggar
Copy link
Member Author

pbiggar commented Mar 23, 2023

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.

@StachuDotNet StachuDotNet changed the title Re-add compression Re-add compression to BwdServer Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
temporarily-abandoned We'll come back to this, but it isn't a high priority right now.
Projects
Status: Ready to go
Development

Successfully merging this pull request may close these issues.

2 participants