Skip to content

Conversation

@RaisinTen
Copy link
Member

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 <[email protected]>
Copy link
Member

@targostargos 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
MemberAuthor

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

@tony-go
Copy link
Member

Let me start my windows machine ^^

@RaisinTenRaisinTen 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-gotony-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 :/

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

@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

@mhdawsonmhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@RaisinTenRaisinTen removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 24, 2023
@RaisinTenRaisinTen 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
MemberAuthor

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

Copy link
Member

@ovflowdovflowd 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
Contributor

@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 <[email protected]> PR-URL: #46764 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
@RaisinTen
Copy link
MemberAuthor

Landed in 4cde39e

@RaisinTenRaisinTen 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 <[email protected]> PR-URL: #46764 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> PR-URL: #46764 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
@targostargos mentioned this pull request Mar 14, 2023
@danielleadams
Copy link
Contributor

Blocked by #45038

@RaisinTenRaisinTen 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 <[email protected]> PR-URL: #46764 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
@targostargos mentioned this pull request Nov 28, 2023
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 <[email protected]> PR-URL: nodejs/node#46764 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> PR-URL: nodejs/node#46764 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.docIssues and PRs related to the documentations.single-executableIssues 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

11 participants

@RaisinTen@tony-go@ShenHongFei@danielleadams@jasnell@targos@cjihrig@mhdawson@ovflowd@aymen94@nodejs-github-bot