sparkle-project / Sparkle

A software update framework for macOS
https://sparkle-project.org
Other
7.34k stars 1.04k forks source link

Sparkle refuses http SUFeedURL with NSAllowsArbitraryLoads set to true #1856

Closed Akazm closed 3 years ago

Akazm commented 3 years ago

Description of the problem

According to this issue, AppTransportSecurity is not required when specified explicitly by Info.plist.

    <key>NSAppTransportSecurity</key>
    <dict>
        <key>NSAllowsLocalNetworking</key>
        <true/>
        <key>NSAllowsArbitraryLoads</key>
        <true/>
    </dict>

Sparkle nevertheless is not able to check for updates and complains about using http, which I only intend to use for testing/development and not during production.

Do you use Sandboxing in your app?

No.

Version of Sparkle.framework in the latest version of your app

1.26.0

Version of Sparkle.framework in the old version of app that your users have (or N/A)

N/A

Sparkle's output from Console.app

Encountered download feed error: Error Domain=NSURLErrorDomain Code=-1022 "The resource could not be loaded because the App Transport Security policy requires the use of a secure connection." UserInfo={NSLocalizedDescription=The resource could not be loaded because the App Transport Security policy requires the use of a secure connection., NSErrorFailingURLStringKey=http:localhost/, NSErrorFailingURLKey=http:localhost/, _NSURLErrorRelatedURLSessionTaskErrorKey=(
    "LocalDownloadTask <DDE3DEA4-FDD5-4DA9-AF86-B5B81B645F34>.<1>"
), _NSURLErrorFailingURLSessionTaskErrorKey=LocalDownloadTask <DDE3DEA4-FDD5-4DA9-AF86-B5B81B645F34>.<1>, NSUnderlyingError=0x6000006552c0 {Error Domain=kCFErrorDomainCFNetwork Code=-1022 "(null)"}}

Steps to reproduce the behavior

  1. Disable NSAppTransportSecurity.
  2. Use SUFeedURL with http instead of https.
Akazm commented 3 years ago

Side note: The file name of my Info.plist is Info.development.plist, if that's somehow relevant.

zorgiepoo commented 3 years ago

Side note: The file name of my Info.plist is Info.development.plist, if that's somehow relevant.

Make sure Xcode is using that as your Info.plist file for development. What you should check is your generated app bundle, and inspect the Contents/Info.plist file (it needs to be named Info.plist in the generated app bundle).

The Sparkle Test App in our repo disables ATS in much the same way, so check that as a reference.

Akazm commented 3 years ago

The Sparkle Test App in our repo disables ATS in much the same way, so check that as a reference.

I did and the configuration is fine.

What was however not fine is my xcconfig file. I applied this solution to my URL within my xcconfig file and it works now just as expected.

So the issue here is not that Sparkle ignores NSAllowsArbitraryLoads, just that Sparkle somehow treats a malformed URL (see console log entries above) as an ATS issue.

zorgiepoo commented 3 years ago

That error is coming from using NSURLSession, so the bug might be that NSURLSession is treating a malformed URL as an ATS issue. Not a Sparkle issue.

trss commented 2 years ago

The Sparkle Test App in our repo disables ATS in much the same way, so check that as a reference.

The one elusive thing was figuring I had to enable outgoing connections in app sandbox. The only reason I could figure this out was by seeing it's enabled in the Sparkle Test App even in 2.0.0.

So, why is this needed for localhost but not for external https URLs?

zorgiepoo commented 2 years ago

The sandbox is supposed to behave the same way regardless if the connection is local or external. This matches what I have seen so I'm more betting you're looking at something slightly wrong (like having the sandbox disabled in one case).

Most sandboxed apps probably enable outgoing connections (and this is why the test app does too), but it's not strictly necessary if you enable the Downloader XPC Service. This is explained in Sparkle's Sandboxing docs.

trss commented 2 years ago

Ah, I think I get it now!

https://sparkle-project.org/documentation/sandboxing/#code-signing

Also the --deep option is not used here because the Downloader XPC Service needs to be signed with a specific entitlement that ~aren’t~ isn't applicable to the other binaries.

I was deep code signing all through and that might be the cause of confusion. Will just follow the manual code signing process as explained and that should do. Thanks.

zorgiepoo commented 2 years ago

If you use --deep -s on your own app bundle but don't provide --entitlements you are stripping out the sandbox completely on your app. But some things in Sparkle.framework also expect to have no entitlements (like Autoupdate, InstallerLauncher XPC Service) or a specific entitlement (like Downloader XPC Service). So --deep is the wrong way to go about it when requirements differ from binary to binary.

trss commented 2 years ago

~Thanks for the quick summary of the relevant parts of the documentation I linked to earlier.~

_Thanks for figuring I didn't realize the link between --entitlements argument and sandboxing!_

The one thing I still can't figure is how to have ATS with NSAllowsLocalNetworking for the Downloader XPC service. There doesn't appear to be a sandbox entitlement corresponding to what we provide in Info.plist for ATS.

This is only a matter of curiosity since disabling the Downloader XPC and going for app sandbox with ATS instead does the job of testing through Xcode on localhost.

trss commented 2 years ago

Putting out a couple things which were hugely helpful in my struggle to understand code signing sufficiently well:

Note: The deep display is subtle in that it only displays "Nested=Frameworks/Sparkle.framework" at the end which we then have to provide as an argument and repeat the process to find them all.

Reference: https://developer.apple.com/library/archive/technotes/tn2206/_index.html#//apple_ref/doc/uid/DTS40007919-CH1-TNTAG404

Using the codesign Tool's --deep Option Correctly

When verifying signatures, add --deep to perform recursive validation of nested code. Without --deep, validation will be shallow: it will check the immediate nested content but not check that fully. Note that Gatekeeper always performs --deep style validation.

Important: While the --deep option can be applied to a signing operation, this is not recommended. We recommend that you sign code inside out in individual stages (as Xcode does automatically). Signing with --deep is for emergency repairs and temporary adjustments only.

zorgiepoo commented 2 years ago

Yes that's a good rule of thumb.

ATS isn't related to sandboxing. I don't remember all the keys ATS provides but if you're using the Downloader XPC Service you must supply the ATS rules in the downloader XPC Service's Info.plist, instead of the main app Info.plist. The test app used to do this until I changed it to not use the service.