-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
cask/audit: update signing checks for app, binary, and pkg #17031
Conversation
118e066
to
7cd11b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work so far, thanks @krehel!
This is still not complete, as this will still fail some valid Casks (such as GitHub Desktop)
What's the failure? A (unnotarized) binary? If so: I think we should just make that check strict only.
Library/Homebrew/cask/audit.rb
Outdated
when Artifact::App | ||
system_command("spctl", args: ["--assess", "--type", "execute", path], print_stderr: false) | ||
when Artifact::Binary | ||
system_command("codesign", args: ["-vvvv", "-R=notarized", "--check-notarization", path], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think checking for notarisation should be a separate (strict) check. My understanding is it's not required for ARM/Gatekeeper/Quarantine.
The message below can also probably remove "notarize" from it, too.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
@krehel What's the latest here? |
@MikeMcQuaid sorry about that. Will pick this up and see if I can get it over the line. Need to solve some corner cases involving binaries. |
7cd11b7
to
30bf2c5
Compare
30bf2c5
to
344a502
Compare
@@ -22,7 +22,7 @@ def extract_nestedly(to: nil, basename: nil, verbose: false, prioritize_extensio | |||
|
|||
sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).returns(T.untyped) } | |||
def extract_to_dir(unpack_dir, basename:, verbose: false) | |||
FileUtils.cp path, unpack_dir/basename, preserve: true, verbose: | |||
FileUtils.cp path, unpack_dir/basename.sub(/^[\da-f]{64}--/, ""), preserve: true, verbose: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed during the audit signing that this caused audit to fail as the paths didn't exist. This applied to direct binary downloads only. Making this adjustment now causes the paths to be found.
|
||
next unless path.exist? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was silently skipping over paths not found, which was causing us not to audit direct binary downloads properly.
when Artifact::Pkg | ||
system_command("spctl", args: ["--assess", "--type", "install", path], print_stderr: false) | ||
when Artifact::App | ||
system_command("spctl", args: ["--assess", "--type", "execute", path], print_stderr: false) | ||
when Artifact::Binary | ||
system_command("codesign", args: ["--verify", path], print_stderr: false) | ||
else | ||
add_error "Unknown artifact type: #{artifact.class}", location: cask.url.location | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested against a range of applications and binaries, this reports the same as tools like Apparency and Whatsyoursign.
Happy to publish a results report run against applications locally for review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I believe you, great work!
@@ -482,13 +482,25 @@ def audit_signing | |||
odebug "Auditing signing" | |||
|
|||
extract_artifacts do |artifacts, tmpdir| | |||
is_container = artifacts.any? { |a| a.is_a?(Artifact::App) || a.is_a?(Artifact::Pkg) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's a standalone binary (typically a shell script) included in or with an application bundle, we can skip auditing it.
Many times those scripts aren't signed, but if the application is we'll consider it trusted. If the app isn't trusted, it will fail the normal check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @krehel, looks good to me!
@@ -482,13 +482,25 @@ def audit_signing | |||
odebug "Auditing signing" | |||
|
|||
extract_artifacts do |artifacts, tmpdir| | |||
is_container = artifacts.any? { |a| a.is_a?(Artifact::App) || a.is_a?(Artifact::Pkg) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense!
when Artifact::Pkg | ||
system_command("spctl", args: ["--assess", "--type", "install", path], print_stderr: false) | ||
when Artifact::App | ||
system_command("spctl", args: ["--assess", "--type", "execute", path], print_stderr: false) | ||
when Artifact::Binary | ||
system_command("codesign", args: ["--verify", path], print_stderr: false) | ||
else | ||
add_error "Unknown artifact type: #{artifact.class}", location: cask.url.location | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I believe you, great work!
Thanks @MikeMcQuaid - apologies it took a bit to pick back up, will do better going forward ❤️ |
@krehel Zero apologies needed, thanks for the PR! |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?POC changes to address some issues in auditing Casks, where we are failing some valid Casks.
This doc used as the source material for updating the checks. Based on it, the checks should be different where it is an app, a pkg, or a binary.
This is still not complete, as this will still fail some valid Casks (such as GitHub Desktop), and we need to implement (IMHO) some checking directly DMG's to check signature. But hopeful this kickstarts a conversation.