Add wslc login, wslc logout, and wslc push commands for registry authentication#40173
Add wslc login, wslc logout, and wslc push commands for registry authentication#40173kvega005 wants to merge 81 commits intomicrosoft:feature/wsl-for-appsfrom
wslc login, wslc logout, and wslc push commands for registry authentication#40173Conversation
… into user/kevinve/registry
… into user/kevinve/registry
… into user/kevinve/registry
| auto serverAddress = std::string(RegistryService::DefaultServer); | ||
|
|
||
| if (context.Args.Contains(ArgType::Server)) | ||
| { | ||
| serverAddress = WideToMultiByte(context.Args.Get<ArgType::Server>()); | ||
| } |
There was a problem hiding this comment.
Server addresses are stored exactly as provided. This can break credential lookup because ImageService extracts the registry as a host[:port] (no scheme/path), while a user may reasonably pass https://host or host/v1/ to login/logout. A concrete fix is to normalize serverAddress before Store/Get/Erase (e.g., strip scheme, strip any path, and normalize Docker Hub aliases) so login, push/pull, and logout all use the same key format.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 43 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
test/windows/wslc/e2e/WSLCE2ERegistryTests.cpp:1
- The logout help text is using
WSLCCLI_LoginServerArgDescription()for itsserverargument. Even if the underlying localization string is shared today, the identifier is misleading and makes future maintenance/error-prone (especially once login/logout diverge). Consider introducing a more neutral localization key (e.g.,WSLCCLI_RegistryServerArgDescription) and using it for both commands, then update the expected help output accordingly.
| std::string ResolveCredentialKey(const std::string& serverAddress) | ||
| { | ||
| // Normalize known Docker Hub aliases to the canonical DefaultServer key, | ||
| // matching Docker CLI's getAuthConfigKey() behavior. | ||
| if (serverAddress == "docker.io" || serverAddress == "index.docker.io" || serverAddress == "registry-1.docker.io") | ||
| { | ||
| return wsl::windows::wslc::services::RegistryService::DefaultServer; | ||
| } | ||
|
|
||
| return serverAddress; | ||
| } |
There was a problem hiding this comment.
The credential key normalization does not handle commonly-entered server forms like https://registry.example.com/, registry.example.com/, or registry.example.com/v1/. Since GetServerFromImage() extracts only the domain (e.g., registry.example.com:5000), a user who logs in with a URL/suffixed path will store credentials under a key that will never be looked up, causing push/pull auth to fail. Consider normalizing by stripping scheme, trimming trailing slashes, and removing any path component so keys consistently match the image-derived registry host[:port].
…/WSL into user/kevinve/wslc-login
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 39 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
test/windows/wslc/e2e/WSLCE2EHelpers.cpp:1
OpenDefaultElevatedSession()usesVERIFY_SUCCEEDEDbut then unconditionally dereferencessessionManager.get()/session.get(). IfCoCreateInstanceorOpenSessionByNamefails, this can lead to null deref/crash and hide the real failure. Use fail-fast error handling here (e.g.,THROW_IF_FAILED(...)), or explicitly return early when VERIFY fails.
test/windows/wslc/e2e/WSLCE2ERegistryTests.cpp:1- This assertion depends on an exact Docker/engine error substring (
\"no basic auth credentials\"), which can vary across engine versions or registry implementations and potentially make the E2E test flaky. Consider asserting on a more stable signal (e.g., exit code + presence of 401/denied wording, or a wslc-defined error code/message if available) while still validating auth failure.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 39 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
test/windows/wslc/WSLCCLICredStorageUnitTests.cpp:1
- These unit tests instantiate
FileCredStorage, which (per its implementation) persists credentials under%LocalAppData%\\wslc\\registry-credentials.json. That means the test suite will modify a real user profile file on the machine running tests and may have cross-test/process interference. Recommend makingFileCredStorageaccept an override path (constructor parameter or test-only hook) and using a temp directory in unit tests so they are hermetic and parallel-safe.
test/windows/wslc/e2e/WSLCE2EPushPullTests.cpp:1 StartLocalRegistry(*session)uses the helper default port (currently 5000). Fixed ports can make E2E tests flaky when ports are already in use (local dev environment, parallel test runs, previous crashed tests). Prefer selecting an available ephemeral port (or havingStartLocalRegistrychoose/return a free port automatically) and using that across all registry-starting tests.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 39 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
test/windows/wslc/e2e/WSLCE2EPushPullTests.cpp:1
- This test uses a fixed port (
15002), andWSLCE2ERegistryTests.cppalso starts a registry on15002. If the test runner executes E2E tests concurrently, this can produce intermittent bind failures/flakiness. Prefer allocating unique ports per test (e.g., distinct constants per file) or using a helper that finds a free ephemeral port and returns it.
| #include <optional> | ||
| #include <string> | ||
| #include <vector> | ||
|
|
||
| namespace wsl::windows::wslc::services { | ||
|
|
||
| // Abstract interface for credential storage backends (WinCred, file-based, etc.). | ||
| struct ICredentialStorage | ||
| { | ||
| virtual ~ICredentialStorage() = default; | ||
|
|
||
| virtual void Store(const std::string& serverAddress, const std::string& credential) = 0; | ||
| virtual std::optional<std::string> Get(const std::string& serverAddress) = 0; | ||
| virtual void Erase(const std::string& serverAddress) = 0; | ||
| virtual std::vector<std::wstring> List() = 0; | ||
| }; | ||
|
|
||
| // Returns the credential storage implementation based on user configuration. | ||
| std::unique_ptr<ICredentialStorage> OpenCredentialStorage(); |
There was a problem hiding this comment.
ICredentialStorage.h declares std::unique_ptr but doesn’t include <memory>. This makes the header non-self-contained and can break compilation depending on include order. Add #include <memory> here.
| #include "ICredentialStorage.h" | ||
|
|
||
| namespace wsl::windows::wslc::services { | ||
|
|
||
| class FileCredStorage final : public ICredentialStorage | ||
| { | ||
| public: | ||
| void Store(const std::string& serverAddress, const std::string& credential) override; | ||
| std::optional<std::string> Get(const std::string& serverAddress) override; | ||
| void Erase(const std::string& serverAddress) override; | ||
| std::vector<std::wstring> List() override; | ||
|
|
||
| private: | ||
| static std::string Protect(const std::string& plaintext); | ||
| static std::string Unprotect(const std::string& cipherBase64); | ||
| static void ModifyFileStore(FILE* f, const std::function<bool(nlohmann::json&)>& modifier); | ||
| }; |
There was a problem hiding this comment.
This header references std::function and nlohmann::json but doesn’t include the corresponding headers. To keep the header self-contained (and avoid relying on PCH/include order), either include <functional> and the appropriate nlohmann json header, or move the ModifyFileStore declaration so the header no longer needs to mention nlohmann::json (e.g., keep it entirely in the .cpp).
Summary of the Pull Request
Adds
wslc login,wslc logout, andwslc image pushcommands, enabling authenticated container registry workflows in wslc.PR Checklist
Detailed Description of the Pull Request / Additional comments
New Commands
wslc login [-u <user>] [-p <pass>] [--password-stdin] [<server>]— Authenticates with a container registry and stores credentials for subsequent push/pull operations. Supports interactive prompts, inline flags, and piped password input. Defaults to Docker Hub when no server is specified.wslc logout [<server>]— Removes stored credentials for a registry.wslc image push <image>— Pushes a local image to a registry, automatically using stored credentials when available.wslc registry— Parent command group for login/logout. All three commands are also available as root-level aliases (wslc login,wslc logout,wslc push).Credential Storage (
RegistryService)credentialStorein wslc user settings:wslc-credential/target name prefix to isolate wslc entries from other applications.%LocalAppData%\wslc\registry-credentials.jsonwith retry logic for concurrent access (sharing violation handling).PullImageandPushImageAPI calls based on the image's registry domain.Auth Integration
ImageService::PullandImageService::Pushextract the registry domain from the image reference and look up stored credentials before calling the session API.Authenticatedelegates to the session's Docker engine and supports both identity token and username/password auth header formats.Validation Steps Performed
-u/-pflag login, and--password-stdinlogin all verified.