Skip to content

Check files are signed after Submit-SigningRequest#19113

Merged
SaschaCowley merged 10 commits into
rcfrom
checkSigning
Oct 24, 2025
Merged

Check files are signed after Submit-SigningRequest#19113
SaschaCowley merged 10 commits into
rcfrom
checkSigning

Conversation

@SaschaCowley

@SaschaCowley SaschaCowley commented Oct 17, 2025

Copy link
Copy Markdown
Member

Link to issue number:

Fixes #19099
Supersedes #18384

Summary of the issue:

Some files are not signed some of the time

Description of user facing changes:

None

Description of developer facing changes:

In the case of alpha, beta, rc, stable and try builds, failure to sign the launcher or most of our executables will fail the build.
All CI builds now run serially to avoid race conditions.

Description of development approach:

Set the ErrorAction common parameter on Send-SigningRequest to Stop to cause the signing script to fail if the cmd-let fails.
Use Get-AuthenticodeSignature to verify the signature of files after Submit-SigningRequest returns.

Testing strategy:

CI

  • Successful signing: https://github.com/nvaccess/nvda/actions/runs/18733609673

    • Note that this run's failure was due to a system test failure

    • Checked that the launcher and all dll and exe files are signed. Found unsigned exes and dll s by unzipping the controller client and launcher into the same directory, and running:

      (Get-ChildItem -Recurse -Include *.exe, *.dll -Name | Get-AuthenticodeSignature | where-object {$_.Status -ne 'Valid'}).Path

      The following files do not have valid signatures:

      • app\brailleDisplayDrivers\lilli.dll
      • app\miscDeps\tools\msgfmt.exe
      • app\synthDrivers\espeak.dll
      • app\synthDrivers\sonic.dll
      • app\brlapi-0.8.dll
      • app\libgcc_s_dw2-1.dll
      • app\wxbase32u_net_vc140.dll
      • app\wxbase32u_vc140.dll
      • app\wxmsw32u_aui_vc140.dll
      • app\wxmsw32u_core_vc140.dll
      • app\wxmsw32u_html_vc140.dll
      • app\wxmsw32u_stc_vc140.dll
      • Banner.dll
      • System.dll

      However, as best as I can tell, we never attempt to sign these files.

  • Intentionally don't sign a DLL: https://github.com/nvaccess/nvda/actions/runs/18703904287

  • Intentionally don't sign the launcher: https://github.com/nvaccess/nvda/actions/runs/18707118372

Known issues with pull request:

None

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@SaschaCowley SaschaCowley requested a review from a team as a code owner October 17, 2025 07:17
@SaschaCowley

Copy link
Copy Markdown
Member Author

@michaelDCurran thoughts on this? Any ideas on how we can test? I thought about just intentionally bypassing the Submit-SigningRequest call, which I think would be a reasonable stand-in for the call timing out

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

Adds a post-signing validation step to ensure artifacts are actually signed after Submit-SigningRequest completes, failing CI with diagnostic details if validation fails.

  • Verify signature with Get-AuthenticodeSignature and check Status
  • On invalid signature, append formatted details to $GITHUB_STEP_SUMMARY and exit 1

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread ci/scripts/sign.ps1 Outdated
Comment thread ci/scripts/sign.ps1 Outdated
Comment thread ci/scripts/sign.ps1
@SaschaCowley SaschaCowley added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Oct 21, 2025
@SaschaCowley SaschaCowley marked this pull request as draft October 21, 2025 01:03
@SaschaCowley SaschaCowley changed the base branch from master to rc October 21, 2025 02:11
@michaelDCurran

michaelDCurran commented Oct 21, 2025 via email

Copy link
Copy Markdown
Member

@github-actions github-actions Bot requested a deployment to snapshot October 21, 2025 05:03 Abandoned
@github-actions github-actions Bot requested a deployment to snapshot October 21, 2025 06:26 Abandoned
Comment thread ci/scripts/sign.ps1 Outdated
@SaschaCowley SaschaCowley marked this pull request as ready for review October 22, 2025 07:57
@SaschaCowley

Copy link
Copy Markdown
Member Author

@michaelDCurran NB my notes about some executables still not being signed in the testing section of the PR description

@LeonarddeR

Copy link
Copy Markdown
Collaborator

The remote loaders should definitely be signed. They Are signed in 2025.3 at least.

@SaschaCowley SaschaCowley marked this pull request as draft October 22, 2025 22:49
@SaschaCowley SaschaCowley marked this pull request as ready for review October 23, 2025 03:34
@SaschaCowley SaschaCowley merged commit bb2ae62 into rc Oct 24, 2025
30 checks passed
@SaschaCowley SaschaCowley deleted the checkSigning branch October 24, 2025 02:57
@github-actions github-actions Bot added this to the 2026.1 milestone Oct 24, 2025
@SaschaCowley SaschaCowley modified the milestones: 2026.1, 2025.3.1 Oct 24, 2025
bramd pushed a commit to bramd/nvda that referenced this pull request Nov 11, 2025
Fixes nvaccess#19099
Supersedes nvaccess#18384

### Summary of the issue:

Some files are not signed some of the time

### Description of user facing changes:

None

### Description of developer facing changes:

In the case of alpha, beta, rc, stable and try builds, failure to sign
the launcher or most of our executables will fail the build.
All CI builds now run serially to avoid race conditions.

### Description of development approach:

Set the `ErrorAction` common parameter on `Send-SigningRequest` to
`Stop` to cause the signing script to fail if the cmd-let fails.
Use `Get-AuthenticodeSignature` to verify the signature of files after
`Submit-SigningRequest` returns.

### Testing strategy:

CI

* [x] Successful signing:
https://github.com/nvaccess/nvda/actions/runs/18733609673
  * Note that this run's failure was due to a system test failure
* Checked that the launcher and all `dll` and `exe` files are signed.
Found unsigned `exe`s and `dll` s by unzipping the controller client and
launcher into the same directory, and running:
    
    ```ps1
(Get-ChildItem -Recurse -Include *.exe, *.dll -Name |
Get-AuthenticodeSignature | where-object {$_.Status -ne 'Valid'}).Path
    ```

    The following files do not have valid signatures:

    * `app\brailleDisplayDrivers\lilli.dll`
    * `app\miscDeps\tools\msgfmt.exe`
    * `app\synthDrivers\espeak.dll`
    * `app\synthDrivers\sonic.dll`
    * `app\brlapi-0.8.dll`
    * `app\libgcc_s_dw2-1.dll`
    * `app\wxbase32u_net_vc140.dll`
    * `app\wxbase32u_vc140.dll`
    * `app\wxmsw32u_aui_vc140.dll`
    * `app\wxmsw32u_core_vc140.dll`
    * `app\wxmsw32u_html_vc140.dll`
    * `app\wxmsw32u_stc_vc140.dll`
    * `Banner.dll`
    * `System.dll`

However, as best as I can tell, we never attempt to sign these files.

* [x] Intentionally don't sign a DLL:
https://github.com/nvaccess/nvda/actions/runs/18703904287
* [x] Intentionally don't sign the launcher:
https://github.com/nvaccess/nvda/actions/runs/18707118372

### Known issues with pull request:

None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

X86 NVDAHelper remote not signed

4 participants