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

win,build: try multiple timeservers when signing #9155

Closed
wants to merge 1 commit into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Oct 18, 2016

Can't get any release builds done right now because globalsign's timeserver isn't responding properly for Authenticode signing requests on Windows!

This PR adds a new file, tools/sign.bat, to do the signing, it tries each of the big available timeservers (globalsign, comodo, verisign, starfield) until it finds one that it can actually use.

Looking for a very speedy review so I can land this and backport to v6, v4, v0.12 and v0.10 and get these releases built! It does work but I've had to make a couple of tweaks while testing. Final testing build should be ready soon in download/release/test/

/cc @nodejs/build @nodejs/platform-windows

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Oct 18, 2016
@rvagg
Copy link
Member Author

rvagg commented Oct 18, 2016

Finally worked as I wanted, see the result @ https://nodejs.org/download/test/v8.0.0-test201610181280ed2be7/

SignTool Error: The specified timestamp server either could not be reached or
returned an invalid response.
SignTool Error: An error occurred while attempting to sign: node-v8.0.0-test201610181280ed2be7-x86.msi

Signing failed using http://timestamp.globalsign.com/scripts/timestamp.dll

Done Adding Additional Store
Successfully signed: node-v8.0.0-test201610181280ed2be7-x86.msi

Successfully signed node-v8.0.0-test201610181280ed2be7-x86.msi using timeserver http://timestamp.comodoca.com/authenticode

@jbergstroem
Copy link
Member

LGTM


for %%s in %timeservers% do (
signtool sign /a /d "Node.js" /du "https://nodejs.org" /t %%s %1
if ERRORLEVEL 0 if not ERRORLEVEL 1 (
Copy link
Member

Choose a reason for hiding this comment

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

This is just a nit at this point, but if ERRORLEVEL 0 is redundant here. (Ref: http://ss64.com/nt/errorlevel.html )

Also, according to https://msdn.microsoft.com/en-us/library/8s9b9yaz(v=vs.110)#Anchor_6 , perhaps we should also accept exit code 2. But we never did and it's not clear what kind of warnings might happen, so let's just be watchful for binaries signed twice or all servers failing with a different error in the first one.

@joaocgreis
Copy link
Member

LGTM with or without nit applied

PR-URL: nodejs#9155
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@rvagg
Copy link
Member Author

rvagg commented Oct 18, 2016

fixed nit, ignoring exit code 2 for now

@rvagg
Copy link
Member Author

rvagg commented Oct 18, 2016

208586a

@rvagg rvagg closed this Oct 18, 2016
@rvagg rvagg deleted the ALL-THE-TIMESERVERS.BAT branch October 18, 2016 13:02
rvagg added a commit that referenced this pull request Oct 18, 2016
PR-URL: #9155
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: João Reis <reis@janeasystems.com>
rvagg added a commit that referenced this pull request Oct 18, 2016
PR-URL: #9155
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: João Reis <reis@janeasystems.com>
rvagg added a commit to rvagg/io.js that referenced this pull request Oct 18, 2016
PR-URL: nodejs#9155
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: João Reis <reis@janeasystems.com>
rvagg added a commit to rvagg/io.js that referenced this pull request Oct 18, 2016
PR-URL: nodejs#9155
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: João Reis <reis@janeasystems.com>
rvagg added a commit to rvagg/io.js that referenced this pull request Oct 18, 2016
PR-URL: nodejs#9155
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: João Reis <reis@janeasystems.com>
jasnell pushed a commit that referenced this pull request Oct 18, 2016
PR-URL: #9155
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: João Reis <reis@janeasystems.com>
@richardlau richardlau mentioned this pull request Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants