New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable quarantining of Homebrew-Cask's downloads #4656

Merged
merged 1 commit into from Aug 31, 2018

Conversation

Projects
None yet
6 participants
@amyspark
Copy link
Member

amyspark commented Aug 10, 2018

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This pull requests adds an additional step to the Hbc::Download class that quarantines all downloaded files.

The original proposal by @reitermarkus tinkered with Gatekeeper's download database (~/Library/Preferences/com.apple.LaunchServices.QuarantineEventsV2, see #3526), which I found really prone to breaking the main Gatekeeper policy database (i.e. /private/var/db/SystemPolicy).

Inspired in Transmission's source code, I implemented download quarantining through the use of a Swift script. In this way, we can add Gatekeeper support while keeping the code completely visible to the user.

As regards the metadata, I added a modicum of fields:

  • Agent: Homebrew-Cask
  • Download type: WEB_DOWNLOAD (see a list of the possible types here)
  • Download URL: this is obtained from the Cask url field
  • Originl URL: the Cask's homepage

This also includes:

  • tests for install, fetch, and audit, across multiple types of containers
  • the ability to disable quarantining either when Swift is not found, or when using the special flag --no-quarantine

If merged, this pull request closes Homebrew/homebrew-cask#22388.

Please review, and thanks for your comments!

@vitorgalvao vitorgalvao requested a review from Homebrew/cask Aug 10, 2018

@vitorgalvao

This comment has been minimized.

Copy link
Member

vitorgalvao commented Aug 10, 2018

If merged, this pull request closes Homebrew/homebrew-cask#22388, Homebrew/homebrew-cask#48862.

Not quite, as it’d still need the --no-quarantine flag to close the issues. But that is definitely the least important part.

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 10, 2018

This needs some tests.

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Aug 10, 2018

@vitorgalvao: which commands should --no-quarantine affect?
@reitermarkus: I've replied to your comments inline. About tests - these would need more logic to detect the com.apple.quarantine extended attribute, and besides, every time we quarantine in the tests, we'll pollute the system database.

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 10, 2018

these would need more logic to detect the com.apple.quarantine extended attribute

Yes, that's what the tests should be checking.

we'll pollute the system database

How? Everytime you download something with a browser it will also “pollute” the system database, so I don't think that is a reason not to have tests.

@vitorgalvao

This comment has been minimized.

Copy link
Member

vitorgalvao commented Aug 10, 2018

which commands should --no-quarantine affect?

Anything that downloads, so fetch, install, audit --download.

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 10, 2018

Anything that downloads, so fetch, install, audit --download.

Although this should probably be a global option, so you can set it in HOMEBREW_CASK_OPTS.

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Aug 11, 2018

I've found that after #4401's changes, the extended attributes are not preserved between ditto's unpacking and the move to the Caskroom. Upon further inspection (xattr over path and unpack_dir), FileUtils's copy_entry is not preserving the extended attributes necessary to keep the app quarantined.

@chdiza

This comment has been minimized.

Copy link
Contributor

chdiza commented Aug 11, 2018

I thought it had been decided that there will not be any swift code added to Homebrew. Someone added a swift script once and it was quickly (and correctly) removed. @MikeMcQuaid is this ringing a bell?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 11, 2018

Can this be done without a Swift script? If so, what are the other options (Objective-C, AppleScript, Ruby, Bash, etc.)? If it cannot be done without Swift or a compiled alternative (e.g. Objective-C) I'd like to propose that literally every line of the Swift script that can be instead moved into Ruby is done that way. For example, printing human readable errors can be done in the Ruby instead based on the exit code.

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Aug 11, 2018

@MikeMcQuaid , @chdiza: as I explained in Homebrew/homebrew-cask#48862, the Swift approach enables us to quickly access MacOS's APIs without having to worry about compilation, FFI, etc. The only drawback comes (as you noticed) from having to detect errors based on the exit code.

If you want to use pure Ruby, you can, but the drawbacks outweigh the benefits IMO. There are two approaches:

  1. Manipulate the SQLite database and the extended attributes by hand. This is really prone to breaking Gatekeeper's policy database, which is why I abandoned this approach.
  2. Program the logic in a Objective-C library which exposes a C API, and then use FFI to call them from within Ruby. You'd get the ease of access that comes from using Ruby, but you'd have to worry about how to compile and install the library when installing or upgrading Homebrew.
@claui

This comment has been minimized.

Copy link
Member

claui commented Aug 11, 2018

(Just a PSA, I’m only catching up on previous discussions, no strong opinion.)

Points I’ve seen made against using Swift in Homebrew:

@chdiza

This comment has been minimized.

Copy link
Contributor

chdiza commented Aug 11, 2018

Also, and correct me if I'm wrong here, but doesn't the user have to have Xcode or the CLT installed in order to run a swift script? My impression is that lots of cask users won't have those installed because they're only looking to install .apps and not cmdline stuff.

@vitorgalvao

This comment has been minimized.

Copy link
Member

vitorgalvao commented Aug 11, 2018

and Swift’s track record in language stability.

but doesn't the user have to have Xcode or the CLT installed in order to run a swift script?

It’s my understanding that from Mojave on, Swift will be (finally) included as part of macOS. That will remove the need for further software installation (Xcode/CLT) and (presumably, I’m guessing on this one) backward-incompatibility will start to be handled more carefully.

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 11, 2018

Swift’s bus factor among maintainers

I still don't understand that argument when AppleScript is the alternative. There is much more documentation for Swift than for AppleScript, whose syntax is also much more convoluted.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 11, 2018

I'd be interested at looking at an AppleScript version of these scripts. I think adding Xcode/CLT and/or Swift as a hard dependency for all of brew cask isn't something that should be taken lightly. I do agree that an officially supported solution that does not require such as Swift or AppleScript is superior to Objective-C, Ruby or C.

@claui

This comment has been minimized.

Copy link
Member

claui commented Aug 11, 2018

I still don't understand that argument when AppleScript is the alternative. There is much more documentation for Swift than for AppleScript

Well documented or not, people need to practise for a language to be useful to them.

I could probably churn out an IOKit driver in AppleScript for you in a day.1 I’d still barely manage to write a Hello World in Swift to save my life.

whose syntax is also much more convoluted.

Absolutely.


1 Dramatization

@vitorgalvao

This comment has been minimized.

Copy link
Member

vitorgalvao commented Aug 11, 2018

Well documented or not, people need to practise for a language to be useful to them.

True. But as someone who writes a bunch of AppleScript regularly (and loathes every second of it) and close to no Swift, I still feel that if I needed to do something a bit more advanced, I’d more easily find information on the latter than the former.

AppleScript’s (and JXA’s) documentation is terrible and code you find online is typically bad (as in, it does the job but it’s longer and less optimal than it could be).

All that said, if we’re going to try to rewrite the scripts in AppleScript, we should at least do it in JXA. Not only is the syntax saner, it can do some things AppleScript can’t, and it should be more auditable by more people (being JavaScript).

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 11, 2018

Well documented or not, people need to practise for a language to be useful to them.

I'd argue it's harder to practise without good documentation.

@claui

This comment has been minimized.

Copy link
Member

claui commented Aug 12, 2018

I'd argue it's harder to practise without good documentation.

Very much so! Learning AppleScript was a royal pain.

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

I would like to see a sample AppleScript implementation, more logic removed from the Swift implementation and handling the absence of Swift on the system before this is merged.

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Aug 12, 2018

@MikeMcQuaid, I'll try and port the Swift script to AppleScript (I'll need to switch to the original Objective-C classes first).
In the meanwhile, I'll change the current script according to your comments.

@claui

This comment has been minimized.

Copy link
Member

claui commented Aug 12, 2018

I'll try and port the Swift script to AppleScript

@amyspark Already done. PR incoming.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 12, 2018

@amyspark Thanks! We can iterate on the Swift code first if that's easier.

@claui Nice work.

claui added a commit to claui/brew that referenced this pull request Aug 12, 2018

Port Swift script to AppleScript
This commit ports the Swift script to AppleScript according to [1].

The Swift version is preserved so both can be inspected side by side.

[1]: Homebrew#4656 (review)

@claui claui referenced this pull request Aug 12, 2018

Closed

Sample AppleScript implementation of quarantining #4665

4 of 6 tasks complete
@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

One tiny nit but otherwise 👍 from me. Nice work, particularly on simplifying and better handling the Swift script.

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Aug 17, 2018

Every nitpick is an opportunity to improve. Thanks again for your comments @MikeMcQuaid.
@vitorgalvao, did I forget any Cask issues to close?
@reitermarkus, ping again 😅

Show resolved Hide resolved Library/Homebrew/cask/lib/hbc/artifact/moved.rb Outdated
Show resolved Hide resolved Library/Homebrew/cask/lib/hbc/exceptions.rb Outdated
Show resolved Hide resolved Library/Homebrew/cask/lib/hbc/exceptions.rb Outdated
Show resolved Hide resolved Library/Homebrew/test/cask/cli/quarantine_spec.rb Outdated
@vitorgalvao

This comment has been minimized.

Copy link
Member

vitorgalvao commented Aug 19, 2018

@vitorgalvao, did I forget any Cask issues to close?

Don’t auto-close Homebrew/homebrew-cask#48862, since we may have to revise the policy depending on the installed artifact instead of scraping it completely. After this change is live, I’ll likely replace that issue with a new one.

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Aug 24, 2018

@reitermarkus, I've completed all your requests.

As regards performance - the bottleneck is in propagate, which touches the whole Cask payload in @cask.staged_path. Therefore, I added tests to find out which containers depend on it to preserve quarantine. (To try yourself, comment out propagate in installer.rb and run brew tests --only=cask/cli/quarantine.)

According to the existing Cask installer_spec.rb test suite, we use seven types of containers:

  • dmg
  • bzip2
  • xar
  • gzip
  • pkg
  • tar.gz
  • zip

My tests show that none of them preserve quarantine by themselves; in particular, dmgs who are processed for their EULA lose their quarantine. pkgs can write anywhere; therefore, we rely on /usr/sbin/installer to respect the quarantine.
However, if one extends #4731 to uncompressed.rb, naked dmg, tar.gz and zip are fixed; the rest still need propagate.

Would it be a better option to issue a single xattr and quarantine everything in @cask.staged_path instead of forking for each child?

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 24, 2018

However, if one extends #4731 to uncompressed.rb, naked dmg, tar.gz and zip are fixed; the rest still need propagate.

I don't think this matters, because the only time we use naked archives is as a workaround.

Would it be a better option to issue a single xattr and quarantine everything in @cask.staged_path instead of forking for each child?

Forking is definitely the bottleneck here, so yes.

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Aug 24, 2018

@reitermarkus, done. Thanks for the review.

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Aug 25, 2018

Heads up: I found that some Casks ship with the wrong permissions (tried to install Fritzing for a course and quarantine failed on an internal .git folder). I've added a line that ensures all extracted files are writable previous to quarantining, but I'm not sure if this should rather go in dmg.rb.

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 27, 2018

Dmg already has this:

FileUtils.chmod "u+w", Pathname.glob(unpack_dir/"**/*").reject(&:symlink?)

So it seems this is only missing the File::FNM_DOTMATCH flag.

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Aug 30, 2018

Everyone, please let me know if this is ready for merging. Do tell me too if I should merge/rebase on master, as I forked this over two weeks ago.

(edited for English redundancy. sorry!)

@claui
Copy link
Member

claui left a comment

@amyspark Yes, a rebase would be awesome so we can test out an up-to-date version of the PR one last time.

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Aug 30, 2018

@MikeMcQuaid @reitermarkus, please let me know if I should merge, rebase, or rebase+squash this.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 30, 2018

@amyspark I'm fine with any once @claui and @reitermarkus are

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Aug 30, 2018

@claui, @reitermarkus -- I've rebased+squashed on my side. Before cleaning this pull, let me know if the current state is OK with you.

@claui

claui approved these changes Aug 30, 2018

@claui

This comment has been minimized.

Copy link
Member

claui commented Aug 30, 2018

I ran a final smoke test on a clean VM (30-ish casks). Everything seems to work great so far.
Some PKG installers didn’t quarantine their payload but that’s expected.
Three Casks failed to install but I could confirm those errors are unrelated.
LGTM :shipit:

@claui

This comment has been minimized.

Copy link
Member

claui commented Aug 31, 2018

@amyspark Ready for cleanup 😉

@amyspark

This comment has been minimized.

Copy link
Member

amyspark commented Aug 31, 2018

Ok everyone, incoming rebase+squash! 🚀

@claui claui removed the do not merge label Aug 31, 2018

@claui

This comment has been minimized.

Copy link
Member

claui commented Aug 31, 2018

Thanks a lot @amyspark for tackling this! This feature was much needed.

@claui claui merged commit 47d3bbe into Homebrew:master Aug 31, 2018

3 checks passed

codecov/patch 81.63% of diff hit (target 71.23%)
Details
codecov/project 71.33% (+0.11%) compared to 6e31c66
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@claui

This comment has been minimized.

Copy link
Member

claui commented Aug 31, 2018

BONAPARTE – Quarantine

@amyspark amyspark deleted the amyspark:com-apple-quarantine branch Aug 31, 2018

@lock lock bot added the outdated label Sep 30, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Sep 30, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.