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

Transient Get Certificate Issues #219

Closed
dsanders11 opened this issue Aug 1, 2017 · 9 comments
Closed

Transient Get Certificate Issues #219

dsanders11 opened this issue Aug 1, 2017 · 9 comments
Assignees
Labels
Milestone

Comments

@dsanders11
Copy link
Contributor

dsanders11 commented Aug 1, 2017

It appears that the default behavior of callCert in the JavaScript is not great in how it handles transient errors, such as Internet connectivity issues. It causes behavior that is very unforgiving.

Issue:

  1. Certificate promise is set to be a jQuery AJAX call like the example on the wiki
  2. The AJAX call fails due to Internet connection issues, and rejects the promise.
  3. qz-tray.js executes sendCert even for a rejected callCert promise, causing the rejected promise from jQuery to be sent to QZ Tray (likely over the local network which won't have connection issues). This means QZ Tray sees a JSON message that has something like {"certificate": {"readyState": 0, "responseText": "error"}} (snipped out the irrelevant details).
  4. That obviously fails to parse as a certificate on QZ Tray's end so it causes the UNKNOWN certificate to be displayed and forces the user to allow or deny the connection request.
  5. This QZ Tray instance is now effectively "bricked" until the dialog is cleared. We've been trying to use a Raspberry Pi with nothing but a printer connection and WiFi, but this issue forces us to restart it on a regular basis.

Suggested Resolution:

  1. Change the default behavior of getCert to use resolve(), not reject() like it currently does.
  2. Don't catch and call sendCert if callCert rejects and instead actually reject the open connection promise.
  3. Find a way to differentiate which kind of error occurred so that retry logic can be written if callCert is what failed, instead of sendCert. Maybe something like: _qz.security.callCert().catch(rejectAsCallCertFailure).then(sendCert).catch(rejectAsSendCertFailure);

Rationale for the suggested resolution is the connection failure due to not being able to get the certificate should happen on the browser side, not on the QZ Tray side which is what currently happens due to the bad certificate info which can't be parsed. At least in that case the error can be recovered from on the individual browser instead of bricking the instance.

BTW, this also highlights a way to do a denial-of-service on a QZ Tray instance which is running with trusted certificates (and so never shows dialogs). If you provide gibberish for the certificate info you'll force the dialog to be shown and grind everything to a halt until it's cleared. IMO it should be possible to easily configure QZ Tray to auto-deny any untrusted certificates so that this wouldn't be possible. Looks like this is already provided via block anonymous requests. Would this also block a parseable certificate that's untrusted? In that case it's not an anonymous request, I think.

@tresf, thoughts?

@tresf
Copy link
Contributor

tresf commented Aug 1, 2017

@dsanders11 2.1 removes the dialog in headless mode, which should decrease the severity of this: https://qz.io/wiki/2.1-Headless-Deployment

Also, blocking anonymous connections is already part of the UI, so you should be able to mitigate the UNKNOWN issues by setting that preference. This will add an entry to blocked.dat that will look something like this:

UNKNOWN REQUEST	An anonymous request	Unknown	0002-11-30 00:00:00	0002-11-30 00:00:00	false

@tresf
Copy link
Contributor

tresf commented Aug 1, 2017

Would this also block a parseable certificate that's untrusted?

Currently, we only check the generic UNKNOWN for being blocked as the parsable ones would have a blacklist-able fingerprint (so each one would have to be blocked individually). If a DOS-er is generating unique certificates fast enough to spam the requests at the server and if the server is not running in headless mode, it could halt productivity from that instance, the dialog becomes the bottleneck. This may call for a spam prevention utility, similar to how Chrome has the "stop showing these" on JavaScript alert boxes -- at least for the UI implementations. It may also call for a "single cert" mode. Print-servers running in headless mode would be OK though. Regardless, this is a good find.

@dsanders11
Copy link
Contributor Author

If a DOS-er is generating unique certificates fast enough to spam the requests at the server and if the server is not running in headless mode, it could halt productivity from that instance, the dialog becomes the bottleneck.

Even one certificate is enough to throw a decent wrench in things. Even with the second instance we have which is on a Windows computer with monitor and keyboard/mouse, it's unmanned most of the time so no one will notice the dialog sitting there until there's problems printing (from the front office, a ways away from the computer running QZ Tray).

@tresf
Copy link
Contributor

tresf commented Aug 1, 2017

Yeah we need to rethink how we play traffic cop to allow known, good traffic through first.

@Vzor-
Copy link
Contributor

Vzor- commented Aug 24, 2017

👍 On it!

@tresf
Copy link
Contributor

tresf commented Aug 25, 2017

@klabarge can you reach out to @Vzor- tonight with some help getting the trusted certificate setup?

@klabarge
Copy link
Member

klabarge commented Aug 25, 2017

@tresf @Vzor-

@klabarge can you reach out to @Vzor- tonight with some help getting the trusted certificate setup?

Done.

@tresf tresf added this to the 2.0.5 milestone Sep 19, 2017
@tresf tresf added the bug label Sep 19, 2017
@tresf
Copy link
Contributor

tresf commented Sep 28, 2017

Don't catch and call sendCert if callCert rejects and instead actually reject the open connection promise.

This may seem logical in principle, but in practicality, this has the side effect of blocking printing when the cert promise is broken. Most clients want to have a fallback in this scenario and that is the logic we'll be keeping.

This QZ Tray instance is now effectively "bricked" until the dialog is cleared. We've been trying to use a Raspberry Pi with nothing but a printer connection and WiFi, but this issue forces us to restart it on a regular basis.

This has been patched in #242. The fix is so small that we're going to rebase it against 2.0.5. If you can help us test this out, that would be great.

I don't mean to dismiss the other great points made in the original bug report, but I think this "denial of service" bug is serious enough to roll out in the stable release and I'd like to close this bug report out once merged.

differentiate which kind of error occurred so that retry logic can be written

Likewise, this could be farmed out to the cert promise, since it's self-aware of its own resolve()/reject() status and can do it's own retry/failure. We'd be happy to track this in a separate bug report if you'd like.

@tresf
Copy link
Contributor

tresf commented Oct 11, 2017

I'm closing this via #242. If there are issues with the implementation, please reopen. There are several other good points in the conversation above. If they warrant a new issue, please feel free to create one for each request/bug. Thanks for reporting this.

Edit: 2.0.5 has been released with this fixed. You can grab it at https://qz.io/download

@tresf tresf closed this as completed Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants