Add --secret flag to image build command#41106
Conversation
Replace the hand-maintained DeleteAllBuiltImages() static list (run in both TEST_METHOD_SETUP and TEST_METHOD_CLEANUP, sweeping every built image twice per test and flooding the log with VERIFY lines) with: - A per-test DeleteImageOnExit() RAII guard so each test owns and cleans up exactly the image(s) it builds. It is best-effort and non-throwing (no VERIFY) since it may run during stack unwinding after a failure. - A single class-level DeleteImagesWithRepositoryPrefix() prune in ClassSetup and ClassCleanup as the authoritative safety net for images left behind by a crashed run. This removes the per-method sweeps, the static image list, and the excessive per-field VERIFY logging that dominated test output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Implements --secret for wslc image build, matching the Docker/BuildKit CLI so Dockerfiles can consume build secrets via RUN --mount=type=secret. Supported forms (Docker parity): - id=NAME,src=PATH / id=NAME,source=PATH - secret from a file - id=NAME,env=VAR - secret from an environment variable - id=NAME,type=env|file - explicit source type - id=NAME - bare form; falls back to env var NAME When both env= and src= are given, the environment variable wins (Docker parity). Delivery: secret contents are read client-side as raw bytes and passed to the service over COM as a counted byte array, then streamed into a per-build, root-only tmpfs file (0700 dir / 0600 file) under /run/wslc-secrets/<guid> in the utility VM and exposed to buildkit via --secret id=<id>,src=<path>. The per-build directory is removed on completion. This is binary-safe (embedded NULs / arbitrary bytes) and mounts no host paths into the guest. Known gaps vs Docker: - The secret value is buffered whole in memory on the client->service hop rather than streamed; fine for typical (small) secrets, but not suited to very large payloads. - A bare id=NAME whose environment variable is unset yields an empty secret rather than erroring (minor divergence). - The accepted id character set is deliberately stricter than Docker's, as an injection guard for the generated buildkit argument. - --ssh forwarding is not implemented (separate feature). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Docker parity: --secret id=NAME with no env=/src= now fails when the id-named environment variable is undefined, instead of forwarding an empty secret. Adds an e2e test for the failure path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request adds Docker-style --secret support to wslc image build, including CLI argument wiring, client-side parsing/validation and transport of raw secret bytes to the service, and server-side staging of secrets into VM tmpfs files for docker build --secret id=...,src=... consumption. It also expands E2E coverage for secret semantics and adjusts image-build test cleanup to be more robust.
Changes:
- Added
--secretCLI argument, localized help/error strings, and parsing/validation forid=...[,type=env|file][,env=...][,src=...]specs. - Plumbed secrets through the internal API (new structs + IDL) and implemented VM-side staging/cleanup for secret bytes during image builds.
- Added extensive E2E tests for secret behavior and updated test cleanup to avoid leaving built images behind.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/windows/wslc/e2e/WSLCE2EImageBuildTests.cpp | Adds E2E coverage for --secret and refactors cleanup to per-test RAII + prefix sweep safety net |
| test/windows/wslc/e2e/WSLCE2EHelpers.h | Declares new helper to delete images by repository prefix |
| test/windows/wslc/e2e/WSLCE2EHelpers.cpp | Implements DeleteImagesWithRepositoryPrefix using image list --format json |
| src/windows/wslcsession/WSLCSession.cpp | Adds secrets array validation and stages secrets into VM tmpfs files for docker build |
| src/windows/wslc/tasks/ImageTasks.cpp | Implements --secret parsing/validation and forwards secrets to the service layer |
| src/windows/wslc/services/ImageService.h | Introduces BuildSecret and extends Build API to accept secrets |
| src/windows/wslc/services/ImageService.cpp | Marshals secrets into WSLCBuildSecretArray for the COM call |
| src/windows/wslc/commands/ImageBuildCommand.cpp | Registers --secret as a build argument |
| src/windows/wslc/arguments/ArgumentDefinitions.h | Adds new argument type Secret |
| src/windows/service/inc/wslc.idl | Adds WSLCBuildSecret / WSLCBuildSecretArray and extends WSLCBuildImageOptions |
| localization/strings/en-US/Resources.resw | Adds localized strings for invalid secret spec and --secret help text |
Reject an empty --secret id up-front (would produce an invalid id=,src=... spec), and record the VM mount path before calling MountWindowsFolder so an allocation failure can never leak an untracked mount. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Docker parity: a secret 'id' may not start with '-' as it would be interpreted as a command-line option. Validate this up-front in ParseSecretSpec with an actionable error. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Relocate all --secret spec validation (key=value syntax, unknown keys, id required/format, unsupported type, type=file requires src, bare-id env var must be set, src file must exist) from ParseSecretSpec in ImageTasks into validation::ValidateSecretSpec so invalid specs are rejected during argument validation. ParseSecretSpec now only parses the validated spec and resolves the secret bytes. Update the UnknownType e2e test to match the argument-validation error presentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…soft/WSL into user/johnstep/build-secret
| auto contextDir = testRoot / L"context"; | ||
| std::error_code ec; | ||
| std::filesystem::create_directories(contextDir, ec); | ||
| THROW_HR_IF(E_FAIL, ec.value() != 0 || !std::filesystem::exists(contextDir)); |
| auto contextDir = testRoot / L"context"; | ||
| std::error_code ec; | ||
| std::filesystem::create_directories(contextDir, ec); | ||
| THROW_HR_IF(E_FAIL, ec.value() != 0 || !std::filesystem::exists(contextDir)); |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (4)
src/windows/wslc/arguments/ArgumentValidation.cpp:244
env=andsrc=are accepted even when the value is empty (e.g.env=). That makes the spec ambiguous and changes behavior (it gets treated like the key was omitted) instead of reporting an invalid--secretvalue.
else if (key == L"env")
{
envName = value;
}
else if (key == L"src" || key == L"source")
{
srcPath = value;
}
test/windows/wslc/e2e/WSLCE2EImageBuildTests.cpp:364
- This test creates a unique context directory (
testRoot/context), which will mount a distinct path into the VM and consume an additional virtiofs share slot. That contradicts the shared-context mitigation comment above and can still exhaust the session's share budget across the secret tests.
auto contextDir = testRoot / L"context";
std::error_code ec;
std::filesystem::create_directories(contextDir, ec);
THROW_HR_IF(E_FAIL, ec.value() != 0 || !std::filesystem::exists(contextDir));
test/windows/wslc/e2e/WSLCE2EImageBuildTests.cpp:498
- This test creates a unique context directory (
testRoot/context), which will mount a distinct path into the VM and consume an additional virtiofs share slot. For secret tests, preferSharedSecretBuildContext()so the session doesn't exhaust its share slots.
auto contextDir = testRoot / L"context";
std::error_code ec;
std::filesystem::create_directories(contextDir, ec);
THROW_HR_IF(E_FAIL, ec.value() != 0 || !std::filesystem::exists(contextDir));
test/windows/wslc/e2e/WSLCE2EImageBuildTests.cpp:695
- This test creates a unique context directory (
testRoot/context), which mounts a distinct path and consumes another virtiofs share slot. To match the shared-context strategy for secret tests, useSharedSecretBuildContext()here too.
auto contextDir = testRoot / L"context";
std::error_code ec;
std::filesystem::create_directories(contextDir, ec);
THROW_HR_IF(E_FAIL, ec.value() != 0 || !std::filesystem::exists(contextDir));
| std::error_code ec; | ||
| // Resolve symlinks (and normalize '..') so we read the file that actually holds the secret's | ||
| // bytes rather than the link node itself. | ||
| auto absPath = std::filesystem::weakly_canonical(std::filesystem::absolute(srcPath), ec); | ||
|
|
| std::error_code ec; | ||
| // Resolve symlinks (and normalize '..') so we validate the file that actually holds the | ||
| // secret's bytes rather than the link node itself. | ||
| auto absPath = std::filesystem::weakly_canonical(std::filesystem::absolute(srcPath), ec); | ||
| if (ec.value() != 0 || !std::filesystem::is_regular_file(absPath, ec)) | ||
| { | ||
| throw ArgumentException(Localization::MessageWslcSecretInvalidSpec( | ||
| spec, std::format(L"source file not found or not a regular file: {}", absPath.wstring()))); | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/windows/wslc/services/ImageService.cpp:190
secret.Value.size()is cast toULONGwithout bounds checking. For secrets > 4GB this truncates the size, causing COM marshalling to copy only the low 32 bits and silently stage a corrupted/partial secret.
secretEntries.push_back(WSLCBuildSecret{
.Id = secretIdStrings.back().c_str(),
.Value = secret.Value.data(),
.ValueSize = static_cast<ULONG>(secret.Value.size()),
});
localization/strings/en-US/Resources.resw:2876
- The
--secrethelp text currently impliesenv=andsrc=are mutually exclusive (env=VAR|,src=PATH), but the implementation explicitly allows both (with env winning). This mismatch can confuse users.
<value>Expose secret to the build (id=NAME[,type=env|file][,env=VAR|,src=PATH]; defaults to env var NAME)</value>
| }; | ||
| } | ||
|
|
||
| static bool TryInspectImage(Session& session, const std::string& imageId, std::optional<wslc_schema::InspectImage>& inspectData) |
| auto cancelEvent = context.CreateCancelEvent(); | ||
| BuildImageCallback callback(context.Reporter, cancelEvent, context.Args.Contains(ArgType::Verbose)); | ||
| services::ImageService::Build(session, contextPath, tags, buildArgs, labels, dockerfilePath, target, flags, &callback, cancelEvent); | ||
| BuildImageCallback callback(cancelEvent, context.Args.Contains(ArgType::Verbose)); | ||
| services::ImageService::Build(session, contextPath, tags, buildArgs, labels, secrets, dockerfilePath, target, flags, &callback, cancelEvent); |
This pull request adds support for Docker-style
--secretarguments to the image build command, enabling secure injection of secrets (from environment variables or files) into the build process. It introduces robust parsing and validation for secret specifications, updates the internal data structures and APIs to handle secrets, and ensures safe handling and cleanup of resources during the build. The most important changes are grouped below.Feature: Add Docker-style
--secretsupport to image buildsSecret) to the CLI and command definitions, along with user-facing descriptions and error messages for invalid secret specs. (src/windows/wslc/arguments/ArgumentDefinitions.h[1]localization/strings/en-US/Resources.resw[2] [3]--secretspecs, supporting secrets from environment variables or files, and matching Docker's semantics and error handling. (src/windows/wslc/tasks/ImageTasks.cpp[1] [2]Internal API and Data Structure Updates
BuildSecretandWSLCBuildSecretstructs, and updated the image build options and service interfaces to pass secrets through all layers. (src/windows/service/inc/wslc.idl[1] [2]src/windows/wslc/services/ImageService.h[3] [4]src/windows/wslc/services/ImageService.cpp[5] [6] [7]src/windows/wslc/commands/ImageBuildCommand.cpp[1]src/windows/wslc/tasks/ImageTasks.cpp[2]Resource Management and Robustness
src/windows/wslcsession/WSLCSession.cppsrc/windows/wslcsession/WSLCSession.cppL910-R934)src/windows/wslcsession/WSLCSession.cppsrc/windows/wslcsession/WSLCSession.cppR883)Summary of the Pull Request
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed