Add "BaseTLS" options

[mtrmac]

For now absolutely untested, and with Rekor+Fulcio not configurable.

On top of #619.

[aguidirh]

Even with limited knowledge about the library, I tried my best to give my contribution o the code review.

[aguidirh]

Is this API used only internally? I'm asking because it is a breaking API change so I'm not sure the impact of it.

[aguidirh]

How about extending the API by passing a struct and adding types on that struct? It could reduce frictio in future if the API needs to be extended again.

[mtrmac]

c/common does not have a stable API commitment [and forcing callers to update means less risk of overlooking that we have to do something].

Using a struct here does make sense, I’ll update the PR.

[mtrmac]

Updated.

[aguidirh]

Nice to see we are also maintaining the comments and not only the code. This is something rare, thanks!

[packit-as-a-service]

Packit jobs failed. @containers/packit-build please check.

[mtrmac]

Extended to also cover c/common/libimage.

Fulcio + the related OIDC workflows are missing — we might need to change the clients upstream, or replace them.
The underlying Rekor functionality is implemented but not exposed — we might want to share the option with Fulcio.

Other than that, this should be comprehensive for container-libs.

Still untested, other than the docker:// transport.

[mtrmac]

This is conceptually orthogonal to #619, I’m including #619 here only to allow easy vendoring into consumers.

We could also review+merge in the opposite order.

[mtrmac]

Minimal manual tests, using containers/skopeo#2797 :

% cat tls-details-pqc-only.yaml                                                          
minVersion: "1.3"
namedGroups:
  - "X25519MLKEM768"

# docker:
% bin/skopeo --tls-details ./tls-details-pqc-only.yaml inspect --raw docker://quay.io/foo
FATA[0000] Error parsing image name "docker://quay.io/foo": pinging container registry quay.io: Get "https://quay.io/v2/": EOF 

# oci:
# manually prepared image, with manifest
% cat oci-layout/blobs/sha256/8083ab158310d4a23b04ff72f5485fe913c457e033811c159e4314f095492378                                                                  
{
  "schemaVersion": 2,
  "mediaType": "application/vnd.oci.image.manifest.v1+json",
  "config": {
    "mediaType": "application/vnd.oci.image.config.v1+json",
    "digest": "sha256:31003e2979e8f5fec9358644591ce51df452fb489624bd09a999f2d6e091e936",
    "size": 604
  },
  "layers": [
    {
      "mediaType": "application/vnd.oci.image.layer.v1.tar+gzip",
      "digest": "sha256:161a397ac5ee9e631cd79cdfb5f0b36a861071d42e67afe98ed3cb69155ea5ff",
      "size": 2776947,
      "urls": ["https://quay.io/v2/"]
    }
  ]
}
% bin/skopeo --insecure-policy --tls-details ./tls-details-pqc-only.yaml copy oci:oci-layout dir:t2
Getting image source signatures
Copying blob 161a397ac5ee [--------------------------------------] 0.0b / 2.6MiB | 0.0 b/s
FATA[0000] reading blob sha256:161a397ac5ee9e631cd79cdfb5f0b36a861071d42e67afe98ed3cb69155ea5ff: fetching "https://quay.io/v2/" failed Get "https://quay.io/v2/": EOF: failed fetching external blob from all urls 

# atomic:
% KUBERNETES_MASTER=https://quay.io/ bin/skopeo --tls-details ./tls-details-pqc-only.yaml inspect --raw atomic:quay.io:443/foo/bar:baz
FATA[0000] Error retrieving manifest for image: Get "https://quay.io/oapi/v1/namespaces/foo/imagestreams/bar": EOF 

# docker-daemon:
% bin/skopeo --tls-details ./tls-details-pqc-only.yaml inspect --daemon-host=https://quay.io --raw docker-daemon:foo:bar 
FATA[0000] Error parsing image name "docker-daemon:foo:bar": loading image from docker engine: error during connect: Get "https://quay.io/v1.53/images/get?names=foo%3Abar": EOF 

Untested: c/common/pkg/download.FromURL, that requires ~Podman.

[aguidirh]

Added one minor suggestion, since this is draft I will keep an eye for updates so I can code review again when new changes are pushed.

[aguidirh]

Centralizing this code on a helper would help to reduce the duplicated code and prevent mistakes of consumers not using .Clone() ?

func (sys *SystemContext) GetBaseTLSConfig() *tls.Config {
    if sys == nil || sys.BaseTLSConfig == nil {
        return nil
    }
    return sys.BaseTLSConfig.Clone()
}

[mtrmac]

I don’t think we can prevent mistakes: the BaseTLSConfig field is, necessarily, public, so consumers can always do sys.BaseTLSConfig without the Clone.

(Also, using BaseTLSConfig without Clone is not actually incorrect, if the user has no other configuration to set. Really we want const, but this is Go…)

If we had a linter that can detect reads (but not writes) to BaseTLSConfig, that would be more interesting … but we’d have to have that linter active in callers of c/image.

---

So there are two kinds of consumers:

• Those that have no other options to set:

  var tlsConfig *tls.Config
  if sys != nil {
    tlsconfig = sys.BaseTLSConfig // may still be nil
    // or
    tlsconfig = sys.BaseTLSConfig.Clone() // may still be nil: nil.Clone() == nil
  }

  A helper would help here, but only for the sys == nil case, and that’s fairly rare — many users of SystemContext start with making it non-nil.

• Those that want to adjust the configuration:

  var tlsConfig *tls.Config
  if sys != nil && sys.BaseTLSConfig != nil {
    tlsConfig = sys.BaseTLSConfig.Clone()
  } else {
    tlsConfig = &tls.Config{ /* something */ }
  }

  With a helper, this is

  …
  if cfg := sys.GetBaseTLSConfig(); cfg != nil {
       tlsConfig = cfg
  } else {
    …

  and otherwise the same; that’s a bit of an improvement but the if would still remain (at least right now, various users really differ in the /* something */ part).

The way I think about it, SystemContext is really a historical artifact of how c/image was created by quickly splitting it from Skopeo and then randomly freezing the API one unexpected day, it’s not a good design (namely it allows setting a lot of transport-specific or situation-specific options that will be silently ignored), and I’d like to slowly move away from it. See how the download.FromURL got its own download.Options instead of a SystemContext — and in that situation, the sys == nil case probably goes away entirely, and a helper on SystemContext would not help.

(Admittedly SystemContext is rather useful for this work, due to how it already is plumbed all over the call stack.)

---

Net, it’s a bit awkward to add code to a “declarations-only” c/image/types, and I’m not sure the improvement in callers would be worth it. But I’m open to being overruled.

[aguidirh]

Got it, thanks for the explanation. Now that I understand I agree that it is difficult to prevent mistakes, the only thing would avoid code duplication, but then we increase coupling. So I'm not sure if there is advantage on creating a helper anymore.

[aguidirh]

Thanks for making it clear why it is not possible to use the moby client.WithTLSClientConfigFromEnv.

[giuseppe]

still Draft, but codewise LGTM