Skip to content

Add registry allowlist support to image builds - #41211

Draft
beena352 wants to merge 8 commits into
microsoft:masterfrom
beena352:users/beenachauhan/wslc-registry-allowlist-buildkit-policy
Draft

Add registry allowlist support to image builds#41211
beena352 wants to merge 8 commits into
microsoft:masterfrom
beena352:users/beenachauhan/wslc-registry-allowlist-buildkit-policy

Conversation

@beena352

@beena352 beena352 commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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

  • Closes: Link to issue #xxx
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated if needed and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated if needed
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

  • The policy JSON is generated in the user-side wslcsession process during VM boot (ConfigureBuildKitPolicy in WSLCVirtualMachine::Initialize) and sent to the guest over the existing init message channel. The guest handler writes it to /run/wsl/buildkit-policy.json on tmpfs. On each wslc image build, if the policy was configured at boot, EXPERIMENTAL_BUILDKIT_SOURCE_POLICY is set in the build environment pointing at that path
  • The JSON contains a DENY-all rule followed by per-allowlisted-host ALLOW rules (REGEX match, last-rule-wins), with hostnames lowercased and regex-metacharacters escaped
  • Fail-closed: if the allowlist policy is configured but can't be read (e.g. registry access denied), the build is refused with WSLC_E_REGISTRY_BLOCKED_BY_POLICY rather than silently proceeding without enforcement. If the message transaction or guest write fails, the VM fails to initialize entirely
  • The registry is read once at boot rather than per-build - this closes a race where a compromised user process could tamper with the policy between write and build. The staleness window is acceptable because wslc VMs are short-lived and Group Policy itself refreshes on a ~90-minute cadence

Validation Steps Performed

  • Automated: PolicyTests.cpp - RegistryAllowlistBlocksImageBuild, RegistryAllowlistAllowsImageBuild, RegistryAllowlistImageBuildIsCaseInsensitive, RegistryAllowlistBlocksImageBuildCopyFrom, RegistryAllowlistBlocksImageBuildImplicitDockerIo, RegistryAllowlistDenies, ReadRegistryAllowlistSnapshotFromPoliciesRoot_Logic. Tests now terminate the session before each build to ensure a fresh boot-time snapshot
  • Manual: verified an allowed FROM builds successfully and a disallowed FROM/COPY --from is blocked with the policy-block error surfaced to the user.

Copilot AI review requested due to automatic review settings July 30, 2026 20:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 build allowlist 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.

Comment thread src/windows/service/exe/HcsVirtualMachine.cpp Outdated
Copilot AI review requested due to automatic review settings July 30, 2026 22:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 WriteFile call doesn’t capture/validate the number of bytes written. If a short write occurs, policy.json can 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.h first (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"

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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));

Copilot AI review requested due to automatic review settings July 30, 2026 23:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

  • CreateFileW returns INVALID_HANDLE_VALUE on failure, but wil::unique_handle treats nullptr as the invalid value. This means failures won’t be detected and CloseHandle(INVALID_HANDLE_VALUE) may run. Use wil::unique_hfile (or explicitly check for INVALID_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_all error, and then clears m_preparedPolicyFolders unconditionally. 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();

Comment thread src/windows/wslcsession/WSLCSession.cpp Outdated
Copilot AI review requested due to automatic review settings July 31, 2026 20:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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, and std::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>

Comment thread src/windows/wslcsession/WSLCSession.cpp Outdated
// 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();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@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

@beena352 beena352 Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copilot AI review requested due to automatic review settings August 3, 2026 22:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.io in 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.io or alpine in 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. Include precomp.h before 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%\Temp and mounts it into the guest, but the implementation here writes the policy inside the VM via WSLC_SET_BUILDKIT_POLICY to /run/wsl/buildkit-policy.json and only injects EXPERIMENTAL_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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants