TLS configuration options

[mtrmac]

This is sufficient, and if users use …/tlscobra, we can change the details without changing the callers again; but we might want to do something else.

[mtrmac]

Perhaps we don’t want individual options at all, and instead ask users to point at a config file. (That would also somewhat disincentivize unnecessary and overzealous use of --tls-min-version.)

Perhaps we should read a system-wide config file by default? Probably not; the long-term future should be Go following /etc/crypto-policies (any decade now?), and we don’t want to establish, and start shipping, a config file, only to have to incompatibly remove it later.

Perhaps we should just put this into containers.conf. Skopeo doesn’t currently use that file at all.

---

Either way, the API of this package can support all of those scenarios, we “only” have to make a decision before tagging a release of any of the users.

[aguidirh]

If we believe the long-term future should be go following /etc/crypto-policies I agree that putting the tls options in a config file is better than exposing them as flags.

[Luap99]

I think overall the abstraction is nice, now whenever we actually want all this options or not is a separate conversation I don't feel like pushing back on.

Ultimately I feel like this is wrong no matter how we implement it and should be handled by the go std lib instead but here we are so I am fine merging this with the AGENTS file dropped.

[Luap99]

non blocking

I had to double check since I was not sure if cobra/pflag wraps the right option name becaus ewithout this it would be hard to kjnow for the user which flag was meant but it seems the wrapped message is fine
invalid argument "1.2" for "--tls-min-version" flag: flag already set

so maybe a small comment that cobra wraps the flag name to the error here would be helpful

[Luap99]

non blocking:
I like to either have the true error type or a string here that actual matches the correct error message via assert.ErrorContains() because only then you can be sure the correct error condition is triggered and we don;t cover the wrong thing

same for the other tls test cases

[mtrmac]

OK, a complete rewrite:

• Renamed the primary config object from tlsopts.BaseTLSConfig to basetls.Config
• To support Podman’s remote operation, the object now exposes MarshalText and UnmarshalText: the CLI can provide a basetls.Config to the engine abstraction, and the engine can either consume the config directly, or send it over the wire. (Alternatively, the config could be set at Podman machine creation, and when starting the remote server; this API just allows for passing the data around, with a decision to be made later.)
• The configuration is now primarily in a file, using YAML (matching sigstore-params, more or less to allow comments). Added a centralized man page.
• This makes the tlscobra wrappers less necessary, it’s just one option now. And with the way buildah bud is wired into podman build, where the Cobra flag definitions and their consumption are split across different subpackages (I don’t like that much but I have no ambition to change that now), requiring the tlscobra abstraction would be difficult. So, now, programs are expected to define their own CLI flags, and consume them via basetls.NewFromOptionalFile.

PTAL again.

[Luap99]

To support Podman’s remote operation, the object now exposes MarshalText and UnmarshalText: the CLI can provide a basetls.Config to the engine abstraction, and the engine can either consume the config directly, or send it over the wire. (Alternatively, the config could be set at Podman machine creation, and when starting the remote server; this API just allows for passing the data around, with a decision to be made later.)

I think we are better off not accepting those via remote API. In the case of podman system service you could just add the option to that command to cover all of the service actions.
And in terms of podman buildah I really think we need the containers.conf option anyway.

Looking at your podman PR you have the option defined globally which seem right but then you pass it to remote only on pull? That is broken design wise because both play kube and podman build both send the content via file and the pull is triggered on the remote not via the /pull endpoint

So IMO the only sensible thing never send this thing via remote and get rid of the custom marshal format. Everything else will just end up a security issue if we forget some code path that triggers a remote pull without sending that config.

This makes the tlscobra wrappers less necessary, it’s just one option now. And with the way buildah bud is wired into podman build, where the Cobra flag definitions and their consumption are split across different subpackage

It this not defined once globally for buildah? I don't think adding it on bud would help there either, you have to expose this to buildah from and buildah add (if we care regular about https downloads) so then I don't think this inter dependency should matter.

[mtrmac]

I think we are better off not accepting those via remote API.

I’m open to that, it would certainly simplify things a lot. (Maybe we wouldn’t need to worry about ciphers in ssh at all.)

The marshaling would then be remain unused, but I don’t think it hurts to have it (there are fairly comprehensive unit tests).

---

but then you pass it to remote only on pull?

That part of the Podman PR is really a proof of concept, I was assuming that we’d have to wire every single Podman operation individually. (And it would certainly be easy to forget something, but that’s always going to be the case with the various operation that don’t go through libimage.)

---

And with the way buildah bud is wired into podman build, where the Cobra flag definitions and their consumption are split across different subpackage

It this not defined once globally for buildah?

It is ~centralized but not encapsulated: compare https://github.com/containers/buildah/blob/0e32de077a526d7e2bfd4f552136357b2f0a1502/pkg/cli/common.go#L251 and https://github.com/containers/buildah/blob/0e32de077a526d7e2bfd4f552136357b2f0a1502/pkg/parse/parse.go#L446 , connected just by a string literal.

I have chosen this flag entirely randomly, and it seems that it might actually be parsed three ways:

• For buildah bud, from the variable defined in pkg/cli via BoolVarP
• For commit and push, from a variable defined in cmd/buildah/… via BoolVarP
• From pkg/parse setting up a SystemContext, by matching a string literal to either of the above

That’s an even stronger argument why the original version of the PR, where a single function returned both a FlagSet and a struct holding a field where the value is to be written, would be harder to integrate into the existing Buildah structure.

[mtrmac]

Restructed a bit more: TLSDetailsFile is now in basetls, and the CLI-consuming NewFrom[Optional]File are now tlsdetails.BaseTLSFrom[Optional]File, so that basetls only depends on the standard library.

[TomSweeneyRedHat]

Not for this review, or even a quarterly look, but I think a default location for the file would be a good addition. Or at least a way to specify it once in containers.conf.

[mtrmac]

Today we have talked about how, hopefully, eventually a Go-native policy mechanism will be added (and dare we hope for full integration with /etc/crypto-policies?); and at that time, we would deprecate all of this and tell users to use something with a wider scope.

So I’m weakly leaning towards not making it any easier to use this option — OTOH as is, OpenShift will have to edit every single podman command in their bootstrap path (and wherever else they might be using it), and it might be easier for that code to, instead, write a file to /etc at the beginning.

[TomSweeneyRedHat]

The "don't" vs "do not" thing is arguably a personal bias. I just like to avoid contractions in general in man pages.

Suggested change

	they don't affect other executables (e.g. **git**(1), **ssh**(1)) that may be executed internally to perform another operation.
	they do not affect other executables (e.g., **git**(1), **ssh**(1)) that may be executed internally to perform another operation.

[mtrmac]

Thanks, updated.

[TomSweeneyRedHat]

Code LGTM, but I definitely want @Luap99 or @mheon to have a looksee.

[Luap99]

One thing that is missing from the first iteration is the onceValue flag definition. I understand you want to define the flag just once in the cli tools which is fine overall but raises the question are you fine now with people specifying the cli option multiple times now?

I guess if you do want to guard aginst that missuse we could add that oncevlaue flag type to common/pkg/flag/flag.go and then have the other tools use that?

Anyhow this is not a blocker for this merge here, the code here LGTM overall (well modulo the complaint that none of this should need to exist)

[aguidirh]

Overall it LGTM, I added one nit comment which is not a blocker for the merge.

[mtrmac]

One thing that is missing from the first iteration is the onceValue flag definition. I understand you want to define the flag just once in the cli tools which is fine overall but raises the question are you fine now with people specifying the cli option multiple times now?

I think it’s just too hard now.

That was easy to do with the previous design, where the callers got an encapsulated state object and a FlagSet; the code could enforce it all within this package, without the callers having to care.

Now that the callers define the StringVar on their own (and Buildah uses flags.GetString without any state object available), it’s not possible to do it invisibly.

Each caller still can do it, as you say:

I guess if you do want to guard aginst that missuse we could add that oncevlaue flag type to common/pkg/flag/flag.go and then have the other tools use that?

and that would be invisible to this package.

But I don’t care strongly enough to propose making this the default behavior for all of Podman/Buildah/Skopeo , at least for all future options (and to fight against the Go ecosystem’s conventions all the way).

[Luap99]

But I don’t care strongly enough to propose making this the default behavior for all of Podman/Buildah/Skopeo , at least for all future options (and to fight against the Go ecosystem’s conventions all the way).

My point was more it was important enough for you before but now it isn't just because we use a filepath? I do not object just trying to ask if we should still have that check and the flag type in common.
If you do not think so I am cool

[mtrmac]

It’s both that the risk was higher with the individual options, where a user could reasonably use --tls-cipher-suites twice with different values, and that doing it now is harder.

I wouldn’t mind being precise here, and if someone wants to add a subpackage to c/common and propose using it in most future flags, I’d support that, but I don’t think it’s a blocker for this work any more, on balance.