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

Add https upstream support #257

Merged
merged 2 commits into from
Apr 5, 2017
Merged

Add https upstream support #257

merged 2 commits into from
Apr 5, 2017

Conversation

tmessi
Copy link
Contributor

@tmessi tmessi commented Apr 4, 2017

This seems to work for me. I generally have a internal root CA that is trusted via standard system certs. The skipverify option could be used to deal with self-signed certs, but would obviously skip verification.

I was also thinking of maybe having another certificate store for CAs that could then be selected via a tag option. If that seems like a good idea I can make another PR or amend this one.

Ref: #181

@CLAassistant
Copy link

CLAassistant commented Apr 4, 2017

CLA assistant check
All committers have signed the CLA.

@magiconair
Copy link
Contributor

@shadowfax-chc Thanks for working on this but this isn't correct.

There are two TCP connections in play here: one inbound and one outbound/upstream. The listener is the inbound TCP connection and the proxy is the outbound TCP connection. To allow outbound https connections you only need to set the protocol in the target URL which your first patch accomplishes. The Go std library should do the rest.

This requires however that the certificates can be validated through the default root CAs as configured on the system. If you want to allow certificate verification with a custom certificate source then this must be configured in the route command/urlprefix- tag.

Lets start with supporting skipVerify on the outbound connection since this is rather simple and should solve most of the problems. Supporting custom root CAs would require to either override the http.Client that is used by the reverse proxy (which isn't possible ATM) or use a custom transport per request (which defeats connection caching). This most likely requires a somewhat larger refactor.

To support skipVerify you need to do several things:

  1. Add a TLSSkipVerify bool flag to the route.Target and set it in the route.addTarget method in route/route.go
  2. Create another http.Transport which has the InsecureSkipVerify flag set to true and store it in the HTTPProxy.
  3. Pick which transport to use in the HTTPProxy.ServeHTTP method based on the flag in the selected target.

Once we find a way to have a custom client/transport per request this solution can easily be refactored.

You don't have to (and shouldn't) configure the Listener since this is for the inbound connection. skipVerify may be useful here as well but that's a different thing.

Also, please provide an integration test for both cases in proxy/http_integration_test.go.

@tmessi
Copy link
Contributor Author

tmessi commented Apr 4, 2017

Ah, that makes sense. Let me make sure I am setting this option in the correct place now, and I will get some tests.

route/route.go Outdated
@@ -70,6 +71,13 @@ func (r *Route) addTarget(service string, targetURL *url.URL, fixedWeight float6
}
if r.Opts != nil {
t.StripPath = r.Opts["strip"]

tlsskipverify := false
Copy link
Contributor

Choose a reason for hiding this comment

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

This is enough since this is also in line with the option parser in config

t.TLSSkipVerify := r.Opts["skipverify"] == "true"

@@ -20,6 +20,8 @@ type HTTPProxy struct {
// The proxy will panic if this value is nil.
Transport http.RoundTripper

InsecureTransport http.RoundTripper
Copy link
Contributor

Choose a reason for hiding this comment

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

Pls add a comment.

route/target.go Outdated
@@ -32,4 +32,7 @@ type Target struct {

// timerName is the name of the timer in the metrics registry
timerName string

Copy link
Contributor

Choose a reason for hiding this comment

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

"TLSSkipVerify disables certificate validation for upstream TLS connections."

Could you also move the variable below the StripPath variable, please?

main.go Outdated
@@ -101,6 +101,17 @@ func newHTTPProxy(cfg *config.Config) http.Handler {
KeepAlive: cfg.Proxy.KeepAliveTimeout,
}).Dial,
},
InsecureTransport: &http.Transport{
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe move this into a local newTransport := func(tlscfg *tls.Config) http.Transport func to avoid the duplication.

@magiconair
Copy link
Contributor

Oh, I can actually change the files myself now. Interesting. Didn't know that.

@magiconair
Copy link
Contributor

I'll try that on the next PR, though.

tmessi added 2 commits April 4, 2017 18:12
If `proto=https` option is set on a route, set the protocal to https
@tmessi
Copy link
Contributor Author

tmessi commented Apr 4, 2017

@magiconair made the requested changes and added tests

@magiconair magiconair merged commit 1a267ce into fabiolb:master Apr 5, 2017
@magiconair
Copy link
Contributor

LGTM. Merged it. Thank you!

magiconair added a commit that referenced this pull request Apr 10, 2017
Update fabio.properties documentation
@tmessi tmessi deleted the https-upstream-support branch April 11, 2017 01:57
@magiconair magiconair added this to the 1.4.2 milestone Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants