influxdata / telegraf

Agent for collecting, processing, aggregating, and writing metrics, logs, and other arbitrary data.
https://influxdata.com/telegraf
MIT License
14.45k stars 5.55k forks source link

Mac app code-signing is unusually fragile #15646

Closed llamafilm closed 1 month ago

llamafilm commented 1 month ago

System info

Telegraf 1.31.1, MacOS 13.6.7

Steps to reproduce

When Telegraf MacOS app is repackaged using tools like Autopkg or quickpkg, the code signature breaks. Although the signature if technically correct on the original app in the release DMG, the current method of code-signing is very unusual and info.plist is not capitalized. @rsaldinger (creator of Apparency) explains it in detail better than I can in this comment.

Expected behavior

I should be able to repackage the app and maintain a valid code signature.

Actual behavior

Here's an example:

% quickpkg /Volumes/Telegraf/Telegraf.app --output /tmp/Telegraf.pkg
/tmp/Telegraf.pkg

% pkgutil --expand-full /tmp/Telegraf.pkg /tmp/Telegraf                                                                     

% codesign --verify --deep -vvv -r- /tmp/Telegraf/Payload/Telegraf.app 
/tmp/Telegraf/Payload/Telegraf.app: code object is not signed at all
In subcomponent: /private/tmp/Telegraf/Payload/Telegraf.app/Contents/info.plist
rsaldinger commented 1 month ago

The core problem here is that mac-signing.sh creates the Info.plist with the non-standard case of "info.plist". Most of macOS doesn't care about the difference, but the code signing infrastructure is an exception.

The code signing "resource rules" say that any non-standard files at the top level of the bundle (immediately under "Contents") are considered "nested code" and must be independently signed. Because those rules recognize "Info.plist" but not "info.plist", this codesign --deep will decide that it must sign the "info.plist" file separately first. That signature gets stored in extended attributes on the info.plist file.

The subsequent verification works fine, as long as the extended attributes are preserved. But these are almost always lost in the processing of packaging and then installing. (Apple doesn't even document the package flag that would preserve them, much less preserve them by default.) So, after installing, code signature verification still wants "info.plist" to be independently signed, but it no longer is, resulting in the confusing error above.

Just using the standard case for Info.plist ought to resolve this issue.

(As an aside, the separate signing of the telegraf_entry_mac script here is unnecessary, because it's the main executable of the containing bundle, and so when the bundle is signed here, that signature [stored as xattrs] is going to get replaced anyway, per --force, with files in Contents/_CodeSignature.)

powersj commented 1 month ago

Hi,

Thanks for bringing this to our attention. Before making any changes to signing, I have couple questions:

When Telegraf MacOS app is repackaged using tools

What is the use case for repackaging? I would like to understand what you were doing and how you came across this?

"Info.plist" but not "info.plist"

The solution then is to change the capitalization of the file? That is all that is required? It does seem that the Code Signing Guide references a capitalized Info.plist.

What is the impact on existing users who upgrade if we make this change?

(As an aside, the separate signing of the telegraf_entry_mac script here is unnecessary, because it's the main executable of the containing bundle, and so when the bundle is signed here, that signature [stored as xattrs] is going to get replaced anyway, per --force, with files in Contents/_CodeSignature.)

Do we still need to sign the binary first? That means we sign the binary, .app and .dmg?

I have put up https://github.com/influxdata/telegraf/pull/15647 with what I think are the changes, but I would still like to understand the use-case of this situation as well.

llamafilm commented 1 month ago

I'm trying to deploy Telegraf to a bunch of machines via munki. Autopkg allows me to add a LaunchDaemon and customize the Telegraf config file based on the machine's Jamf Config Profile.

rsaldinger commented 1 month ago

The solution then is to change the capitalization of the file? That is all that is required? It does seem that the Code Signing Guide references a capitalized Info.plist.

Yes, changing the case of the file name is the important thing here. "Info.plist" is definitely the standard case for this file. (That macOS cares about the case here is stupid, but the chances of getting Apple to fix that are minuscule.)

After making this change, the output of a codesign verify should no longer reference the plist. That is, in this:

$ codesign --verify --verbose=2 Telegraf.app
--prepared:/Users/randy/Development/Telegraf.app/Contents/info.plist
--validated:/Users/randy/Development/Telegraf.app/Contents/info.plist
Telegraf.app: valid on disk
Telegraf.app: satisfies its Designated Requirement

those "prepared" and "validated" lines should go away, because the plist is no longer being separately signed/verified.

Another way to check this is to ensure that the signature still verifies even if all xattrs are stripped, i.e.:

$ xattr -r -c Telegraf.app
$ codesign --verify --verbose=2 Telegraf.app

should still validate.

What is the impact on existing users who upgrade if we make this change?

As long as the old app bundle is completely replaced by the new one, there shouldn't be any potential for problems. (I assume there is no Sparkle-like patching going on here?)

Do we still need to sign the binary first? That means we sign the binary, .app and .dmg?

Correct. The signing of the actual binary is not redundant. (The --deep signing of the app bundle won't do it, because it lives in "Resources" and code signing won't automatically find code there.)

powersj commented 1 month ago

munki

TIL

(I assume there is no Sparkle-like patching going on here?)

Correct and thanks for the details!

ok I'll get the PR reviewed and I will ask one of you to validate a nightly build is now signed correctly.

llamafilm commented 1 month ago

Sounds good, thanks Josh and Randy for your input!

rsaldinger commented 1 month ago

@powersj I may be wrong here, but it looks like the diff removes not only the signing of the telegraf_entry_mac script, but also the copying of the telegraf_entry_mac script (to Contents/MacOS in the bundle). The signing step was unnecessary, but the script itself still needs to be put into the MacOS directory (unless that's happening somewhere else?).

powersj commented 1 month ago

woops, thank you for reviewing the diff! I've re-added that line.