feat: sign Linux system packages when building with Docker - #2987
feat: sign Linux system packages when building with Docker#2987siddhant-bayas wants to merge 17 commits into
Conversation
Add GnuPG.export_secret_key(), which exports a secret key from the local keyring to a file. This will be used to inject a signing identity into a Docker container.
The Docker image is built before the signing identity is selected, so the signing tool (debsigs, rpm-sign, or gnupg) is added to SYSTEM_REQUIRES for every image build. On SUSE, no rpm-sign package is installed, as rpmsign is provided by rpm-build, which is already part of the image. The "system" packaging format is also resolved to its concrete format before the app context is verified, so the image can be built with the tools needed to package and sign the app.
When the package is built inside Docker, the selected secret key is exported from the host's GPG keyring to a file in the data path (which is mounted into the container), then imported and used to sign the package in a single container run. The exported key is deleted immediately after signing, so it is never stored in the image. The dist folder is mounted into the container for the duration of the signing step.
For consistency with other platforms, the signing tool error now hints that the package can be produced unsigned with --adhoc-sign.
freakboy3742
left a comment
There was a problem hiding this comment.
This looks pretty good, and works great in my testing. A couple of tweaks and questions inline.
More generally - the process of exporting a key into the container still makes me a little nervous, because it's difficult to be 100% certain whether the key has been persisted into a cache somewhere.
I know we've ruled out being able to mount the .gnupg folder directly because of version incompatibilities - but I've seen some suggestion online that it might be possible to use gpg in agent mode to export the gpg socket (via gpgconf --list-dirs agent-extra-socket) into the docker container, and do the signing using the host gpg. Could I ask you to investigate that option to see what might be possible?
| f"({system_version!r})." | ||
| ) | ||
|
|
||
| def _docker_signing_tool(self, app: LinuxSystemAppConfig) -> list[str]: |
There was a problem hiding this comment.
How is this method any different to the existing _signing_tool()? There's clearly more logic required because of the differences related to SuSE - but we shouldn't have 2 independent places to define the package names for signing tools.
There was a problem hiding this comment.
They're now unified with no duplicate mappings. _signing_tool() (moved to LinuxSystemMixin, system.py:408) is the single source of truth: it resolves the format and returns the (tool, executable, package) triple from the shared _SIGNING_TOOLS map. _docker_signing_tool() (system.py:760) delegates to it and only adds the SUSE case (rpm-sign comes from rpm-build, already in the image) plus the list return type.
There was a problem hiding this comment.
This is an example of a place where you need to keep an AI agent on a short leash.
I'm guessing what you've done here is point your agent at this code, and my comment, and asked it to refactor. And that's exactly what it done... but it's gone too far, and somewhat in the wrong direction. _SIGNING_TOOLS is used in exactly one location, so there's no re-use value in having the constant. _SYSTEM_PACKAGING_FORMATS is used in 2 locations... but based on what I can see, one of those should be redundant - by the time you're invoking _signing_tool(), the packaging format should be a resolved property of the app.
And, my original question still stands - what is the difference between _signing_tool() and _docker_signing_tool()? Ultimately, both are returning details of the tools used to sign apps. _docker_signing_tool() does two things:
- Drops all but the package name from the results
- Performs a light transformation when we know the target platform is SuSE.
The former doesn't need a separate method - it's "call the same method and ignore the first two results"; and AFAICT, the second is just as applicable to local SuSE installs - right now, if you run a local signing on a SuSE machine, and it can't find rpmsign, Briefcase will tell you to install rpm-sign... which is incorrect.
I'm guessing this actually can't happen in practice because the packaging tools will be verified before the signing tools are checked - but that's not a reason to have 2 methods for one task.
|
|
||
| subprocess_kwargs: dict[str, Any] = {} | ||
| key_file_path: Path | None = None | ||
| if isinstance(self.tools[app].app_context, DockerAppContext): |
There was a problem hiding this comment.
I'm pretty sure this could be simplified to:
| if isinstance(self.tools[app].app_context, DockerAppContext): | |
| if self.use_docker: |
| # to the container. Export the key to the data path (which is mounted | ||
| # into the container), then import it and sign the package in a single | ||
| # container run, so the key is not retained in the image or container. | ||
| key_file_path = self.data_path / f"{app.app_name}-signing-key.gpg" |
There was a problem hiding this comment.
The idea here is sound, but I don't think self.data_path is the best location. While it is a safe and mounted location, it's a little opaque (in that most users won't ever look in there), and shared between projects.
I think it would be better to use a location in the bundle_path - that becomes an added level of safety because the bundle path is transient (so it won't get added to version control) and app specific (so there's no risk of name collision).
| key_file_path = self.data_path / f"{app.app_name}-signing-key.gpg" | |
| key_file_path = self.bundle_path(app) / "signing-key.gpg" |
Address review feedback on the Docker signing implementation:
* Move _signing_tool to LinuxSystemMixin and unify it with
_docker_signing_tool, sharing a single _SIGNING_TOOLS mapping and a
_SYSTEM_PACKAGING_FORMATS mapping.
* Select the Docker key handling based on use_docker, rather than
inspecting the app context type.
* Export the signing key to the bundle path, and use container paths
(/app, /dist) for the key import and package signing steps so the
signing step works correctly with Windows paths on the host.
* Fix vendor base names in tests ('redhat' -> 'rhel').
* Document the passphrase requirement for keys used with Docker builds.
* Drop the separate changelog fragment; signing is covered by the
existing fragment.
freakboy3742
left a comment
There was a problem hiding this comment.
A couple of refactoring/cleanup issues flagged.
The bigger issue, however, is that you haven't addressed the question I asked in my last review - is the socket export approach workable? That would be much more preferable to a file export, as it completely avoids the "file in a layer" issue, and I think it might even resolve the key-with-password issue (since it will be the system GPG, outside the Docker container) that does the signing.
| f"({system_version!r})." | ||
| ) | ||
|
|
||
| def _docker_signing_tool(self, app: LinuxSystemAppConfig) -> list[str]: |
There was a problem hiding this comment.
This is an example of a place where you need to keep an AI agent on a short leash.
I'm guessing what you've done here is point your agent at this code, and my comment, and asked it to refactor. And that's exactly what it done... but it's gone too far, and somewhat in the wrong direction. _SIGNING_TOOLS is used in exactly one location, so there's no re-use value in having the constant. _SYSTEM_PACKAGING_FORMATS is used in 2 locations... but based on what I can see, one of those should be redundant - by the time you're invoking _signing_tool(), the packaging format should be a resolved property of the app.
And, my original question still stands - what is the difference between _signing_tool() and _docker_signing_tool()? Ultimately, both are returning details of the tools used to sign apps. _docker_signing_tool() does two things:
- Drops all but the package name from the results
- Performs a light transformation when we know the target platform is SuSE.
The former doesn't need a separate method - it's "call the same method and ignore the first two results"; and AFAICT, the second is just as applicable to local SuSE installs - right now, if you run a local signing on a SuSE machine, and it can't find rpmsign, Briefcase will tell you to install rpm-sign... which is incorrect.
I'm guessing this actually can't happen in practice because the packaging tools will be verified before the signing tools are checked - but that's not a reason to have 2 methods for one task.
| str(signature_path), container_signature_path | ||
| ) | ||
| for arg in sign_command | ||
| ] |
There was a problem hiding this comment.
This argument re-writing shouldn't be needed - one of the things the Docker layer does is path transformation. If that isn't working, it's possibly an indicator of a bigger bug.
Move the Docker-specific handling inside the try/finally block, and key all conditional behavior on the use_docker property. The manual rewriting of paths in the sign command is removed; the Docker layer already rewrites host paths to their container equivalents based on the mount definitions.
Collapse _docker_signing_tool() into _signing_tool(); the Docker image install step now uses the same method, ignoring the tool and executable names. The SUSE correction (rpmsign is provided by rpm-build, not a separate rpm-sign package) is now applied to local installs as well, so error messages suggest installing the correct package. The single-use _SIGNING_TOOLS constant is folded into _signing_tool(), and resolution of the "system" packaging format moves into app config finalization, so the packaging format is a resolved property of the app by the time any signing logic runs.
|
Thanks for the review! Both commits (
On agent socket forwarding: I actually tried this instead of guessing. Bind-mounting the gpg-agent socket straight into the container only works on Linux-native Docker; Docker Desktop (macOS/Windows) can't forward arbitrary Unix sockets (see docker/for-mac #5297 and #7204). But briefcase already solves this kind of problem for X11 passthrough with a socat/TCP bridge, and the same trick works for gpg-agent:
I got this working end to end: signed inside the container using the host's agent, then verified the signature back on the host. The secret key never leaves the host machine. This is literally what the extra-socket was designed for (it's in the gpg-agent docs), and the GnuPG wiki documents this exact socat pattern for SSH forwarding. Your hunch about passphrases was right too. Passphrase prompts go to the host agent, so pinentry pops up on the user's desktop like normal. My test actually failed with "Operation cancelled" at first, which turned out to be because it was trying to launch pinentry on my headless host, so that confirms the routing works as expected. A few caveats I hit:
Given all that, I'd lean toward keeping the current export/import approach for this PR. It works everywhere including CI, needs no extra tooling, and cleans up the key afterwards. Agent forwarding feels like a good follow-up for interactive users who want password-protected keys with Docker builds. Happy to open a tracking issue if you agree. |
The expected sign command is now built from the same Path objects the code under test uses, rather than hard-coded POSIX style strings. On Windows, pathlib normalises paths to backslash separators, which also affects the shell quoting applied to the command, so the expectations must be computed at runtime to match.
The package and publish commands annotate the CLI-selected packaging format onto the app after app finalization has occurred. This overwrote the concrete packaging format determined during finalization with the raw "system" alias, causing packaging tool verification to fail. The package and publish commands for Linux system backends now resolve the "system" alias to the format determined during finalization before invoking the base command behavior.
If the app configuration doesn't specify a packaging format, the format implied by the vendor base is now determined during app finalization. Previously, resolution only occurred if "system" was explicitly set, so apps with no configured format kept the raw CLI default until packaging tool verification failed.
freakboy3742
left a comment
There was a problem hiding this comment.
Thanks for the update; I've flagged a couple of issues inline, one of which (the availability of the package_format option) is going to require some thought.
The one remaining questions are around the export vs socket handling.
Given the sensitive nature of signing keys, I'd much rather err on the side of caution. Although the window for exposure is small with exported files used as they are used in this PR, the window exists, and that means there's a potential problem. Therefore, I'd rather use the socket approach, even if that means there are limitations (in either the short or long term).
Signing macOS packages on Linux is always going to be an edge case. I'm happy for it to be documented as such (or for macOS signing to require some extra config hurdles). If that means we defer macOS signing until a future iteration, that's fine as well.
As for CI environments: CI environments aren't ever going to allow password protected keys, so I don't think we need to go to extraordinary lengths to accommodate that use case (other than documenting the limitation).
| ) | ||
|
|
||
| # The packaging format implied by each system vendor base. | ||
| _SYSTEM_PACKAGING_FORMATS = { |
There was a problem hiding this comment.
As flagged in my previous review - this is an example of a constant that is used in one location. There's no reason to have it pulled out here.
| update, | ||
| _resolve_system_packaging_format(app, packaging_format), | ||
| **options, | ||
| ) |
There was a problem hiding this comment.
I can't see why this abstraction is needed. At the point _package_app is invoked the app config will be finalised. That means it has (or should be able to guarantee) a known-good app.packaging_format setting.
| _resolve_system_packaging_format(app, packaging_format), | ||
| channel, | ||
| **options, | ||
| ) |
There was a problem hiding this comment.
As with package - this seems redundant.
| raise BriefcaseCommandError( | ||
| "Briefcase doesn't know the system packaging format for " | ||
| f"{app.target_vendor}. You may be able to build a package " | ||
| "by manually specifying a format with -p/--packaging-format" |
There was a problem hiding this comment.
Ok - this has exposed an interesting gap.
finalize_app_config is run on every command - including create and run. Those commands don't expose a -p option, so this help command is, at the very least, misleading.
It also means that it's no longer possible to even create a Linux app for an unidentified Linux distribution without modifying the app configuration to add packaging_format.
I think this might also be the cause of the roundabout logic you've added for _package_app - you're essentially adding code to work around the fact that finalise is setting the value for -p, which can then be overridden during packaging. Except that if the user has already had to bake in packaging_format to get past create, passing in any different value with -p will be an error.
So - there's a workflow issue we need to resolve here. At the very least, this needs to be only enforced for Docker builds; not supporting Docker builds for unknown linux distros would also be acceptable.
Finalization now resolves an absent or "system" packaging format when the vendor base is known; native builds on unrecognized distributions retain the unresolved alias, deferring any error until packaging, where a clear "use -p/--packaging-format" message is raised. Docker builds still require a known vendor base at finalization, since the target image must be built with the tools needed to package and sign the app; those commands have no -p option, so finalization raises an error suggesting the packaging_format configuration option. Rather than working around the CLI annotation of packaging formats, the Linux package and publish commands now follow the macOS pattern of declaring no default packaging format; the base commands only annotate the app when a format was explicitly specified.
6c476e7 to
7c9e1d9
Compare
|
Looks like you've broken something on the macOS builds - they're all failing with an error about As an aside - please don't force-push branches; it can cause historical comments to become orphaned. It's not a problem for commit history to be messy, as we use merge commits when we accept a PR. |
Add Docker support for code signing Linux system packages.
briefcase package linux systemcan now sign.deb,.rpmand.pkg.tar.zstpackages when the package is built in a Docker container (i.e., using the--targetoption), using the same GPG signing identity as native builds.When packaging inside Docker:
debsigsfor.deb,rpm-signfor.rpm,gnupgfor.pkg.tar.zst) is installed in the build container. The tool is installed for every image build, since the image is built before the signing identity is selected. On SUSE, no additional package is installed, asrpmsignis provided byrpm-build, which is already part of the image.distfolder is mounted into the container for the duration of the signing step only.If the required signing tool is missing, the error now also suggests
--adhoc-signas an alternative to skipping signing, for consistency with other platforms.As with native builds, a key that requires a passphrase cannot be used when signing a package built with Docker, since GnuPG cannot prompt for a passphrase inside a headless container. The code signing docs now describe the failure mode, and explain how to create a key without a passphrase for use with Docker builds.
Refs #2984
PR Checklist:
Assisted-by: big-pickle (docs), claude-opus-4-8 (debugging)