docker/config - get all credentials #928
Conversation
Add a `GetAllCredentials` function to the config package. It's implementation is motivated by the API v2 work for Podman, where we need to send all local credentials over the wire to the endpoint. Signed-off-by: Valentin Rothberg <[email protected]>
Make sure that the maps are initialized. This allows for specifying empty json files without running into nil dereferences. Signed-off-by: Valentin Rothberg <[email protected]>
|
LGTM |
There was a problem hiding this comment.
I’m really not happy about this — it exposes too much about what “all” credentials look like, and how they are structured, and it makes it easier for callers to extract the data “for caching” and to do their own lookups, risking inconsistency and losing our ability to change the lookup semantics in the future.
Also consider interactive credential helpers, like GUI keyrings that may require extra passphrase inputs for high-risk keys. Just asking for all secrets when they may not be needed is not at all ideal.
Do we have to do this?
Sure, the server side must accept the projectatomic/docker map (and that does not need this API), but is there any chance the new callers would instead use a callback-based system, where the server-side search code only asks for credentials for registries one at a time, as the search code tries registries in turn? I realize that would make the API so much more complex … OTOH some infrastructure for interactive operation must already exist for podman run -it to work.
Or, maybe, provide a “get relevant search list” operation in the Podman API, so that the API client can only ask for credentials that could possibly be relevant for the operation in question. (That could still require user interactivity for all credentials on the search path, even if most of them were not necessary for that particular image.)
| @@ -73,6 +73,42 @@ func SetAuthentication(sys *types.SystemContext, registry, username, password st | |||
| }) | |||
| } | |||
|
|
|||
| // GetAllCredentials returns the registry credentials for all registries stored | |||
| // in either the auth.json file or the docker/config.json. | |||
| func GetAllCredentials(sys *types.SystemContext) (map[string]types.DockerAuthConfig, error) { | |||
There was a problem hiding this comment.
API evolution concern: There’s talk about storing different credentials for different namespaces of a single registry (case in point: OpenShift pull secret vs. personal credentials for a repo, both on Quay.io). Would this function then just not return the per-namespace credentials?
(Note that the design, if any, of per-namespace credentials is completely unknown right now.)
| } | ||
| if auths.CredHelpers == nil { | ||
| auths.CredHelpers = make(map[string]string) | ||
| } |
There was a problem hiding this comment.
It would be nice to have tests for this (and the rest as well, ideally) — sure, only after the API settles.
There was a problem hiding this comment.
Added tests in #942. Apologies for the cross posts.
| conf := types.DockerAuthConfig{Username: username, Password: password} | ||
| authConfigs[registry] = conf | ||
| } | ||
|
|
There was a problem hiding this comment.
Kernel keyring? In general I’m rather concerned that this is not going to stay in sync with GetCredentials.
There was a problem hiding this comment.
The kernel keyring is currently disabled. Not sure how the future will look like.
|
quick rant: I really really don't like "search registries". They're inflicting headaches on a regular basis.
We could limit support to auth files for now and call it
I don't think we can implement that in the time frame we need auth to work for v2. We had to wire in such call backs into c/image and get that through the entire stack up to libpod and then wire that into pretty much all endpoints.
To double check: The server would expose an API that would give us a list of all search registries plus their mirrors. The client would then assemble the credential map based on this list and wrap it in the X-Registry-Auth header. Right? @rhatdan PTAL. |
|
Closing in favour of #933. Thanks for the thoughtful comments! |
Add means to get all credentials and fix a minor issues.
I'm currently working on the Podman v2 API and need to throw auth.jsons over the wire.