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

Support azure signtool #771

Merged
merged 23 commits into from
May 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
23b7ae8
Add windows_signing_tool keyword
marcoesters Apr 2, 2024
03cc07c
Refactor to creates classes for signing Windows installers
marcoesters Apr 8, 2024
972f324
Add AzureSignTool support
marcoesters Apr 8, 2024
fff427f
Update documentation
marcoesters Apr 8, 2024
621f6c7
Add news item
marcoesters Apr 9, 2024
d791cd8
Add docs strings
marcoesters Apr 9, 2024
7a3e546
Merge branch 'support-azure-signtool' of github.com:marcoesters/const…
marcoesters Apr 9, 2024
b2b0325
Apply suggestions from code review
marcoesters Apr 10, 2024
6f088ac
Normalize signing tool name in validation function
marcoesters Apr 10, 2024
86f118a
Add typing to make_nsi
marcoesters Apr 10, 2024
11b5dcb
Raise on stderr with AzureSignTool
marcoesters Apr 10, 2024
cca1ce4
Log AzureSignTool authentication method
marcoesters Apr 10, 2024
e6565c7
Move check_required_env_vars to utils
marcoesters Apr 10, 2024
0101175
Add missing debugging section to TOC tree
marcoesters Apr 10, 2024
352209f
Add integration test for AzureSignTool signing
marcoesters Apr 12, 2024
c049abe
Do not overload signtool variables for AzureSignTool to facilitate te…
marcoesters Apr 15, 2024
31110f5
Add AzureSignTool to CI tests
marcoesters Apr 18, 2024
b6e7aa3
Always explicitly copy environment
marcoesters Apr 19, 2024
9e8dff8
Add comment to explain why environment is always copied in _execute
marcoesters Apr 19, 2024
477d7e0
Use get to check for environment variables in AzureSignTool test
marcoesters Apr 22, 2024
be5b259
Update .github/workflows/main.yml
marcoesters Apr 23, 2024
55458a4
Quote environment variables
marcoesters Apr 23, 2024
7f7efd2
Merge branch 'main' into support-azure-signtool
marcoesters Apr 25, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,10 @@ jobs:
activate-environment: constructor-dev
environment-file: dev/environment.yml
python-version: ${{ matrix.python-version }}
- name: Install AzureSignTool
if: matrix.os == 'windows'
run: dotnet.exe tool install --global AzureSignTool
shell: pwsh
- name: Supply extra dependencies and install constructor
run: |
files=(--file "tests/requirements.txt")
Expand Down Expand Up @@ -129,6 +133,11 @@ jobs:
flags: unit
- name: Run examples
env:
AZURE_SIGNTOOL_KEY_VAULT_CERTIFICATE: ${{ secrets.AZURE_SIGNTOOL_KEY_VAULT_CERTIFICATE }}
AZURE_SIGNTOOL_KEY_VAULT_CLIENT_ID: ${{ secrets.AZURE_SIGNTOOL_KEY_VAULT_CLIENT_ID }}
AZURE_SIGNTOOL_KEY_VAULT_SECRET: ${{ secrets.AZURE_SIGNTOOL_KEY_VAULT_SECRET }}
AZURE_SIGNTOOL_KEY_VAULT_TENANT_ID: ${{ secrets.AZURE_SIGNTOOL_KEY_VAULT_TENANT_ID }}
AZURE_SIGNTOOL_KEY_VAULT_URL: ${{ secrets.AZURE_SIGNTOOL_KEY_VAULT_URL }}
CONSTRUCTOR_EXAMPLES_KEEP_ARTIFACTS: "${{ runner.temp }}/examples_artifacts"
CONSTRUCTOR_SIGNTOOL_PATH: "C:/Program Files (x86)/Windows Kits/10/bin/10.0.17763.0/x86/signtool.exe"
run: |
Expand Down
21 changes: 14 additions & 7 deletions CONSTRUCT.md
Original file line number Diff line number Diff line change
Expand Up @@ -332,18 +332,25 @@ to sign `conda.exe`. For this, you need an "Application certificate" (different
"Installer certificate" mentioned above). Common values for this option follow the format
`Developer ID Application: Name of the owner (XXXXXX)`.

### `signing_certificate`
### `windows_signing_tool`

_required:_ no<br/>
_type:_ string<br/>

On Windows only, set this key to the path of a PFX certificate to be used with `signtool`.
Additional environment variables can be used to configure this step, namely:
The tool used to sign Windows installers. Must be one of: azuresigntool, signtool.
Some tools require `signing_certificate` to be set.
Defaults to `signtool` if `signing_certificate` is set.
Additional environment variables may need to be used to configure signing.
See the documentation for details:
https://conda.github.io/constructor/howto/#signing-exe-installers

### `signing_certificate`

_required:_ no<br/>
_type:_ string<br/>

- `CONSTRUCTOR_PFX_CERTIFICATE_PASSWORD` (password to unlock the certificate, if needed)
- `CONSTRUCTOR_SIGNTOOL_PATH` (absolute path to `signtool.exe`, in case is not in `PATH`)
- `CONSTRUCTOR_SIGNTOOL_TIMESTAMP_SERVER_URL` (custom RFC 3161 timestamping server, default is
http://timestamp.sectigo.com)
On Windows only, set this key to the path of the certificate file to be used
with the `windows_signing_tool`.

### `attempt_hardlinks`

Expand Down
32 changes: 25 additions & 7 deletions constructor/construct.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@
from constructor.exceptions import UnableToParse, UnableToParseMissingJinja2, YamlParsingError
from constructor.utils import yaml

WIN_SIGNTOOLS = [
"azuresigntool",
"signtool",
]

# list of tuples (key name, required, type, description)
KEYS = [
('name', True, str, '''
Expand Down Expand Up @@ -242,14 +247,18 @@
`Developer ID Application: Name of the owner (XXXXXX)`.
'''),

('signing_certificate', False, str, '''
On Windows only, set this key to the path of a PFX certificate to be used with `signtool`.
Additional environment variables can be used to configure this step, namely:
('windows_signing_tool', False, str, f'''
The tool used to sign Windows installers. Must be one of: {", ".join(WIN_SIGNTOOLS)}.
Some tools require `signing_certificate` to be set.
Defaults to `signtool` if `signing_certificate` is set.
Additional environment variables may need to be used to configure signing.
See the documentation for details:
https://conda.github.io/constructor/howto/#signing-exe-installers
'''),

- `CONSTRUCTOR_PFX_CERTIFICATE_PASSWORD` (password to unlock the certificate, if needed)
- `CONSTRUCTOR_SIGNTOOL_PATH` (absolute path to `signtool.exe`, in case is not in `PATH`)
- `CONSTRUCTOR_SIGNTOOL_TIMESTAMP_SERVER_URL` (custom RFC 3161 timestamping server, default is
http://timestamp.sectigo.com)
('signing_certificate', False, str, '''
On Windows only, set this key to the path of the certificate file to be used
with the `windows_signing_tool`.
'''),

('attempt_hardlinks', False, (bool, str), '''
Expand Down Expand Up @@ -792,6 +801,15 @@ def verify(info):
types_str = " or ".join([type_.__name__ for type_ in types])
sys.exit(f"Value for 'extra_envs.{env_name}.{key}' "
f"must be an instance of {types_str}")
if signtool := info.get("windows_signing_tool"):
if signtool.lower().replace(".exe", "") not in WIN_SIGNTOOLS:
sys.exit(
marcoesters marked this conversation as resolved.
Show resolved Hide resolved
"Value for 'windows_signing_tool' must be one of: "
f"{', '.join(WIN_SIGNTOOLS)}. You tried to use: {signtool}."
)
need_cert_file = ["signtool"]
if signtool in need_cert_file and not info.get("signing_certificate"):
sys.exit(f"The signing tool {signtool} requires 'signing_certificate' to be set.")


def generate_doc():
Expand Down
6 changes: 6 additions & 0 deletions constructor/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,12 @@ def main_build(dir_path, output_dir='.', platform=cc_platform,
if info.get(key): # only join if there's a truthy value set
info[key] = abspath(join(dir_path, info[key]))

# Normalize name and set default value
if info.get("windows_signing_tool"):
info["windows_signing_tool"] = info["windows_signing_tool"].lower().replace(".exe", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if a user tries to set this to a full path? Might be tempting 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

At the same time, if we are validating the input in construct.py:805, why do we need normalization here? 🤔 For library usage? In that case it should be the same logic (or move the logic here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What happens if a user tries to set this to a full path?

constructor will and should fail. windows_signing_tool is the name. Otherwise, I would have to infer from the binary what the appropriate signing too is.

In that case it should be the same logic

Yes, that's true, good catch!

(or move the logic here).

I'm not sure about this. Setting default values and manipulating input is done in main_build. validate() performs validation here. If we move this line into validate(), we are mixing scopes, which makes things harder to track.

elif info.get("signing_certificate"):
info["windows_signing_tool"] = "signtool"
jaimergp marked this conversation as resolved.
Show resolved Hide resolved

for key in 'specs', 'packages':
if key not in info:
continue
Expand Down
220 changes: 220 additions & 0 deletions constructor/signing.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,220 @@
import logging
import os
import shutil
from pathlib import Path
from subprocess import PIPE, STDOUT, check_call, run
from typing import Union

from .utils import check_required_env_vars, win_str_esc

logger = logging.getLogger(__name__)


class SigningTool:
"""Base class to sign installers.

Attributes
----------
executable: str | Path
Path to the signing tool binary.
certificate_file: str | Path
Path to the certificate file
"""
def __init__(
self,
executable: Union[str, Path],
certificate_file: Union[str, Path] = None,
):
self.executable = str(executable)
if certificate_file and not Path(certificate_file).exists():
raise FileNotFoundError(f"Certificate file {certificate_file} does not exist.")
self.certificate_file = certificate_file

def _verify_tool_is_available(self):
"""Helper function to verify that the signing tool executable exists.

This is a minimum verification step and should be done even if other steps are performed
to verify the signing tool (e.g., signtool.exe /?) to receive better error messages.
For example, using `signtool.exe /?` when the path does not exist, results in a misleading
Permission Denied error.
"""
logger.info(f"Checking for {self.executable}...")
if not shutil.which(self.executable):
raise FileNotFoundError(
f"Could not find {self.executable}. Verify that the file exists or is in PATH."
)

def verify_signing_tool(self):
"""Verify that the signing tool is usable."""
self._verify_tool_is_available()

def get_signing_command(self):
"""Get the string of the signing command to be executed.

For Windows, this command is inserted into the NSIS template.
"""
return self.executable

def verify_signature(self):
"""Verify the signed installer."""
raise NotImplementedError("Signature verification not implemented for base class.")


class WindowsSignTool(SigningTool):
def __init__(self, certificate_file=None):
super().__init__(
os.environ.get("CONSTRUCTOR_SIGNTOOL_PATH", "signtool"),
certificate_file=certificate_file,
)

def get_signing_command(self) -> str:
timestamp_server = os.environ.get(
"CONSTRUCTOR_SIGNTOOL_TIMESTAMP_SERVER_URL",
"http://timestamp.sectigo.com"
)
timestamp_digest = os.environ.get(
"CONSTRUCTOR_SIGNTOOL_TIMESTAMP_DIGEST",
"sha256"
)
file_digest = os.environ.get(
"CONSTRUCTOR_SIGNTOOL_FILE_DIGEST",
"sha256"
)
command = (
f"{win_str_esc(self.executable)} sign /f {win_str_esc(self.certificate_file)} "
f"/tr {win_str_esc(timestamp_server)} /td {timestamp_digest} /fd {file_digest}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noting here that no quotes are needed because we don't expect spaces here.

)
if "CONSTRUCTOR_PFX_CERTIFICATE_PASSWORD" in os.environ:
# signtool can get the password from the env var on its own
command += ' /p "%CONSTRUCTOR_PFX_CERTIFICATE_PASSWORD%"'
return command

def verify_signing_tool(self):
super()._verify_tool_is_available()
if not Path(self.certificate_file).exists():
raise FileNotFoundError(f"Could not find certificate file {self.certificate_file}.")
check_call([self.executable, "/?"], stdout=PIPE, stderr=PIPE)

def verify_signature(self, installer_file: Union[str, Path]):
proc = run(
[self.executable, "verify", "/v", str(installer_file)],
stdout=PIPE,
stderr=STDOUT,
text=True,
)
logger.info(proc.stdout)
if "SignTool Error: No signature found" in proc.stdout:
# This is a signing error!
proc.check_returncode()
elif proc.returncode:
# we had errors but maybe not critical ones
logger.error(
f"SignTool could find a signature in {installer_file} but detected errors. "
"This is expected for untrusted (development) certificates. "
"If it is supposed to be trusted, please check your certificate!"
)


class AzureSignTool(SigningTool):
def __init__(self):
super().__init__(os.environ.get("AZURE_SIGNTOOL_PATH", "AzureSignTool"))

def get_signing_command(self) -> str:

required_env_vars = (
"AZURE_SIGNTOOL_KEY_VAULT_URL",
"AZURE_SIGNTOOL_KEY_VAULT_CERTIFICATE",
)
check_required_env_vars(required_env_vars)
timestamp_server = os.environ.get(
"AZURE_SIGNTOOL_TIMESTAMP_SERVER_URL",
"http://timestamp.sectigo.com"
)
timestamp_digest = os.environ.get(
"AZURE_SIGNTOOL_TIMESTAMP_DIGEST",
"sha256"
)
file_digest = os.environ.get(
"AZURE_SIGNTOOL_FILE_DIGEST",
"sha256"
)

command = (
f"{win_str_esc(self.executable)} sign -v"
' -kvu "%AZURE_SIGNTOOL_KEY_VAULT_URL%"'
' -kvc "%AZURE_SIGNTOOL_KEY_VAULT_CERTIFICATE%"'
f' -tr "{timestamp_server}"'
f" -td {timestamp_digest}"
f" -fd {file_digest}"
)
# There are three ways to sign:
# 1. Access token
# 2. Secret (requires tenant ID)
# 3. Managed identity (requires prior login to Azure)
marcoesters marked this conversation as resolved.
Show resolved Hide resolved
if "AZURE_SIGNTOOL_KEY_VAULT_ACCESSTOKEN" in os.environ:
logger.info("AzureSignTool: signing binary using access token.")
command += ' -kva "%AZURE_SIGNTOOL_KEY_VAULT_ACCESSTOKEN%"'
elif "AZURE_SIGNTOOL_KEY_VAULT_SECRET" in os.environ:
# Authentication via secret required client and tenant ID
logger.info("AzureSignTool: signing binary using secret.")
required_env_vars = (
"AZURE_SIGNTOOL_KEY_VAULT_CLIENT_ID",
"AZURE_SIGNTOOL_KEY_VAULT_TENANT_ID",
)
check_required_env_vars(required_env_vars)
command += (
' -kvi "%AZURE_SIGNTOOL_KEY_VAULT_CLIENT_ID%"'
' -kvt "%AZURE_SIGNTOOL_KEY_VAULT_TENANT_ID%"'
' -kvs "%AZURE_SIGNTOOL_KEY_VAULT_SECRET%"'
)
else:
# No token or secret found, assume managed identity
logger.info("AzureSignTool: signing binary using managed identity.")
command += " -kvm"
return command

def verify_signing_tool(self):
self._verify_tool_is_available()
check_call([self.executable, "--help"], stdout=PIPE, stderr=PIPE)

def verify_signature(self, installer_file: Union[str, Path]):
"""Use Powershell to verify signature.

For available statuses, see the Microsoft documentation:
https://learn.microsoft.com/en-us/dotnet/api/system.management.automation.signaturestatus
"""
if shutil.which("powershell") is None:
logger.error("Could not verify signature: PowerShell not found.")
return
command = (
f"$sig = Get-AuthenticodeSignature -LiteralPath {installer_file};"
"$sig.Status.value__;"
"$sig.StatusMessage"
)
proc = run([
"powershell",
"-c",
command,
],
capture_output=True,
text=True,
)
# The return code will always be 0,
# but stderr will be non-empty on errors
if proc.stderr:
raise RuntimeError(f"Signature verification failed.\n{proc.stderr}")
try:
status, status_message = proc.stdout.strip().split("\n")
status = int(status)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this fail if status is empty or None?

Copy link
Contributor Author

@marcoesters marcoesters Apr 23, 2024

Choose a reason for hiding this comment

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

I'm not sure how this could be None unless proc.stdout can somehow be None.

If status is an empty string, int(status) raises a ValueError, which is caught further down.

if status > 1:
# Includes missing signature
raise RuntimeError(f"Error signing {installer_file}: {status_message}")
elif status == 1:
logger.error(
f"{installer_file} contains a signature that is either invalid or not trusted. "
"This is expected with development certificates. "
"If it is supposed to be trusted, please check your certificate!"
)
except ValueError:
# Something else is in the output
raise RuntimeError(f"Unexpected signature verification output: {proc.stdout}")
19 changes: 18 additions & 1 deletion constructor/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import re
import sys
from io import StringIO
from os import sep, unlink
from os import environ, sep, unlink
from os.path import basename, isdir, isfile, islink, join, normpath
from shutil import rmtree
from subprocess import check_call, check_output
Expand Down Expand Up @@ -265,3 +265,20 @@ def identify_conda_exe(conda_exe=None):
name = "micromamba"
version = output.strip()
return name, version


def win_str_esc(s, newlines=True):
maps = [('$', '$$'), ('"', '$\\"'), ('\t', '$\\t')]
if newlines:
maps.extend([('\n', '$\\n'), ('\r', '$\\r')])
for a, b in maps:
s = s.replace(a, b)
return '"%s"' % s


def check_required_env_vars(env_vars):
missing_vars = {var for var in env_vars if var not in environ}
if missing_vars:
raise RuntimeError(
f"Missing required environment variables {', '.join(missing_vars)}."
)
Loading
Loading