Skip to content

The --skip-tls flag shouldn't use the http protocol #872

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

The --skip-tls flag shouldn't use the http protocol #872

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions pkg/image/containerdregistry/resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,11 @@ func NewResolver(configDir string, insecure bool, roots *x509.CertPool) (remotes
TLSHandshakeTimeout: 10 * time.Second,
ExpectContinueTimeout: 5 * time.Second,
TLSClientConfig: &tls.Config{
InsecureSkipVerify: false,
InsecureSkipVerify: insecure,
RootCAs: roots,
},
}

if insecure {
transport.TLSClientConfig = &tls.Config{
InsecureSkipVerify: insecure,
}
}
headers := http.Header{}
headers.Set("User-Agent", "opm/alpha")

Expand All @@ -55,9 +50,6 @@ func NewResolver(configDir string, insecure bool, roots *x509.CertPool) (remotes
)),
docker.WithClient(client),
}
if insecure {
Copy link
Contributor

@perdasilva perdasilva Nov 29, 2021

Choose a reason for hiding this comment

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

Question: Have you checked your changes still work with http endpoints?
I'm not sure what these regopts do exactly. Should we maybe still keep this but only add if if the url scheme is http?
@njhale any thoughts here?

Copy link
Contributor Author

@omertuc omertuc Nov 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I have not checked that, good point. Didn't think about that.

But if I do think of it, why would someone pass --skip-tls when using HTTP in the first place? And also, how does one even use http registries? e.g. How would you tell docker to use http to communicate with quay.io/foo/bar:latest? It uses https by default. Searching online didn't lead me to a conclusive answer, prefixing with http:// i.e. http://quay.io/foo/bar:latest is illegal

Copy link
Contributor Author

@omertuc omertuc Nov 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Here's how it's done:
https://docs.docker.com/registry/insecure/#deploy-a-plain-http-registry

Basically a config file lists all registries where docker should try to communicate with HTTPS, if it gets a bad cert, ignore any errors, if there's no HTTPS on that server at all then it fallbacks to HTTP.

Doesn't seem like you can specify the protocol in the image address itself in any way I could find

Also this of-course applies to the Docker CLI, not sure about the behavior for the docker library used here

Copy link
Contributor

@perdasilva perdasilva Nov 29, 2021

Choose a reason for hiding this comment

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

I agree with you here. --skip-tls and http are not related at all. I just don't know if these regopts we are removing here are needed for something specific (e.g. pipeline). We may still need to add a --use-http options or something like that. So, I'll set it to ok-to-test but I'd like @njhale (who I think originally added this) to ok.

Copy link
Contributor Author

@omertuc omertuc Nov 29, 2021

Choose a reason for hiding this comment

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

Original author is here a16399f

Copy link
Member

@estroz estroz Dec 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We can't change/remove current --skip-tls functionality, but we can

  • update the flag's description to relay it's current functionality
  • deprecate the flag
  • add new flags --skip-tls-verify and --use-http to enable their respective namesake's functionality

Copy link
Member

@estroz estroz Jan 5, 2022

Choose a reason for hiding this comment

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

Assuming @omertuc does not update this PR soon, @dinhxuanvu @jpower432 can one of you take on the changes in a separate PR?

Copy link
Contributor Author

@omertuc omertuc Jan 5, 2022

Choose a reason for hiding this comment

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

Ah didn't realize your bullets were concrete change suggestions.

Yeah please take it on yourselves as I don't feel confident enough in this code base to make those changes

Copy link
Member

@estroz estroz Jan 5, 2022

Choose a reason for hiding this comment

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

Sounds good, thanks for all you've done so far!

Copy link
Contributor

@jpower432 jpower432 Jan 11, 2022

Choose a reason for hiding this comment

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

I can make these changes.

regopts = append(regopts, docker.WithPlainHTTP(docker.MatchAllHosts))
}

opts := docker.ResolverOptions{
Hosts: docker.ConfigureDefaultRegistries(regopts...),
Expand Down