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

Bitdefender "Scan SSL" setting strips transfer-encoding headers from streaming responses #277

Closed
mattheworiordan opened this issue May 18, 2016 · 6 comments
Labels
bug Something isn't working. It's clear that this does need to be fixed. investigate

Comments

@mattheworiordan
Copy link
Member

A customer could not make XHR requests to Ably as it appears BitDefender Firewall for PC was interfering with the requests and possibly stripping out some headers.

We should see if we can reproduce and implement a workaround if possible.

@mattheworiordan mattheworiordan added bug Something isn't working. It's clear that this does need to be fixed. investigate labels May 18, 2016
@SimonWoolf
Copy link
Member

SimonWoolf commented May 18, 2016

fwiw looking at the xhr code, my guess is we'll find it's stripping the transfer-encoding header. That is workaroundable, but should reproduce in a VM to make sure it is that first

@SimonWoolf
Copy link
Member

SimonWoolf commented May 18, 2016

Installed Bitdefender in a Win10 VM, and: Confirmed. Bitdefender is stripping transfer-encoding, and adding content-length.

Before:

HTTP/1.1 200 OK
Access-Control-Allow-Credentials: true
Access-Control-Allow-Origin: http://localhost:8000
Access-Control-Expose-Headers: Link,Transfer-Encoding,Content-Length,X-Ably-ErrorCode,X-Ably-ErrorMessage,X-Ably-ServerId
Content-Type: application/json
Date: Wed, 18 May 2016 19:04:50 GMT
Set-Cookie: ably_sessionid=1082b; path=/comet; httponly
Vary: Origin
X-Ably-Serverid: frontend.1be5.eu-central-1-A.i-305dca8c
X-Content-Type-Options: nosniff
Transfer-Encoding: chunked

After

HTTP/1.1 200 OK
Access-Control-Allow-Credentials: true
Access-Control-Allow-Origin: http://10.0.2.2:8000
Access-Control-Expose-Headers: Link,Transfer-Encoding,Content-Length,X-Ably-ErrorCode,X-Ably-ErrorMessage,X-Ably-ServerId
Content-Type: application/json
Date: Wed, 18 May 2016 19:04:19 GMT
Set-Cookie: ably_sessionid=1082b; path=/comet; httponly
Vary: Origin
X-Ably-Serverid: frontend.1be5.eu-central-1-A.i-305dca8c
X-Content-Type-Options: nosniff
Content-Length: 396

Experimenting further: the culprit is the "Scan SSL" setting in the "Web Protection module". This setting appears to work by MITMing all ssl traffic through a local proxy (using a new root-level certificate it installs). Said proxy does not understand streaming responses, messes with headers, breaks EV certificates, and uses an outdated and insecure set of cyphersuites (howsmyssl.com goes from a 'probably okay' rating with it off to a 'bad' rating with it on).

We could hack around this by having ably-js switch from using transfer-encoding and content-type to some new custom header that we'd get realtime to send to detect whether response is streamed. (We can't just catch the exception because it isn't thrown until the first heartbeat is sent, as that's the first time that the total response viewed as a single json blob is invalid json). But that feels wrong, the transfer-encoding header is the correct way of detecting streaming. I'm half-tempted to take the high ground and to just have a kb article (and a console msg if it detects that exception) telling people to disable bitdefender ssl scanning if they have this problem since it's so clearly doing the wrong thing here... WDYT @mattheworiordan @paddybyers ?

@SimonWoolf SimonWoolf changed the title Bitdefender work around Bitdefender "Scan SSL" setting strips transfer-encoding headers from streaming responses May 18, 2016
@paddybyers
Copy link
Member

WDYT

mitm proxies are unfortunately a fact of life. Our approach needs to be to make something work, by falling back to a less demanding transport, whilst documenting the badness you get to discourage their use. I don't think we should try to work around by tunnelling custom headers or anything like that.

So presumably we can fall back to xhr polling? So then the question is whether it's the client or server that can make the determination. Unfortunately we can't detect the cert issuer at the client, because that would be a way to detect it.

breaks EV certificates

I would say that it breaks all certs

@mattheworiordan
Copy link
Member Author

So presumably we can fall back to xhr polling?

But from what I could tell, WebSockets were working with BitDefender. Is that not the case @SimonWoolf? If so, we have a far superior transport we can use instead.

@SimonWoolf
Copy link
Member

SimonWoolf commented May 19, 2016

So then the question is whether it's the client or server that can make the determination.

If you have ideas about how to detect this with either client or server, I'm all ears. Currently I can't think of any sensible way of doing it with the client.

I was wrong about being able to do it with a custom header - testing now, it looks like no headers or data arrives, no xhr.onreadystatechange event fires, until 60s has passed and realtime closes the stream (at which point it all comes at once, and we get the parsing failure). Up until that point, as far as the xhr transport is concerned, the request behaves identically to a non-streaming response that just happens to take a very long time to complete. (Which presumably is exactly what the crappy proxy thinks it is, and that thing is holding the streaming data until the request completes).

So we could detect it after 60s and switch transport, but that's obviously not very good. I guess we could also try assuming that this is happening if the first 10s realtimeRequestTimeout expires, switch to xhr polling, and try again, which would cut that down to 10s. But would be pretty messy, mean an unnecessary downgrade for clients who support streaming but just had a temporary network failure, and a 10s pause is still not that great :/

So presumably we can fall back to xhr polling?

But from what I could tell, WebSockets were working with BitDefender.

Sure, but that doesn't solve the problem of detecting this issue - unless we decide to revive #217 and switch to websockets+downgrade rather than comet+upgrade. (And even then, still have the detection problem for browsers that don't support websockets).

@SimonWoolf
Copy link
Member

Fixed by #279

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working. It's clear that this does need to be fixed. investigate
Development

No branches or pull requests

3 participants