Skip TLS check when using insecure host #1526

Timer · 2018-11-17T02:26:10Z

squaremo

Looks like it'll work! May I request that we put this behaviour behind another flag, so it is opt-in, to avoid surprises: I suggest --registry-insecure-tls-skip-verify.

squaremo · 2018-12-11T17:43:25Z

@Timer I added that flag -- mind seeing if it works for your setup, when you have a sec?

I was thinking maybe the right thing to do is to act like docker, that is, for an insecure host:

try HTTPS, but with TLS insecure skip verify
if that fails, use HTTP but don't send credentials

I wonder if that would break some private registry setups ...

donifer · 2018-12-27T18:56:47Z

Any updates on this? I think I'm hitting this issue using an insecure registry when polling images.

err="Get https://registry.internal.mydomain.com/v2/: x509: certificate is valid for ingress.local, not registry.internal.mydomain.com

squaremo · 2018-12-31T14:04:52Z

@donifer Mayyyybe -- that looks more like you're using a registry host with HTTPS, and it has an invalid certificate. That's a closely related problem, but not solved by this PR as it stands -- it would need the alternate formulation I posted above (that is: for insecure hosts, try https:// with TLS_INSECURE_SKIP_VERIFY first, and fall back to HTTP if that doesn't connect).

Nonetheless, I've pushed a build from this branch to quay.io/squaremo/flux:insecure-host-behavior-ad833b9, if you want to try it out. (NB you need to supply the new flag to fluxd: --registry-tls-insecure-skip-verify)

cazzoo · 2019-01-07T16:20:42Z

Looking to this fix

squaremo · 2019-01-08T17:39:16Z

@Timer @donifer @cazzoo I've implemented this idea:

try HTTPS, but with TLS insecure skip verify

if that fails, use HTTP but don't send credentials

i.e., to use your own registry, you now have the option of using a self-signed certificate (or HTTP but no authentication!) so long as you supply the hostname with --registry-insecure-host.

If you have the chance to test this out in a controlled environment, I would appreciate feedback. I've pushed a build from this branch to quay.io/squaremo/flux:insecure-host-behavior-096bd47.

We've since made more changes

2opremio · 2019-01-18T22:26:45Z

registry/client_factory.go

-	tx = transport.NewTransport(tx, auth.NewAuthorizer(manager, tokenHandler, basicauthHandler))
+	authHandlers := []auth.AuthenticationHandler{}
+	// only send creds over HTTPS
+	if registryURL.Scheme == "https" {


I think this is too restrictive and opinionated. For instance, one may intentionally want to use http in a sealed environment in which the registry just happens to require credentials for reasons beyond your control.

Also, it is almost equally unsafe to send the creds over HTTPS when the certificate verification is being skipped

Yeah those two points are fair. So I guess the recommendation would be to

only fall back to http if the domain is flagged as insecure

only use TLS_INSECURE_SKIP_VERIFY if the domain is flagged as insecure

always use credentials if you have them

on the basis that cases 1. and 2. are basically equivalent, and if you've flagged the domain as insecure, you are accepting the risk of a MitM attack in either situation -- ?

Yep, that sounds good

^^ @Timer @donifer @cazzoo Just to bring this significant change to your attention, since you've indicated this PR may be of interest to you (e.g., by creating it)

2opremio · 2019-01-18T22:42:18Z

registry/client_factory.go

+			insecure = true
+			break
+		}
+	}


The size of this function is getting out of hand. I would suggest factoring out the challenge obtention (or part of it at least)

2opremio · 2019-01-18T22:44:06Z

registry/client_factory.go

 		Host:   repo.Domain,
 		Path:   "/v2/",
 	}

 	// Before we know how to authorise, need to establish which
 	// authorisation challenges the host will send. See if we've been
 	// here before.
+	attemptInsecureFallback := insecure
+attempt:


Maybe make the label more descriptive? (e.g. schemeAttempt or attemptScheme)

2opremio · 2019-01-18T22:46:47Z

cmd/fluxd/main.go

 		registryPollInterval = fs.Duration("registry-poll-interval", 5*time.Minute, "period at which to check for updated images")
 		registryRPS          = fs.Float64("registry-rps", 50, "maximum registry requests per second per host")
 		registryBurst        = fs.Int("registry-burst", defaultRemoteConnections, "maximum number of warmer connections to remote and memcache")
 		registryTrace        = fs.Bool("registry-trace", false, "output trace of image registry requests to log")
-		registryInsecure     = fs.StringSlice("registry-insecure-host", []string{}, "use HTTP for this image registry domain (e.g., registry.cluster.local), instead of HTTPS")
+		registryInsecure     = fs.StringSlice("registry-insecure-host", []string{}, "let these registry hosts skip TLS host verification, and fall back to HTTP (without basic auth) instead of HTTPS; this allows man-in-the-middle attacks, so use with extreme caution")


Is there something we could break in existing installations using the flag by changing its behavior?

We are attempting http if https doesn't work, so, I don't think so, but it's worth giving it some further thought.

Alright, there is something which can break: we are now skipping auth, which I am against, but if we really want to skip basic auth with http, we should at least create a new flag and deprecate this one.

Update: if we're not skipping auth (as agreed above), I don't think this will be a breaking or even surprising change; if you already had a domain listed as insecure, it's because you wanted it to use HTTP (including sending credentials); instead, it will attempt HTTPS and fallback to HTTP, still sending creds. The change is now you can also use it if you want it to use HTTPS with a self-signed cert.

cazzoo · 2019-01-22T19:45:08Z

That kind of solution will be enough for my case, I'm actually using self-signed certificate and would like to use HTTPS anyway. Actually using http as hotfix but this is unwanted here.
Looking for merge to next release

squaremo · 2019-01-24T17:42:50Z

@2opremio I've implemented the "always send creds" scheme we discussed, and split the long method up. PTAL!

We accept a list of image registry hosts for which to use HTTP; in some cases, people also want to switch off TLS host certificate verification, e.g., they have a setup in which the image manifests are on a host using TLS but with a self-signed cert. This extra flag allows people to switch TLS host cert verification off (it's a bad idea to do that by default) explicitly, when the registry is listed as an insecure host.

This commit changes the meaning of --registry-insecure-host to match what Docker considers an insecure host: - the TLS host certificate is not verified - if it can't connect with HTTPS, it'll fall back to HTTP - no credentials are sent over HTTP This gives a better path to using a local registry, since it's relatively easy to set things up so it can use TLS; just a pain to set TLS up with a valid host certificate. Not sending credentials over HTTP is also a little more secure by default -- previously, an insecure host would have used HTTP straight away, _and_ sent credentials over it.

HTTPS without host cert verification is vulnerable to MitM attacks as HTTP; so, accept that marking a host as insecure is just a risk, and send credentials unconditionally.

This mainly just to keep method length a little more managable.

Timer force-pushed the insecure-host-behavior branch from e82fe1e to c0cac76 Compare November 17, 2018 02:27

squaremo previously requested changes Nov 21, 2018

View reviewed changes

squaremo force-pushed the insecure-host-behavior branch from ad833b9 to 096bd47 Compare January 8, 2019 17:34

squaremo requested a review from 2opremio January 17, 2019 17:11

2opremio suggested changes Jan 18, 2019

View reviewed changes

squaremo mentioned this pull request Jan 23, 2019

Allow specifying registry certificate authority #1682

Closed

michaelfig mentioned this pull request Jan 23, 2019

WIP: Modify insecure hosts to first try HTTPS with no CA verfication. #1685

Closed

squaremo force-pushed the insecure-host-behavior branch from 45275ad to 5fa45ea Compare January 24, 2019 17:40

Timer and others added 5 commits February 4, 2019 11:05

Skip TLS check when using insecure host

390365f

Allow credentials to be sent over HTTP after all

bffc54a

HTTPS without host cert verification is vulnerable to MitM attacks as HTTP; so, accept that marking a host as insecure is just a risk, and send credentials unconditionally.

Factor out the doing of the registry challenge

fe5963d

This mainly just to keep method length a little more managable.

squaremo force-pushed the insecure-host-behavior branch from 5fa45ea to fe5963d Compare February 4, 2019 12:20

2opremio approved these changes Feb 4, 2019

View reviewed changes

squaremo merged commit 737815d into fluxcd:master Feb 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip TLS check when using insecure host #1526

Skip TLS check when using insecure host #1526

Timer commented Nov 17, 2018

squaremo left a comment

squaremo commented Dec 11, 2018

donifer commented Dec 27, 2018

squaremo commented Dec 31, 2018

cazzoo commented Jan 7, 2019

squaremo commented Jan 8, 2019 •

edited

Loading

2opremio Jan 18, 2019

squaremo Jan 22, 2019

2opremio Jan 22, 2019

squaremo Jan 22, 2019 •

edited

Loading

2opremio Jan 18, 2019

2opremio Jan 18, 2019

2opremio Jan 18, 2019

2opremio Jan 18, 2019

squaremo Jan 22, 2019

cazzoo commented Jan 22, 2019

squaremo commented Jan 24, 2019

Skip TLS check when using insecure host #1526

Skip TLS check when using insecure host #1526

Conversation

Timer commented Nov 17, 2018

squaremo left a comment

Choose a reason for hiding this comment

squaremo commented Dec 11, 2018

donifer commented Dec 27, 2018

squaremo commented Dec 31, 2018

cazzoo commented Jan 7, 2019

squaremo commented Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squaremo Jan 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cazzoo commented Jan 22, 2019

squaremo commented Jan 24, 2019

squaremo commented Jan 8, 2019 •

edited

Loading

squaremo Jan 22, 2019 •

edited

Loading