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

Support basic authentication for registries #918

Merged
merged 2 commits into from Jan 29, 2018

Conversation

@squaremo
Copy link
Member

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member Author

squaremo commented Jan 25, 2018

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
squaremo added 2 commits 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. 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 Jan 26, 2018
Copy link
Contributor

sambooo left a comment

RCGW

Trace bool
InsecureHosts []string

mu sync.Mutex

This comment has been minimized.

Copy link
@sambooo

sambooo Jan 29, 2018

Contributor

:browniepoint:

f.mu.Unlock()

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

This comment has been minimized.

Copy link
@sambooo

sambooo Jan 29, 2018

Contributor

I can't not comment about this not being a map/set lookup, but it's incredibly unlikely to matter...

This comment has been minimized.

Copy link
@squaremo

squaremo Jan 29, 2018

Author Member

Right -- I wouldn't expect enough entries here for O(n) to matter.

@sambooo

This comment has been minimized.

Copy link
Contributor

sambooo commented Jan 29, 2018

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
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@squaremo squaremo deleted the issue/915-basicauth branch Jan 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.