1484873 - link to the site's certificate on error pages where it makes sense to do so

Status: VERIFIED FIXED

(Firefox :: Security, enhancement, P1)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

In our error pages, we can (and should) link directly to the site's certificate (where "link" means open the certificate viewer for now). The errors where it makes sense to do so are:

SEC_ERROR_UNKNOWN_ISSUER
SSL_ERROR_BAD_CERT_DOMAIN
SEC_ERROR_EXPIRED_CERTIFICATE
MOZILLA_PKIX_ERROR_ADDITIONAL_POLICY_CONSTRAINT_FAILED

and potentially others, but basically none of the SEC_ERROR_OCSP_... ones or errors that indicate that the handshake failed entirely (in which case we won't have a certificate anyway).

Comment 1

[Meridel [:meridel]]

The string for those links is: View the website’s certificate.

Comment 2

[AJ Jordan [:strugee] (he/they)]

Attachment: link-to-cert-on-error.patch

I've written a preliminary patch for this bug. I've manually tested it and it works well. That being said, it opens the "Page Info" dialog instead of going directly to the "View Certificate" window and I'm not sure if that's right; it also adds a button instead of a link. It's super late but I can upload a screenshot in the morning.

Also, it lacks tests. I assume they're needed?

Comment 3

[AJ Jordan [:strugee] (he/they)]

Attachment: fix-typo.patch

Also here's a patch for a random typo I found - wasn't really sure where else to put it since I can't check things into the tree myself.

Comment 4

[AJ Jordan [:strugee] (he/they)]

Attachment: screenshot

Screenshot after I clicked the "More..." button. "Show Certificate" opens the Page Info dialog.

Comment 5

[Bram Pitoyo [:bram]]

Comment on attachment 9007483 [details]
screenshot

Hi AJ,

I believe that we will have a link – instead of a button – called “View the website’s certificate”, located underneath the error details.

https://drive.google.com/file/d/1-aiqUi9hOzr5RfY4yk02tDCr7GiXEN_6/view?usp=sharing


This link also replaces the error code on these errors:

* UNKNOWN_ISSUER
* BAD_CERT_DOMAIN
* EXPIRED_CERTIFICATE
* Symantec TLS Certificate errors


However, on these errors, the error code remains:

* OCSP_INVALID_SIGNING_CERT
* OCSP_FUTURE_RESPONSE


Source: https://docs.google.com/document/d/1PGZFR5fpXAjwJ8B3zg7wmZENYrDL3gWj9qmetJC2XFk/edit?ts=5b913ab2#heading=h.lbq403gyejq6


Meridel can help clarify it further. Thanks!

Comment 6

[AJ Jordan [:strugee] (he/they)]

(In reply to Bram Pitoyo [:bram] from comment #5)

> I believe that we will have a link – instead of a button – called “View the
> website’s certificate”, located underneath the error details.

Great, thanks for getting back to me! Much appreciated.

> https://drive.google.com/file/d/1-aiqUi9hOzr5RfY4yk02tDCr7GiXEN_6/
> view?usp=sharing

Is it possible for this be shared publicly? I don't have access since I'm not MoCo. (Ditto for the Google Docs link.)

> This link also replaces the error code on these errors:
> 
> * UNKNOWN_ISSUER
> * BAD_CERT_DOMAIN
> * EXPIRED_CERTIFICATE
> * Symantec TLS Certificate errors

The error codes here are already links that open up a pane with technical info and a "Copy to clipboard" button. If they get replaced, how is the user supposed to get to this technical information?

Side note, can someone assign this bug to me? I don't have editbugs on BMO.

Comment 7

[Bram Pitoyo [:bram]]

Sorry for(In reply to AJ Jordan [:strugee] (he/they) from comment #6)
> Is it possible for this be shared publicly? I don't have access since I'm
> not MoCo. (Ditto for the Google Docs link.)

I’ve updated the “User Story” field with the link to the publicly accessible InVision mockups.

Comment 8

[Meridel [:meridel]]

Re: "The error codes here are already links that open up a pane with technical info and a "Copy to clipboard" button. If they get replaced, how is the user supposed to get to this technical information?"

It was my understanding that the current error codes link to the certificate, so we are only changing the text for the link. I thought the "technical info" WAS the certificate. Dana, is this not true?

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

The technical info includes the certificate and other information that may be helpful in debugging, so I think we want to keep all that. The problem is that the certificate in that information is encoded, so it's not immediately useful to anyone. Eventually we probably want that pane to be more informative (e.g. actually decode and maybe offer hints as to why the certificate didn't verify), but we don't have that implemented, much less a design for how it would look/work. In the meantime, I think what people are generally looking for is a way to view the contents of the certificate. Right now the only way we have to do that in product is with the certificate viewer, which is a separate dialog (right now you can see it in action by clicking the site identity drop-down, then the right arrow, then "More Information", then "View Certificate"). So, my understanding is we would be adding a distinct button that opens the certificate viewer.

Comment 10

[Johann Hofmann [:johannh]]

Hi AJ,

sorry for getting back to you so late, I'm probably the person who should have been mentoring this bug from the start. Your patch looks like a great start, thanks for digging into this. There are a few things I'd like to get done before this is ready to land:

- As noted above, it should be a text link

- IMO, we should really try to open the certificate viewer directly, you can do this in the following way:
https://searchfox.org/mozilla-central/rev/bdc89dfd7869e418d788b28eb60ab8d94e708a15/browser/base/content/pageinfo/security.js#336
. Simply use the securityInfo.serverCert as the cert in that dialog.

- Is there are reason for only showing the link for these specific error codes? I think you can always show it.

- I would really like to have at least basic tests for this, which might be a bit intimidating for a new Firefox developer, but I'm happy to help. Most of the tests for this page currently live here: https://searchfox.org/mozilla-central/source/browser/base/content/test/about/browser_aboutCertError.js

Are you still interested in this bug? Please let me know if you need any help!

Comment 11

[AJ Jordan [:strugee] (he/they)]

(In reply to Johann Hofmann [:johannh] from comment #10)

> sorry for getting back to you so late, I'm probably the person who should
> have been mentoring this bug from the start. Your patch looks like a great
> start, thanks for digging into this. There are a few things I'd like to get
> done before this is ready to land:

Hi, thanks! My apologies too for responding so late; I kept meaning to come back to this bug but before I could my laptop ended up needing repair.

> - As noted above, it should be a text link
> 
> - IMO, we should really try to open the certificate viewer directly, you can
> do this in the following way:
> https://searchfox.org/mozilla-central/rev/
> bdc89dfd7869e418d788b28eb60ab8d94e708a15/browser/base/content/pageinfo/
> security.js#336
> . Simply use the securityInfo.serverCert as the cert in that dialog.

Thanks for the pointer, that's *super* helpful. It was on my list but I would've taken forever to find the right way.

> - Is there are reason for only showing the link for these specific error
> codes? I think you can always show it.

I was just following comment #0. Happy to amend.

> - I would really like to have at least basic tests for this, which might be
> a bit intimidating for a new Firefox developer, but I'm happy to help. Most
> of the tests for this page currently live here:
> https://searchfox.org/mozilla-central/source/browser/base/content/test/about/
> browser_aboutCertError.js

Great. Will needinfo you or ask #introduction if I get stuck.

> Are you still interested in this bug? Please let me know if you need any
> help!

I'm definitely still interested! However as I said above my laptop's still getting repaired. I would love to finish this up but I don't want to block any work so if someone wants to take my patch and run with it, that's fine too.

Comment 12

[Johann Hofmann [:johannh]]

Attachment: Bug 1484873 - Add a "View Certificate" link to all cert error pages. r=nhnt11

Comment 13

[Johann Hofmann [:johannh]]

Hi Alex, thank you for the efforts! Since this bug is supposed to go into 64 still I'll take it from here. Please let me know when you're ready to contribute again, since I would be happy to forward other patches to you.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/868c99ece921
Add a "View Certificate" link to all cert error pages. r=nhnt11

Comment 15

[Eliza Balazs [:ebalazs_]]

Backed out changeset 868c99ece921 (Bug 1484873) for ES lint failure /builds/worker/checkouts/gecko/browser/base/content/browser.js CLOSED TREE

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=868c99ece921edeec1b06259ee4060d35adeceea&selectedJob=206101656

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=206101656&repo=autoland&lineNumber=278

Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=29c0fc82e879891cd13464dc028c1d1c4da14ece

Comment 16

[Johann Hofmann [:johannh]]

Already relanded!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55c7172f12a4
Add a "View Certificate" link to all cert error pages. r=nhnt11

Comment 18

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c175c006e65
Backed out changeset 868c99ece921 for ES lint failure /builds/worker/checkouts/gecko/browser/base/content/browser.js CLOSED TREE

Comment 19

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/55c7172f12a4

Comment 20

[Cristian Fogel [:cfogel]]

Tried to verify this fix with 64.0b4 but the changes don't seem to have been introduced in this version.
With 64.0a1(20181018123730) the changes are in effect. 

Is it something expected for the current beta build?

Comment 21

[Johann Hofmann [:johannh]]

(In reply to Cristi Fogel [:cfogel] from comment #20)
> Tried to verify this fix with 64.0b4 but the changes don't seem to have been
> introduced in this version.
> With 64.0a1(20181018123730) the changes are in effect. 
> 
> Is it something expected for the current beta build?

Yes, you need to flip the browser.security.newcerterrorpage.enabled pref to see it for now. We're still figuring out the best release process for this.

Comment 22

[Cristian Fogel [:cfogel]]

Thank you.
Verified with 64.0b5 on Win10, macOS 10.12, Ubuntu16.04.

Tested example pages for the errors mentioned in comment 0

SSL_ERROR_BAD_CERT_DOMAIN
SEC_ERROR_EXPIRED_CERTIFICATE
MOZILLA_PKIX_ERROR_ADDITIONAL_POLICY_CONSTRAINT_FAILED
SEC_ERROR_UNKNOWN_ISSUER

Just as a heads-up, the last 2 do not have the yellow border as the rest. Is that intended or tracked in another issue?

Comment 23

[Johann Hofmann [:johannh]]

(In reply to Cristi Fogel [:cfogel] from comment #22)
> Thank you.
> Verified with 64.0b5 on Win10, macOS 10.12, Ubuntu16.04.
> 
> Tested example pages for the errors mentioned in comment 0
> 
> SSL_ERROR_BAD_CERT_DOMAIN
> SEC_ERROR_EXPIRED_CERTIFICATE
> MOZILLA_PKIX_ERROR_ADDITIONAL_POLICY_CONSTRAINT_FAILED
> SEC_ERROR_UNKNOWN_ISSUER
> 
> Just as a heads-up, the last 2 do not have the yellow border as the rest. Is
> that intended or tracked in another issue?

I think that's intended, assuming that the SEC_ERROR_UNKNOWN_ISSUER was not overridable (HSTS enabled)