cask/audit: update signing checks for app, binary, and pkg #17031

krehel · 2024-04-04T19:47:02Z

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 typecheck with your changes locally?
Have you successfully run 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.

MikeMcQuaid

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.

MikeMcQuaid · 2024-04-05T07:31:46Z

Library/Homebrew/cask/audit.rb

+          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],


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.

github-actions · 2024-04-27T00:11:58Z

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.

github-actions · 2024-05-21T00:12:12Z

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.

MikeMcQuaid · 2024-05-21T07:39:38Z

@krehel What's the latest here?

krehel · 2024-05-22T02:54:50Z

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

krehel · 2024-05-22T14:49:10Z

Library/Homebrew/unpack_strategy/uncompressed.rb

@@ -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:


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.

krehel · 2024-05-22T14:49:48Z

Library/Homebrew/cask/audit.rb


-          next unless path.exist?


This was silently skipping over paths not found, which was causing us not to audit direct binary downloads properly.

krehel · 2024-05-22T14:52:10Z

Library/Homebrew/cask/audit.rb

+          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


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.

No, I believe you, great work!

krehel · 2024-05-22T14:54:07Z

Library/Homebrew/cask/audit.rb

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


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.

Makes sense!

MikeMcQuaid

Thanks @krehel, looks good to me!

MikeMcQuaid · 2024-05-22T16:37:27Z

Library/Homebrew/cask/audit.rb

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


Makes sense!

MikeMcQuaid · 2024-05-22T16:37:45Z

Library/Homebrew/cask/audit.rb

+          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


No, I believe you, great work!

krehel · 2024-05-22T16:56:21Z

Thanks @MikeMcQuaid - apologies it took a bit to pick back up, will do better going forward ❤️

MikeMcQuaid · 2024-05-22T18:18:01Z

@krehel Zero apologies needed, thanks for the PR!

krehel changed the title ~~cask/audit.rb: update signing checks for app, binary, and pkg~~ cask/audit: update signing checks for app, binary, and pkg Apr 4, 2024

krehel force-pushed the update-artifact-audit branch from 118e066 to 7cd11b7 Compare April 4, 2024 19:49

MikeMcQuaid reviewed Apr 5, 2024

View reviewed changes

github-actions bot added the stale No recent activity label Apr 27, 2024

krehel removed the stale No recent activity label Apr 29, 2024

github-actions bot added the stale No recent activity label May 21, 2024

krehel force-pushed the update-artifact-audit branch from 7cd11b7 to 30bf2c5 Compare May 22, 2024 14:25

cask/audit.rb: update signing checks for app, binary, and pkg

344a502

krehel force-pushed the update-artifact-audit branch from 30bf2c5 to 344a502 Compare May 22, 2024 14:29

krehel removed the stale No recent activity label May 22, 2024

krehel commented May 22, 2024

View reviewed changes

MikeMcQuaid approved these changes May 22, 2024

View reviewed changes

MikeMcQuaid merged commit 82591d1 into Homebrew:master May 22, 2024
25 checks passed

krehel deleted the update-artifact-audit branch May 22, 2024 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cask/audit: update signing checks for app, binary, and pkg #17031

cask/audit: update signing checks for app, binary, and pkg #17031

krehel commented Apr 4, 2024 •

edited

MikeMcQuaid left a comment

MikeMcQuaid Apr 5, 2024

github-actions bot commented Apr 27, 2024

github-actions bot commented May 21, 2024

MikeMcQuaid commented May 21, 2024

krehel commented May 22, 2024 •

edited

krehel May 22, 2024

krehel May 22, 2024

krehel May 22, 2024

MikeMcQuaid May 22, 2024

krehel May 22, 2024

MikeMcQuaid May 22, 2024

MikeMcQuaid left a comment

MikeMcQuaid May 22, 2024

MikeMcQuaid May 22, 2024

krehel commented May 22, 2024

MikeMcQuaid commented May 22, 2024 •

edited

cask/audit: update signing checks for app, binary, and pkg #17031

cask/audit: update signing checks for app, binary, and pkg #17031

Conversation

krehel commented Apr 4, 2024 • edited

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 27, 2024

github-actions bot commented May 21, 2024

MikeMcQuaid commented May 21, 2024

krehel commented May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krehel commented May 22, 2024

MikeMcQuaid commented May 22, 2024 • edited

krehel commented Apr 4, 2024 •

edited

krehel commented May 22, 2024 •

edited

MikeMcQuaid commented May 22, 2024 •

edited