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
Feature: Sign Windows builds #9294
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bunch of nitpicks, sorry :) But in my experience we better make this obvious and clear, as maintaining this will otherwise be a pita ;)
.github/workflows/release.yml
Outdated
with: | ||
filename: 'openttd.p12' | ||
# The certificates in a PKCS12 file encoded as a base64 string | ||
encodedString: ${{ secrets.WIN_CERTIFICATE_P12 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you place call this WINDOWS_CERTIFICATE_P12
? Being a bit more verbose here won't hurt anyone :)
.github/workflows/release.yml
Outdated
continue-on-error: true | ||
env: | ||
WIN_CODESIGN_P12: ${{ steps.save_cert.outputs.filePath }} | ||
WIN_CODESIGN_PWD: ${{ secrets.WIN_CERTIFICATE_PASSWORD }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, WINDOWS_
:)
.github/workflows/release.yml
Outdated
env: | ||
WIN_CODESIGN_P12: ${{ steps.save_cert.outputs.filePath }} | ||
WIN_CODESIGN_PWD: ${{ secrets.WIN_CERTIFICATE_PASSWORD }} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove the newline and move the run
block above the env
block.
.github/workflows/release.yml
Outdated
# If this is run on a fork, there may not be a certificate set up - continue in this case | ||
continue-on-error: true | ||
env: | ||
WIN_CODESIGN_P12: ${{ steps.save_cert.outputs.filePath }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CODESIGN
vs CERTIFICATE
. Any reason for the naming like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not particularly; this bit was perhaps written before I normalised the secret names against the Apple equivalents!
.github/workflows/release.yml
Outdated
- name: Build (with installer) | ||
if: needs.source.outputs.is_tag == 'true' | ||
shell: bash | ||
env: | ||
WIN_CODESIGN_CN: ${{ secrets.WIN_CERTIFICATE_COMMON_NAME }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move env
block below run
block (all other places do it like that, with the exception of your MacOS work :P I spotted that way too late, sorry :D)
.github/workflows/release.yml
Outdated
@@ -771,6 +796,8 @@ jobs: | |||
- name: Build (without installer) | |||
if: needs.source.outputs.is_tag != 'true' | |||
shell: bash | |||
env: | |||
WIN_CODESIGN_CN: ${{ secrets.WIN_CERTIFICATE_COMMON_NAME }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CN
? Why not just COMMON_NAME
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly because it's referred to as "CN=OpenTTD Distribution Ltd" in the certificate itself, it's the usual abbreviation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that the left side uses CN and the right side COMMON_NAME :)
.github/workflows/release.yml
Outdated
continue-on-error: true | ||
run: | | ||
cd ${GITHUB_WORKSPACE}/build/bundles | ||
../../os/windows/sign.bat *.exe "${WIN_CODESIGN_CN}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything we can do about *.exe
? Being a bit more specific here might help? (honest question, no is a valid answer, as always)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we assume cmake >=3.19 (GHA windows runners have 3.20.2), maybe we could try to use CPACK_POST_BUILD_SCRIPTS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CPACK_POST_BUILD_SCRIPTS was my first port of call, but actually getting the executable name/build directory out of it seemed to be problematic. If somebody can get it working later, it's easy enough to swap out.
TrueBrain: Probably, yes, but that involves more effort (which honestly I didn't have time for) and the possibility for errors to creep in (e.g., if somebody changes the naming convention later but misses this bit), and in practice there are going to be no other .exe files in that folder anyway - so I don't think it's really an issue.
cmake/InstallAndPackage.cmake
Outdated
@@ -138,6 +138,13 @@ elseif(WIN32) | |||
endif() | |||
|
|||
set(CPACK_PACKAGE_FILE_NAME "openttd-#CPACK_PACKAGE_VERSION#-windows-${CPACK_SYSTEM_NAME}") | |||
|
|||
if(CMAKE_SIGNING_CERTIFICATE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here yet another variation of naming a variable :) I suggest we make them all similar .. here there is no mention of WIN
(or WINDOWS
), for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will rename it. In this case it is the certificate CN that's needed.
REM Arguments: sign.bat exe_to_sign certificate_subject_name | ||
|
||
IF NOT DEFINED SIGNTOOL_PATH (SET SIGNTOOL_PATH=signtool) | ||
IF NOT DEFINED SIGNTOOL_TIMESTAMP_URL (SET SIGNTOOL_TIMESTAMP_URL=http://timestamp.digicert.com) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http
? digicert
? No clue what this is, but it reads weird :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol, okay, so this is in the official Microsoft documentation:
https://docs.microsoft.com/en-us/dotnet/framework/tools/signtool-exe
It feels really odd, to have a http
mixed in with signing .. but what-ever :P I would suggest we just add a comment above, saying the URL comes from https://docs.microsoft.com/en-us/dotnet/framework/tools/signtool-exe and leave it at that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used to timestamp the file with a cryptographic signature; there are a variety of servers available. Agreed that it does seem a bit odd that it's not HTTPS. I'll comment it a bit more.
.github/workflows/release.yml
Outdated
@@ -746,9 +746,33 @@ jobs: | |||
with: | |||
arch: ${{ matrix.host }} | |||
|
|||
- name: Save code signing certificate to disk | |||
id: save_cert | |||
uses: timheuer/base64-to-file@v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance we can prevent this? I really do not like that we store it on disk in another step then it being used, especially, as it is not removed afterwards.
Maybe a solution is, as Powershell can base64 decode, doing it in the same step as Import code signing certificate
, and removing the (now temporary) file after the import?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately there is no PS native way to decode base64, but maybe this is helpful, assuming the certificate is mounted as environment variable
$bytes = [System.Convert]::FromBase64String("$(cat env:/WIN_CERTIFICATE_P12)")
[IO.File]::WriteAllBytes("$(pwd)\temp.p12", $bytes)
# import it
rm "$(pwd)\temp.p12"
You would think there are some nice .NET methods to import a cert directly from a byte array, but these methods are not nice at all ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure if multi-line environment variables were even a thing on Windows to be honest, so didn't try it that way, but can certainly see if it works. Thanks for the sample.
fbd350a
to
30fd727
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice :)
Fixes #8056
Motivation / Problem
Windows builds are currently unsigned, which can result in security warnings. We have finally managed to obtain a code signing certificate, in the name of OpenTTD Distribution Ltd, which should offer some reassurance to users.
Description
Adds signing of the openttd.exe for Windows builds, alongside the installer if applicable.
Limitations
The certificate details are stored in GitHub Secrets, in the same way as the Apple signing certificate. Signing of the openttd.exe file is performed as part of the build process, if CMAKE_SIGNING_CERTIFICATE is set to the common name of the certificate, which must be imported into the local user's certificate store. Signing of the installer is currently a manual process (integrating it with CPack is not as straightforward for Windows as it is for Apple), performed in the Actions script.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.