Sparkle should look like Keystone to the rest of the code

[bridiver]

brave/brave-core#2705 (comment) takes some steps in this direction, but I think we would like to just replace keystone methods with spark implementation. That would eliminate https://github.com/brave/brave-core/pull/2705/files#diff-ab380474ef25041e585235860f291ed5 for instance

[mherrmann]

I think we should put tackling this issue on hold until Google switch to the new cross-platform update implementation later this year.

[bridiver]

Well, I don't know that we ever want actually use their updater implementation because it's horrible and runs in the background even when Chrome isn't running. This change would just prevent some duplicated and patched code

[BrendanEich]

We definitely don't want anything like Keystone or whatever always-running background updater might be coming from upstream. I had to root out Keystone as well as uninstall Chrome to get out of macOS page-swapping hell.

[mherrmann]

I'm not sure whether you are both exclusively speaking about macOS because on Windows we already have an updater that runs in the background. For security alone, my suspicion is that we will have to switch from this to Chromium's new implementation on at least Windows. Whether to do the corresponding switch on macOS from Sparkle to Chromium's new updater is an interesting question.

@bridiver is your statement specifically about the new updater implementation? Or is it a more general statement that you don't like background update processes? If the latter, do your reservations also apply to Windows, where we have been using background updates successfully for a long time?

[bridiver]

I wasn't aware we were running an updater in the background for windows and I don't understand why we would ever need a long running process for that on any OS. Why would the updater ever need to run if Brave is not running? Apart from unnecessary use of system resources, I have privacy concerns around a process always running in the background checking for updates

[mherrmann]

I believe there are several motivations for doing this. One is that a background updater can run as admin and thus allow updating system-wide installations even when the browser only ever runs with user privileges. Another is that background updates essentially make sure that users always run the latest version when they start the browser, which reduces the time they may be running insecure versions. Finally, on Windows, Google use the updater (which is a separate application) to update multiple products. This way, they don't have to duplicate most of the update logic in the code base of those products.

[bridiver]

Imagine every application you have on your computer running an always on background updater taking up resources and phoning home on a regular basis even if you never open the app. I am very strongly against this.

[mherrmann]

I hate background processes on my system just as much. However, I believe we are pretty special users, quite unlike the 1bn people who use Chrome. Google's background updater on Windows ("Omaha") is optimized to use as few system resources and be as low-priority as possible. And it achieves the benefits I described above. I would make the same trade-off.

[bridiver]

In my opinion it's not worth the trade off. If you're using Brave on a regular basis then you are going to get updates on a timely basis. If you're not then you probably don't want a process running in the background installing updates that you rarely use. Any application can come up with a justification for it, but I think unless we need it for technical reasons (install privileges on windows) then we shouldn't have one. I'm sure keystone is designed to be lightweight and it's still a major problem on macOS for a lot of people. I had to hack directory permissions to prevent Chrome from reinstalling it automatically.

[bridiver]

I think there's also a privacy issue with making update requests in the background when Brave isn't even running. Has this gone through a privacy review? If not it needs one because it's making new network requests.

[bridiver]

Also does this background windows updater potentially skew our usage stats for windows?

[mherrmann]

Has this gone through a privacy review?

Omaha was integrated into Brave before I joined, so I don't know.

I think unless we need it for technical reasons (install privileges on windows) then we shouldn't have one

Install privileges are also relevant on macOS, where deployments that are installed machine-wide but are only ever run by non-admin users cannot get updated.

Also does this background windows updater potentially skew our usage stats for windows?

I don't know how the usage stats are calculated. If they come from inside the browser process, then no. The Windows updater is a separate application that knows basically nothing about Brave.

[bridiver]

It doesn't matter when Omaha was first used in Brave and it probably predates privacy review, but changing the macOS installer to make pings that it wasn't previously making definitely requires privacy review. These are new network requests and that is one of the things that requires security/privacy review.

I suppose it could exist somewhere, but I've never even heard of a system wide macOS browser install. Even the chrome background installer runs in the user dir with user permissions. In fact I subverted it by changing the directory permissions so my user account was no longer allowed to write to it.

On the browser I believe the stats and updater pings are the same endpoint which is why I'm concerned that this might be inflating the numbers for daily usage in particular.

[bridiver]

Sorry, I should revise that because I was thinking about stuff in /System or /Library. Pretty much all macOS installs are system wide (/Applications) and Brave updates itself just fine there. For unusual installs where the user account does not have permissions we could have a separate package for updating that is optional and not part of the standard install used by the vast majority of users who do not have this problem.

[bridiver]

It's also possible to have an admin install process that runs in the background but does nothing until the browser sends a signal to check for updates. This would prevent it from doing things in the background independent of whether the browser is even running or not and also places more limitations on the resource usage because it can be idled waiting for new socket connections. In fact on macOS and maybe windows as well, the os will start the process only when a connection is made to it

[bridiver]

Also changing the updater for macOS should require security review even without the pings.

[mherrmann]

changing the macOS installer to make pings that it wasn't previously making definitely requires privacy review. These are new network requests and that is one of the things that requires security/privacy review.
...
Also changing the updater for macOS should require security review even without the pings.

Sure, yeah, that makes sense.

Pretty much all macOS installs are system wide (/Applications) and Brave updates itself just fine there.

Only if your user can elevate to admin. See for example #9562 (comment).

It's also possible to have an admin install process that runs in the background but does nothing until the browser sends a signal to check for updates.

That's true. Though you'd lose the flow where users always start the latest version and thus receive potentially critical security updates earlier.

[bridiver]

That's true. Though you'd lose the flow where users always start the latest version and thus receive potentially critical security updates earlier.

This is a trade-off I can say pretty confidently that we're willing to make cc @BrendanEich @pes10k

I think the ideal solution for anything requiring admin would the windows equivalent of using xpc/launchd to launch an admin process on demand https://developer.apple.com/library/archive/samplecode/EvenBetterAuthorizationSample/Introduction/Intro.html

Looks like this probably for windows? https://docs.microsoft.com/en-us/dotnet/core/extensions/windows-service

[mherrmann]

It's not just this trade-off. Google are working on their new updater, which as far as I know will replace Keystone. An advantage of using their solution is that we wouldn't have to implement something from scratch and would get a maintained, tested, secure and integrated product. I understand you don't like Keystone. But maybe the new implementation will be better and run as nicely in the background as Omaha currently does on Windows. In that case, my feeling, which is of course just one data point, is that the trade-off will be worth it.

[mihaiplesa]

Details on the new cross-platform updater can be found at https://bit.ly/chromium-updater

[bridiver]

It's not just this trade-off. Google are working on their new updater, which as far as I know will replace Keystone. An advantage of using their solution is that we wouldn't have to implement something from scratch and would get a maintained, tested, secure and integrated product. I understand you don't like Keystone. But maybe the new implementation will be better and run as nicely in the background as Omaha currently does on Windows. In that case, my feeling, which is of course just one data point, is that the trade-off will be worth it.

I think this is one place where we don't want to follow Google. From @BrendanEich - The “hide from sight and run always” pattern is very dark, do not want

[bridiver]

Details on the new cross-platform updater can be found at https://bit.ly/chromium-updater

Looking at this I think we can still leverage their work using only on-demand updates. It's registered using XPC/COM so we can trigger it to run from the browser as I mentioned above
https://docs.google.com/document/d/1VlozzSjriRD5Yn9cLzjTrSvXPkxtq47mk2JejkczAss/edit#heading=h.xppgn76evxyc

I believe this would just involve leaving out ~/Library/LaunchAgents/org.chromium.ChromiumUpdater.wake.86.0.4230.0.plist which triggers it to periodically start up and check for updates

[bridiver]

also it looks like there's a policy to disable the auto update check so it looks like we can use what they have without the background check/update

[mherrmann]

it looks like there's a policy to disable the auto update check

They have this in Omaha 3 as well; However it does not disable checks in all cases. I've patched Omaha 3 in other projects that also wanted to completely disable background checks.

Note that if the policy is set (and respected by Omaha 4 also in future versions), then I believe the background update task will still run regularly. It simply then exits almost immediately.

Looking at this I think we can still leverage their work using only on-demand updates.

As briefly mentioned above, I've already done this for other projects. However, the technical implementation still involves a separate application that gets installed on the user's system.

[bridiver]

If a periodic background ping that happens when the browser isn't even running doesn't violate our privacy policy (it may), then I think we need adjust our privacy policy because it should. At least on macos leaving out the wake plist will prevent it from running periodically, but if it runs and immediately quits without making a ping or update attempt that's fine, too. The problem is not having a separate application, the problem is having that application running when Brave is not (except during an actual Browser triggered update).

[mherrmann]

To make sure that this worthwhile discussion does not get lost when it comes to the question of migrating to Omaha 4, I've created a new issue: #21609

[mherrmann]

(Hope that's OK with you @bridiver. I was afraid that your points would get lost by the time the Omaha 4 discussion comes around.)

[pes10k]

Just wanted to chime in with two things:

1. I agree with @bridiver , that we should avoid situations where Brave communicates with Brave servers in ways that are not obvious to users. "Brave initiated network traffic when Brave is closed" is pretty clearly in this category. We modify Blink to prevent sites from doing this (re Background Sync) and we should do the same here
2. These kinds of things (new or changed network traffic) needs a sec+privacy review before shipping things. Just wanted to remind incase that wasn't already scheduled / planned

[PrivacyMatters]

I'm with @bridiver and @pes10k on this. I'd be concerned about this running in the background, phoning home, unless that is strictly to essential to a service requested - and I don't see how this is essential tbh. Also, I do not believe it is consistent with Brave's privacy committments made via marketing statements and the privacy policy. Does it align with 'The best privacy online'?

It's akin to monitoring users and under EU data protection law would require a detailed data protection and privacy impact assessment (DPIA).

[mherrmann]

The privacy review was conducted in https://github.com/brave/reviews/issues/1739.