8000 Enable quarantining of Homebrew-Cask's downloads by amyspark · Pull Request #4656 · Homebrew/brew · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Enable quarantining of Homebrew-Cask's downloads #4656

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 31, 2018
Merged

Enable quarantining of Homebrew-Cask's downloads #4656

merged 1 commit into from
Aug 31, 2018

Conversation

amyspark
Copy link
Contributor
@amyspark amyspark commented Aug 10, 2018 •
edited
Loading
  • 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 a team August 10, 2018 18:25
@vitorgalvao
Copy link
Contributor

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
Copy link
Member

This needs some tests.

reitermarkus
reitermarkus requested changes Aug 10, 2018
View reviewed changes
@amyspark
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor

which commands should --no-quarantine affect?

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

@reitermarkus
Copy link
Member

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
Copy link
Contributor Author

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
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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Contributor
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
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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor
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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor
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
MikeMcQuaid requested changes Aug 12, 2018
View reviewed changes
Copy link
Member
@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@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
Copy link
Contributor
claui commented Aug 12, 2018

I'll try and port the Swift script to AppleScript

@amyspark Already done. PR incoming.

@MikeMcQuaid
Copy link
Member

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

@claui Nice work.

@claui claui mentioned this pull request Aug 12, 2018
Closed
6 tasks
@amyspark
Copy link
Contributor Author

Thanks for your review @MikeMcQuaid! Please let me know if everything is OK for you.

MikeMcQuaid
MikeMcQuaid approved these changes Aug 16, 2018
View reviewed changes
Copy link
Member
@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@vitorgalvao vitorgalvao mentioned this pull request Aug 17, 2018
Closed
@amyspark
Copy link
Contributor Author

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 😅

reitermarkus
reitermarkus requested changes Aug 18, 2018
View reviewed changes
@vitorgalvao
Copy link
Contributor

@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
Copy link
Contributor Author

@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 -->.)

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
Copy link
Member

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
Copy link
Contributor Author

@reitermarkus, done. Thanks for the review.

@amyspark
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author
amyspark commented Aug 30, 2018 •
edited
Loading

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
claui suggested changes Aug 30, 2018
View reviewed changes
Copy link
Contributor
@claui claui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@amyspark
Copy link
Contributor Author

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

@MikeMcQuaid
Copy link
Member

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

@amyspark
Copy link
Contributor Author

@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
Copy link
Contributor
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
Copy link
Contributor
claui commented Aug 31, 2018

@amyspark Ready for cleanup 😉

@amyspark
Copy link
Contributor Author

Ok everyone, incoming rebase+squash! 🚀

@claui
Copy link
Contributor
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
@claui
Copy link
Contributor
claui commented Aug 31, 2018

BONAPARTE – Quarantine

@amyspark amyspark deleted the com-apple-quarantine branch August 31, 2018 13:52
@jabenninghoff jabenninghoff mentioned this pull request Sep 3, 2018
Closed
5 tasks
@lock lock bot added the outdated PR was locked due to age 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.
Reviewers

@vitorgalvao vitorgalvao vitorgalvao left review comments

@chdiza chdiza chdiza left review comments

@MikeMcQuaid MikeMcQuaid MikeMcQuaid approved these changes

@claui claui claui approved these changes

@reitermarkus reitermarkus reitermarkus approved these changes

Assignees
No one assigned
Labels
outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add quarantine attribute to downloads
7 participants
@amyspark @vitorgalvao @reitermarkus @chdiza @MikeMcQuaid @claui @Kentzo
0