The new about:certificates incorrectly reports the OCSP Must-Staple extension
Categories
(Firefox :: Security, defect, P1)
Tracking
()
People
(Reporter: eddiecarswell13, Assigned: carolina.jimenez.g)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
47 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0
Steps to reproduce:
- Browse to a site with the OCSP Must-Staple extension in the x509 certificate.
- Check the site's certificate in about:certificates, and look for the OCSP Stapling section.
Example: Compare the about:certificates view of dm4productions.com to the parsed certificate from Censys.
Actual results:
The about:certificates page incorrectly reports that OCSP stapling is not required.
Expected results:
The about:certificates page should report that OCSP stapling is required if the tls_feature
extension is present with the value of status_request
(Unparsed extension: 1.3.6.1.5.5.7.1.24 = DER:30:03:02:01:05
).
Reporter | ||
Updated•5 years ago
|
Comment 1•5 years ago
|
||
April, can you take a look at this? Is this different from Certainly Something?
Comment 2•5 years ago
|
||
Certainly Something correctly shows that OCSP Stapling is required on dm4productions.com. It's a regression from my code.
Comment 3•5 years ago
|
||
Carolina, is this something you'd have time to look into?
Updated•5 years ago
|
Assignee | ||
Comment 4•5 years ago
|
||
Hey, I could take a look after November 4, is that OK? If yes, please assign this bug to me!
Comment 5•5 years ago
|
||
Hey, that seems okay, considering that nobody else is likely to have time for it. Considering that Certainly Something supports it, it would be great if we could manage an uplift into 71.
Assignee | ||
Comment 6•5 years ago
|
||
Taking a look right now! I'll try to get a patch for this asap.
Assignee | ||
Comment 7•5 years ago
|
||
Comment 8•5 years ago
|
||
[Tracking Requested - why for this release]: Correctness bug in new security UI feature rolling out in 71
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 9•5 years ago
|
||
I see the keyword checkin-needed
is not available, hope checkin-needed-tb
does the same!
Comment 10•5 years ago
|
||
Pushed by opoprus@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1bae5c13eea9
Fixes OCSP section.r=johannh
Comment 11•5 years ago
|
||
(In reply to Carolina Jimenez Gomez from comment #9)
I see the keyword
checkin-needed
is not available, hopecheckin-needed-tb
does the same!
Hi Carolina! The checkin-needed tag is no longer available in Bugzilla.
If you would like to have a patch landed, please put the tag directly in Phabricator- from Edit Revision put Check-in Needed in the Tag field.
Assignee | ||
Comment 12•5 years ago
|
||
(In reply to Natalia Csoregi [:nataliaCs] from comment #11)
Hi Carolina! The checkin-needed tag is no longer available in Bugzilla.
If you would like to have a patch landed, please put the tag directly in Phabricator- from Edit Revision put Check-in Needed in the Tag field.
Thank you! Do you know how can I nominate the patch for beta uplift? Johann asked me to do so but I don't really know how (I don't see the option in the tags) and he is on PTO until December
Thank you!
Comment 13•5 years ago
|
||
bugherder |
Assignee | ||
Comment 14•5 years ago
•
|
||
Comment on attachment 9106337 [details]
Bug 1589883 - Fixes OCSP section.r=johannh
Beta/Release Uplift Approval Request
- User impact if declined: The certificate viewer was reporting a wrong OCSP
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: No
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): I think this patch is not risky since I changed an if statement
- String changes made/needed: None.
Assignee | ||
Comment 15•5 years ago
|
||
Apologies for the "I don't know" responses, is my first time nominating a bug for beta uplift and really don't know how to verify those
Comment 16•5 years ago
•
|
||
(In reply to Carolina Jimenez Gomez from comment #15)
Apologies for the "I don't know" responses, is my first time nominating a bug for beta uplift and really don't know how to verify those
No worries, I took the liberty of editing your response for you -
Has the fix been verified in Nightly?
This would be true if someone from QE had tested that the fix worked on Nightly. Nobody has done so here (or there'd be comments and the bug status would have been changed to "VERIFIED" instead of "RESOLVED".
Needs manual test from QE?
Because there are automated tests, I don't think this needs additional manual testing.
String changes made/needed
This question is about whether you modified localizable strings (in .dtd, .properties or .ftl files). Your patch didn't do that, so "No" is the right answer. The question is here because if we change localizable strings, that creates work for localizers, and we try to avoid doing that while things are already in beta. In this case, although you changed when some strings get displayed, there's no work for localizers because the actual strings themselves did not change.
Assignee | ||
Comment 17•5 years ago
|
||
Thank you very much!!!
Comment 18•5 years ago
|
||
Comment on attachment 9106337 [details]
Bug 1589883 - Fixes OCSP section.r=johannh
P1 fix for an upcoming 71 feature, patch has tests and looks safe, uplift approved for 71 beta 9, thanks.
Comment 19•5 years ago
|
||
bugherder uplift |
Description
•