928489 - Disable update xml certificate checks on Windows

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Robert Strong (they/them - no direct email)]

+++ 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.

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch - disable app update xml cert checks on Windows

Comment 2

[Robert Strong (they/them - no direct email)]

Pushed to try to make sure it doesn't break cert check tests
https://tbpl.mozilla.org/?tree=Try&rev=206665f8f38e

Comment 3

[Robert Strong (they/them - no direct email)]

Attachment: patch - disable app update xml cert checks on Windows rev2

per irc convo

Comment 4

[Brian R. Bondy [:bbondy]]

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.

Comment 5

[Brian R. Bondy [:bbondy]]

Also the r+ is assuming that security has agreed to this, which I believe is the case :)

Comment 6

[Robert Strong (they/them - no direct email)]

Note: dveditz agreed to remove the checks when we have mar signing and we do on Windows.

Comment 7

[Robert Strong (they/them - no direct email)]

Pushed to try now that it has reopened
https://tbpl.mozilla.org/?tree=Try&rev=08adbd319ead

Comment 8

[Robert Strong (they/them - no direct email)]

Attachment: 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

Comment 9

[Brian R. Bondy [:bbondy]]

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)?

Comment 10

[Robert Strong (they/them - no direct email)]

(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.

Comment 11

[Robert Strong (they/them - no direct email)]

Try is green... I'll check this in over the weekend

Comment 12

[Robert Strong (they/them - no direct email)]

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/92c512181d1c

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/92c512181d1c

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Robert, does that mean we can remove the Mozmill test, which has initially been created via bug 584002?

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Just to add, I know that we have to wait for bug 920750 to be fixed.

Comment 16

[Robert Strong (they/them - no direct email)]

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.