Skip to content
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

Codesign node binary for macOS #11936

Closed
raphaelokon opened this issue Mar 20, 2017 · 28 comments
Closed

Codesign node binary for macOS #11936

raphaelokon opened this issue Mar 20, 2017 · 28 comments
Labels
build Issues and PRs related to build files or the CI. macos Issues and PRs related to the macOS platform / OSX.

Comments

@raphaelokon
Copy link
Contributor

Hi there.

I recently had a quick chat with @MylesBorins via 🐦 who encouraged me to post the issue here.

Tools like Little Flocker reports node with an invalid Developer ID whenever I run it on my machine. So I checked the node binary with codesign -vvvv $node_bin to see if it is code signed – which returns code object is not signed at all.

Is it possible to have the macOS Binaries signed by a Developer ID? I know that the .pkg/.tar.gz are signed, but would be great to have another layer of confidence.

I found this two issues in relation: https://github.com/nodejs/node/issues?q=codesign

@mscdex mscdex added build Issues and PRs related to build files or the CI. macos Issues and PRs related to the macOS platform / OSX. labels Mar 20, 2017
@gibfahn
Copy link
Member

gibfahn commented Mar 20, 2017

I assume this would be something we could do for at least windows as well?

@mscdex
Copy link
Contributor

mscdex commented Mar 20, 2017

@gibfahn I thought Windows binaries already were being signed, hence the 'nosign' command-line option for vcbuild?

@gibfahn
Copy link
Member

gibfahn commented Mar 20, 2017

Yeah okay, we are signing both the node.exe binary and the .msi installer. Didn't realise we were doing both.

@raphaelokon
Copy link
Contributor Author

raphaelokon commented Mar 20, 2017

@gibfahn That's cool. I just saw this here in the Makefiledoes that mean that the macOS installer is getting signed as well? just checked it myself, and indeed the .pkg seems to be signed when inspected with pkgutil --check-signature

@raphaelokon
Copy link
Contributor Author

If I can do anything to push this forward please let me know.

@gibfahn
Copy link
Member

gibfahn commented Mar 22, 2017

It looks like the package signing on macOS is done by the two scripts linked to from the Makefile as you pointed out. osx-codesign.sh signs the binary, and osx-productsign.sh signs the pkg file.

Are you sure the binaries aren't signed? I ran codesign -dvvv ./node on a node binary I built locally, and I get:

➜  codesign -dvvv ./node
./node: code object is not signed at all

If I run the same thing on a node binary from the pkg or the tar.gz, I get:

➜  codesign -dvvv `which node`
Executable=/Users/gib/.nvm/versions/node/v6.10.0/bin/node
Identifier=node-55554944ea42999f6a9f32508a5ebb5ac25b716a
Format=Mach-O thin (x86_64)
CodeDirectory v=20100 size=240350 flags=0x2(adhoc) hashes=7506+2 location=system
Hash type=sha256 size=32
CandidateCDHash sha1=9834f282795fc5294e7cdadf1481e85765a1a0fd
CandidateCDHash sha256=529de940acb145410eb799453cf00243f79b1f49
Hash choices=sha1,sha256
CDHash=529de940acb145410eb799453cf00243f79b1f49
Signature=adhoc
Info.plist=not bound
TeamIdentifier=not set
Sealed Resources=none
Internal requirements count=0 size=12

I guess cc/ @rvagg as he seems to be the only person to have ever touched those files.

@gibfahn
Copy link
Member

gibfahn commented Mar 22, 2017

Also cc/ @nodejs/platform-macos

@gibfahn
Copy link
Member

gibfahn commented Mar 22, 2017

I didn't actually install the pkg (don't want to mess with the multiple versions of node I have installed already), I just did:

curl -O https://nodejs.org/dist/v6.10.1/node-v6.10.1.pkg
pkgutil --expand node-v6.10.1.pkg nodepkg
cd nodepkg/local.pkg
tar -xf Payload
codesign -vvvv bin/node

Output:

➜  bin codesign -vvvv ./node                                       ~/nodepkg/local.pkg/bin
./node: valid on disk
./node: satisfies its Designated Requirement

@raphaelokon
Copy link
Contributor Author

raphaelokon commented Mar 22, 2017

@gibfahn Thanks for the detailed response. Yeah, running codesign -dvvv actually gave me the same output as yours.
Not sure why then, but Little Flocker still tells me that the 'Code Signature Invalid'. Tracking it down a bit … it seems like its SecStaticCodeCheckValidityWithErrors returns a err​Sec​CSSignature​Invalid error.

@raphaelokon
Copy link
Contributor Author

According to the Code Signing Guide this test mimics what macOS Gatekeeper does

→ codesign --verify --deep --strict --verbose=2 `which node`

Expected and actual result:

./node: valid on disk
./node: satisfies its Designated Requirement

So maybe this is just a local issue with Little Flocker.

@raphaelokon
Copy link
Contributor Author

raphaelokon commented Apr 12, 2017

I am reopening this issues because I have done some more tests and been monitoring this.

I downloaded the node-v7.9.0-darwin-x64.tar.gz, unzipped it and ran

→  codesign -dvvv ./node
./node: code object is not signed at all

When downloading the node-v7.9.0.pkg and running

pkgutil --expand node-v7.9.0.pkg nodepkg
cd nodepkg/local.pkg
tar -xvf Payload
cd bin
codesign -vvvv ./node

→  bin codesign -vvvv ./node                                      
./node: valid on disk
./node: satisfies its Designated Requirement

There seems to be a regression here or the .tar.gz is not being codesigned at all.

Any clues? Can you test it this way as well @gibfahn?

@raphaelokon raphaelokon reopened this Apr 12, 2017
@raphaelokon
Copy link
Contributor Author

/cc @rvagg @isaacs

@gibfahn
Copy link
Member

gibfahn commented Apr 12, 2017

@raphaelokon it seems you're right, only some of the binaries are being signed. Using the highly unscientific method of just looking at whatever's in my ~/.nvm directory, it seems some are signed and some aren't.

Test script:

cd ~/.nvm/versions/node
for i in *; do echo ">>> $i"; codesign -d $i/bin/node; done

Output:

>>> v4.7.2
v4.7.2/bin/node: code object is not signed at all
>>> v5.12.0
v5.12.0/bin/node: code object is not signed at all
>>> v5.7.0
v5.7.0/bin/node: code object is not signed at all
>>> v6.10.0
Executable=/Users/gib/.nvm/versions/node/v6.10.0/bin/node
>>> v6.10.1
Executable=/Users/gib/.nvm/versions/node/v6.10.1/bin/node
>>> v6.4.0
v6.4.0/bin/node: code object is not signed at all
>>> v6.9.2
Executable=/Users/gib/.nvm/versions/node/v6.9.2/bin/node
>>> v7.3.0
v7.3.0/bin/node: code object is not signed at all
>>> v7.4.0
Executable=/Users/gib/.nvm/versions/node/v7.4.0/bin/node
>>> v7.6.0
v7.6.0/bin/node: code object is not signed at all
>>> v7.8.0
v7.8.0/bin/node: code object is not signed at all

Detailed output:

If you use codesign -dvvv you get more info.

>>> v7.4.0
Executable=/Users/gib/.nvm/versions/node/v7.4.0/bin/node
Identifier=node-5555494415470c8751ea3e66af83825f87f0f64a
Format=Mach-O thin (x86_64)
CodeDirectory v=20100 size=271198 flags=0x2(adhoc) hashes=8470+2 location=system
Hash type=sha256 size=32
CandidateCDHash sha1=6eb9ec8431730bd1c80b1ab7fd67df5570ea26ca
CandidateCDHash sha256=af61ae735715a0df417dadbb57b9329938619279
Hash choices=sha1,sha256
CDHash=af61ae735715a0df417dadbb57b9329938619279
Signature=adhoc
Info.plist=not bound
TeamIdentifier=not set
Sealed Resources=none
Internal requirements count=0 size=12

@gibfahn
Copy link
Member

gibfahn commented Apr 12, 2017

@nodejs/release , any ideas why this might happen?

@raphaelokon
Copy link
Contributor Author

Great test @gibfahn – I also noticed that the binary from the pkg is slightly bigger than the one from the tar.gz 🙃

@raphaelokon
Copy link
Contributor Author

Any news on this one?

@gibfahn
Copy link
Member

gibfahn commented May 14, 2017

@nodejs/release it looks like releases aren't being signed properly, the pkg is signed but the tarball isn't.

Pkg:

curl -O https://nodejs.org/dist/v7.10.0/node-v7.10.0.pkg
pkgutil --expand node-v7.10.0.pkg nodepkg
cd nodepkg/local.pkg
tar -xf Payload
codesign -d bin/node

Output:

Executable=/Users/gib/tmp/nodepkg/local.pkg/bin/node

Tarball:

curl -O https://nodejs.org/dist/v7.10.0/node-v7.10.0-darwin-x64.tar.gz
tar -xf node-v7.10.0-darwin-x64.tar.gz
codesign -d node-v7.10.0-darwin-x64/bin/node

Output:

node-v7.10.0-darwin-x64/bin/node: code object is not signed at all

@gibfahn
Copy link
Member

gibfahn commented May 14, 2017

@evanlucas I think you did v7.10.0? You also did v7.4.0, which is the last release I can find which was signed.

Any ideas about why this would be happening?

@evanlucas
Copy link
Contributor

That is strange, codesigning is part of the build step in the Makefile, so it isn't something I'm manually doing. It is done by the release machines.

@raphaelokon
Copy link
Contributor Author

So is there a regression in terms of a release machine sometimes signing and sometimes not signing the bin at all?

@gibfahn
Copy link
Member

gibfahn commented May 24, 2017

So is there a regression in terms of a release machine sometimes signing and sometimes not signing the bin at all?

Yeah, it seems like it. Unfortunately I don't think anyone has dug into this.

Maybe cc/ @nodejs/build

@raphaelokon
Copy link
Contributor Author

Same problem still occurs for v8.0.0 …

codesign -d node-v8.0.0-darwin-x64/bin/node 
node-v8.0.0-darwin-x64/bin/node: code object is not signed at all

@raphaelokon
Copy link
Contributor Author

Same for v8.1.0 as well :(

@raphaelokon
Copy link
Contributor Author

And again for v8.1.2 the same as above

@jbergstroem
Copy link
Member

We are looking into setting up additional macos hosts; with that also "ansible:fying" the process. Hopefully that can make the setup part more transparent (and pull requestable).

@raphaelokon
Copy link
Contributor Author

Issue still present in v8.1.4

@evanlucas
Copy link
Contributor

Thanks for continuing to let us know @raphaelokon. I finally got around to looking into this. This should be fixed by #14179. :]

@raphaelokon
Copy link
Contributor Author

Cheers @evanlucas – and sorry for keeping bugging you guys :D

evanlucas added a commit to evanlucas/node that referenced this issue Jul 19, 2017
Previously, we were signing the binary that was released in the .pkg,
but not the binary released in the tarball.

PR-URL: nodejs#14179
Fixes: nodejs#11936
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this issue Jul 22, 2017
Previously, we were signing the binary that was released in the .pkg,
but not the binary released in the tarball.

PR-URL: #14179
Fixes: #11936
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this issue Jul 24, 2017
Previously, we were signing the binary that was released in the .pkg,
but not the binary released in the tarball.

PR-URL: #14179
Fixes: #11936
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 16, 2017
Previously, we were signing the binary that was released in the .pkg,
but not the binary released in the tarball.

PR-URL: #14179
Fixes: #11936
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 16, 2017
Previously, we were signing the binary that was released in the .pkg,
but not the binary released in the tarball.

PR-URL: #14179
Fixes: #11936
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 16, 2017
Previously, we were signing the binary that was released in the .pkg,
but not the binary released in the tarball.

PR-URL: #14179
Fixes: #11936
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 3, 2017
Previously, we were signing the binary that was released in the .pkg,
but not the binary released in the tarball.

PR-URL: #14179
Fixes: #11936
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 5, 2017
Previously, we were signing the binary that was released in the .pkg,
but not the binary released in the tarball.

PR-URL: #14179
Fixes: #11936
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
abernix pushed a commit to meteor/node-legacy that referenced this issue Sep 28, 2017
Previously, we were signing the binary that was released in the .pkg,
but not the binary released in the tarball.

PR-URL: nodejs/node#14179
Fixes: nodejs/node#11936
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
abernix pushed a commit to meteor/node-legacy that referenced this issue Oct 24, 2017
Previously, we were signing the binary that was released in the .pkg,
but not the binary released in the tarball.

PR-URL: nodejs/node#14179
Fixes: nodejs/node#11936
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 25, 2017
Previously, we were signing the binary that was released in the .pkg,
but not the binary released in the tarball.

PR-URL: #14179
Fixes: #11936
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
abernix pushed a commit to meteor/node-legacy that referenced this issue Oct 27, 2017
Previously, we were signing the binary that was released in the .pkg,
but not the binary released in the tarball.

PR-URL: nodejs/node#14179
Fixes: nodejs/node#11936
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. macos Issues and PRs related to the macOS platform / OSX.
Projects
None yet
Development

No branches or pull requests

5 participants