P2: Code signing for exe + installer (CI integration) #43
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideAdds optional Windows code-signing support to the release workflow by preparing a signing certificate from encrypted secrets and signing both the CLI executable and installer artifacts when a certificate is available. Sequence diagram for optional certificate preparation and artifact signing in release workflowsequenceDiagram
participant GA as GitHubActions
participant Job as ReleaseJob
participant Secrets as GitHubSecrets
participant Runner as RunnerEnvironment
participant SignScript as sign-exe.ps1
GA->>Job: Start release workflow
Job->>Runner: Checkout repository
Job->>Runner: Step Prepare signing certificate
Runner->>Secrets: Read CLOUDSQLCTL_SIGN_CERT_B64
Runner->>Secrets: Read CLOUDSQLCTL_SIGN_PWD
alt Signing cert provided
Runner->>Runner: Decode base64 PFX
Runner->>Runner: Write PFX to runner temp
Runner->>Runner: Export CLOUDSQLCTL_SIGN_CERT to GITHUB_ENV
Runner->>Runner: Export CLOUDSQLCTL_SIGN_PWD to GITHUB_ENV
else No signing cert
Runner->>Runner: Log skipping signing setup
end
Job->>Runner: Use Node.js 22.x
Job->>Runner: Build exe and installer
Job->>Runner: Step Sign artifacts (conditional)
alt CLOUDSQLCTL_SIGN_CERT not empty
Runner->>SignScript: Sign bin/cloudsqlctl.exe
Runner->>SignScript: Sign dist/cloudsqlctl-setup.exe
else CLOUDSQLCTL_SIGN_CERT empty
Runner->>Runner: Skip signing step
end
Job->>Runner: Generate docs
Runner-->>GA: Complete release job
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the
Sign artifactsstep you’re already usingshell: pwsh, so you can invoketools/sign-exe.ps1directly (e.g.,./tools/sign-exe.ps1 -ExePath ...) instead of spawningpowershelltwice with-ExecutionPolicy Bypass, which simplifies the step and avoids mixing Windows PowerShell vs. PowerShell Core.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `Sign artifacts` step you’re already using `shell: pwsh`, so you can invoke `tools/sign-exe.ps1` directly (e.g., `./tools/sign-exe.ps1 -ExePath ...`) instead of spawning `powershell` twice with `-ExecutionPolicy Bypass`, which simplifies the step and avoids mixing Windows PowerShell vs. PowerShell Core.
## Individual Comments
### Comment 1
<location> `.github/workflows/release.yml:40` </location>
<code_context>
+ $certPath = Join-Path $env:RUNNER_TEMP "cloudsqlctl-signing.pfx"
+ [IO.File]::WriteAllBytes($certPath, [Convert]::FromBase64String($env:CLOUDSQLCTL_SIGN_CERT_B64))
+ "CLOUDSQLCTL_SIGN_CERT=$certPath" | Out-File -FilePath $env:GITHUB_ENV -Append
+ "CLOUDSQLCTL_SIGN_PWD=$env:CLOUDSQLCTL_SIGN_PWD" | Out-File -FilePath $env:GITHUB_ENV -Append
+
- name: Use Node.js 22.x
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Avoid broadening the exposure of the signing password by putting it into GITHUB_ENV.
Writing `CLOUDSQLCTL_SIGN_PWD` to `GITHUB_ENV` makes it available to all later steps in the job, unnecessarily widening exposure if any of those steps are compromised or misconfigured. Since the secret is already available in this step’s environment, prefer passing it only to the signing step (e.g., via `env:` on that step or as an argument to the signing script) instead of promoting it to a job-wide variable.
Suggested implementation:
```
$certPath = Join-Path $env:RUNNER_TEMP "cloudsqlctl-signing.pfx"
[IO.File]::WriteAllBytes($certPath, [Convert]::FromBase64String($env:CLOUDSQLCTL_SIGN_CERT_B64))
"CLOUDSQLCTL_SIGN_CERT=$certPath" | Out-File -FilePath $env:GITHUB_ENV -Append
```
```
- name: Sign artifacts
if: ${{ env.CLOUDSQLCTL_SIGN_CERT != '' }}
env:
CLOUDSQLCTL_SIGN_PWD: ${{ secrets.CLOUDSQLCTL_SIGN_PWD }}
shell: pwsh
run: |
powershell -ExecutionPolicy Bypass -File tools/sign-exe.ps1 -ExePath "bin/cloudsqlctl.exe"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| $certPath = Join-Path $env:RUNNER_TEMP "cloudsqlctl-signing.pfx" | ||
| [IO.File]::WriteAllBytes($certPath, [Convert]::FromBase64String($env:CLOUDSQLCTL_SIGN_CERT_B64)) | ||
| "CLOUDSQLCTL_SIGN_CERT=$certPath" | Out-File -FilePath $env:GITHUB_ENV -Append | ||
| "CLOUDSQLCTL_SIGN_PWD=$env:CLOUDSQLCTL_SIGN_PWD" | Out-File -FilePath $env:GITHUB_ENV -Append |
There was a problem hiding this comment.
🚨 suggestion (security): Avoid broadening the exposure of the signing password by putting it into GITHUB_ENV.
Writing CLOUDSQLCTL_SIGN_PWD to GITHUB_ENV makes it available to all later steps in the job, unnecessarily widening exposure if any of those steps are compromised or misconfigured. Since the secret is already available in this step’s environment, prefer passing it only to the signing step (e.g., via env: on that step or as an argument to the signing script) instead of promoting it to a job-wide variable.
Suggested implementation:
$certPath = Join-Path $env:RUNNER_TEMP "cloudsqlctl-signing.pfx"
[IO.File]::WriteAllBytes($certPath, [Convert]::FromBase64String($env:CLOUDSQLCTL_SIGN_CERT_B64))
"CLOUDSQLCTL_SIGN_CERT=$certPath" | Out-File -FilePath $env:GITHUB_ENV -Append
- name: Sign artifacts
if: ${{ env.CLOUDSQLCTL_SIGN_CERT != '' }}
env:
CLOUDSQLCTL_SIGN_PWD: ${{ secrets.CLOUDSQLCTL_SIGN_PWD }}
shell: pwsh
run: |
powershell -ExecutionPolicy Bypass -File tools/sign-exe.ps1 -ExePath "bin/cloudsqlctl.exe"
There was a problem hiding this comment.
Pull request overview
This PR adds optional code signing support for Windows executables and installers in the release workflow. Signing is performed using a base64-encoded PFX certificate stored in GitHub Secrets, and gracefully skips signing when credentials are not available.
Key Changes:
- Adds certificate preparation step that decodes base64 PFX and stores it temporarily
- Implements artifact signing step for both the standalone exe and installer
- Uses conditional logic to make signing optional based on secret availability
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+76
to
+77
| powershell -ExecutionPolicy Bypass -File tools/sign-exe.ps1 -ExePath "bin/cloudsqlctl.exe" | ||
| powershell -ExecutionPolicy Bypass -File tools/sign-exe.ps1 -ExePath "dist/cloudsqlctl-setup.exe" |
There was a problem hiding this comment.
Redundant PowerShell invocation. Since this step already uses shell: pwsh (line 74), the commands are already running in PowerShell. The powershell -ExecutionPolicy Bypass -File wrapper spawns an unnecessary nested PowerShell process.
The commands should be called directly using dot-sourcing or by invoking the script file with &. For example:
- Change to:
& tools/sign-exe.ps1 -ExePath "bin/cloudsqlctl.exe"
This pattern is consistent with lines 33-40 and 97-120 in the same workflow, where PowerShell commands are executed directly when using shell: pwsh.
Suggested change
| powershell -ExecutionPolicy Bypass -File tools/sign-exe.ps1 -ExePath "bin/cloudsqlctl.exe" | |
| powershell -ExecutionPolicy Bypass -File tools/sign-exe.ps1 -ExePath "dist/cloudsqlctl-setup.exe" | |
| & tools/sign-exe.ps1 -ExePath "bin/cloudsqlctl.exe" | |
| & tools/sign-exe.ps1 -ExePath "dist/cloudsqlctl-setup.exe" |
| run: | | ||
| powershell -ExecutionPolicy Bypass -File tools/sign-exe.ps1 -ExePath "bin/cloudsqlctl.exe" | ||
| powershell -ExecutionPolicy Bypass -File tools/sign-exe.ps1 -ExePath "dist/cloudsqlctl-setup.exe" | ||
There was a problem hiding this comment.
The certificate file should be explicitly deleted after signing completes to minimize the time sensitive credentials exist on disk. While RUNNER_TEMP is automatically cleaned up after the job finishes, explicitly removing the certificate immediately after use follows security best practices for credential handling.
Consider adding cleanup after the signing step, either:
- Add a cleanup command at the end of the "Sign artifacts" step
- Add a separate cleanup step with
if: always()to ensure cleanup even if signing fails
For example, at the end of the signing step:
Remove-Item -Path $env:CLOUDSQLCTL_SIGN_CERT -Force -ErrorAction SilentlyContinue
Suggested change
| - name: Cleanup signing certificate | |
| if: ${{ always() && env.CLOUDSQLCTL_SIGN_CERT != '' }} | |
| shell: pwsh | |
| run: | | |
| Remove-Item -Path $env:CLOUDSQLCTL_SIGN_CERT -Force -ErrorAction SilentlyContinue |
Owner
Author
|
Checks green; ready to merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #24
Summary:
Tests:
Rollback:
Summary by Sourcery
Integrate optional Windows code signing into the release workflow for the CLI executable and installer when a signing certificate is provided.
CI: