feat(acrauth): add ACR login helper that writes container auth files - #321
feat(acrauth): add ACR login helper that writes container auth files#321Wenqi He (weherdh) wants to merge 4 commits into
Conversation
The oc-mirror image authenticates to ACR by running 'az acr login' with DOCKER_COMMAND pointed at a shim script, so that az's internal 'docker login' call is intercepted and the credential can be scraped out of the argv. That coupling broke silently when azure-cli 2.88.0 switched to --password-stdin, and the shim's jq rewrite also clobbered the source-registry credentials that the init container had already written to the same auth file. Do the exchange directly instead: trade the managed identity's Entra token for an ACR refresh token and upsert a single registry key into the auth file, leaving other registries and unmodelled fields intact.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: weherdh The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi Wenqi He (@weherdh). Thanks for your PR. I'm waiting for a Azure member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Pull request overview
Adds a new tools/acrauth Go module intended to provide a stable, CLI-internal ACR authentication flow (managed identity → ACR refresh token) and safely upsert the resulting credential into a container auth file without clobbering other registry entries.
Changes:
- Introduces
acrauth loginCobra command with option validation/completion and Azure credential selection (default vs user-assigned MI). - Implements ACR refresh-token exchange (with bounded retry) and auth-file upsert logic that preserves unrelated registries/fields and writes via temp file + rename.
- Wires the new module into the repo workspace (
go.work) and adds module/workspace dependency sums.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/acrauth/options.go | CLI flag binding and options lifecycle; constructs Azure credential and performs login flow. |
| tools/acrauth/command.go | Defines the login Cobra command and runs validation/completion/login with interrupt-aware context. |
| tools/acrauth/acr.go | Implements Entra/ARM token → ACR refresh token exchange and retry loop for role propagation delays. |
| tools/acrauth/authfile.go | Upserts a single registry credential into a container auth JSON file via read/merge + atomic replace. |
| tools/acrauth/authfile_test.go | Unit tests covering merge-preservation, replacement, file creation/permissions, and empty-registry rejection. |
| tools/acrauth/go.mod | New module definition and direct/indirect dependency list for the acrauth tool. |
| tools/acrauth/go.sum | Checksums for the new module’s dependencies. |
| go.work | Adds ./tools/acrauth to the workspace modules list. |
| go.work.sum | Updates workspace sums to account for newly introduced dependencies. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…t be renamed over A bind-mounted auth file cannot be replaced with rename(2) -- the kernel returns EBUSY. The oc-mirror container mounts auth.json directly, so the atomic write path fails there and the login aborts. Fall back to writing through the existing file in that case.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tools/acrauth/acr.go:40
acrFQDNis treated as a bare registry host, but if a caller passes a URL (e.g.https://myregistry.azurecr.io) or includes a path/trailing slash,fmt.Sprintf("https://%s", acrFQDN)will produce an invalid endpoint and a misleading hostname (e.g. host becomeshttps). Consider validating the input and failing fast with a clear error.
// ExchangeForRefreshToken trades an Entra token for an ACR refresh token, which is what
// container tooling stores as the registry password.
func ExchangeForRefreshToken(ctx context.Context, cred azcore.TokenCredential, acrFQDN string) (string, error) {
endpoint, err := url.Parse(fmt.Sprintf("https://%s", acrFQDN))
if err != nil {
return "", fmt.Errorf("failed to parse ACR endpoint: %w", err)
}
tools/acrauth/acr.go:82
- This loop won't compile:
rangecannot be used over anintconstant. Use a conventional counter loop so the retry behavior actually builds and runs.
for attempt := range retryAttempts {
A non-nil pointer to an empty string would have been written out as a credential and reported as success -- the same silent-success failure this tool exists to remove.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
tools/acrauth/command.go:31
- The command ignores positional args; without an Args validator, extra arguments are accepted silently even though
Use: "login"implies none. AddArgs: cobra.NoArgsso user mistakes fail fast.
cmd := &cobra.Command{
Use: "login",
Short: "Authenticate to an Azure Container Registry and record the credential in a container auth file",
SilenceUsage: true,
SilenceErrors: true,
os.WriteFile only applies its mode argument when it creates the file, so the fallback path left a pre-existing credential file at whatever mode it already had -- observed as 0644 in a container run. Chmod it explicitly, and stop claiming atomicity for a path that cannot have it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tools/acrauth/authfile.go:110
tmp.Write(raw)can (rarely) return a short write witherr == nil, which would produce a truncated auth file and break subsequent reads. It’s safer to assert the full buffer was written before proceeding.
if _, err := tmp.Write(raw); err != nil {
_ = tmp.Close()
return fmt.Errorf("failed to write temporary auth file: %w", err)
}
if err := tmp.Close(); err != nil {
|
Validated end-to-end against a live ACR, driven from the consumer PR (Azure/ARO-HCP#6668) with a local Against
Full container run — oc-mirror image built with this binary, real mirror, no So Two bugs surfaced during that testing and are already fixed in this branch:
Only the managed-identity branch of |
|
/ok-to-test |
Rael Garcia (raelga)
left a comment
Rael Garcia (raelga)
left a comment
There was a problem hiding this comment.
PR-standard note: please link a Jira or GitHub tracking issue as required by CONTRIBUTING.md. The current references are both pull requests.
| @@ -0,0 +1,100 @@ | |||
| // Copyright 2025 Microsoft Corporation | |||
There was a problem hiding this comment.
All five new Go files have CRLF line endings. gofmt -l tools/acrauth/*.go lists every file, and go tool golangci-lint run ./tools/acrauth/... fails the gci formatter check. Please run the repository formatter and commit the LF-normalized files so the verify job can pass.
| gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= | ||
| gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= | ||
| gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= | ||
| gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= |
There was a problem hiding this comment.
go mod tidy -diff reports this module is not tidy: it adds the missing github.com/rogpeppe/go-internal v1.14.1/go.mod and github.com/spf13/pflag v1.0.10/go.mod checksums. Please run go mod tidy and commit the resulting go.sum.
| if !ok { | ||
| entry = map[string]any{} | ||
| } | ||
| entry["auth"] = base64.StdEncoding.EncodeToString([]byte(username + ":" + password)) |
There was a problem hiding this comment.
This preserves an existing identitytoken on the target registry entry while replacing only auth. containers/image prefers a non-empty identity token and uses it for the OAuth refresh-token flow, so a stale identity token can make oc-mirror ignore the newly written ACR credential. Please remove identitytoken from the target entry when replacing its credential, while continuing to preserve unrelated registries and fields.
|
Wenqi He (@weherdh): The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Updated that in the description: https://redhat.atlassian.net/browse/AROSLSRE-1819 |
Why
The oc-mirror image authenticates to ACR by running
az acr loginwithDOCKER_COMMANDpointed at a shim script, so that az's internal
docker logincall is intercepted and thecredential scraped out of
argv. Two problems:--password-stdin— the shim'spositional args shifted and it wrote a garbage credential. Mirroring reported success
while syncing nothing.
jqrewrite overwrote the auth file, clobbering theregistry.redhat.iocredentials an init container had already written there.
Coupling our auth to the internals of a CLI we don't pin is not a stable contract.
What
This is to resolve https://redhat.atlassian.net/browse/AROSLSRE-1819
A small
acrauth logincommand that does the exchange directly:acr.go— trades the managed identity's Entra token for an ACR refresh token, withbounded retry for role-assignment propagation.
authfile.go— upserts a single registry key into the container auth file.Round-trips through
map[string]anyso other registries and fields we don't model(
email,identitytoken) survive. Writes via temp file + atomic rename.Tests cover the merge-preserves-other-registries case, credential replacement, cold start
with file-mode assertion, and empty input.
Note on #257
This overlaps with the ACR token exchange in #257, where the equivalent helpers are
unexported. Happy to rebase onto that and drop
acr.goif it lands first — or the reverse.The rest is disjoint: #257 replaces
on-demand.shand copies via ORAS in-process, so itnever writes an auth file; oc-mirror is an external binary that must read one.
Follow-up
The ARO-HCP change (thin
main, Dockerfile,mirror.sh, deleting the shim) landsseparately once this is available as a pseudo-version.