Skip to content
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

Implement tls-verify switch in libpod API #16491

Closed
wants to merge 1 commit into from

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Nov 11, 2022

Signed-off-by: Daniel J Walsh [email protected]

Does this PR introduce a user-facing change?

--tls-verify switch now works with podman -remote

@openshift-ci openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Nov 11, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 11, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 11, 2022
@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Nov 11, 2022
@openshift-ci openshift-ci bot added release-note and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Nov 11, 2022
@TomSweeneyRedHat
Copy link
Member

LGTM
but ugly red tests

@SoMuchForSubtlety
Copy link
Contributor

As far as I can tell, this PR does no change the default behaviour of not allowing pulling from http, correct?

@vrothberg
Copy link
Member

As far as I can tell, this PR does no change the default behaviour of not allowing pulling from http, correct?

The default behavior must not change. Docker doesn't disable TLS verification by default either as it would be a huge security risk.

Platform string `schema:"platform"`
TLSVerify bool `schema:"tlsVerify"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the Docker API having that parameter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhatdan the Docker API doesn't have such a parameter. I don't think this PR is needed.

@vrothberg
Copy link
Member

So far, I cannot see how this is fixing the issue. Docker doesn't have a CLI/parameter switch to turn off TLS verification.

It's configured locally, see https://docs.docker.com/registry/insecure/#deploy-a-plain-http-registry, so testcontainers should be dropping a .conf file in /etc/containers/registries.conf.d instead.

@SoMuchForSubtlety
Copy link
Contributor

SoMuchForSubtlety commented Nov 14, 2022

Looks like docker has a special case for localhost hardcoded, that's why it works without any configuration.

// loadInsecureRegistries loads insecure registries to config
func (config *serviceConfig) loadInsecureRegistries(registries []string) error {
	// Localhost is by default considered as an insecure registry. This is a
	// stop-gap for people who are running a private registry on localhost.
	registries = append(registries, "127.0.0.0/8")

https://github.com/moby/moby/blob/6eab4f55faf77bb0f253553d7d6fadb2ef6f4669/registry/config.go#L182-L186

@vrothberg
Copy link
Member

Great catch, @SoMuchForSubtlety!

@mtrmac WDYT? Should c/image follow that example?

@mtrmac
Copy link
Collaborator

mtrmac commented Nov 14, 2022

  • I’m unsure about excluding localhost. What about network namespaces, multi-user systems, and the like? Accessing localhost is not obviously not crossing a security boundary, although it’s certainly likely that most uses will not [just like most TLS connections are not under attack…]
  • We definitely must not use the InsecureRegistryCIDRs approach of a separate DNS lookup; that’s a TOCTOU that allows bypassing TLS entirely. If anything, a plain text match on localhost.
    • So, we would not be truly compatible either way.
    • Last time I have checked, which was quite a few years ago, there really was no way to avoid the separate DNS lookup / TOCTOU in the Go network / HTTP stack. Maybe that has changed.
    • (Users can already disable TLS for the localhost name in registries.conf. Admittedly that’s not an answer to the question of whether we should do that by default.)

@vrothberg
Copy link
Member

Thanks, @mtrmac!

Should we recommend configuring registries.conf instead? I think that' acceptable to turn testcontainers-CI green.

@SoMuchForSubtlety, what do you think?

@mtrmac
Copy link
Collaborator

mtrmac commented Nov 14, 2022

It seems to me that if we should recommend that without qualification, we should also ship that configuration by default … which brings the question full circle.

Alternatively, recommend it for single-user workstations; that would probably be more correct but I don’t know where we would document it so that people read it.

@SoMuchForSubtlety
Copy link
Contributor

Should we recommend configuring registries.conf instead? I think that' acceptable to turn testcontainers-CI green.

It's currently not possible to allow localhost without specifying a port.

The following config yields the error invalid condition: location is unset and prefix is not in the format: *.example.com"

[[registry]]
prefix = "localhost"
insecure = true

This config works, but only for the specified port, and the tests use randomly assigned ports.

[[registry]]
location = "localhost:123123"
insecure = true

@mtrmac
Copy link
Collaborator

mtrmac commented Nov 14, 2022

Huh?

[[registry]]
location = "localhost"
insecure = true

is expected to work, and seems to work for me manually modifying the existing unit tests. If it doesn’t work, please file a c/image bug with a reproducer.

@SoMuchForSubtlety
Copy link
Contributor

SoMuchForSubtlety commented Nov 14, 2022

is expected to work, and seems to work for me manually modifying the existing unit tests. If it doesn’t work, please file a c/image bug with a reproducer.

Seems to be a known issue limitation?
https://github.com/containers/image/blob/0d85878d7a774dd753a4e211455282af94d78556/pkg/sysregistriesv2/system_registries_v2.go#L392-L400

// FIXME: allow config authors to always use Prefix.
// https://github.com/containers/image/pull/1191#discussion_r610622495
if !strings.HasPrefix(reg.Prefix, "*.") && reg.Location == "" {
	return &InvalidRegistries{s: "invalid condition: location is unset and prefix is not in the format: *.example.com"}
}

@vrothberg
Copy link
Member

@mtrmac set the location while the issue happens when only "prefix` is set. What doesn't work is to set any port on localhost as insecure.

@mtrmac
Copy link
Collaborator

mtrmac commented Nov 14, 2022

What doesn't work is to set any port on localhost as insecure.

Good point, that’s true.

@mtrmac
Copy link
Collaborator

mtrmac commented Nov 14, 2022

… and that weakens my argument that users can already set this in registries.conf. It’s kind of true but it’s noticeably more of a hassle for temporary per-user registries.

@vrothberg
Copy link
Member

… and that weakens my argument that users can already set this in registries.conf. It’s kind of true but it’s noticeably more of a hassle for temporary per-user registries.

Could we turn this into an RFE for registries.conf? For instance, to allow matching for any port?

@mtrmac
Copy link
Collaborator

mtrmac commented Nov 14, 2022

Very generally speaking there the concern there is maintaining a comprehensible “closest match” ordering between the registry entries. In this instance, I think a new localhost:* syntax could work — but note that the implementation (sorting refMatchingPrefix matches by len(r.Prefix) would need changing to explicitly order localhost:* vs localhost/a correctly.

I don’t know that this resolves the question about having a built-in default — but making that config possible seems with a localhost:* entry might be valuable either way.

@vrothberg
Copy link
Member

@mtrmac since it's a special case one way or another: Would do you think of a boolean option "unsafe_localhost_insecure=true"?

@mtrmac
Copy link
Collaborator

mtrmac commented Nov 15, 2022

(localhost-no-tls? maybe we don’t need to warn so strongly in this case, and “insecure” is unspecific about this being TLS.)

Where would that option live? As a special case in registries.conf? Right now I’d mildly prefer to have the more generic $host:* feature in that case…

But really I’m uninformed and undecided at this point. Should this be a built-in default? I’m almost sure we have had that discussion before, both about localhost special cases, and about host:anyPort matching; I can’t remember a thing about any upsides/downsides discussed. Given enough time, I will research this eventually…

@vrothberg
Copy link
Member

(localhost-no-tls? maybe we don’t need to warn so strongly in this case, and “insecure” is unspecific about this being TLS.)

SGTM

Where would that option live? As a special case in registries.conf? Right now I’d mildly prefer to have the more generic $host:* feature in that case…

Yes, I had registries.conf in mind.

But really I’m uninformed and undecided at this point. Should this be a built-in default? I’m almost sure we have had that discussion before, both about localhost special cases, and about host:anyPort matching; I can’t remember a thing about any upsides/downsides discussed. Given enough time, I will research this eventually…

I had a simple boolean in mind. False unless set in registries.conf. The code treating insecure can deal with it if the domain is localhost.

A general host:anyPort would certainly be cool but I am concerned about the time it may consume. I think our books are quite full at the moment.

@vrothberg
Copy link
Member

Closing but we should continue the design discussion and then move the issue over to c/image.

@vrothberg vrothberg closed this Nov 17, 2022
@rhatdan
Copy link
Member Author

rhatdan commented Nov 18, 2022

Why is this closed from a Podman point of view.

Currently
podman pull --tls-verify=false works,
podman --remote pull --tls-verify does not?

@vrothberg
Copy link
Member

Why is this closed from a Podman point of view.

It claims to fix #16486 but is the wrong approach.

Currently podman pull --tls-verify=false works, podman --remote pull --tls-verify does not?

Shouldn't that change the libpod endpoint? This PR changes the compat one.

@rhatdan
Copy link
Member Author

rhatdan commented Nov 22, 2022

Ok I will reopen to fix the libpod endpoint.

@rhatdan rhatdan reopened this Nov 22, 2022
@github-actions
Copy link

A friendly reminder that this PR had no activity for 30 days.

@vrothberg
Copy link
Member

@rhatdan shall we close?

@rhatdan
Copy link
Member Author

rhatdan commented Jan 17, 2023

No I hope to get back to this one.

@github-actions
Copy link

A friendly reminder that this PR had no activity for 30 days.

@rhatdan rhatdan changed the title Implement tls-verify switch in compat API Implement tls-verify switch in libpod API Apr 4, 2023
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhatdan, the compat endpoint does not support a tlsVerify parameter. podman-remote uses the libpod specific /images/pull endpoint.

I don't think the code change has an effect.

@vrothberg
Copy link
Member

@rhatdan, friendly ping.

@vrothberg vrothberg closed this Jun 19, 2023
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 18, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Change to remote API; merits scrutiny locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants