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

Feature: Sign Windows builds #9294

Merged
merged 1 commit into from May 26, 2021
Merged

Conversation

orudge
Copy link
Contributor

@orudge orudge commented May 25, 2021

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.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

Copy link
Member

@TrueBrain TrueBrain left a 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 ;)

with:
filename: 'openttd.p12'
# The certificates in a PKCS12 file encoded as a base64 string
encodedString: ${{ secrets.WIN_CERTIFICATE_P12 }}
Copy link
Member

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 :)

continue-on-error: true
env:
WIN_CODESIGN_P12: ${{ steps.save_cert.outputs.filePath }}
WIN_CODESIGN_PWD: ${{ secrets.WIN_CERTIFICATE_PASSWORD }}
Copy link
Member

Choose a reason for hiding this comment

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

Same here, WINDOWS_ :)

env:
WIN_CODESIGN_P12: ${{ steps.save_cert.outputs.filePath }}
WIN_CODESIGN_PWD: ${{ secrets.WIN_CERTIFICATE_PASSWORD }}

Copy link
Member

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.

# 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 }}
Copy link
Member

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?

Copy link
Contributor Author

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!

- name: Build (with installer)
if: needs.source.outputs.is_tag == 'true'
shell: bash
env:
WIN_CODESIGN_CN: ${{ secrets.WIN_CERTIFICATE_COMMON_NAME }}
Copy link
Member

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)

@@ -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 }}
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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 :)

continue-on-error: true
run: |
cd ${GITHUB_WORKSPACE}/build/bundles
../../os/windows/sign.bat *.exe "${WIN_CODESIGN_CN}"
Copy link
Member

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)

Copy link
Contributor

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

Copy link
Contributor Author

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.

@@ -138,6 +138,13 @@ elseif(WIN32)
endif()

set(CPACK_PACKAGE_FILE_NAME "openttd-#CPACK_PACKAGE_VERSION#-windows-${CPACK_SYSTEM_NAME}")

if(CMAKE_SIGNING_CERTIFICATE)
Copy link
Member

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

Copy link
Contributor Author

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)
Copy link
Member

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 :)

Copy link
Member

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 :)

Copy link
Contributor Author

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.

@@ -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
Copy link
Member

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?

Copy link

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 ;-)

Copy link
Contributor Author

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.

Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

Very nice :)

@orudge orudge merged commit 3ce7e31 into OpenTTD:master May 26, 2021
@orudge orudge deleted the sign-windows-builds branch May 26, 2021 12:40
@glx22 glx22 added the backport requested This PR should be backport to current release (RC / stable) label May 26, 2021
@TrueBrain TrueBrain removed the backport requested This PR should be backport to current release (RC / stable) label Oct 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenTTD's Windows installer should be signed
4 participants