Closed Bug 1531176 Opened 6 years ago Closed 6 years ago

Split the Google key management between gls and safe browsing

Categories

(Firefox Build System :: General, enhancement, P1)

Product:
Firefox Build System
Component:
General
Type:
enhancement

Tracking

(firefox-esr6066+ verified, firefox65 wontfix, firefox66blocking verified, firefox67+ fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox-esr60 66+ verified
firefox65 --- wontfix
firefox66 blocking verified
firefox67 + fixed

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

Details

(Whiteboard: [qa-triaged])

Attachments

(2 files)

Bug 1531176 - Split the Google key management between gls and safe browsing r?glandium
47 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-release+
RyanVM
: approval-mozilla-esr60+
Details | Review
Bug 1531176 - about:support: also support the split of the key r?florian
47 bytes, text/x-phabricator-request
Details | Review

For a bunch of reasons, we want to be able to manage differently GLS and safe browsing keys instead of using the same one.

Severity: normal → enhancement
status-firefox65: --- → wontfix
status-firefox66: --- → affected
status-firefox67: --- → affected
status-firefox-esr60: --- → affected
Priority: -- → P1
Depends on: 1531178
Pushed by sledru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6c2e00bcd2bb Split the Google key management between gls and safe browsing r=glandium

There is also the following bc failure: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=232373599&repo=autoland&lineNumber=4642

[task 2019-03-07T09:17:19.389Z] 09:17:19 INFO - TEST-START | toolkit/modules/tests/browser/browser_Troubleshoot.js
[task 2019-03-07T09:17:19.730Z] 09:17:19 INFO - GECKO(5268) | WebGL(0x61b000415a80)::ForceLoseContext
[task 2019-03-07T09:17:19.786Z] 09:17:19 INFO - GECKO(5268) | WebGL(0x61b000444780)::ForceLoseContext
[task 2019-03-07T09:17:19.847Z] 09:17:19 INFO - TEST-INFO | started process screentopng
[task 2019-03-07T09:17:20.397Z] 09:17:20 INFO - TEST-INFO | screentopng: exit 0
[task 2019-03-07T09:17:20.401Z] 09:17:20 INFO - TEST-UNEXPECTED-FAIL | toolkit/modules/tests/browser/browser_Troubleshoot.js | Schema mismatch, Error: Validation error: Object has property keyGLSGoogleFound not in schema: object={"name":"Firefox","osVersion":"Linux 4.4.0-1014-aws","version":"67.0a1","buildID":"20190307083426","userAgent":"Mozilla/5.0 (X11; Linux x86_64; rv:67.0) Gecko/20100101 Firefox/67.0","safeMode":false,"updateChannel":"default","supportURL":"http://127.0.0.1:8888/support-dummy/","numTotalWindows":1,"numRemoteWindows":1,"remoteAutoStart":true,"currentContentProcesses":5,"maxContentProcesses":4,"autoStartStatus":1,"policiesStatus":0,"keyGLSGoogleFound":true,"keySBGoogleFound":false,"keyMozillaFound":false}, schema={"required":true,"type":"object","properties":{"name":{"required":true,"type":"string"},"version":{"required":true,"type":"string"},"buildID":{"required":true,"type":"string"},"userAgent":{"required":true,"type":"string"},"osVersion":{"required":true,"type":"string"},"vendor":{"type":"string"},"updateChannel":{"type":"string"},"supportURL":{"type":"string"},"launcherProcessState":{"type":"number"},"remoteAutoStart":{"type":"boolean","required":true},"autoStartStatus":{"type":"number"},"numTotalWindows":{"type":"number"},"numRemoteWindows":{"type":"number"},"currentContentProcesses":{"type":"number"},"maxContentProcesses":{"type":"number"},"policiesStatus":{"type":"number"},"keyGoogleFound":{"type":"boolean"},"keyMozillaFound":{"type":"boolean"},"safeMode":{"type":"boolean"}}} -
[task 2019-03-07T09:17:20.401Z] 09:17:20 INFO - Stack trace:
[task 2019-03-07T09:17:20.402Z] 09:17:20 INFO - chrome://mochikit/content/browser-test.js:test_ok:1304
[task 2019-03-07T09:17:20.403Z] 09:17:20 INFO - chrome://mochitests/content/browser/toolkit/modules/tests/browser/browser_Troubleshoot.js:snapshotSchema/<:38
[task 2019-03-07T09:17:20.405Z] 09:17:20 INFO - TEST-PASS | toolkit/modules/tests/browser/browser_Troubleshoot.js | The pref should be set: javascript.troubleshoot -
[task 2019-03-07T09:17:20.407Z] 09:17:20 INFO - TEST-PASS | toolkit/modules/tests/browser/browser_Troubleshoot.js | The pref should be set: troubleshoot.foo -
[task 2019-03-07T09:17:20.411Z] 09:17:20 INFO - TEST-PASS | toolkit/modules/tests/browser/browser_Troubleshoot.js | The pref should be set: javascript.print_to_filename -
[task 2019-03-07T09:17:20.413Z] 09:17:20 INFO - TEST-PASS | toolkit/modules/tests/browser/browser_Troubleshoot.js | The pref should be set: network.proxy.troubleshoot -

Pushed by sledru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0f2b9b0bf9b9 Split the Google key management between gls and safe browsing r=glandium
Pushed by sledru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3fb6c01dd2b0 Split the Google key management between gls and safe browsing r=glandium
Pushed by sledru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e908fbc7b930 Split the Google key management between gls and safe browsing r=glandium
Attachment #9047222 - Attachment description: Bug 1531176 - Split the Google key management between gls and safe browsing r?glandium → hg logBug 1531176 - Split the Google key management between gls and safe browsing r?glandium
Attachment #9047222 - Attachment description: hg logBug 1531176 - Split the Google key management between gls and safe browsing r?glandium → Bug 1531176 - Split the Google key management between gls and safe browsing r?glandium
Pushed by sledru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/43376e0c3563 Split the Google key management between gls and safe browsing r=glandium https://hg.mozilla.org/integration/autoland/rev/b59042f5f5bc about:support: also support the split of the key r=florian,flod

https://hg.mozilla.org/mozilla-central/rev/43376e0c3563
https://hg.mozilla.org/mozilla-central/rev/b59042f5f5bc

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox67: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Comment on attachment 9047222 [details]
Bug 1531176 - Split the Google key management between gls and safe browsing r?glandium

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: None
  • User impact if declined: We want to separate the GLS and safebrowsing to disable the old key.
    Not doing it so would be costly.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce:
  • Look at about:support to see if GLS and safebrowsing are separated
  • Check that geolocation still works
  • Check that safebrowsing still works
  • List of other uplifts needed: Bug 1531178
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): Well understood change but we had so many surprises ...
  • String changes made/needed: Yes but we can live with the english string in about:support in beta and release (I already told Flod)

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: The same key is used for ESR
  • User impact if declined: Same as beta
  • Fix Landed on Version: nightly
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): see above
  • String or UUID changes made by this patch: see above
Flags: needinfo?(sledru)
Attachment #9047222 - Flags: approval-mozilla-esr60?
Attachment #9047222 - Flags: approval-mozilla-beta?

Comment on attachment 9047222 [details]
Bug 1531176 - Split the Google key management between gls and safe browsing r?glandium

Not crazy about taking this so late in the cycle, but I'd rather it get into today's first RC build while there's still time to test rather than one later in the week. Approved for 66.0rc1 and 60.6esr.

Attachment #9047222 - Flags: approval-mozilla-release+
Attachment #9047222 - Flags: approval-mozilla-esr60?
Attachment #9047222 - Flags: approval-mozilla-esr60+
Attachment #9047222 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Backed out changeset for failing many about:support tests, e.g. browser_Troubleshoot.js:

https://hg.mozilla.org/releases/mozilla-release/rev/fb9abfe2e9028289d21213cd848e353c1292b277
https://hg.mozilla.org/releases/mozilla-esr60/rev/e5f3c7404734c5808858056e5fedaa89192180dc

Release push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-release&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&group_state=expanded&revision=88b0bd0acc785ac894150231f37fa3557b6075c4

ESR60 push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr60&resultStatus=usercancel%2Cretry%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&group_state=expanded&revision=01b634d28203ce7be051454f700be3d52af08494

The "part 2" patch contains the troubleshoot schema expectation updates but also adds new strings. That's why it can't be uplifted in its current state.

Release needs the string update to be done the old way with textContent according to the failure message. This had been catched for ESR60 because it was part of one of the many conflicts when the patch got applied.

status-firefox66: fixed → affected
status-firefox-esr60: fixed → affected
Flags: needinfo?(sledru)

I have a look for the broken test but for the string being added, this is expected (see in comment #20)

String changes made/needed: Yes but we can live with the english string in about:support in beta and release (I already told Flod)

Flags: needinfo?(sledru)

(In reply to Sylvestre Ledru [:sylvestre] from comment #25)

I have a look for the broken test but for the string being added, this is expected (see in comment #20)

String changes made/needed: Yes but we can live with the english string in about:support in beta and release (I already told Flod)

This is not about string frozen branches. about:support was migrated to Fluent recently, which means you need a completely different patch for those branches. At that point, it might be easier to hard-code the strings in the XHTML/XUL file.

Sylvestre, can you request extra testing here from QA? Probably best to file an urgent PI request.

Flags: needinfo?(sledru)
Whiteboard: [qa-triaged]

Done, PI-39

Flags: needinfo?(sledru)

This was covered as part of the sign off conducted by QA for 66.0-build1, using Windows 7 64-bit, Windows 10 64-bit, macOS 10.13, Ubuntu 16.04 64-bit.

All of the associated manual tests passed:

  • the Google Location Service Key and Google Safebrowsing Key fields are displayed separately in about:support
  • following smoke and exploratory testing, both Safebrowsing and Geolocation work as expected
  • the correct key is used for the Google Location Service

PI-39 has been updated as well.

status-firefox66: fixed → verified

(In reply to Sylvestre Ledru [:sylvestre] from comment #0)

For a bunch of reasons, we want to be able to manage differently GLS and safe browsing keys instead of using the same one.

From a distro packager perspective, and for posterity, it would be useful if these reasons could be listed here.

This would help (me and others) make an informed decision on whether to request a separate key for safe browsing.

Yeah, sorry, I should have explain.
Before this patch, we had a single key for both gls and safebrowsing.
This means that if we decide to kill the safebrowsing or gls (or limit the permissions of the key), it is much harder to do.

Here, that means that I can just update the safebrowsing key without impacting gls (and the other way around).

Is that clear?

Thanks Andrei!

Thanks Sylvestre.

Do I understand correctly that using the same key for both should just work, by passing the same key file to --with-google-location-service-api-keyfile and --with-google-safebrowsing-api-keyfile ?

Yes, this is currently the case with Firefox.

It's probably too late, but maybe we should re-add --with-google-api-keyfile, and make it imply both new options.

(In reply to Andrei Vaida [:avaida] – please ni? me from comment #32)

This was covered as part of the sign off conducted by QA for 66.0-build1, using Windows 7 64-bit, Windows 10 64-bit, macOS 10.13, Ubuntu 16.04 64-bit.

All of the associated manual tests passed:

  • the Google Location Service Key and Google Safebrowsing Key fields are displayed separately in about:support
  • following smoke and exploratory testing, both Safebrowsing and Geolocation work as expected
  • the correct key is used for the Google Location Service

PI-39 has been updated as well.

Same results after testing 60.6esr-build2 across platforms (Windows 7 64-bit, Windows 10 64-bit, macOS 10.14 and Ubuntu 18.04 64-bit).

PI-39 has been updated as well.

status-firefox-esr60: fixed → verified

Many thanks for your testing!

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: