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

doc: add steps about signing the binary in single-executable docs #46764

Conversation

RaisinTen
Copy link
Contributor

We didn't catch this in #45038 because the binary wasn't signed by default unlike the official Node.js binary, which is signed by the Node.js Foundation identity by default.

Refs: nodejs/postject#76 (macOS arm64 part only)
Fixes: nodejs/postject#75

cc @nodejs/single-executable @targos @ShenHongFei

We didn't catch this in nodejs#45038
because the binary wasn't signed by default unlike the official Node.js
binary, which is signed by the Node.js Foundation identity by default.

Refs: nodejs/postject#76 (macOS arm64 part only)
Fixes: nodejs/postject#75
Signed-off-by: Darshan Sen <raisinten@gmail.com>
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

LGTM for the macOS part.

@RaisinTen
Copy link
Contributor Author

@nodejs/platform-windows can someone please review the Windows part?

@tony-go
Copy link
Member

tony-go commented Feb 22, 2023

Let me start my windows machine ^^

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 22, 2023
Copy link
Member

@tony-go tony-go left a comment

Choose a reason for hiding this comment

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

LGTM for mac

Not able to test on my windows machine due to some reasons :/

@RaisinTen RaisinTen added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 23, 2023
@ShenHongFei
Copy link

@RaisinTen
For windows part, I tried it according to the process in the document, and I needed to change hello to hello.exe (executable programs on windows need a .exe suffix to run).

I executed the following command in powershell:

$ echo 'console.log(`Hello, ${process.argv[2]}!`);' > hello.js
$ cp e:/sdk/nodejs/node.exe hello.exe
$ &"C:/Program Files (x86)/Windows Kits/10/bin/10.0.22621.0/x64/signtool.exe" remove /s hello.exe
$ npx postject hello.exe NODE_JS_CODE hello.js --sentinel-fuse NODE_JS_FUSE_fce680ab2cc467b6e072b8b5df1996b2
$ &"C:/Program Files (x86)/Windows Kits/10/bin/10.0.22621.0/x64/signtool.exe" sign /fd SHA256 hello.exe

The last step went wrong:
SignTool Error: No certificates were found that met all the given criteria.

It may be that I don't have a local certificate that can be used for digital signatures, but the generated unsigned hello.exe can also run normally

Using signtools (https://learn.microsoft.com/en-us/windows/win32/seccrypto/signtool) in Windows system to remove node.exe signature, and to sign the final generated hello.exe, is not Required steps, and users need to manually install windows sdk (https://developer.microsoft.com/en-us/windows/downloads/windows-sdk/)

I think it would be more convenient to inform users that they can ignore the warnings output by postject, or make the method of deleting and re-signing as an optional step, explaining how to obtain and use signtools to sign.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@RaisinTen RaisinTen removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 24, 2023
Refs: nodejs#46764 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen RaisinTen added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Feb 24, 2023
@RaisinTen
Copy link
Contributor Author

@ShenHongFei updated the docs to clarify that. PTAL reviewers.

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

Docs seem legit for me for both Windows and macOS :)

@ShenHongFei
Copy link

@ShenHongFei updated the docs to clarify that. PTAL reviewers.

LGTM 😄

RaisinTen added a commit that referenced this pull request Feb 26, 2023
We didn't catch this in #45038
because the binary wasn't signed by default unlike the official Node.js
binary, which is signed by the Node.js Foundation identity by default.

Refs: nodejs/postject#76 (macOS arm64 part only)
Fixes: nodejs/postject#75
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #46764
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RaisinTen
Copy link
Contributor Author

Landed in 4cde39e

@RaisinTen RaisinTen closed this Feb 26, 2023
@RaisinTen RaisinTen deleted the doc-add-steps-about-signing-the-binary-in-single-executable-docs branch February 26, 2023 06:59
targos pushed a commit that referenced this pull request Mar 13, 2023
We didn't catch this in #45038
because the binary wasn't signed by default unlike the official Node.js
binary, which is signed by the Node.js Foundation identity by default.

Refs: nodejs/postject#76 (macOS arm64 part only)
Fixes: nodejs/postject#75
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #46764
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Mar 14, 2023
We didn't catch this in #45038
because the binary wasn't signed by default unlike the official Node.js
binary, which is signed by the Node.js Foundation identity by default.

Refs: nodejs/postject#76 (macOS arm64 part only)
Fixes: nodejs/postject#75
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #46764
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams danielleadams added the backport-blocked-v18.x PRs that should land on the v18.x-staging branch but are blocked by another PR's pending backport. label Apr 3, 2023
@danielleadams
Copy link
Member

Blocked by #45038

@RaisinTen RaisinTen added the single-executable Issues and PRs related to single-executable applications label May 5, 2023
targos pushed a commit that referenced this pull request Nov 10, 2023
We didn't catch this in #45038
because the binary wasn't signed by default unlike the official Node.js
binary, which is signed by the Node.js Foundation identity by default.

Refs: nodejs/postject#76 (macOS arm64 part only)
Fixes: nodejs/postject#75
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #46764
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
We didn't catch this in nodejs/node#45038
because the binary wasn't signed by default unlike the official Node.js
binary, which is signed by the Node.js Foundation identity by default.

Refs: nodejs/postject#76 (macOS arm64 part only)
Fixes: nodejs/postject#75
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs/node#46764
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
We didn't catch this in nodejs/node#45038
because the binary wasn't signed by default unlike the official Node.js
binary, which is signed by the Node.js Foundation identity by default.

Refs: nodejs/postject#76 (macOS arm64 part only)
Fixes: nodejs/postject#75
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs/node#46764
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-blocked-v18.x PRs that should land on the v18.x-staging branch but are blocked by another PR's pending backport. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations. single-executable Issues and PRs related to single-executable applications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weired output message The signature seems corrupted! on Windows