Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Support basic authentication for registries #918

Merged
merged 2 commits into from
Jan 29, 2018
Merged

Conversation

squaremo
Copy link
Member

@squaremo squaremo commented Jan 25, 2018

The image registry code assumed

  • all image registries use HTTPS
  • all image registries use token authentication

..but if you run your own registry in your cluster, neither of these things is likely to be true.

To support basic authentication, all we need to do is add a handler in when constructing the registry client; it will get used if the authentication challenge indicates so.

Supporting HTTP is (oddly) a bit trickier, since there's no indication from an image name (which is all we have) whether the registry uses HTTP or HTTPS. We want to use HTTPS in general, so make the user tell us which hosts to use HTTP for (so that it's at least possible to use it), and otherwise use HTTPS.

Fixes #915.

EDIT: describe alternate solution, from comment below, to HTTP vs HTTPS.

@squaremo
Copy link
Member Author

squaremo commented Jan 25, 2018

One caveat: since we try HTTP first (in the expectation that we'll be redirected if HTTPS is required), if a registry accepts HTTP we might end up sending auth headers over an insecure connection. All of {index.docker.io, quay.io, gcr.io} require HTTPS, but it's possible that somebody could set up a private registry that's not in their cluster and doesn't use HTTPS. Suggestions welcome!

@squaremo
Copy link
Member Author

One caveat: since we try HTTP first

An alternative would be to require people using insecure registries to tell us about them explicitly, e.g., as a command-line argument something like

--registry-insecure-host=kube-registry.kube-system.svc.cluster.local

The image registry code assumed
 - all image registries use HTTPS
 - all image registries use token authentication

..but if you run your own registry in your cluster, neither of these
things is likely to be true.

To support basic authentication, all we need to do is add a handler in
when constructing the registry client; it will get used if the
authentication challenge indicates so.

Supporting HTTP is (oddly) a bit trickier, since there's no indication
from an image name (which is all we have) whether the registry uses
HTTP or HTTPS. Registries will tend to redirect any HTTP
requests to HTTPS, so we _could_ try HTTP first and follow any
redirection. However, if a registry supported both HTTP and HTTPS, and
didn't redirect, we'd end up sending credentials over an insecure
connection unnecessarily.

Instead of that, make it possible to tell the daemon which registries
to use HTTP for, with the (multiply-valued) argument
`--registry-insecure-host`.
@squaremo squaremo force-pushed the issue/915-basicauth branch from 6ce7c92 to ae9d267 Compare January 26, 2018 15:14
Copy link
Contributor

@samb1729 samb1729 left a comment

Choose a reason for hiding this comment

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

RCGW

Trace bool
InsecureHosts []string

mu sync.Mutex

This comment was marked as abuse.

f.mu.Unlock()

scheme := "https"
for _, h := range f.InsecureHosts {

This comment was marked as abuse.

This comment was marked as abuse.

@samb1729
Copy link
Contributor

Capturing thoughts:
In general we would benefit from integration tests for this sort of thing, but I don't think it's necessary to require it here. Perhaps we should capture "integration test all the things" in its own issue since it's likely not a small task.
For now let's leave the bar at "I ran it a few times locally and it seemed to work and it also looks sane"

@squaremo squaremo merged commit b5a285b into master Jan 29, 2018
@squaremo squaremo deleted the issue/915-basicauth branch January 29, 2018 14:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants