Disable user impersonation in maintenance service on release

RESOLVED FIXED in Firefox 65



8 months ago
10 days ago


(Reporter: agashlin, Assigned: agashlin)


(Blocks 1 bug, {sec-moderate})

65 Branch
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox64 unaffected, firefox65+ fixed, firefox66+ fixed)


(Whiteboard: [post-critsmash-triage])


(1 attachment)

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

rstrong reported in bug 1486637 comment 18 that there has been a significant rate of impersonation failures (0.2% of all startup records, 0.8% of unique client startup records) in recent telemetry. Until we understand these failures better it seems like a good idea to prevent this from hitting release with 65.
[Tracking Requested - why for this release]:
need to disable a fix for release 65
Did you want to put it behind an EARLY_BETA_OR_EARLIER ifdef for now?
This introduces a DISABLE_USER_IMPERSONATION define when EARLY_BETA_OR_EARLIER is set, if that is present the maintenance service will not attempt to get an impersonation token for the user's updater process, and the updater will not attempt to use any token it is given. The bulk of the changes are restoring the old failure status codes and the tests that expect them, sorry for the noise that causes.
Group: toolkit-core-security → core-security-release
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

65 is already past the early beta phase, so I guess we can just wontfix it at this point?

Flags: needinfo?(agashlin)

Comment on attachment 9034528 [details]
Bug 1514898 - Disable user impersonation on release

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1486637

User impact if declined: Using the maintenance service to update will fail in some unknown cases. This might mean the user will just see a UAC prompt when updating, but it could be causing updates to fail altogether.

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

Why is the change risky/not risky? (and alternatives if risky): This can't be properly exercised on Nightly because it is only really effective without EARLY_BETA_OR_EARLIER. It is intended to disable the effects of bug 1486637, but it isn't a straightforward backout in order to make it conditional.

I have manually tested it locally, and the automated tests have been updated to cover it, with successful try runs when applied to beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd618226d6349d33c62768ec4ed91eeeec1b9b0b

I understand it is late in the cycle, but without knowing what is causing the failures (or whether they are mostly recoverable) we don't want bug 1486637 to hit release. Alternatively we could do a more straightforward backout of that.

String changes made/needed:

Attachment #9034528 - Flags: approval-mozilla-beta?

Comment on attachment 9034528 [details]
Bug 1514898 - Disable user impersonation on release

[Triage Comment]
Sorry, I had my logic reversed there. This will revert us back to the pre-1486637 behavior for late Beta and Release builds while those changes continue to bake on Nightly and early Beta. Sounds like a good plan if this isn't ship-ready yet. Approved for 65.0b10.

Attachment #9034528 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(agashlin)
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.