Remaining leftover changes from my fork

[zorgiepoo]

Here I'll keep an update a track of a list of some "small" changes from my fork that could possibly be made. I may not have much time left so this is what I want to focus on. Let me know what you think should or should not be addressed in the near future.

• Expect the length attribute for appcast items to exist.
  • Warn if the attribute is not specified (a fair number of developers leave it out)
  • Use this field value instead of value from http header (what if that value is unknown?)
  • Warn if the attribute != expected content value from http header (if both are available)
  • Note or be aware that the download did receive responses can be received multiple times in rare cases for a single download according to the docs.
• Make guided packages the default rather than interactive packages being the default for packages.
  • Opt into foo.sparkle_interactive.pkg or foo.sparkle_interactive.mpkg
  • Interactive installations have real problems (lack of success status handling, strange task spawning code) and worse user experience.
  • Some developers unpleasantly ship e.g. foo.sparkle_guided.pkg for their website distribution.
• Remove the old application instead of moving the old application to the trash (Old apps get sent to trash? #827)
  • Not seamless: clutters user's Trash; Sparkle is the only updater that does this.
  • More inefficient across volumes potentially, strange issues (Sparkle requesting authentication when it doesn't need it #752)
  • Moving old app to trash may not always be possible/easy in my fork (I think, regarding running as root).
  • Could make atomic-safe directory replacements more difficult in the future with APFS
  • Would cause removal of some code
• Don't ask for update permission after 3 (or x) number of hours, instead of 2nd launch
  • 2nd launch is a poor metric; user could launch application more than once in a short period of time.
  • In my fork where support for updating other bundles is improved, the lifetime of the updater and bundle to update are distinct, and the updater be started up repeatably and not ran for long (e.g. sparkle-cli)
• Rewritten SULog() usage
  • Let us stick with reporting errors for the time being. We shouldn't be logging information unless something unexpected happen (i.e, don't always log when extracting update).
  • A global file location (in master) to write logs is not a good idea. It is rewritten when multiple applications are using Sparkle simultaneously, and when multiple processes are using SULog() simultaneously (very common in my fork). Always writes to disk.
  • Should take leverage of Apple's logging APIs; my fork uses ASL APIs currently. 10.12 introduced a new logging API that I haven't looked into yet.
  • Added a private function to disable logging to standard error and output streams, but it still should log info to Console.app. Currently sparkle-cli uses this when under release mode. Would be suitable for generate_appcast as well I bet.
  • Need to verify how system logging has changed in 10.12 and how if that affects Sparkle.
• Add ability for SUHost to reload the Info dictionary (bypassing NSBundle cache)
  • My fork improves being able to update other bundles, so master is not that affected.
  • Need to verify the correctness of the current code in my fork.
• Rewrote termination listener code in my fork
  • It is daemon safe and doesn't use AppKit
  • It works on processes that aren't regular applications
  • It is not polling/timer based.
  • The code is more complex.
• Obj-C type generic annotations (unless we support old old versions of Xcode?)
• Add AppKit prevention guards to implementation files where possible

Changes that are too big or not relevant to master:

• Add an installation type tag to the appcast items, which is mandated if the installation will be a package type install.
  • My fork needs to know what type of package installation it is to see if authorization is necessary (the type is later validated against the install type that was found in the archive since the feed is untrusted)
  • master currently suffers from a potential issue of thinking an install can be automatically downloaded/installed in the background when encountering package installs. Likely a rare issue however because app is most likely in a location not writable by user (that the initial package installer placed it at)
• Merging the automatic and manual update alert windows/nibs
  • Required major re-architecturing for the internal update drivers logic.
• Merging downloader code that downloads temporary items (appcast) and permanent items (updates)
  • Done so I could have one instead of two XPC services
  • Some XPC and new related code is involved. Would rather not rewrite for master.
• Automatic downloading & installing of updates
  • Downloading and installing updates is more decoupled. Updates can be downloaded in the background even if authorization will be necessary later. (i.e, automatic download and automatic install OR automatic download and manual install OR manual download and manual install).
  • Once downloaded in the background, the update can a) be set to be installed immediately and waiting to be resumed later if authorization isn't necessary, or b) can alert the user to start the installer in background with authorization
  • To clarify there are now four "modes" that the update alert can be presented. One is "an update is available. Would you like to download and install it?" (non background scheduled or initiated update). Second is "an update has been downloaded and started installing. Would you like to relaunch and finish installation?" (Automatic impatience timer limit met or critical marked update). Third is "an update has been downloaded. Would you like to install it?" (authorization needed to start installer). Last is "an update is available. Go learn more about this update here" (informational only, automatic update driver sets update to be resumed later by scheduled UI driver).
  • Once the installer starts and after download extraction is done & validated, the update will be installed automatically no matter what after the targeted application has been terminated.
  • master suffers from issues when waiting for the host app to terminate by using an AppKit event, which is not very reliable. My fork relies on listening for termination via a pid instead from the background installer process. Eg: master's behavior would fail with Qt apps, or apps that just did exit(0) which can be a valid approach in some cases. master also denies an app from utilizing sudden termination in the case of automatic installs.
  • Required major re-architecturing for the internal update drivers logic.
• New public/exported SPUUpdaterSetttings class.
  • Allows you to conveniently read certain updater settings without an updater instance, by looking at the user defaults and Info.plist.
  • A user driver, which may not have access to an updater or SUHost, may choose to use this class.
  • master's logic for automatically downloading updates depends on system write access which is no good here.
  • Perhaps a questionable addition..
• Adopting NSSecureCoding for some classes (e.g. SUAppcastItem)
  • master has no need for this now.
• User Drivers
  • Every form of user presentation now maps to a method in the user driver protocol.
  • Some methods may specify a reply block which can either be an acknowledgement that something was presented or could supply input back on what the user decided to do.
  • Allows custom user interfaces.
  • Required major re-architecturing for internal update drivers logic.
• launchd
  • The installer (Autoupdate) is now submitted as a launchd background job (via SMJobSubmit).
  • Authorization is now requested (if necessary) before submitting the installer job. So the installer can run as a root-level daemon if needed. The deprecated authorization function that master uses no longer is referenced.
  • Autoupdate can't link to any App Kit code and shouldn't interact with Launch Services.
  • Another background agent (that runs under the user's login session) is needed (Updater.app). This app provides a service so Sparkle can query if an update is being installed, is responsible for sending a quit Apple event to applications, and is responsible for re-launching applications after installation is finished and requested by the user.
• XPC Services
  • XPC services exist for being able to do different types of stuff that needs entitlements or no sandboxing: downloading updates, starting the installer, interacting with the background progress agent, interacting with the installer daemon..
  • XPC services are optional. So the functionality provided by a service must also be provided by Sparkle. Non-sandboxed apps do not need to use XPC services (note that they can though if they want).
  • Sparkle tries to determine whether or not to use an XPC service by detecting if a service exists in the updater app's bundle.
  • XPC services isn't just about sandboxing. Fault tolerance, lazy process management, ...
• Internal update driver changes
  • Protocol instead of subclass based, not prone to method overriding bugs we're familiar with
  • The automatic update driver does not provide custom UI anymore when an update can't be installed automatically without the user's interaction.. Instead when it's done downloading, it hands off the "resumable" update for another UI driver to handle -- if user interaction is necessary.
  • The concept of "an update did finish" callback now exists because the updater doesn't have to be terminated when an update is finished installing (if the updater's lifetime is not the same as the bundle being updated).

[kornelski]

I would use appcast's length only as a fallback (in case server doesn't set content-length, and perhaps for the first fraction of a second when showing "Zero KB of X KB downloaded")

[zorgiepoo]

Interesting. Why not the other way around? Only using the http content length as the fallback. From one perspective, the appcast length is more trustworthy and should be correct.

[kornelski]

Because HTTP is authoritative in this regard. Appcast author can put an incorrect value (which isn't hard when writing appcasts manually), but it's not possible to report an incorrect value on HTTP level (if you try to, either it will become the size you actually get, or the connection will get stuck waiting forever).

[kornelski]

• +1 on guided pkg.
• +1 on removing old apps.
• +0 on 2nd launch. I don't mind either way. I'm not sure if that's a big problem.
• +1 on using proper per-app logging. However, I'd keep non-error messages, since they can help when investigating problems, e.g. you can know whether error happened before or after extraction, or that it didn't extract, but you expected it to extract, etc.

[kornelski]

I'm not familiar with the details why SUHost would need to bypass cache. Is it because you read details from a daemon that lives throughout app updates?

[kornelski]

What would be the consequence of dropping older versions of Xcode? Since we require 64-bit Intel, probably nothing serious, since even apps that require older Xcode to build could link with pre-built Sparkle.

[kornelski]

Merging of update nibs would be great. That bloats the framework for just tiny UI variation.

[zorgiepoo]

I'm not familiar with the details why SUHost would need to bypass cache. Is it because you read details from a daemon that lives throughout app updates?

You have an updater that updates a bundle different from itself. Once the bundle is updated, the updater (still living) needs to know that bundle has been updated. The info dictionary needs to be re-read. master which doesn't properly support updating other bundles assumes the updater should be terminated.. if I recall correctly.

+1 on using proper per-app logging. However, I'd keep non-error messages, since they can help when investigating problems, e.g. you can know whether error happened before or after extraction, or that it didn't extract, but you expected it to extract, etc

One thing I may have to look into short-term is if I should make a different flag or different API for logging info vs errors, like all logging APIs (including Apple's) tend to have. But I suppose I could just log info for now using the same call.

+0 on 2nd launch. I don't mind either way. I'm not sure if that's a big problem.

I may rethink this. Time may make more sense though. The 2nd launch would refer to the updater in any case, not the application. I did once see someone screen recording my app though and hitting 'No' when the permission prompt showed up :(.

[jkbullard]

I would use appcast's length only as a fallback (in case server doesn't set content-length, and perhaps for the first fraction of a second when showing "Zero KB of X KB downloaded")
...
Because HTTP is authoritative in this regard. Appcast author can put an incorrect value (which isn't hard when writing appcasts manually), but it's not possible to report an incorrect value on HTTP level (if you try to, either it will become the size you actually get, or the connection will get stuck waiting forever).

http is not authoritative. An attacker (MITM) can cause an incorrect http length to be reported. An attacker can't modify the length in the appcast if the appcast is signed (a modification I use in Tunnelblick).

I like @zorgiepoo's defensive approach much better. If the length is specified in the appcast, the http length must agree or it should be considered an error.

[zorgiepoo]

On the other hand, we don't sign the feed in our branch, and it may be more likely the case someone inserts an incorrect length in the appcast than for an attacker intercepting the value. Regardless if there's an attacker or not, a client (Sparkle in our case) needs to be able to handle the case if the value is incorrect.

[kornelski]

@jkbullard I don't mean that authoritative is secure and bulletproof, but rather that by design it's the canonical source of that information (whether it's sent by the author or attacker).

We don't sign appcasts and don't intend to, so for Sparkle length in HTTP response headers and HTTP response body is equally (in)secure, but the latter is easier to get wrong for authors.

Note also that we (and Apple via ATS) strongly urge to use HTTPS, which protects response length. We verify integrity of updates, so even over HTTP false length isn't a security issue (it'll cause update to be rejected, or cause connection to time out).

[jkbullard]

@pornel https: provides no protection if a CA (any CA) has been compromised (which has happened) or if the web server has been compromised (which has happened). Only DSA signing is fully under the control of the developer.

We have different threat models, I guess. I try to protect (as best I can) against state-level attackers, so Tunnelblick requires https: and DSA signing of both the appcast and the update. Defense in depth.

Anyway, thanks for all your and zorgiepoo's work on Sparkle, it is much appreciated.

[kornelski]

Like I've said previously, false length isn't a security issue for Sparkle.

[zorgiepoo]

Submitted PR's for length-fallback, removing old bundle, and making guided packages the default.

Added info about automatic downloading & installing, and merging common downloading code under list of big changes.

Currently we just have two calls to SULog that act as info calls, one in the piped unarchiver, and one in the dmg unarchiver. I also added a note in the list of small changes about adding a private function to disable logging to stderr/stdout, but still routes through Console.app. "Info" logs are just a bit annoying for debugging, but there absolutely needs to be a way to "hide" them for production CLI tools.

[zorgiepoo]

I upgraded SULog.h & SULog.m to support the new logging facility in 10.12 (which deprecates the older ones). The overall goal is to just use Apple's logging facility and not reinvent the wheel (the path to least resistance). Have a look before I start any merging work here.

[kornelski]

I'd keep SULog and add SULogError or such. Easier to type and less code to change.

[kornelski]

// Act the same way os_log() does; don't log to stderr if a terminal device is attached

it does this!? I'd prefer logging to always go to stderr.

The code is fine in general.

[zorgiepoo]

Almost all of our logging code is for errors. Adding SULogError would not result in much less code changing. I didn't create multiple functions because of variable list processing. I think a single function makes sense minus refactoring reasons. Also having SULog being easy to call because you don't specify a level isn't necessarily a good thing ;).

I'd prefer logging to not log to stderr if I had to choose one extreme or the other, so I agree with Apple here. A CLI tool should really not spit out debug logging to stderr, or it should at least have control over such info being spit out on screen.

[zorgiepoo]

I have updated the original post with new information. It's probably worth just re-reading the post. I have reverted the 2nd launch timing to match master's behavior simply because I did not think that was a change worthwhile to make right now.

[kornelski]

Merging the automatic and manual update alert windows/nibs

This would be very nice

Automatic downloading & installing of updates

That would be good refactoring too. I feel that currently "update driver" is a wrong abstraction that couples logic of download+install+UI, so we can't have #1007

[zorgiepoo]

Yeah, when we say that automaticallyDownloadsUpdates is YES, I'd like it (as in my fork) to mean "automatically download updates + IF possible, automatically install them + present them when needed." In master, this currently means, "IF possible, automatically download & install updates + present them when needed." [If possible being related to authorization, presenting them related to critical or informational updates or needing authorization]... if that makes sense.

The task list here was completed. Do you want to pull my fork as a branch in this repository now @pornel? If so, I can start to do work, hopefully mostly documentation related, here now.