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

build: codesign tarball binary on macOS #14179

Merged
merged 2 commits into from
Jul 19, 2017

Conversation

evanlucas
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

build,tools

Fixes: #11936

I ran it locally and confirmed that it does codesign the binary when make binary is run. I also confirmed that the change to make pkg works as well.

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. macos Issues and PRs related to the macOS platform / OSX. tools Issues and PRs related to the tools directory. labels Jul 12, 2017
@gibfahn
Copy link
Member

gibfahn commented Jul 12, 2017

Do you know why it seemed to be intermittently unsigned (see #11936 (comment))?

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

LGTM.
Can't verify it actually works.

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM if we can do a test build and make sure tarball is signed that would be great

@evanlucas
Copy link
Contributor Author

@gibfahn I'm not seeing any of the node binaries from the tarball as being signed. From the looks of the Makefile, I'm not sure how it could be possible that they are either. Unless make binary isn't being used in the release builds?

Here is one that you were showing was signed, but I can't reproduce:

(S) [~]
:] (v8.1.4) ➜ n v6.10.0                                                                                                 

     install : node-v6.10.0
       mkdir : /usr/local/n/versions/node/6.10.0
       fetch : https://nodejs.org/dist/v6.10.0/node-v6.10.0-darwin-x64.tar.gz
######################################################################## 100.0%
   installed : v6.10.0


(S) [~]
:] (v6.10.0) ➜ codesign -d (which node)                                                                                 
/usr/local/bin/node: code object is not signed at all

@evanlucas
Copy link
Contributor Author

@MylesBorins a test build would be great. Do I just use the same method as when cutting a release?

@MylesBorins
Copy link
Contributor

yup, just select test instead of release in the drop down

You will need to include the date :D

@evanlucas
Copy link
Contributor Author

Trying out a test build at https://ci-release.nodejs.org/job/iojs+release/1837/

@evanlucas
Copy link
Contributor Author

Ok, so looks like the job that runs make binary-upload would need the CODESIGN_CERT env var the same way the job that make pkg-upload does.

/cc @nodejs/build could we possibly get that added?

@gibfahn
Copy link
Member

gibfahn commented Jul 12, 2017

@evanlucas seems to work for me

image

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

I think it's because you're using () and thus executing in a subshell. Does codesign -d $(which node) work?

@evanlucas
Copy link
Contributor Author

@gibfahn no I use fish, so () is fish's version of $()

Pulling that same file is not working for me still. What version of macOS are you running?

(S) [/private/tmp/node-v6.10.0-darwin-x64]
:] (v8.1.4) ➜ ./bin/node -v                                                                                             
v6.10.0

(S) [/private/tmp/node-v6.10.0-darwin-x64]
:] (v8.1.4) ➜ codesign -d bin/node                                                                                      
bin/node: code object is not signed at all

@gibfahn
Copy link
Member

gibfahn commented Jul 12, 2017

10.12.5

image

@joaocgreis
Copy link
Member

joaocgreis commented Jul 13, 2017

Confirmed that CODESIGN_CERT was not defined for the job that generates the OSX tarball and signing was skipped in @evanlucas test run. Added the variable, here is another test run: https://ci-release.nodejs.org/job/iojs+release/1841/ https://ci-release.nodejs.org/job/iojs+release/1842/ - will overwrite the previous test run at https://nodejs.org/download/test/v9.0.0-test20170712056c85c8df/

EDIT: Looks like it worked. Please confirm the binary is signed.

@evanlucas
Copy link
Contributor Author

(P) [/Volumes/code/help/services/org-customer-service]
:] (v8.1.4) ➜ cd /Users/evan/Downloads/node-v9.0.0-test20170712056c85c8df-darwin-x64                                                                                                               (fixroutes) 

(P) [~/Downloads/node-v9.0.0-test20170712056c85c8df-darwin-x64]
:] (v8.1.4) ➜ codesign -d bin/node                                                                                                                                                                             
Executable=/Users/evan/Downloads/node-v9.0.0-test20170712056c85c8df-darwin-x64/bin/node

woo hoo!!!! Thanks @joaocgreis!!!!

@evanlucas
Copy link
Contributor Author

@gibfahn I'm at a loss here. Those same commands are not working for me locally.

screen shot 2017-07-13 at 7 10 23 am

@gdams
Copy link
Member

gdams commented Jul 17, 2017

here is the output when I run the same commands:

➜  tmp rm -rf node*                                                           
➜  tmp curl -O https://nodejs.org/dist/v6.10.0/node-v6.10.0-darwin-x64.tar.gz 
#  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 11.5M  100 11.5M    0     0  2850k      0  0:00:04  0:00:04 --:--:-- 2850k
➜  tmp tar -xf node-v6.10.0-darwin-x64.tar.gz 
➜  tmp codesign -d node-v6.10.0-darwin-x64/bin/node 
Executable=/Users/george/tmp/node-v6.10.0-darwin-x64/bin/node

@evanlucas
Copy link
Contributor Author

I'm really confused on how that's possible...both of my macOS machines are showing that it is not signed. What confuses me the most though, is if they are signed, how are they being signed??

@evanlucas
Copy link
Contributor Author

@gibfahn @gdams any issue with me going ahead and landing this? I really don't have an explanation on how those are showing as signed for yall.

Copy link
Member

@gdams gdams left a comment

Choose a reason for hiding this comment

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

LGTM

Allow passing the prefix in via the PKGDIR env var. This will allow us
to use this same script to codesign the binary tarball.

PR-URL: nodejs#14179
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]>
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]>
@evanlucas evanlucas closed this Jul 19, 2017
@evanlucas evanlucas deleted the codesigntarballosx branch July 19, 2017 21:49
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
Allow passing the prefix in via the PKGDIR env var. This will allow us
to use this same script to codesign the binary tarball.

PR-URL: #14179
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 pull request 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 pull request Sep 3, 2017
Allow passing the prefix in via the PKGDIR env var. This will allow us
to use this same script to codesign the binary tarball.

PR-URL: #14179
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 pull request 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 pull request Sep 5, 2017
Allow passing the prefix in via the PKGDIR env var. This will allow us
to use this same script to codesign the binary tarball.

PR-URL: #14179
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 pull request 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]>
MylesBorins added a commit that referenced this pull request Sep 5, 2017
This LTS release comes with 152 commits. This includes 75 which are
test related, 25 which are doc related, 21 which are build / tool
related and 3 commits which are updates to dependencies.

Notable Changes:

* build:
 - Codesigning is fixed on macOS (Evan Lucas)
   #14179
* deps:
 - Snapshots are turned back on!!! (Yang Guo)
   #14385
* path:
 - win32 volume-relative paths are working again! (Timothy Gu)
   #14440
* tools:
 - v6.x can now build with ICU 59 (Steven R. Loomis)
   #12078

PR-URL: #14852
MylesBorins added a commit that referenced this pull request Sep 5, 2017
This LTS release comes with 152 commits. This includes 75 which are
test related, 25 which are doc related, 21 which are build / tool
related and 3 commits which are updates to dependencies.

Notable Changes:

* build:
 - Codesigning is fixed on macOS (Evan Lucas)
   #14179
* deps:
 - Snapshots are turned back on!!! (Yang Guo)
   #14385
* path:
 - win32 volume-relative paths are working again! (Timothy Gu)
   #14440
* tools:
 - v6.x can now build with ICU 59 (Steven R. Loomis)
   #12078

PR-URL: #14852
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
This LTS release comes with 152 commits. This includes 75 which are
test related, 25 which are doc related, 21 which are build / tool
related and 3 commits which are updates to dependencies.

Notable Changes:

* build:
 - Codesigning is fixed on macOS (Evan Lucas)
   nodejs#14179
* deps:
 - Snapshots are turned back on!!! (Yang Guo)
   nodejs#14385
* path:
 - win32 volume-relative paths are working again! (Timothy Gu)
   nodejs#14440
* tools:
 - v6.x can now build with ICU 59 (Steven R. Loomis)
   nodejs#12078

PR-URL: nodejs#14852
@MylesBorins MylesBorins mentioned this pull request Sep 20, 2017
abernix pushed a commit to meteor/node-legacy that referenced this pull request Sep 28, 2017
Allow passing the prefix in via the PKGDIR env var. This will allow us
to use this same script to codesign the binary tarball.

PR-URL: nodejs/node#14179
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 pull request 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 pull request Oct 24, 2017
Allow passing the prefix in via the PKGDIR env var. This will allow us
to use this same script to codesign the binary tarball.

PR-URL: nodejs/node#14179
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 pull request 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 pull request Oct 25, 2017
Allow passing the prefix in via the PKGDIR env var. This will allow us
to use this same script to codesign the binary tarball.

PR-URL: #14179
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 pull request 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]>
@MylesBorins MylesBorins mentioned this pull request Oct 25, 2017
abernix pushed a commit to meteor/node-legacy that referenced this pull request Oct 27, 2017
Allow passing the prefix in via the PKGDIR env var. This will allow us
to use this same script to codesign the binary tarball.

PR-URL: nodejs/node#14179
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 pull request 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. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants