-
Notifications
You must be signed in to change notification settings - Fork 3k
Prototype: Add --tls-details, only for non-remote pulling for now #28043
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
mtrmac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this highlights the basic design trade-offs:
- We can add
--tls-detailsas a global option.- That automatically forwards it to libimage, and hopefully affects all image operations (but not any other operations), for fairly little work.
- We will still need to scope/audit all other TLS users, and use the
--tls-detailsoption. - At a first glance it’s a bit unsatisfactory to have
registry.PodmanConfig().TLSDetailsFilecontain only the path, not the parsed data. The parsing code would have to be repeated in many users. There is probably a trivially way to do better here, I didn’t spend much time looking. - This doesn’t affect remote processes at all; how would the option get to the remote server??!
- We can add it as a per-operation option
- Rather more tedious, but we would ~clearly delineate where we expect it to work and where it isn’t ready yet.
- Clearly generalizes to remote operation: add the
basetls.Config.MarshalTextformat as an option field. - Right now
libimage.CopyOptionsignores theSystemContextfield AFAICT. We don’t need to change that, we can add a newlibimage.CopyOptions.BaseTLSConfig(perhaps preferringCopyOptionsbut falling back to the initialization-time runtimeSystemContext).
| Retry uint `schema:"retry"` | ||
| RetryDelay string `schema:"retrydelay"` | ||
| TLSVerify bool `schema:"tlsVerify"` | ||
| BaseTLSConfig string `schema:"baseTLSConfig"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this really need to be manually, separately from pkg/bindings/images.PullOptions? it It seems tedious and error-prone.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
063c7e0 to
3336e4b
Compare
fedoraMinimal is referenced in non-_test config_arm64.go. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This is only looking at plain pull, and does not correctly handle remote operation. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
3336e4b to
d43ef94
Compare
|
OK a minimal smoke test is promising: # cat tls-details-pqc-only.yaml
minVersion: "1.3"
namedGroups:
- "X25519MLKEM768"
# bin/podman --tls-details tls-details-pqc-only.yaml pull --retry 0 quay.io/foo
Trying to pull quay.io/foo:latest...
Error: unable to copy from source docker://quay.io/foo:latest: initializing source docker://quay.io/foo:latest: pinging container registry quay.io: Get "https://quay.io/v2/": EOF
# bin/podman --tls-details tls-details-pqc-only.yaml push --retry 0 quay.io/libpod/alpine
Getting image source signatures
Error: trying to reuse blob sha256:03901b4a2ea88eeaad62dbe59b072b28b6efa00491962b8741081c5df50c65e0 at destination: pinging container registry quay.io: Get "https://quay.io/v2/": EOF("EOF" is how the Note that this does not yet include even |
|
@containers/podman-maintainers RFC on the approach. (In containers/container-libs#619 , Paul suggests not supporting the option for remote at all, or maybe exposing it as an option for the remote server only. That would certainly simplify things.) |
Well my point is that it is already exposed for the server because it is a global option, |
I think so, the way the current version of this PR initializes the runtime. I suppose machine creation would still need extra code to copy the file + set the flag, or maybe we would leave that for users to do manually. |
This exposes containers/container-libs#623 , looking for ways to do that.
Absolutely untested at this point.
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?