ociregistry/ociauth: support mixing auth sources#49
Conversation
Changes the way CUE reads authentication from Docker, Podman, and other tools to allow multiple files being in use instead of just one. Before: - find first file in a list of locations, and use that - ignores the remaining files After: 1. load configs from all locations 2. merge them together 3. registry auth still has precedence rules, but it's per registry instead of per "config file source location" Fixes cue-labs#48, fixes cue-lang/cue#4307 Related discussion: cue-lang/cue#4306 Signed-off-by: Kalle Fagerberg <kalle.fagerberg@riskident.com> On-behalf-of: @RiskIdent <info@riskident.com>
applejag
force-pushed
the
feature/use-multiple-configs
branch
from
8ef3b38 to
3751604
Compare
mvdan
left a comment
mvdan
left a comment
There was a problem hiding this comment.
Thanks for the PR! A couple of comments below.
| return filepath.Join(home, ".docker", "config.json") | ||
| return b, nil | ||
| } | ||
| return nil, nil |
There was a problem hiding this comment.
These nil-map guards are currently dead code in production: LoadWithEnv always initializes the target config.data with non-nil maps before calling mergeInto. The guards are only exercisable through the unit test (TestConfigDataMergeInto), never through a real call path.
Since mergeInto is unexported with a single call site, I'd lean toward dropping the nil checks and relying on the caller contract, or at least adding a brief comment explaining they're defensive. As-is they suggest a contract that doesn't actually exist.
| } | ||
| for host, value := range c.CredHelpers { | ||
| if other.CredHelpers == nil { | ||
| other.CredHelpers = map[string]string{} |
There was a problem hiding this comment.
CredsStore is a fallback credential helper — it's consulted for any host not in credHelpers. With this first-non-empty-wins rule, if DOCKER_AUTH_CONFIG sets credsStore: "secretservice" and ~/.docker/config.json sets credsStore: "osxkeychain", the env var's value wins globally. That means the first source's credsStore now affects lookups for hosts that were only defined in later sources, which is a subtle semantic change.
I think this deserves at minimum:
- A sentence in the commit message / PR description calling out how
credsStoremerging works. - A test covering
CredsStore(and ideallyCredHelpers) merge precedence across sources, especially the interaction where a mergedcredHelpersentry comes from one source and thecredsStorefrom another.
There was a problem hiding this comment.
Or rather, a comment summarizing this potential inconsistency in the code, so that we can later find it and change the semantics if we want to. There's no need to add a test for it right away, as I imagine that this kind of situation should be very rare.
|
Friendly nudge, @applejag, if you can look at my code review :) If not I'm happy to make these adjustments myself as a co-author. |
I've a busy era at work atm, so feel free to make the adjustments as I won't get around to until in a month or more |
2 participants
Changes the way CUE reads authentication from Docker, Podman,
and other tools to allow multiple files being in use instead
of just one.
Before:
After:
instead of per "config file source location"
Fixes #48, fixes cue-lang/cue#4307
Related discussion: cue-lang/cue#4306