Disable update xml certificate checks on Windows

RESOLVED FIXED in Firefox 27

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

unspecified
Firefox 27
x86_64
Windows 7
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

+++ This bug was initially created as a clone of Bug #920750 +++

We already have signed mar checks on Windows and we should disable via preference the certificate attribute checks and the certificate built-in check.
No longer depends on: 920750
Created attachment 819164 [details] [diff] [review]
patch - disable app update xml cert checks on Windows
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Attachment #819164 - Flags: review?(netzen)
Pushed to try to make sure it doesn't break cert check tests
https://tbpl.mozilla.org/?tree=Try&rev=206665f8f38e
Created attachment 819175 [details] [diff] [review]
patch - disable app update xml cert checks on Windows rev2

per irc convo
Attachment #819164 - Attachment is obsolete: true
Attachment #819164 - Flags: review?(netzen)
Attachment #819175 - Flags: review?(netzen)
Comment on attachment 819175 [details] [diff] [review]
patch - disable app update xml cert checks on Windows rev2

Review of attachment 819175 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good assuming it passes try, please re-push given the change to default preferences.
Attachment #819175 - Flags: review?(netzen) → review+
Also the r+ is assuming that security has agreed to this, which I believe is the case :)
Note: dveditz agreed to remove the checks when we have mar signing and we do on Windows.
Summary: Disabled certificate checks on Windows for the update xml check → Disable certificate checks on Windows for the update xml check
Summary: Disable certificate checks on Windows for the update xml check → Disable update xml certificate checks on Windows
Created attachment 819274 [details] [diff] [review]
patch for tests rev1

Some of the preferences weren't set in the tests.

Pushed both patches to try
https://tbpl.mozilla.org/?tree=Try&rev=11c8c3410ff5
Attachment #819274 - Flags: review?(netzen)
Comment on attachment 819274 [details] [diff] [review]
patch for tests rev1

Review of attachment 819274 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/update/test/chrome/test_0121_check_requireBuiltinCert.xul
@@ +81,5 @@
>                                 aCertAttrName, cert[aCertAttrName]);
>    });
>  
> +  Services.prefs.setBoolPref(PREF_APP_UPDATE_CERT_REQUIREBUILTIN, true);
> +  Services.prefs.setBoolPref(PREF_APP_UPDATE_CERT_CHECKATTRS, false);

maybe there's value in having two tests:
1) PREF_APP_UPDATE_CERT_REQUIREBUILTIN true
   PREF_APP_UPDATE_CERT_CHECKATTRS true

and

2) PREF_APP_UPDATE_CERT_REQUIREBUILTIN true
   PREF_APP_UPDATE_CERT_CHECKATTRS false

(in a follow up bug)?
Attachment #819274 - Flags: review?(netzen) → review+
(In reply to Brian R. Bondy [:bbondy] from comment #9)
>...
> maybe there's value in having two tests:
> 1) PREF_APP_UPDATE_CERT_REQUIREBUILTIN true
>    PREF_APP_UPDATE_CERT_CHECKATTRS true
> 
> and
> 
> 2) PREF_APP_UPDATE_CERT_REQUIREBUILTIN true
>    PREF_APP_UPDATE_CERT_CHECKATTRS false
> 
> (in a follow up bug)?
I don't think it will provide any value since there is already a test that checks both.
Try is green... I'll check this in over the weekend
Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/92c512181d1c
Flags: in-testsuite+
Target Milestone: --- → Firefox 27
https://hg.mozilla.org/mozilla-central/rev/92c512181d1c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Robert, does that mean we can remove the Mozmill test, which has initially been created via bug 584002?
Flags: needinfo?(robert.strong.bugs)
Just to add, I know that we have to wait for bug 920750 to be fixed.
That is actually due to xmlhttprequest using a fake redirect for the first request and not app update so you should probably have a test for other consumers of that code. See bug 471889 comment #5 and bug 471889 comment #6.
Flags: needinfo?(robert.strong.bugs)
You need to log in before you can comment on or make changes to this bug.