feat(client)!: add Docker Hub auth sub-client - #618
Conversation
785b066 to
12bdd57
Compare
Add a client/dockerhub package that reads Docker Hub access tokens and account profiles from the secrets engine and decodes them into typed values, so consumers no longer need to know the realm layout or the JSON payload format. The Client interface gains a HubAuth(...dockerhub.Option) accessor returning the dockerhub.ClientAuth sub-client; dockerhub.New wires the same sub-client over any bare secrets.Resolver. dockerhub.Staging() switches the lookups to the Docker Hub staging realms. ClientAuth resolves the default signed-in account through the profile metadata realm (docker/auth/metadata/hub/default), fetches a specific account under docker/auth/hub/<username>, and lists all signed-in profiles. Usernames and stored user ids are validated to name exactly one account entry inside the accounts realm, so a tampered profile or crafted username cannot address another secret. Claim decoding is dependency-free: NumericDate accepts integer, fractional, and exponent epochs and marshals whole seconds (matching golang-jwt/jwt v5 defaults), and Audience accepts a single string or an array. BREAKING CHANGE: the Client interface gains a HubAuth method; implementations must add it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
a8c778e to
a05fcc7
Compare
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
This PR introduces a well-structured typed sub-client for Docker Hub authentication. The security design (realm pinning, username validation, default-session guard) is sound overall. One correctness bug was confirmed in the nil-envelope-ID path of ListProfiles, and a best-effort error-suppression behaviour is undocumented.
| seen := make(map[string]bool, len(envelopes)) | ||
| for _, envelope := range envelopes { | ||
| // The default entry duplicates an account's profile. | ||
| if envelope.ID != nil && c.cfg.defaultEntry.Match(envelope.ID) { |
There was a problem hiding this comment.
[MEDIUM] Default entry included in results when secrets engine omits envelope IDs
The guard at line 250 uses if envelope.ID != nil && c.cfg.defaultEntry.Match(envelope.ID) — when envelope.ID is nil the entire condition is false and the default entry is not skipped. It falls through to parseProfile and can be appended to the returned slice.
The seen-map deduplication masks this in the existing test (skips the default entry without envelope IDs) because default and alice share the same UserID — whichever is iterated first is kept, the other is dropped. If the two entries ever hold different data (e.g. the default profile was updated but the per-account entry hasn't been flushed yet, or any future provider populates them differently), a caller of ListProfiles may receive the default entry as a regular profile instead of the canonical per-account one.
Suggested fix:
// Skip the default entry: either we have an ID and it matches, or the
// engine omits IDs entirely and we fall back to matching by key name.
if envelope.ID == nil || c.cfg.defaultEntry.Match(envelope.ID) {
continue
}| seen[profile.UserID] = true | ||
| profiles = append(profiles, profile) | ||
| } | ||
| if len(profiles) == 0 && len(errs) > 0 { |
There was a problem hiding this comment.
[LOW] Parse errors silently discarded when at least one profile succeeds
ListProfiles returns errors only when zero profiles parsed successfully (line 264: if len(profiles) == 0 && len(errs) > 0). If 9 out of 10 stored profiles have a corrupt payload, the caller receives 1 profile and a nil error — with no indication that any accounts are missing.
The test skips undecodable entries explicitly asserts require.NoError in this scenario, so the best-effort behaviour is deliberate. However, the API contract is not documented, which could mislead callers who need to know whether the returned list is complete.
Consider adding a note to the ListProfiles godoc — e.g. "Profiles that cannot be decoded are silently skipped; the error is returned only if no profiles could be decoded at all." — so callers that need reliable completeness can decide whether to handle this at their layer.
What
Adds
client/dockerhub: a typed sub-client for reading Docker Hub authentication state (access tokens and account profiles) from the secrets engine.Why
Fetching a Hub token today means knowing the realm layout (
docker/auth/hub/**,docker/auth/metadata/hub/**) and the raw JSON payload format. This PR hides both behind a small typed API.API
client.Clientgains an accessor (kubernetes-clientset style);dockerhub.Newwires the same sub-client over any baresecrets.Resolver:GetDefaultSession(ctx)GetSession(ctx, username)GetDefaultProfile(ctx)ListProfiles(ctx)A
UserSessioncarries the raw JWT (AccessToken) plus decodedClaims— typedNumericDateandAudience, no JWT library dependency.Errors:
ErrNoSession(no stored credential) andErrNoDefaultProfile(no default account set). The latter wraps the former, so callers that only care about having a usable session check one sentinel.Safety
GetSessionrejects usernames containing/or wildcards, so a username can only ever address one account entry.GetDefaultSessionvalidates that the storeduser_idnames exactly one account entry inside the accounts realm (docker/auth/hub/*), so a tampered profile cannot redirect the lookup to another secret.Also in this PR
docker/auth/metadata/hub/**corrected: it holds one profile entry per signed-in account plus adefaultentry duplicating the default account's profile (previously described as a single default-user pointer).Breaking: the
Clientinterface gains aHubAuthmethod — implementations must add it.🤖 Generated with Claude Code