dockerClient: skip TLS verification if configured in registries

[vrothberg]

Based on a discussion from: #464 (comment)

• sysregistriesv2: adjust API to SystemContext
• dockerClient: skip TLS verification if configured in registries

[vrothberg]

@mtrmac The API change hasn't been discussed but I think it makes the API easier to use and ultimately avoids boilerplate code for its users.

[mtrmac]

ACK on the API change, that is much better for callers.

[mtrmac]

(Some of these should be require. so that future asserts don’t fail with nil references. But that’s trivial to retrofit if the tests ever fail, so not blocking.

A fair amount of this would probably be smaller if the tests were table-driven, but again, this works just fine, absolutely non-blocking.)

[vrothberg]

cmd/skopeo/utils.go:21:30: cannot use !c.GlobalBoolT("tls-verify") (type bool) as type types.OptionalBool in field value
cmd/skopeo/utils.go:31:35: cannot use !c.BoolT(flagPrefix + "tls-verify") (type bool) as type types.OptionalBool in assignment

Tests fail expectedly as skopeo needs to be updated. I suggest to merge this PR, then update Skopeo and after that I'll take care of the pull code.

[mtrmac]

Not a full review yet, just a few notes.

[mtrmac]

(This should be a complete review.)

[mtrmac]

One more code fix please, and then just a few stylistic nits.

FWIW I have checked with #256 that this does not modify other on-the-wire behavior.

[mtrmac]

Tests fail expectedly as skopeo needs to be updated. I suggest to merge this PR, then update Skopeo and after that I'll take care of the pull code.

We generally want to see the skopeo PR in advance because most of the integration tests (notably ~all tests for c/image/docker are in that repo), using the process in https://github.com/projectatomic/skopeo#dependencies-management .

I have prepared containers/skopeo#521 for this.

[vrothberg]

I have prepared containers/skopeo#521 for this.

Thanks a lot! I've been testing it locally but wasn't sure about the process.

[mtrmac]

Please move this to a global variable; we don’t need to compile the regex on every call to isPrefix (i.e. on every FindRegistry call, for each configured registry).

[mtrmac]

(Probably skip these bullet points for now, and read the end first.)

• If I understand the spec correctly, (?m) means that the ^ and $ characters also match newlines; why is that desirable?
• In :.[^…], why is the first character after : treated specially? AFAICT it doesn’t matter because :: or :/ are invalid :tag suffixes either way, but it is an extra element to think about. Is there some extra nuance I am missing? (If I’m not, this could be made completely precise by matching ":"+reference.TagRegexp from c/image/docker/reference.)
• If matching at the level of individual repos (i.e. with the :tag suffix) is desirable, symmetrically @digest should be accepted as well. And, maybe surprisingly, it’s possible to say repo:tag@digest (see reference.ReferenceRegexp), where digest is algo:hex-digits, i.e. the suffix can legitimately contain two colons.

Ultimately, matching on the suffix only seems insufficient, because :5000 can either be a valid tag or a valid port number. So, example.com:5000 should not match example.com, but example.com/image:5000 should match example.com/image. This matters because of the CheckAuth/SearchRegistry uses in c/image/docker, which pass a repo hostname but not an image reference—strictly speaking that’s a violation of the FindRegistry API as documented, but it is a situation that we need to handle.

It should be possible to ultimately implement the desired semantics, with the current inputs, exactly correctly (e.g. by exporting DomainWithOptionalName in c/image/docker/registry, or something like that—and we’ve run into a few other cases where exporting that regexp would be useful), but maybe we can do something much simpler: redefine the input to FindRegistry to be a hostname or a repo name, not a full image reference, i.e. prohibit the tag/digest suffixes entirely and let the caller handle that. That’s especially attractive because this PR already calls FindRegistry that way: newDockerClientFromRef calls ref.ref.Name(); also, the implementation would be a trivial suffix == "" || suffix[0] == "/" if I’m not mistaken.

[vrothberg]

Ultimately, matching on the suffix only seems insufficient, because :5000 can either be a valid tag or a valid port number. So, example.com:5000 should not match example.com, but example.com/image:5000 should match example.com/image.

This is already working and the regex detects that (minus the digest case).

• If matching at the level of individual repos (i.e. with the :tag suffix) is desirable, symmetrically @ digest should be accepted as well. And, maybe surprisingly, it’s possible to say repo:tag@digest (see reference.ReferenceRegexp), where digest is algo:hex-digits, i.e. the suffix can legitimately contain two colons.

[...] but maybe we can do something much simpler: redefine the input to FindRegistry to be a hostname or a repo name, not a full image reference, i.e. prohibit the tag/digest suffixes entirely and let the caller handle that [...]

I feely strongly against this as this would cause boilerplate code at (and move complexity to) the callees before the call (e.g., extracting the hostname from some input) and after the call (i.e., doing the suffix magic).

It should be possible to ultimately implement the desired semantics, with the current inputs, exactly correctly (e.g. by exporting DomainWithOptionalName in c/image/docker/registry, or something like that [...]

That's my favorit so far. It makes sense to put that into c/image/docker/* as we can reuse the regexes from there. I will have a look at that and ping you once the PR is updated.

Thanks for the great review, as always!

[vrothberg]

It should be possible to ultimately implement the desired semantics, with the current inputs, exactly correctly (e.g. by exporting DomainWithOptionalName in c/image/docker/registry, or something like that [...].

I believe we can reduce complexity by just requiring the suffix/remainder to either be empty or to start either with / or with :. This would also match at port boundaries, but that might not be such a big deal. Some users might even find a usecase for it when wanting to mirror multiple registries running on one host.

We can still introduce a DomainWithOptionalName regex (or something similar) for input validation, but I don't think that's necessary now can be added at a later point.

[mtrmac]

Ultimately, matching on the suffix only seems insufficient, because :5000 can either be a valid tag or a valid port number. So, example.com:5000 should not match example.com, but example.com/image:5000 should match example.com/image.

This is already working and the regex detects that (minus the digest case).

How? AFAICS it can’t work because the suffix is the same, and testing confirms it: https://play.golang.org/p/0QHfsmaoApO .

Am I overlooking something that can distinguish the two cases?

[mtrmac]

I believe we can reduce complexity by just requiring the suffix/remainder to either be empty or to start either with / or with :. This would also match at port boundaries, but that might not be such a big deal. Some users might even find a usecase for it when wanting to mirror multiple registries running on one host.

I’d rather do this precisely correctly; it seems to me it should not be that much code, maybe I could try to do that next week.

(Either way, the problematic matching code already exists, so it is not strictly blocking this PR. The skopeo test failures are — and I’m getting close to finishing the CLI rework in there, containers/skopeo#523 — and that rework seems mostly unnecessary for the purpose of this PR, after all; I will know for sure after I finish the tests in there.)

[mtrmac]

I have prepared containers/skopeo#521 for this.

FWIW, the failed tests in there prompted me to write unit tests for contextFromGlobalOptions, and I’m not at all happy with how that handles the --tls-verify flag (notably if one of the subcommands does not declare the flag, it is silently set to the insecure value right now). So I’m trying to overhaul the CLI parsing in there to actually work reliably, I’m not yet quite sure where that will lead me (WIP https://github.com/mtrmac/skopeo/tree/cli-parsing ).

There might be a simple way to make the specifically the --tls-verify reliable enough without that overhaul, I haven’t really been looking to be honest.

[vrothberg]

FWIW, the failed tests in there prompted me to write unit tests for contextFromGlobalOptions, and I’m not at all happy with how that handles the --tls-verify flag (notably if one of the subcommands does not declare the flag, it is silently set to the insecure value right now). So I’m trying to overhaul the CLI parsing in there to actually work reliably, I’m not yet quite sure where that will lead me (WIP https://github.com/mtrmac/skopeo/tree/cli-parsing ).

Ouch. There are quite some unexpected pain points in the stack. CLI parsing sucks big time.
Thanks for doing that!

There might be a simple way to make the specifically the --tls-verify reliable enough without that overhaul, I haven’t really been looking to be honest.

I may find time next week to help you untangle the code.

[mtrmac]

I have prepared containers/skopeo#521 for this.

FWIW, the failed tests in there prompted me to write unit tests for contextFromGlobalOptions, and I’m not at all happy with how that handles the --tls-verify flag (notably if one of the subcommands does not declare the flag, it is silently set to the insecure value right now). So I’m trying to overhaul the CLI parsing in there to actually work reliably, I’m not yet quite sure where that will lead me (WIP https://github.com/mtrmac/skopeo/tree/cli-parsing ).

There might be a simple way to make the specifically the --tls-verify reliable enough without that overhaul, I haven’t really been looking to be honest.

It turns out that while cli.Context.BoolT is pretty unreliable/risky, IsSet/GlobalIsSet work as necessary, and the original test failures were pointing out my stupid thinko when updating skopeo for this API. containers/skopeo#521 now passes tests independently of the larger CLI rework, with added flag parsing tests to ensure that the results are as expected.

So, only the namespace matching is outstanding. Arguably this PR should not block on it at all and that part could be split — OTOH enabling this implicit behavior with the strings.HasPrefix seems risky. I’ll hopefully return to this in a few days.

[vrothberg]

@mtrmac Thanks for the info!

So, only the namespace matching is outstanding. Arguably this PR should not block on it at all and that part could be split — OTOH enabling this implicit behavior with the strings.HasPrefix seems risky. I’ll hopefully return to this in a few days.

Yesterday, we hit a pretty interesting detail with the prefix/mirror downstream patch we maintain for Docker-ce. The issue is that registries can be nested under some path on the host (see "prefix" at https://docs.docker.com/registry/configuration/#http), which can lead to auth issues when doing a namespace translation.

An example is a registry running at example.com/reg-a/ and we configure URL: "example.com/reg-a/docker.io" and Prefix: "docker.io/". What we want is to have docker.io mirrored at example.com/reg-a/docker.io where the registry root is example.com/reg-a ... in this case we need to know what the registry is, as we cannot parse it from a given string/reference. In other words, that needs to be covered by the registries.conf and sysregistriesv2 and will influence our isPrefix(..) issue above.

[mtrmac]

Yesterday, we hit a pretty interesting detail with the prefix/mirror downstream patch we maintain for Docker-ce. The issue is that registries can be nested under some path on the host (see "prefix" at https://docs.docker.com/registry/configuration/#http),

The ”Docker reference” format just can’t express the difference between an image available through the API paths example.com/v2/manifests/ns/repo and through example.com/ns/v2/manifests/repo. Only the former one can be specified.

So, such registries can’t be used with c/image/docker at all; it always uses ${reference.Domain(ref.ref)}/v2/.

Similarly, looking at moby/moby, lookupV2Endpoints can’t turn a Docker reference into something referring to a registry running at a non-root path. It can only return a non-hostname-only path only if it is present in s.config.Mirrors — but registry.ValidateMirror rejects mirror URLs with paths.

So, this seems to be a server-side capability with no client-side implementation.

Or are you saying that you want to add support for this in clients?

which can lead to auth issues when doing a namespace translation.

What does “auth issues” mean? (Overall, it seems reasonable to me that clients should use credentials they have stored for the mirror, and not for the underlying primary source, if they connect to a mirror. I can’t see how using non-empty path prefixes changes that, except maybe for the need to support them in pkg/docker/config.)

(If the example below is supposed to be the “auth issue”, I can’t see any connection to authentication.)

An example is a registry running at example.com/reg-a/ and we configure URL: "example.com/reg-a/docker.io" and Prefix: "docker.io/". What we want is to have docker.io mirrored at example.com/reg-a/docker.io where the registry root is example.com/reg-a ... in this case we need to know what the registry is, as we cannot parse it from a given string/reference.

Which of the strings in this paragraph are Docker references (hostname/ns…/repo) and which are API URLs? There isn’t a single /v2/ above, so they all seem to be just Docker references; in that case, mapping docker.io to example.com/reg-a/docker.io seems trivial and already supported by the format.

Or are you saying that you want to deploy a registry at the URL example.com/reg-a/v2/, and want to map docker.io inside that registry as a /docker.io namespace? Sure, that would be new, and we would need to differentiate between the target API URL and the target namespace — but even if so…

In other words, that needs to be covered by the registries.conf and sysregistriesv2 and will influence our isPrefix(..) issue above.

… I can’t see how; isPrefix is used to match Docker reference strings, not URLs containing the /v2/ fragment, against the Docker reference namespace hierarchy.

[vrothberg]

In contrast to my previous statement, I just added the InvalidateCache API.

The remaining bits regarding prefix matching etc. require some more thought. Maybe we can set up a meeting in the near future to share some ideas?

What I mostly want to check with you is if we could rewire the sysregsv2 config. I find some beauty in the last design of the PR for Moby, which looks as follows:

{
    "registries": [
        {
          "prefix": "docker.io/",
          "pull": [
            {"url": "http://localhost:5000"}, // Will be used as `http://localhost:5000/v2/`
            {"url": "https://registry-1.docker.io"} // Will be used as `https://registry-1.docker.io/v2/`
          ],
          "push": [
            {"url": "https://registry-1.docker.io"} // Will be used as `https://registry-1.docker.io/v2/`
          ]
        },
    "registries": [
        {
          "prefix": "docker.io/library/",
          "pull": [
            {"url": "http://localhost:5000/v2/library"},
            {"url": "https://registry-1.docker.io/v2/library"}
          ],
          "push": [
            {"url": "https://registry-1.docker.io/v2/"} 
          ]
        },
    ]
}

We can do the same with TOML. Prefixes must adhere to paths and paths only, so either host, namespace or registry. Another nice improvement is that the concept applies to pushing AND pulling, each configured separately.

[mtrmac]

In contrast to my previous statement, I just added the InvalidateCache API.

ACK, though this is primarily up to CRI-O maintainers. @runcom ? Or who can comment on this?

The remaining bits regarding prefix matching etc. require some more thought. Maybe we can set up a meeting in the near future to share some ideas?

Yeah, a meeting could be quite useful.

What I mostly want to check with you is if we could rewire the sysregsv2 config.

The one thing that is blocking this PR is incorrectly matching prefixes and unwanted configurations; changing the config format is, I think, quite unrelated (both the current and any proposed different config format would need the prefix matching fixed).

---

That said…

The sysregistriesv2 code has already been vendored into buildah and presumably deployed in released binaries. I’ve heard people actively asking for mirror support. What is the status of the file format? Is it unstable until documented in https://github.com/containers/image/blob/master/docs/containers-registries.conf.5.md ?

Sure, as long as it is compatible with the previous syntax, we can just keep pointing people at the old syntax and keep tweaking this one — as long as everyone understands that this is the status.

---

I find some beauty in the last design of the PR for Moby, which looks as follows:

{
    "registries": [
        {
          "prefix": "docker.io/",
          "pull": [
            {"url": "http://localhost:5000"}, // Will be used as `http://localhost:5000/v2/`
            {"url": "https://registry-1.docker.io"} // Will be used as `https://registry-1.docker.io/v2/`
          ],
          "push": [
            {"url": "https://registry-1.docker.io"} // Will be used as `https://registry-1.docker.io/v2/`
          ]
…
          {"url": "https://registry-1.docker.io/v2/library"}

We can do the same with TOML. Prefixes must adhere to paths and paths only, so either host, namespace or registry. Another nice improvement is that the concept applies to pushing AND pulling, each configured separately.

This is nice, an improvement over the earlier discussion around #468 (comment) .

• Eliminating the “mirror” concept is convenient (I assume that push and pull can be omitted to default to the standard URL).
• Forcing the users to say /v2/library looks rather confusing — though I appreciate how that unifies the /v2/library and /different-deploment/within-a-hostname/v2/ cases.
• This still duplicates the secure/insecure configurations across all the mirrored repositories.

[mtrmac]

The one thing that is blocking this PR is incorrectly matching prefixes and unwanted configurations

For now, something like #534 ?

[vrothberg]

The sysregistriesv2 code has already been vendored into buildah and presumably deployed in released binaries. I’ve heard people actively asking for mirror support. What is the status of the file format? Is it unstable until documented in https://github.com/containers/image/blob/master/docs/containers-registries.conf.5.md ?

I think I am the wrong audience for this statement given I am trying to push those things forward here and for Moby for quite some time :^)

Sure, as long as it is compatible with the previous syntax, we can just keep pointing people at the old syntax and keep tweaking this one — as long as everyone understands that this is the status.

The progress of this package certainly depends on the priority we give it to be finished. I think we're close for it to be useful in production. One thing I think we should have is an API to abstract from using those endpoints/mirrors for pushing and pulling. I wouldn't like libpod to keep all abstractions and others to start using those.

Yeah, a meeting could be quite useful.

This week is packed for me and next week I'll be on vacation. Maybe we can set something up for early December? I always assumed you're located at the West Coast, are you?

[vrothberg]

For now, something like #534 ?

I'll comment over there :)

[vrothberg]

The sysregistriesv2 code has already been vendored into buildah [...]

Turns out, it was a mistake doing that. Podman isn't using sysregsv2 yet but vendors buildah, so podman pull uses entirely different code for pulling than podman build. A user just reported this in #podman on IRC.

The registries.conf looks as follows:

[registries.search]
registries = ['docker.io', 'registry.fedoraproject.org', 'quay.io', 'registry.access.redhat.com', 'registry.centos.org']

[registries.insecure]
registries = ['10.1.8.88:8787']

[registries.block]
registries = []

podman pull from 10.1.8.88:8787 works while a podman build breaks immediately with:
invalid URL '123.456.789.0:5000/with/path': parse 123.456.789.0:5000/with/path: first path segment in URL cannot contain colon

It's again the URL parsing and the fact that we're not using URI prefixes which confuses net/url.

[mtrmac]

The sysregistriesv2 code has already been vendored into buildah and presumably deployed in released binaries. I’ve heard people actively asking for mirror support. What is the status of the file format? Is it unstable until documented in https://github.com/containers/image/blob/master/docs/containers-registries.conf.5.md ?

I think I am the wrong audience for this statement given I am trying to push those things forward here and for Moby for quite some time :^)

In one sense, this is primarily your work so it is perfectly up to you to declare it stable or not yet stable; in another, as soon as a few packages vendor in a particular implementation, no matter which one, and start relying on the new features, I’d expect significant pushback to future unstable changes.

Basically I don’t know where we are in this process (as you say, Buildah has already vendored the subpackage) and whether this is widely understood; I’d like to prevent a misunderstanding. @rhatdan ?

[mtrmac]

One thing I think we should have is an API to abstract from using those endpoints/mirrors for pushing and pulling. I wouldn't like libpod to keep all abstractions and others to start using those.

I default to thinking c/image/docker should handle this transparently as well, though that might turn out to be non-trivial — the current “one src/dest == one client == one host” design would need to be broken up — or, just guessing, maybe too opaque for some callers

Yeah, a meeting could be quite useful.

This week is packed for me and next week I'll be on vacation. Maybe we can set something up for early December?

I’ll be available for about one more week, and then I’ll be on vacation, in turn.

I always assumed you're located at the West Coast, are you?

:) Central Europe. But given >24-hr advance notice, and no conflicts, I can be available any time at all.

[vrothberg]

:) Central Europe. But given >24-hr advance notice, and no conflicts, I can be available any time at all.

Perfect! How about Tuesday 4th of Dec at 2PM (CET)? Maybe you can join #podman to setup a conference tool of our choice?

[vrothberg]

The sysregistriesv2 code has already been vendored into buildah and presumably deployed in released binaries. I’ve heard people actively asking for mirror support. What is the status of the file format? Is it unstable until documented in https://github.com/containers/image/blob/master/docs/containers-registries.conf.5.md ?

I think I am the wrong audience for this statement given I am trying to push those things forward here and for Moby for quite some time :^)

In one sense, this is primarily your work so it is perfectly up to you to declare it stable or not yet stable; in another, as soon as a few packages vendor in a particular implementation, no matter which one, and start relying on the new features, I’d expect significant pushback to future unstable changes.

Basically I don’t know where we are in this process (as you say, Buildah has already vendored the subpackage) and whether this is widely understood; I’d like to prevent a misunderstanding. @rhatdan ?

Buildah only uses the package but not the new format which gives us the chance to still change the format (i.e., extend it). I would really love to integrate the push/pull endpoints semantics similar to the ones of the Moby PR. This would also enable users to configure "push proxies". Starting from next week, I will have time to finish the remaining bits.

@mtrmac, can we merge this PR? I think it does now what we wanted it to do. The remaining bits and potential extensions can be sorted out in a fresh PR.

[mtrmac]

@mtrmac, can we merge this PR? I think it does now what we wanted it to do. The remaining bits and potential extensions can be sorted out in a fresh PR.

The one blocker for me is the prefix matching – sure, we can tell people not to use namespace/repo prefixes for now, but this matches prefixes of simple hostnames as well.

I suppose we can do ~what you propose in #534 , and as a quick fix use

strings.HasPrefix(ref, r.Prefix + "/") || ref == r.Prefix

; the first part would work for named references vs. hostnames, and the second part for CheckAuth — though, warning, I didn’t test this one bit.

[mtrmac]

Perfect! How about Tuesday 4th of Dec at 2PM (CET)? Maybe you can join #podman to setup a conference tool of our choice?

Sure. FWIW RH has a BlueJeans instance which should allow external connections.

[rhatdan]

I scheduled a BlueJeans Meeting for Dec 4th 2pm (CET) 8am (EST) for containers/image discussions.
You should get an invite.

[vrothberg]

@mtrmac, can we merge this PR? I think it does now what we wanted it to do. The remaining bits and potential extensions can be sorted out in a fresh PR.

The one blocker for me is the prefix matching – sure, we can tell people not to use namespace/repo prefixes for now, but this matches prefixes of simple hostnames as well.

I suppose we can do ~what you propose in #534 , and as a quick fix use

strings.HasPrefix(ref, r.Prefix + "/") || ref == r.Prefix

; the first part would work for named references vs. hostnames, and the second part for CheckAuth — though, warning, I didn’t test this one bit.

@mtrmac, it's now addressed. Prefixes will now always end with a '/'. I also removed net/url to allow IP addresses and avoid the voodoo trying to make it work without URI schemes.

[mtrmac]

Couldn't this be just moved to the check in FindRegistry instead of leaking into user-visible data?

Also, AFAICS this breaks the CheckAuth case (FindRegistry(…, hostnameWithoutAnySlashes)).

[mtrmac]

… sure, independently it’s perfectly reasonable to define that canonical Registry.Prefix values always end with exactly one slash, and this is just normalizes the values to confirm to this definition, but if FindRegistry, the primary user, ends up having to undo that, then it’s a bit impractical.

[vrothberg]

Also, AFAICS this breaks the CheckAuth case (FindRegistry(…, hostnameWithoutAnySlashes)).

I think that hostnameWithoutAnySlashes should be appended with a "/" in FindRegistry(...)? I didn't (yet) add code to throw an error if a Prefix doesn't end with slash but likely that's going to happen (to make it more transparent to the user). Once that happens, hostnameWithoutAnySlashes is problematic.

[mtrmac]

I think that hostnameWithoutAnySlashes should be appended with a "/" in FindRegistry(...)?

Would that involve adding an extra check whether the input is a hostname without any slashes, or would a slash be unconditionally added (even if the input is a named ref, possibly ending with a tag or digest)?

Even to implement precisely the proposed semantics (.Prefix matches hostnames and namespaces, not repos or tagged/digested images — at this point I’m fine with that, I’m not arguing about what the semantics should be, except that CheckAuth should work), adding the trailing slash to the .Prefix value just seems to make things more complex instead of simpler.

Or, of course, I’m being stupid — I haven’t actually tested anything about this in practice, so I may well be misunderstanding how the code actually works.

[vrothberg]

No at all. You have a very valid point. Then let's go with your initial proposal (similar to strings.HasPrefix(ref, r.Prefix + "/") || ref == r.Prefix).

[vrothberg]

Pushed the changes.

[mtrmac]

Looks good, but make validate fails.

gofmt -s -w pkg/sysregistriesv2/system_registries_v2.go, please.

[mtrmac]

👍

Tests in containers/skopeo#521 pass . 🎉

Thanks again!

[vrothberg]

Awesome, thanks a lot!

…

On Thu 29. Nov 2018 at 13:20, Miloslav Trmač ***@***.***> wrote: 👍 Tests in containers/skopeo#521 <containers/skopeo#521> pass . 🎉 Thanks again! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#468 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALI4g2jGafRC5QwNd4bZqSBkxkBfcV5yks5uz9D0gaJpZM4Uu5TC> .