Add registry allowlist support to image builds - #41211
Conversation
…to close a user-tampering race
There was a problem hiding this comment.
Pull request overview
This PR adds enforcement of the WSLContainerRegistryAllowlist policy for wslc image build by generating a BuildKit source-policy JSON on the SYSTEM side, mounting it into the VM, and passing it to BuildKit via EXPERIMENTAL_BUILDKIT_SOURCE_POLICY. This replaces the previous behavior that rejected image builds outright whenever an allowlist was configured.
Changes:
- Route
wslc image buildallowlist enforcement through a SYSTEM-owned BuildKit source policy (prepare + cleanup) instead of hard-blocking builds. - Add SYSTEM-side implementation to materialize and validate lifecycle of the policy folder, and wire new COM methods through session/VM layers.
- Expand policy tests to cover allow/deny/case-insensitivity/multistage/implicit-docker.io scenarios and add snapshot logic tests; update the localized fail-closed message.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/windows/PolicyTests.cpp | Updates/extends policy tests for BuildKit source-policy based allowlist enforcement and snapshot reading logic. |
| src/windows/wslcsession/WSLCVirtualMachine.h | Adds VM wrapper method declarations for preparing/cleaning the BuildKit source policy. |
| src/windows/wslcsession/WSLCVirtualMachine.cpp | Implements VM wrapper forwarding calls for the new BuildKit source policy methods. |
| src/windows/wslcsession/WSLCSession.cpp | Uses SYSTEM-prepared policy folder, mounts it, and sets EXPERIMENTAL_BUILDKIT_SOURCE_POLICY for docker buildx build. |
| src/windows/service/inc/wslc.idl | Extends IWSLCVirtualMachine with PrepareBuildKitSourcePolicy / CleanupBuildKitSourcePolicy. |
| src/windows/service/exe/HcsVirtualMachine.h | Declares new COM methods and adds tracking for prepared policy folders. |
| src/windows/service/exe/HcsVirtualMachine.cpp | Implements BuildKit source-policy JSON generation, secure policy folder creation, and lifecycle management. |
| src/windows/inc/wslpolicies.h | Adds registry allowlist snapshot APIs to distinguish not-configured vs read-failed (fail closed). |
| localization/strings/en-US/Resources.resw | Updates the user-facing message for the fail-closed “policy could not be evaluated” path. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/windows/service/exe/HcsVirtualMachine.cpp:937
- The
WriteFilecall doesn’t capture/validate the number of bytes written. If a short write occurs,policy.jsoncan be truncated and policy enforcement may behave unpredictably (invalid JSON / partial rules).
wil::unique_handle file{CreateFileW(policyFile.c_str(), GENERIC_WRITE, 0, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr)};
THROW_LAST_ERROR_IF(!file);
THROW_IF_WIN32_BOOL_FALSE(WriteFile(file.get(), policyJson.data(), gsl::narrow_cast<DWORD>(policyJson.size()), nullptr, nullptr));
}
src/windows/service/exe/HcsVirtualMachine.cpp:15
- This .cpp file doesn’t include the Windows-side precompiled header first. In this repo’s Windows service code, .cpp files include
precomp.hfirst (e.g.src/windows/service/exe/LxssUserSession.cpp:15), and this file now relies on standard library headers (e.g.<algorithm>,<ranges>) that are typically pulled in via the precompiled header.
#include "HcsVirtualMachine.h"
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/windows/service/exe/HcsVirtualMachine.cpp:936
- WriteFile is being called with lpNumberOfBytesWritten == nullptr while lpOverlapped is also nullptr. Per Win32 WriteFile contract, lpNumberOfBytesWritten must be non-null for synchronous writes; as written this will fail (typically ERROR_INVALID_PARAMETER) and prevent BuildKit policy materialization whenever the allowlist is configured.
wil::unique_handle file{CreateFileW(policyFile.c_str(), GENERIC_WRITE, 0, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr)};
THROW_LAST_ERROR_IF(!file);
THROW_IF_WIN32_BOOL_FALSE(WriteFile(file.get(), policyJson.data(), gsl::narrow_cast<DWORD>(policyJson.size()), nullptr, nullptr));
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/windows/service/exe/HcsVirtualMachine.cpp:428
- The destructor comment says the policy folder DACL is "SYSTEM-only", but CreatePolicyFolder explicitly grants the calling user read+traverse (FRFX) so the guest file server can impersonate the user. This is a documentation mismatch that could confuse future audits/reviews.
// Sweep any BuildKit source-policy folders that were prepared but never cleaned up (e.g. the
// client crashed between PrepareBuildKitSourcePolicy and CleanupBuildKitSourcePolicy). The
// DACL is SYSTEM-only, so a leak is not exploitable, but folders would otherwise accumulate
// in %SystemRoot%\Temp across crashes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
src/windows/service/exe/HcsVirtualMachine.cpp:934
CreateFileWreturnsINVALID_HANDLE_VALUEon failure, butwil::unique_handletreatsnullptras the invalid value. This means failures won’t be detected andCloseHandle(INVALID_HANDLE_VALUE)may run. Usewil::unique_hfile(or explicitly check forINVALID_HANDLE_VALUE).
wil::unique_handle file{CreateFileW(policyFile.c_str(), GENERIC_WRITE, 0, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr)};
src/windows/service/exe/HcsVirtualMachine.cpp:435
- The destructor sweep deletes policy folders while the VM/shares may still be alive, ignores any
remove_allerror, and then clearsm_preparedPolicyFoldersunconditionally. If deletion fails due to an in-use Plan9/VirtioFS share, the folder will leak (and won’t be retried later in the destructor), which conflicts with the intended “retry at shutdown” behavior described elsewhere in this change.
// Sweep any BuildKit source-policy folders that were prepared but never cleaned up (e.g. the
// client crashed between PrepareBuildKitSourcePolicy and CleanupBuildKitSourcePolicy). The
// DACL is SYSTEM-only, so a leak is not exploitable, but folders would otherwise accumulate
// in %SystemRoot%\Temp across crashes.
for (const auto& folder : m_preparedPolicyFolders)
{
std::error_code ec;
std::filesystem::remove_all(folder, ec);
}
m_preparedPolicyFolders.clear();
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/windows/service/exe/HcsVirtualMachine.cpp:21
- This file uses
std::transform,std::ranges::transform,std::back_inserter, andstd::tolower, but it doesn't explicitly include the standard headers that declare them. Relying on transitive includes makes the build brittle and can break with toolset/header changes. Add the missing headers (<algorithm>,<iterator>,<cctype>) near the top of the file.
#include "HcsVirtualMachine.h"
#include <format>
#include <fstream>
#include <string>
#include <string_view>
| // https://github.com/moby/buildkit/blob/master/docs/sourcepolicy.md | ||
| std::string BuildBuildKitSourcePolicyJson(const std::vector<std::string>& allowedHosts) | ||
| { | ||
| nlohmann::json rules = nlohmann::json::array(); |
There was a problem hiding this comment.
@craigloewen-msft : Given that we can use buildkit policies, what do you think of putting the entire policy in the registry / well known file path ? That would both allow users to do more powerful things without having us to parse / validate the policies ourselves
There was a problem hiding this comment.
My concern with pure passthrough is that push/pull already use WSLContainerRegistryAllowlist and don't go through BuildKit, so they can't consume a BuildKit source-policy JSON. Admins would end up setting two different things for the same idea.
Could we do both? Keep the allowlist as the default, and add a file path option later (something like WSLContainerBuildKitSourcePolicyFile ) for people who need the full BuildKit policy syntax. I'd rather not put that in this PR though - happy to do it as a follow-up if we hear from users who actually need it.
Waiting on Craig's take before I change anything here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (5)
test/windows/PolicyTests.cpp:548
- Same as the earlier build-block test: checking only for
docker.ioin the output can allow unrelated failures (e.g., network errors) to satisfy the assertion.
Add a check that the error is actually policy-related so the test fails if enforcement is bypassed.
if (output.find(L"docker.io") == std::wstring::npos)
{
LogError("Expected COPY --from=docker.io/... to be blocked, got: '%ls'", output.c_str());
VERIFY_FAIL();
}
test/windows/PolicyTests.cpp:562
- This test only checks for
docker.iooralpinein the output; it can pass if the build fails for unrelated reasons that mention those tokens.
Add a policy-related assertion (e.g., output contains "policy") so the test reliably detects regressions in source-policy enforcement.
if (output.find(L"docker.io") == std::wstring::npos && output.find(L"alpine") == std::wstring::npos)
{
LogError("Expected bare `FROM alpine:latest` to be blocked, got: '%ls'", output.c_str());
VERIFY_FAIL();
}
src/windows/wslcsession/WSLCVirtualMachine.cpp:18
- This .cpp file doesn’t include the project PCH first. Most files in this directory start with
#include "precomp.h"(e.g.src/windows/wslcsession/WSLCSession.cpp:15), and adding new standard/library includes here increases the chance of missing transitive includes and slows builds. Includeprecomp.hbefore any other headers.
#include "WSLCVirtualMachine.h"
src/windows/wslcsession/WSLCSession.cpp:943
- The PR description says the SYSTEM-side service writes a host-side policy.json under
%SystemRoot%\Tempand mounts it into the guest, but the implementation here writes the policy inside the VM viaWSLC_SET_BUILDKIT_POLICYto/run/wsl/buildkit-policy.jsonand only injectsEXPERIMENTAL_BUILDKIT_SOURCE_POLICY.
Please update the PR description to match the current design (or add the missing host-side materialization/mounting pieces if they were intended).
if (policyState == WSLCVirtualMachine::BuildKitPolicyState::Configured)
{
buildEnv.emplace_back(std::string{"EXPERIMENTAL_BUILDKIT_SOURCE_POLICY="} + WSLCVirtualMachine::c_buildKitPolicyPath);
}
test/windows/PolicyTests.cpp:501
- This assertion can produce false positives: it only checks that the output mentions
docker.io, so a network/DNS failure involving docker.io could pass even if the source-policy enforcement regresses.
Consider also asserting that the failure is policy-related (e.g., output contains "policy"/"denied") to ensure the test actually validates the allowlist gate.
This issue also appears in the following locations of the same file:
- line 544
- line 558
if (output.find(L"docker.io") == std::wstring::npos)
{
LogError("Expected BuildKit source-policy denial mentioning docker.io, got: '%ls'", output.c_str());
VERIFY_FAIL();
}
| PRETTY_PRINT(FIELD(Header)); | ||
| }; | ||
|
|
||
| struct WSLC_SET_BUILDKIT_POLICY |
There was a problem hiding this comment.
If we want to use a new message to write this file, I would recommend creating a generic "write file" message. Something like:
struct WSLC_WRITE_FILE
{
[...]
unsigned int PathIndex;
int OpenFlags;
int Permissions;
char Buffer[];
};
That way we can reuse this in other places
| if (snapshot.State == wsl::windows::policies::RegistryAllowlistState::ReadFailed) | ||
| { | ||
| // Fail closed: `BuildImage` refuses new builds when it sees this state. | ||
| m_buildKitPolicyState = BuildKitPolicyState::ReadFailed; |
There was a problem hiding this comment.
I think it would be OK to hard fail if we couldn't read the policy. We could throw an exception when we read the registry and let it bubble up to fail the session creation entirely
Summary of the Pull Request
The WSLContainerRegistryAllowlist group policy was not enforced by wslc image build - only wslc image pull respected it, so a Dockerfile's FROM/COPY --from could pull from a registry the admin had explicitly disallowed. This PR closes that gap by generating a BuildKit source-policy document (EXPERIMENTAL_BUILDKIT_SOURCE_POLICY) that denies all image sources except the allowlisted hosts, and passes it to docker buildx build.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed