[RFC] remove .docker/config.json parsing

[runcom]

This patch removes the parsing and credential retrieval from ~/.docker/config.json.
Eventually I'll move the actual json parsing in skopeo according to containers/skopeo#106.

containers/image shouldn't depend nor rely on docker being installed on the system where we are actually running it. Higher level tooling should instead take care of this and the library should just accepts username/password to later attempt an authentication against a registry.

This work is being done as part of kubernetes/kubernetes#25899 and https://github.com/mrunalp/ocid because we don't expect to have any docker daemon installed in the kubelet. /cc @mrunalp

@mtrmac I'm not sure username and password being passed as arguments is good. Eventually I'd expect to have an Auth struct containing useful information about authenticating against a given registry (see https://github.com/kubernetes/kubernetes/pull/25899/files#diff-b99b84f6471ccf2077dedc93530a51a2R401).

@mtrmac That said, if you have any better suggestion on how to achieve this w/o requiring a long list of arguments, I'll rework this patch. Right now, it's just taking username and password because those were what getAuth was actually providing.

@mtrmac I've also thought about having a generic connection option struct to be passed to NewImage... methods so that we don't break any function definition in the future when adding other options.

The ocid part will then receive an AuthConfig struct from the Kubelet Runtime API and we'll pass those info down to containers/image to do pull and push. See https://github.com/mrunalp/ocid/blob/master/vendor/github.com/kubernetes/kubernetes/pkg/kubelet/api/v1alpha1/runtime/api.pb.go#L2130 for instance.

Lastly as said, the config.json parsing will be moved to skopeo (which is where, IMO, it belongs as a mean to integrate better with the host running skopeo; skopeo also has --username and --password flag which are used to override the whole parsing or to ignore the fact that the underlying host has docker installed)

Note: tests against skopeo are rightly failing - containers/image tests pass instead

Signed-off-by: Antonio Murdaca runcom@redhat.com

[mtrmac]

• Yes, we absolutely need a way to avoid reading the users’ home directory. Right now the openshift code is basically untestable without a container.
• In general I don’t think the ~/.docker/config.json parsing should be dropped from containers/image. Made optional, sure, but not dropped. Other tools like oci-fetch or whatever else might want to reuse the docker login state, and it seems to me copy&pasting = forking implementations from skopeo would be worse than carrying an implementation here.
• 4 parameters to each NewImage* call is really pushing it (especially with Reference abstraction #39, which would require passing them even for transports who do not use any of the values like dir:)

How about, instead (per conversation in containers/skopeo#106 ):

type SystemContext struct {
   HomeDir string // "" = use the real home directory
   DockerConfiguration *SomeDockerStruct // nil  = read from the users’ or system configuration
   OpenshiftConfiguration *SomeOpenshiftStruct
   // the two existing params
   InsecureDisableTLSVerification bool // except flipping the default to be secure
   CAPath string // "" = use system defaults
   // etc. for other transports

   // Perhaps also
   DisableReadingAnyOutsideState bool
   // to allow writing reliably-standalone users?
}

instead, and passing that to NewImage* ? Ordinary users who don’t want to deal with this would pass a SystemContext{} or perhaps nil, and anyone who knows and cares can override.

[runcom]

Been looking at this today, so, I agree we should have a type-safe way of passing around options - not sure anymore about the name SystemContext, we probably want something like ImageOpts wdyt?

[mtrmac]

Dunno. Vaguely I think that unless they get really unwieldy, function arguments should be function arguments and not members of a struct [OTOH I have done the exact opposite in https://github.com/containers/image/blob/master/signature/signature.go#L170 , to get readable parameter names; I kind of think that this is an exception, with too many similarly-typed parameters in a critical place].

ImageOpts makes it a bit unclear what should be an argument and what should be in that struct: why wouldn’t the ImageReference be in ImageOpts (it is related to “the image”), and why would Docker username/password be there (it is related to the registry, not “the image”)? SystemContext to me indicates the difference between things the application is managing and things that come from the outside environment.

That said, I don’t like the SystemContext name much at all. There must be a better way to make the distinction between arguments and context, and a better way to name it.

[mtrmac]

… on second thought, if the concept makes sense, and is well documented, I ultimately care very little about the actual name.

[cyphar]

Why not something like ImageContext if it's just for getting the images.

[runcom]

Why not something like ImageContext if it's just for getting the images.

because we'd like to differentiate a context from the underlying system and a context in where there are only stuff related to images (at which point the latter can be just an argument, no need for a struct)

[runcom]

Dunno. Vaguely I think that unless they get really unwieldy, function arguments should be function arguments and not members of a struct [OTOH I have done the exact opposite in https://github.com/containers/image/blob/master/signature/signature.go#L170 , to get readable parameter names; I kind of think that this is an exception, with too many similarly-typed parameters in a critical place].

@mtrmac the thing with all of this is in Golang (and the way I always implemented those concepts) - arguments are actually needed for a function, while an option struct is, as it seems, a box carrying arguments which may or may not be used by the function.

ImageOpts makes it a bit unclear what should be an argument and what should be in that struct: why wouldn’t the ImageReference be in ImageOpts (it is related to “the image”), and why would Docker username/password be there (it is related to the registry, not “the image”)? SystemContext to me indicates the difference between things the application is managing and things that come from the outside environment.

so ImageReference is clearly needed by the function and it's an argument required while the auth stuff isn't (because we can fallback to .docker/config.json)

[runcom]

@mtrmac what about playing with interface and trying to standardize most of the common stuff - I don't really like DockerConfiguration, OpenshiftConfiguration, etc.

 83 type AuthConfig struct {                                                        
 84     Username      string                                                        
 85     Password      string                                                        
 86     Auth          string                                                        
 87     ServerAddress string                                                        
 88     // IdentityToken is used to authenticate the user and get                   
 89     // an access token for the registry.                                        
 90     IdentityToken string                                                        
 91     // RegistryToken is a bearer token to be sent to a registry                 
 92     RegistryToken string                                                        
 93 }                                                                               
 94                                                                                 
 95 type TLSParams struct {                                                         
 96     InsecureDisableTLSVerification bool                                         
 97     CAPath                         string                                       
 98 }                                                                               
 99                                                                                 
100 type ImageContext interface {                                                   
101     GetAuth() AuthConfig                                                        
102     GetTLSParams() TLSParams
103     DisableReadingSystemState() bool                                           
104 }

What if any specific reference is implementing his own context struct and that it's passed to New* methods? Are we pretty confident TLSParams and AuthConfig to be somehow stable? (we can always add fields there?). Or is this just moving [Docker|Openshift]Configuration down?

[mtrmac]

arguments are actually needed for a function, while an option struct is, as it seems, a box carrying arguments which may or may not be used by the function.

Good point, Go has neither optionals nor named params…

standardize most of the common stuff - I don't really like DockerConfiguration, OpenshiftConfiguration, etc.

The trouble with this is that the semantics are not common (how do I use an IdentityToken when I don’t know who to contact with it, and using what API?), so the callers still need to specifically know about the individual transports’ properties and protocols — and they are not even exclusively used: e.g. see how, with OpenShift, you need a separate token for accessing the OpenShift API and an username/password for accessing the docker registry API, and they need to be configured at the same time.

(Yes, this makes it difficult to implement skopeo --password, but this semantic ambiguity on the CLI level can’t be fixed by moving the ambiguity into the API. In parallel, perhaps we could teach containers/image how to get the Docker credentials out of OpenShift, right now we don’t and just assume the user has followed the Cockpit instructions. Perhaps that would make the problem go away, I haven’t looked into it.)

Ultimately, the --password case is I think the most important counterargument: we definitely don’t want to send a password intended for one transport to any other transport, actually we don’t even want to send it to any other server using the same transport (consider skopeo copy docker:oneserver docker:anotherserver).

type ImageContext interface {                                                   

Why make this an interface? Just filling a struct with one or two members seems much simpler at the call site, is there a benefit I’m missing?

[mtrmac]

What if any specific reference is implementing his own context struct and that it's passed to New* methods?

I think there is a semantic difference between “this is the identity of the image” (hostname/namespace/someimage) and “this is the TLS configuration and authentication information necessary to access the image”, in particular there are situations (displaying in the UI, identity match and policy lookup in signatures) where we only care about the identity and not the auth.

That would suggest that ImageReference is the identity and should not need to carry the auth, though it would work fine if the auth were an optional component.

OTOH, the way ImageReference.NewImage* now don’t have any transport-specific parameters, and can be called by generic code which only keeps a reference, can only work because we have docker login / oc login as the default way to look up auth. That is of course very nice for the users’ POV, as long as we can rely on such a mechanism existing — can we? If we couldn’t and passing a password explicitly were mandatory for some transport, that would make the context/options struct a semi-mandatory companion to an ImageReference for generic code, and at that point a single ImageReference carrying its own context/options starts to look more attractive.

I guess I’d prefer trying to preserve the current “primarily rely on environment defaults” approach for most uses, it is nice and clean and simple to understand; and have the options/context only used for comparatively rare cases like unit tests or perhaps explicit overrides, and only to the extent we actually need any individual field.