Replace scons app signing from signtool to signpath#16673
Conversation
See test results for failed build of commit d3246af734 |
See test results for failed build of commit c50fe0ece3 |
WalkthroughThis update introduces a significant shift in handling build and signing processes in the project. Key changes include migrating from certificate-based signing to an API token-based system, enhancing security by replacing sensitive information with secure tokens, and configuring features through a unified Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range and nitpick comments (1)
sconstruct (1)
98-98: Ensure that the description forapiSigningTokenis clear and comprehensive.Consider enhancing the description for
apiSigningTokento provide more context on how and where this token is used, especially since it's a security-sensitive item.
|
@dpy013 - can you please avoid triggering AI reviews of clear drafts/WIP, particularly if they are not your own. the comments here are not helpful and add noise. The AI tool only reviews diffs from each review, so now the "first" AI review will ignore the initial code, and the initial comments aren't helpful as this is a clear draft. |
|
@coderabbitai full review |
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (2)
appveyor/scripts/decryptFilesForSigning.ps1 (1)
Line range hint
3-6: Ensure robust error handling for SSH key decryption failures.Consider adding a more detailed error message or logging mechanism to help diagnose decryption failures more effectively.
sconstruct (1)
Line range hint
393-411: Ensure consistent post-action signing across different build artifacts.The post-action signing steps for different artifacts like the uninstaller and launcher should be consistent and correctly configured to use the new
apiSigningTokenif available. This ensures that all parts of the application are securely signed.- if certFile or apiSigningToken: - env.AddPostAction(uninstaller, [env['signExec']]) - if certFile or apiSigningToken: - env.AddPostAction(launcher, [env['signExec']]) + if apiSigningToken: + env.AddPostAction(uninstaller, [env['signExecApi']]) + env.AddPostAction(launcher, [env['signExecApi']]) + elif certFile: + env.AddPostAction(uninstaller, [env['signExecCert']]) + env.AddPostAction(launcher, [env['signExecCert']])
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
See test results for failed build of commit e5cd6de11d |
| [string]$FileToSign | ||
| ) | ||
|
|
||
| Submit-SigningRequest -ApiToken $ApiToken -InputArtifactPath $FileToSign -OutputArtifactPath $FileToSign -OrganizationId "12147e94-bba9-4fef-b29b-300398e90c5a" -ProjectSlug "NVDA" -SigningPolicySlug "release_signing_policy" -WaitForCompletion -Force |
There was a problem hiding this comment.
Should the organizationId be passable instead of hardcoded? Then it could be drawn from a variable like scons_publisher is, as @seanbudd pointed out.
There was a problem hiding this comment.
It could certainly be passable, that isn't too much of an issue. At the moment it's hard coded because I haven't been able to come up with any circumstances under which it would be changed. It would require someone to have an account with SignPath, have their EV/OV hosted by them, and want to sign with their organisation/account. It makes more sense to me that the developer would simply have a locally signed authenticode certificate that they would use.
There was a problem hiding this comment.
Perhaps it would need to be changed by the NVDA-jp fork? I'm curious as to how they plan to handle the changes to software signing
There was a problem hiding this comment.
I've reached out to NVDA-jp and they're now aware of this PR. Discussions are happening via email.
If we're concerned about hard-coded values here we'd also need to look at -ProjectSlug "NVDA" and -SigningPolicySlug "release_signing_policy". Both of those are defined inside the SignPath web interface and would be set to different values for different forks / projects.
My current opinion is that org ID, project, and signing policy are all fine to be hard coded since they're specific to NV Access and won't change. A variable is better for the API key, since it is likely to be rotated and in any case needs to be passed securely. If another fork wants to sign their code using SignPath, they will put their own org/project/policy information in there. But I'm happy for arguments against this approach too.
|
@coderabbitai full review |
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (3)
appveyor/scripts/decryptFilesForSigning.ps1 (2)
Line range hint
1-10: Ensure the SSH key decryption process is necessary and secure given the new signing method.The decryption of SSH keys in this script is potentially redundant with the introduction of
apiSigningToken. Verify if this step is still required, and if not, consider removing it to simplify the process and reduce the attack surface.
Line range hint
3-6: Review error handling for SSH key decryption failures.The script should handle errors more robustly. Currently, it only logs a message but does not stop execution or clean up potentially sensitive files. Consider adding more comprehensive error handling, such as deleting the decrypted key if it's not needed anymore or ensuring the script exits immediately upon failure.
projectDocs/dev/buildingNVDAOnAppVeyor.md (1)
Line range hint
316-316: This word is normally spelled as one: "troubleshooting".- ### Trouble shooting + ### TroubleshootingTools
LanguageTool
[typographical] ~284-~284: Conjunctions like ‘but’ should not follow semicolons. Consider using a comma, or removing the conjunction. (CONJUNCTION_AFTER_SEMICOLON)
Context: ...icant modifications to the build scripts; but that is beyond the scope of this how-to. The...
[grammar] ~287-~287: Using ‘couple’ without ‘of’ is considered to be informal. (PLENTY_OF_NOUNS)
Context: ...l. In the environment section, after a couple lines of comment about what these variables d...
[style] ~287-~287: Try using a synonym here to strengthen your wording. (COMMENT_REMARK)
Context: ...onment section, after a couple lines of comment about what these variables do, you will...Markdownlint
289-289: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
very approved - thanks github! |
Link to issue number:
n/a
Summary of the issue:
https://www.sslpoint.com/new-private-key-storage-requirement-for-standard-code-signing-certificates/
New requirements for Authenticode signing certificates which require a hardware token
Description of user facing changes
Description of development approach
Testing strategy:
Build locally
Build on AppVeyor - in try branch: master...try-signpath-code-signing
Known issues with pull request:
Code Review Checklist:
Summary by CodeRabbit
Chores
.gitignoreto includeappveyor-tools/.appveyor.ymlto update secure keys and introduce a new authentication token.decryptFilesForSigning.ps1to focus on decrypting an SSH private key.setSconsArgs.ps1for enhanced security by usingapiSigningToken.sign.ps1script for file signing with an API token.apiSigningTokenfor improved security.Documentation
apiSigningTokeninbuildingNVDA.md.buildingNVDAOnAppVeyor.mdto ensure correct configurations.