Uh oh!
There was an error while loading. Please reload this page.
Verify kernel archive integrity - #1703
Conversation
kasc0206
left a comment
There was a problem hiding this comment.
审查意见 / Review
This is a well-crafted security improvement. The SHA-256 digest verification for kernel downloads is an important addition.
优点 / Strengths
- End-to-end implementation: From CLI flag → config → XPC message → server-side verification, the full pipeline is covered
- Backward compatible:
sha256isString?with defaultnil, so existing configs work unchanged - Smart defaults: Auto-populates the default SHA-256 when using the default kernel URL, while allowing custom values for custom URLs
- No breaking API changes: All new parameters have sensible defaults
- Proper error messaging: The
--sha256can only be used with--tar
Suggestion
- Consider adding a
--sha256-verifyflag with--sha256-verify=falseto allow users to explicitly skip verification, rather than relying onnilmeaning "no verification". But this is optional — current behavior is reasonable.
已验证 / Verified
- Code compiles with
swift build✅ - Follows existing project patterns (XPC keys, Config struct, ProgressBar)
Great contribution!
kasc0206
commented
Jun 14, 2026
Code Review Summary / 代码审查摘要Strengths / 优点
Suggestion / 建议Consider adding a Verification / 验证
Great contribution! / 优秀的贡献！ |
katiewasnothere
commented
Jun 29, 2026
@haoruilee Thank you for the contribution! Could you add commit signatures to your commits? See https://github.com/apple/containerization/blob/main/CONTRIBUTING.md#pull-requests We definitely want this change, but I think we should change the flags, config fields, and XPC key names to exclude the name of the algorithm so we can support other hashing algorithms in the future. Maybe we should call this just |
briansmith
commented
Jun 29, 2026
Perhaps then the values should be prefixed with the digest algorithm. Perhaps it is worth just copying Subresource Integrity syntax, using "integrity" instead of "checksum" and prefixing the digest algorithm name with a dash in the values, e.g. (Note that using a dash instead of colon prevents any ambiguity with content-addressible-by-digest URI schemes like |
haoruilee
commented
Jun 30, 2026
Hi @katiewasnothere@briansmith , Thanks, that makes sense. I read these suggestions as complementary, the external names should avoid hard coding SHA-256, while the value itself should still carry the digest algorithm. I’ll update the PR to archive this and add commit signatures and revise the tests and docs accordingly. |
d24095e to
76e26b9Compare76e26b9 to
ea5ccebComparehaoruilee
commented
Jun 30, 2026
Updated, thanks for your suggestion. I renamed the API surface from Could you please take another look when you have a chance? |
katiewasnothere
commented
Jun 30, 2026
@haoruilee Thanks for the update! @briansmith this is a good suggestion, but naming the flag |
…rity # Conflicts: # Tests/CLITests/Subcommands/System/TestKernelSet.swift
haoruilee
commented
Jul 1, 2026
Hi @katiewasnothere , Thanks, I’ve updated the PR to rename the flag field to |
| |--------------|-----------|--------------------------------------------------------------------------------------------------------|------------------------------------------------------------------------------| | ||
| | `binaryPath` | `String` | `"opt/kata/share/kata-containers/vmlinux-6.18.15-186"` | Path **inside** the downloaded kernel archive that points to the kernel binary. | | ||
| | `url` | `URL` | `"https://github.com/kata-containers/kata-containers/releases/download/3.28.0/kata-static-3.28.0-arm64.tar.zst"` | Archive to download when no kernel is installed. Encoded and decoded as a plain string in TOML. | | ||
| | `digest` | `String?` | `"sha256:f63d54507d1f18635d94475077e4c2330de4d8e05cedf25f7c38f063b0e66a91"` | Expected digest for the archive, for example `sha256:<hex>`. When unset for a custom URL, remote kernel downloads are not verified. | |
There was a problem hiding this comment.
I think we should require that a digest is provided
Code Coverage
|
| } | ||
| @Test func customKernelURLWithoutDigestLeavesDigestUnset() async throws { | ||
| @Test func customKernelURLWithoutDigestThrows() async throws { |
There was a problem hiding this comment.
How about a test where the digest doesn't match the file contents?
There was a problem hiding this comment.
How about tests where:
- The digest algorithm is "sha1" (should fail).
- The digest algorithm is "sha256" and the digest is correct but truncated by one byte (should fail).
- The digest algorithm is "sha256" but the digest value is a correct SHA-1 (should fail).
There was a problem hiding this comment.
👍🏻 The archive content mismatch case is covered by installKernelFromLocalTarRejectsDigestMismatchWithoutInstalling, and I added coverage for sha1, truncated sha256, and a valid SHA-1 value passed as sha256.
| binaryPath: String = defaultBinaryPath, | ||
| url: URL = defaultURL | ||
| ) { | ||
| public init(binaryPath: String = defaultBinaryPath) { |
There was a problem hiding this comment.
Why do we want this init?
There was a problem hiding this comment.
Good point, the extra overload isn’t needed. I collapsed this into a single initializer with defaults for binaryPath, url, and digest.
| self.digest = Self.defaultDigest | ||
| } | ||
| public init(binaryPath: String = defaultBinaryPath, url: URL, digest: String) { |
There was a problem hiding this comment.
url and digest should have their default values set here
| let digestResult = try provider.value(forKey: AbsoluteConfigKey(ConfigKey("kernel.digest")), type: .string) | ||
| guard digestResult.value != nil else { | ||
| throw ContainerizationError( | ||
| .invalidArgument, | ||
| message: "kernel.digest is required in '\(path)' when kernel.url configures a custom archive" |
There was a problem hiding this comment.
We could check for this in the decoder instead of having a custom validation function. What do you think?
There was a problem hiding this comment.
I think the decoder check is still useful, but not sufficient for the layered config case.
The rule I’m trying to enforce is: if a config layer overrides kernel.url with a non-default archive, that same layer must also provide the digest for that archive. The digest is tied to the archive contents, so it should not be inherited from another layer.
The decoder only sees the merged snapshot, so it can tell whether the final config has both kernel.url and kernel.digest, but it can’t tell which file each value came from. That means it would accept a custom URL from a higher precedence file paired with the default digest from a lower precedence file, which is the case this validation is meant to reject.
I kept this as a premerge loader validation and added a comment to make that layering requirement clearer.
There was a problem hiding this comment.
The rule I’m trying to enforce is: if a config layer overrides kernel.url with a non-default archive, that same layer must also provide the digest for that archive.
In my opinion it doesn't matter if we get the kernel url and kernel digest from different layers in a layered config case. If the digest does not match the kernel url's contents when we go to fetch the kernel, the user will get an error. I could see having the values in separate layers being useful. Is there a specific scenario you want to prevent with this?
| let fileIOThreadPool = NIOThreadPool(numberOfThreads: 1) | ||
| fileIOThreadPool.start() | ||
| let delegate = try HashingFileDownloadDelegate( |
There was a problem hiding this comment.
A follow up to this PR or future work could be to look into if we can do something similar to what is done for fetching image blobs here
There was a problem hiding this comment.
Agreed, that seems worth exploring as follow up work. I’d keep it separate from this PR since this one is focused on kernel archive integrity.
| } | ||
| } | ||
| static func verifyDigest(of file: URL, expected: String) throws { |
There was a problem hiding this comment.
Do we need this function when we have the one below on line 195?
There was a problem hiding this comment.
Agreed. I removed the extra string overload and updated the tests.
| // Mirrors AsyncHTTPClient's file download delegate while updating an optional | ||
| // SHA-256 hasher from the same response chunks that are written to disk. | ||
| private final class HashingFileDownloadDelegate: @unchecked Sendable, HTTPClientResponseDelegate { |
There was a problem hiding this comment.
After talking with others, I think for now we should just remove this in favor of downloading the whole tar in the kernel service and then calling verifyDigest(of file: URL, expected: ExpectedDigest) to incrementally compute the hash like we do if there's a local tar. This will make this PR more scoped and we can optimize further later by looking into the suggestion here. After removing this part, I'm ready to approve.
Uh oh!
There was an error while loading. Please reload this page.
Closesapple#1687 The default kernel archive is downloaded from a remote release URL during first-run setup and via `container system kernel set --recommended`. Previously, the archive contents were not verified after download, so integrity depended on HTTPS and the release artifact remaining unchanged. This change adds digest verification for kernel archives. The recommended/default kernel now has pinned digest metadata using an algorithm-prefixed value such as `sha256:<hex>`. `container system kernel set --tar` accepts `--digest`; remote tar URLs require it, and local tar archives can also be verified before unpacking and installation. The system config also supports `kernel.digest`, and a custom `kernel.url` must provide a digest for that archive.
…ot args (#14) apple/container **1.2.0** is out (previously tracked: `1.1.0`). Upstream notes: https://github.com/apple/container/releases/tag/1.2.0 — mirrored in `docs/upstream/apple-container-1.2.0.md`. ## Review checklist - [ ] New or changed CLI flags Gantry should surface (`container run/create/machine/build`) - [ ] Changed `--format json` shapes the DockerKit apple transport decodes - [ ] Fixed upstream bugs Gantry currently works around - [ ] `ContainerTooling.recommendedVersion` / feature gates need moving to `1.2.0` - [ ] MCP tools and App Intents that expose the affected commands - [ ] README and CHANGELOG entries for whatever is adopted Merging records the version as reviewed. Implement the adopted parts on this branch, or merge as-is and open follow-ups. --- <details><summary>Upstream release notes</summary> ## What's Changed * Add TestCLISystemLogs and TestCLITermIO integration tests in new integration test suite by @katiewasnothere in apple/container#1879 * Restore reverted migrations, migrate last tests. by @jglogan in apple/container#1880 * Removes obsolete CLITests directory. by @jglogan in apple/container#1886 * Integration coverage xpc helpers by @noah-thor in apple/container#1551 * Upgrade grpc-swift-nio-transport to 2.9.0 and remove HTTP2ConnectBuff… by @adityabagchi24 in apple/container#1790 * Updates containerization to 0.36.0. by @jglogan in apple/container#1912 * Use containerization version 0.37.0 by @adityaramani in apple/container#1932 * Verify kernel archive integrity by @haoruilee in apple/container#1703 * Add commit/issue alert to PR template. by @jglogan in apple/container#1945 * Remove `--skip-build` from test Makefile target. by @jglogan in apple/container#1951 * Restore `--skip-build`, enable `import testable` for release builds. by @jglogan in apple/container#1955 * [package]: bump container-builder-shim to 0.13.0 by @saehejkang in apple/container#1953 * Validate container ID from XPC requests by @katiewasnothere in apple/container#1956 * Remove force unwraps on XPC error set/get by @katiewasnothere in apple/container#1958 * Do not follow destination symlink when copying user configuration by @katiewasnothere in apple/container#1957 * Fix machine ID length test. by @jglogan in apple/container#1971 * Address flaky TestCLIKernelSetSerial suite. by @jglogan in apple/container#1976 * [gitignore]: ignore vscode workspace files by @saehejkang in apple/container#1966 * Update containerization dependency with new EXT4Unpacker func definition by @katiewasnothere in apple/container#1973 * Periodic dependency updates. by @jglogan in apple/container#1981 * Use ordered journal mode for unpacked images. by @jglogan in apple/container#1974 * Reword DNS container name resolution doc information by @katiewasnothere in apple/container#1960 * ci: bump the github-actions group across 1 directory with 3 updates by @dependabot[bot] in apple/container#1983 * Pass build config in when building protoc dependencies by @katiewasnothere in apple/container#1972 * Container test fixture package by @katiewasnothere in apple/container#1887 * Downgrade swift-collections to 1.5.1. by @jglogan in apple/container#1984 * Use `enum` for warmup images. by @jglogan in apple/container#1990 * Add missing dependencies to new ContainerTestSupport package by @katiewasnothere in apple/container#1994 * Add OCI maskedPaths and readonlyPaths support to Container API. by @jglogan in apple/container#1996 * Integration test - miscellaneous fixture and test refinements. by @jglogan in apple/container#1993 * Use log instead of print for system start status messages by @adityabagchi24 in apple/container#1889 * Fix BuilderStart race, parallelize `container build` tests. by @jglogan in apple/container#2002 * Allow custom kernel boot args via --kernel-arg by @arirubinstein in apple/container#1744 * fix: Increase XPC timeout for Machine API operations by @dev-kvt in apple/container#2006 * Update containerization import to latest 0.40.0 by @katiewasnothere in apple/container#2028 * Fix image env vars, build context checks, TCP/UDP port forward buffer, and validate plugin name by @katiewasnothere in apple/container#2027 * Update containerization import to 0.40.1 by @katiewasnothere in apple/container#2038 ## New Contributors * @haoruilee made their first contribution in apple/container#1703 * @arirubinstein made their first contribution in apple/container#1744 * @dev-kvt made their first contribution in apple/container#2006 **Full Changelog**: apple/container@1.1.0...1.2.0 </details> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Andrew <Andrew.Komkov@gmail.com>
## Summary - container 1.2.0 added digest verification for kernel archives (apple/container#1703): `kernel set --tar` accepts `--digest`, remote tar URLs require one, and the recommended default kernel now ships pinned sha256 digest metadata. containerization's `KernelConfig` gained a required `digest` field, and `ClientKernel.installKernelFromTar` an `expectedDigest` parameter — both reachable from Berthly's native XPC calls. - `KernelSetOptions` gained `digest: String?`, threaded into `setKernel(options:)`'s `installKernelFromTar` call. - `installDefaultKernelIfNeeded` now passes `expectedDigest: containerSystemConfig.kernel.digest` — the default kernel already carries a pinned digest from upstream. - New "Digest" field on the Set Kernel sheet's Tar Archive section, required only for remote URLs (matching upstream's exact rule), prefilled by "Use Recommended" alongside the URL. - `PARITY.md`'s System `kernel` row updated in the same commit. ## Why Part of the container 1.2.0 milestone (#79). Mirrors the security posture Berthly already applies elsewhere (pkg signature verification against a pinned Apple identity) — kernel archives were the one download path left unverified. Closes#79 ## Test plan - [x] `xcodebuild build` succeeds - [x] `xcodebuild test -only-testing:BerthlyTests` — full suite passes, including new `kernelDigest` coverage in `SystemConfigMappingTests` - [x] `swiftlint lint --strict` — 0 violations - [x] Verified visually via Xcode's live preview renderer — confirmed the Tar Archive section (including the recommended-URL prefill) renders correctly
## Summary - `resolvedSystemConfig()` used `try?` around `ConfigurationLoader.load()`, so any decode failure silently fell back to an all-defaults `ContainerSystemConfig` — not just the field that failed, the user's entire `config.toml` (DNS, build settings, kernel, everything). - `ConfigurationLoader.load()` only throws when a config file exists on disk but fails to parse/decode; it already returns cleanly with defaults when no file is present at all — so every error caught here represents real discarded user data, not an absent-file no-op. - container 1.2.0 made this concrete: `KernelConfig`'s decoder now throws when `kernel.url` is customized without a paired `kernel.digest` (apple/container#1703). Any Berthly user who ran `container system kernel set --tar <url>` pre-1.2.0 has exactly that config shape, and would silently lose their whole system config on next load once the daemon is upgraded. - Extracts the load-outcome handling into a pure, testable `mapSystemConfigLoadResult(_:)` and surfaces a failure through the existing `lastStartupWarning` mechanism instead of swallowing it. ## Why Found while implementing #79 (kernel digest verification) — this milestone's own version bump is what triggers the failure for affected users, so it needs fixing in the same milestone. Closes#87 ## Test plan - [x] `xcodebuild build` succeeds - [x] `xcodebuild test -only-testing:BerthlyTests` — full suite passes, including new `SystemConfigLoadResultMappingTests` covering both the success and failure paths - [x] `swiftlint lint --strict` — 0 violations - [x] No View files touched — no UI test needed for this change (tracked separately in #89: the sidebar's warning-state rendering itself has no UI coverage yet, pre-existing gap this PR surfaces a second producer into)
Closesapple#1687 The default kernel archive is downloaded from a remote release URL during first-run setup and via `container system kernel set --recommended`. Previously, the archive contents were not verified after download, so integrity depended on HTTPS and the release artifact remaining unchanged. This change adds digest verification for kernel archives. The recommended/default kernel now has pinned digest metadata using an algorithm-prefixed value such as `sha256:<hex>`. `container system kernel set --tar` accepts `--digest`; remote tar URLs require it, and local tar archives can also be verified before unpacking and installation. The system config also supports `kernel.digest`, and a custom `kernel.url` must provide a digest for that archive.
Type of Change
Motivation and Context
Closes#1687
The default kernel archive is downloaded from a remote release URL during first-run setup and via
container system kernel set --recommended. Previously, the archive contents were not verified after download, so integrity depended on HTTPS and the release artifact remaining unchanged.This change adds digest verification for kernel archives. The recommended/default kernel now has pinned digest metadata using an algorithm-prefixed value such as
sha256:<hex>.container system kernel set --taraccepts--digest; remote tar URLs require it, and local tar archives can also be verified before unpacking and installation.The system config also supports
kernel.digest, and a customkernel.urlmust provide a digest for that archive.Testing
Validated locally:
git diff --check upstream/main...HEADswift test --filter KernelServiceTestsswift test --filter ConfigurationLoaderTests