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

Gracefully handle certificate renewals #391

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
2 participants
@lukas-w
Copy link
Contributor

commented Dec 4, 2018

This adds support for reading the renewal information from digital-certificate.txt that will be generated by the website. The renewal information contains a signature that is verified using the intermediate certificate's public key. If a certificate is a renewal and the previous certificate is whitelisted, the renewal will be whitelisted automatically. Same goes for blacklists.

Unfortunately, because of the way certificates used to be parsed, the new certificates are not backwards compatible. This means that certificates containing renewal information will be recognised as untrusted by Tray versions that don't contain this PR's patch f66606b. The current implementation is not able to ignore additional data at the end of the file, so parsing the intermediate certificate will fail in this case. I'm not sure about the severity of this. Is a warning on the website's certificate generation page sufficient? Something along the lines of "Be sure to update QZ Tray to version 2.0.9 or later to use any newly generated certificates".

lukas-w added some commits Dec 4, 2018

Certificate: Don't decode Base64 certificate for forward compatibility
In the future, certificates will contain additional data after the end of
the intermediate certificate (see #43). By using the Base64 string directly
instead of decoding it ourselves, CertificateFactory can detect the
"-----BEGIN CERTIFICATE-----" and "-----END CERTIFICATE-----" boundaries,
ignoring anything outside.
Certificate: Add support for reading renewal information
Reads renewal information from the certificate as generated on the website.
This is used to automatically white- or blacklist a renewed certificate if
the certificate this one is a renewal of was already white- or blacklisted.

Fixes #43
@tresf

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2018

If a certificate is a renewal and the previous certificate is whitelisted, the renewal will be whitelisted automatically. Same goes for blacklists.

Clarification... are you saying you blacklist if the previous cert was blacklisted? I'd like this logic removed. Sometimes when a client clicks "block" it's because the certificate was renewed but a caching issue is serving up the old cert and people get impatient and click "Always block". Once available, the new cert shouldn't be blocked as it will cause more support issues. Raising the dialog is fine, which would mimic the behavior today.

@lukas-w

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

are you saying you blacklist if the previous cert was blacklisted? I'd like this logic removed.

Removed via 305b2c4.

@tresf

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2018

Something along the lines of "Be sure to update QZ Tray to version 2.0.9 or later to use any newly generated certificates".

This would be a huge problem. Backwards compatibility at renewal time is paramount and the confusion of adding two certificate downloads is not ideal.

This is going to look very confusing, but how about we do something like follows...

  -----BEGIN CERTIFICATE-----
  MIIEyCBD...
 --START INTERMEDIATE CERT--
 MIIFEjCCA...
+  --START INTERMEDIATE CERT--
+  --START RENEWAL INFO--
+  AAABBBB...
+  --START RENEWAL SIGNATURE--
+  AAABBBB...

This will allow the split[0] and split[1] to continue working and then we can phase out the invalid --START INTERMEDIATE CERT-- with the improved base code (and eventually remove it from the portal with warning once we believe most of our clients have upgraded).

@tresf

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2018

Nevermind.... if (split.length == 2) { kills that workaround. :(

@tresf

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2018

@lukas-w let's switch back to the idea of storing this information inside the certificate data itself. It will fix the backwards compatibility issues with certificate parsing.

I found a field that is very liberal about what it allows... SAN. SubjectAlternateName is common on SSL certs for the various domains that are allowed. I would much rather use a private extended attribute out of courtesy of the x509 specification but finding documentation for this outside of the RFC has been very difficult.

Here's an example of a SAN URI that can contain the fingerprint.

#3: ObjectId: 2.5.29.17 Criticality=false
SubjectAlternativeName [
  URIName: qz:renew/43:51:43:a1:b5:fc:8b:b7:0a:3a:a9:b1:0f:66:73:a8
]

I'm not proud of this, but forge should be able to write it and Java should be able to read it without the backwards compatibility issues. Thoughts welcome.

@lukas-w

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

Ok, I'll try this and open a new pull request.

@lukas-w lukas-w closed this Dec 12, 2018

@lukas-w lukas-w deleted the lukas-w:cert-renewal branch Dec 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.