Add proxyprotocol directive and listener middleware plugin type #1349

mastercactapus · 2017-01-12T22:10:33Z

This PR adds PROXY protocol v1 support to Caddy as per issue #852

The PROXY protocol provides a convenient way to safely transport connection
information such as a client's address across multiple layers of NAT or TCP
proxies. It is designed to require little changes to existing components and
to limit the performance impact caused by the processing of the transported
information.

A PROXY header is sent as the first chunk of data sent over a socket, and looks like this (from docs.aws.amazon.com):
PROXY TCP4 198.51.100.22 203.0.113.7 35646 80\r\n

Afterwards, the connection behaves as it otherwise would -- there is no further negotiation. (HTTP, TLS, etc, operates as normal)

Support in Caddy is configured with two new flags:
-proxy-protocol-subnets -- A list of CIDR ranges to accept PROXY headers from
-proxy-protocol-timeout -- A timeout to receive said headers

When 1 or more subnets are specified, the listener is wrapped with one that returns a wrapped net.Conn, where RemoteAddr will return the address provided in the PROXY header -- if the socket RemoteAddr is within one or more of the specified subnets.

If the header is not sent (e.g. a normal GET request) the server will behave as it does without this functionally (i.e. graceful fallback).
A PROXY header with invalid content will be rejected
It has also has been tested with the realip directive -- allowing a CDN to set X-Forwarded-For while behind an AWS ELB, for example.

Relevant links (thanks @scalp42):

CLAassistant · 2017-01-12T22:10:34Z

All committers have signed the CLA.

mholt

Thanks for working on this. I think I have more to say (related to command line flags, probably) but I have to go now and this should get you started.

How would you feel about making this a plugin for us? Caddy (or Caddy's HTTP server -- not sure which yet, feedback welcome) will need to be slightly extended to support plugins at the listener level, but that ought not be too difficult. Basically a function that takes a listener and returns a listener. What do you think?

I mainly ask because this technology seems more proprietary than standard, and I'm not 100% convinced I want it to stay in Caddy core.

mholt · 2017-01-14T00:38:08Z

caddyhttp/httpserver/server.go

+		// that care about RemoteAddr, we should be ok.
+		fln, err := newFilteredPROXYListener(ln, ProxyProtocolTimeout, ProxyProtocolSubnets)
+		if err != nil {
+			fmt.Printf("[ERROR] configuring Proxy Protocol support: %v\n", err)


I think this error should be a show-stopper (why would you want it to continue without proxy protocol support -- I assume if you enable it it's because your service depends on it), and Serve() must not return errors before serving begins. Move this wrapping of the listener up to the Listen() method instead, and make the error return. (If you have good reasoning to simply make this a warning, let me know.)

I agree. I actually had an issue today where kubernetes started a pod (using a different plugin) that didn't initialize properly, and it took some wasted time to see that it shouldn't have started in the first place. Lesson learned, and I think it applies here.

mholt · 2017-01-14T00:38:30Z

caddyhttp/httpserver/server_test.go

+	return f(w, r)
+}
+
+func TestServer_Serve_ProxyProtocol(t *testing.T) {


TestServerServeProxyProtocol

mholt · 2017-01-14T00:39:28Z

caddyhttp/httpserver/server_test.go

+
+func (f testHandleFunc) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) {
+	return f(w, r)
+}


Use HandlerFunc instead.

mholt · 2017-01-14T00:40:46Z

caddyhttp/httpserver/proxyprotocol.go

+	subnets []*net.IPNet
+}
+
+func newFilteredPROXYListener(ln net.Listener, headerTimeout time.Duration, subnets string) (*filteredPROXYListener, error) {


I know this is unexported (which is good), but I think this should still be godoc-commented for clarity. What does this method do?

Also document the filteredPROXYListener type above.

mastercactapus · 2017-01-14T03:12:39Z

A plugin should be doable, though I think it will be a little more complicated than simply wrapping it. From what I see in server.go, care has to be taken to preserve the TCP keep-alive functionality as well as the caddy.Listener interface for graceful restarts. We'd also have to decide if that's a hard-dependency (e.g. plugins HAVE to implement caddy.Listener or if it's optional) for listener-type plugins. As a caddy plugin, we could probably make that requirement.

Having said that, it could be a pretty powerful arrangement. I kept the changes to the existing httpserver to a minimum for now though. I tried a couple of different approaches and they got too in-the-weeds trying to go around the struct-type-assertions for the TCP stuff, until I landed on where it is now with c94366f.

If we do make this a plugin, would you see a directive that behaves similar to tls? Meaning, it spans multiple listeners, and we validate that all shared listeners are "agreeable" and merge the subnet configs?

mholt · 2017-01-17T17:30:40Z

@mastercactapus Excellent thoughts. Yes, I think it would be a directive like tls, where the configurations have to be agreeable at startup.

I think this is great work. If you don't mind. I'm going to think on this some more -- trying to work through my backlog, honestly -- before merging. If plugins have the ability to wrap listeners, that would be very powerful and useful. Would rather do it one way instead of this way, then switch it to do the other way later.

So yeah, I think we'll drop the command line flags if we do that.

mholt · 2017-01-21T17:53:52Z

@mastercactapus In Go 1.8, the http.Server type gets a Shutdown method, which will void our need for a gracefulListener. I don't have a time to get around to it right now, but would you be willing to experiment with a way for plugins to wrap listeners?

I envision a map or slice of some sort where plugins' listener wrapper functions will be stored for a certain server type; they are then iterated in the Listen() method; they would each take in a listener and return a listener. (and maybe they would take in more information as context, I'm not sure yet).

Could be just a few lines of code. What do you think?

mastercactapus · 2017-01-24T15:15:07Z

@mholt sorry for the late response, I'd be happy to make a pass at it. I'm hoping to have some time this weekend

mastercactapus · 2017-01-29T17:57:41Z

@mholt Here is listener plugin support and a plugin implementation, respectively:

master...mastercactapus:listener-plugins
https://github.com/mastercactapus/caddy-proxyprotocol

I still need to validate graceful restarts and whatnot don't break, but a quick local test seemed to work. Also, it only requires a net.Listener interface ATM, but maybe listener middleware should require a caddy.Listener interface instead. I'll look to you to chime in on that one.

I'm out of time this weekend (going ice fishing) but I'll try to get back to it this week. If you get a chance, let me know your thoughts.

Going forward, I think I'll update this PR to add "proxyprotocol" to the directives list (for the plugin), and make a new PR for listener plugin support -- once it all looks good.

mholt · 2017-01-29T19:19:44Z

Excellent. Thank you! I won't be available this week, but feel free to continue.

Couple of nit-picks real quick :)

master...mastercactapus:listener-plugins#diff-e74f72185700e304b2047c4af2ad1a48L143 - preserve that comment, this may not be needed after Go 1.8 but for now it's really important.

master...mastercactapus:listener-plugins#diff-6e418c4d8e84bd42679875fa855111abR87 - Rename to RegisterListenerMiddleware, to be consistent with other plugin registration functions.

Looking good so far otherwise!

mastercactapus · 2017-01-31T03:44:45Z

Simple enough changes -- I'll post back once I can do some more thorough testing too.

I did want to mention I eventually settled on "Add" because it felt closer to the existing "AddMiddleware" method for httpserver than the "Register" methods that seemed to deal with directives and extending caddy itself, instead of the httpserver. "Add" felt like the httpserver-API.

Might be worth thinking about what to do with "AddMiddleware" so it's not an outlier, if we go with "Register". (e.g. create "RegisterMiddleware" and deprecate "AddMiddleware" to be consistent or just leave it??)

Though we could probably consider that out-of-scope for this.

// AddMiddleware adds a middleware to a site's middleware stack.
func (s *SiteConfig) AddMiddleware(m Middleware) {
	s.middleware = append(s.middleware, m)
}

// AddListenerMiddleware adds a listener middleware to a site's listenerMiddleware stack.
func (s *SiteConfig) AddListenerMiddleware(l ListenerMiddleware) {
	s.listenerMiddleware = append(s.listenerMiddleware, l)
}

mholt · 2017-02-04T15:46:07Z

@mastercactapus I see now, thanks for explaining. When I was designing Caddy 0.9, I viewed "plugins" as different than "HTTP middleware", where plugins were "registered" and middleware were "added" (because not all plugins registered middleware, for example, startup and shutdown directives) -- but in this case, the middleware is the plugin. I guess either is fine. We can keep it as Add.

I will review this again shortly!

mastercactapus · 2017-02-05T20:48:37Z

@mholt I'm thinking it may be best to postpone the listener middleware support to Go 1.8 -- with the changes from your go18shutdown branch.

My reasoning is that in testing I found that the code in master has a race condition with the gracefulListener, leading to a rare panic under load & graceful restart. Accept() can call Add() on the WaitGroup after Wait() is called during shutdown. I was about to open an issue, however, since that code is being replaced so soon, perhaps it's not worth fixing and just waiting until Go 1.8. What do you think?

Otherwise, the listener middleware doesn't seem to add or exacerbate the issue, but I'd like to get the zero-error warm-and-fuzzies with the addition.

If that sounds good (basing it off of master+go18shutdown) I'll get everything set and tested there -- and get the new PR for the listener middleware opened.

mholt · 2017-02-05T21:02:54Z

A race huh? That is unusual, I haven't seen that one (after running it hundreds of times in various loads with the race detector).

I'm okay with waiting. Judging by my current backlog, I won't be able to merge this until 1.8 is out anyway (which is any day now, maybe this or next week I think).

You don't have to open a new PR, you can force push to this branch if you want to clean up the commits.

mastercactapus · 2017-02-05T21:20:49Z

I ran it with the race detector, killall -USR1 caddy in a while loop, while under load. It took about 10-20 seconds of that before the race hit sometimes (at 5k/req/sec). I got lucky the first time it happened though.

Here's the log if you're interested: https://gist.github.com/mastercactapus/dd1292ff2636386869115ea32649e049

I'll work on setting it up for 1.8. Am I correct in assuming this (the actual PROXY protocol support, not the listener middleware) is to be third-party/addon? Or should it be in core under caddyhttp/proxyprotocol?

…ins-go18

mholt · 2017-02-05T22:08:02Z

Thanks for the log. Yes, the PROXY protocol support will be a plugin; the listener middleware change will be in core so that it becomes a new plugin type. Yours will be the first plugin of its type.

mholt · 2017-02-16T00:56:01Z

@mastercactapus Are there other changes you want to make on this (Go 1.8 should be coming out this week, maybe tomorrow!? 🙌 ) or should I begin a review?

mastercactapus · 2017-02-16T01:49:15Z

@mholt There is one outstanding question left: should we make it strictly require caddy.Listener instead of net.Listener?

If we don't need caddy.Listener anymore (for Go 1.8+) then we should remove the type assertion on line 137
eaf987c#diff-e74f72185700e304b2047c4af2ad1a48R137

I know there was a request I ran across somewhere (#814) about gracefully restarting caddy itself (to update the binary) -- to do that I think we would need .File -- unless we find a nice way to listen with SO_REUSEPORT.

If we do wanna keep .File, then perhaps we should update the signature of AddListenerMiddleware to accept and return caddy.Listener instead of net.Listener. OR maybe stick with net.Listener and wrap it inside Server.Listen

mholt · 2017-02-16T04:49:39Z

@mastercactapus Ah, no, we still need caddy.Listener for restarts. The distinction between in-process restarts and restarting the process is separate from needing access to the underlying file descriptor: all restarts need access to that file descriptor so we can get graceful restarts, both in- and out-of-process. (But now Caddy does in-process restarts; eventually we will do both again when we support seamless upgrades of the binary.)

I think using caddy.Listener is a good idea for Caddy listener middleware. It's just a net.Listener with a way to get at the underlying file descriptor, and implementations could even wrap (embed) a concrete caddy.Listener if they wanted to. But we definitely need the File().

mholt · 2017-02-16T05:17:08Z

caddyhttp/httpserver/middleware.go

@@ -21,7 +22,7 @@ type (

 	// ListenerMiddleware is similar to the Middleware type, except it
 	// chains one net.Listener to the next.
-	ListenerMiddleware func(net.Listener) net.Listener
+	ListenerMiddleware func(caddy.Listener) caddy.Listener


I can't remember, did we already talk about whether this should be defined and implemented here in the httpserver package or whether it should be in the caddy package directly? Would it make sense to register listener wrappers/middlewares for any/all server types or just a specific one (like HTTP) at a time?

Hmm. I don't recall. I could see a case either way. Anything using TCP could make use of the proxyprotocol middleware, I haven't dug in to see how/where to implement it.

Are you thinking the middleware stack would live on caddy, and server types (like caddyhttp) would opt-in and use them?

Are you thinking the middleware stack would live on caddy, and server types (like caddyhttp) would opt-in and use them?

Yes to the first part, and no to the second part because it's unnecessary (except for having the directive in its list of recognized directive). Remember, the HTTP server is given a Listener as input when it's time to Serve. So I think we might be able to move the listener middleware plugin functionality to the caddy package, because it can wrap the listeners there and then just pass the resulting Listener into Serve().

Right?

I mean, we could try it. I don't see why we would have to limit this to just the HTTP server.

At serve time it does, but httpserver provides the initial Listen:
https://github.com/mastercactapus/caddy/blob/9107466685fafa24fc87bb5c7cd99e5da7f0dfc7/caddyhttp/httpserver/server.go#L104

I think that's because the server type could want UDP, or TCP, etc.. DNS vs HTTP for example. Breaking that apart could yield some interesting use cases though. Like getting a net.Conn some other way, like Chrome's socket API. gopherjs is still the only way I know of to run a full http stack client-side in the browser. Believe it or not, I've done that in the past to solve a problem. Though, not sure if we should target Caddy to that :)

Anyway, if we do move it, I think we'd also have to be careful not to break APIs. Maybe httpserver Server.Listen internally calls caddy.GetListener(net,addr) or something instead of net.Listen. It could give us the possibility of moving the graceful wrapping stuff to caddy and out of httpserver.

All in all, a few things to consider.

I'm not sure I follow needing something caddy.GetListener() -- if we move this listener plugin code to the caddy package, we just wrap whatever listener we put into Serve(). If the server type supports graceful restarts, it needs to return a caddy.Listener (as you noticed in your change here) but that's its own responsibility.

@mastercactapus Have you had a chance to look into moving it into the caddy package? What do you think? (Again, not sure we need a GetListener function...)

mholt · 2017-02-16T05:18:22Z

caddyhttp/httpserver/server.go

@@ -126,15 +126,16 @@ func (s *Server) Listen() (net.Listener, error) {
 		ln = tcpKeepAliveListener{TCPListener: tcpLn}
 	}

+	cln := ln.(caddy.Listener)


Is this type assertion really needed? A caddy.Listener is also a net.Listener, and there's a type assertion at the return statement.

It's needed to cast the net.Listener into a caddy.Listener for use in the middleware immediately loop below it.

Oh yeah, duh, because the middleware wrapper takes in a caddy.Listener too. 👍

mholt

I think we're getting real close :)

mholt · 2017-03-05T16:47:06Z

caddyhttp/httpserver/server.go

@@ -215,9 +215,20 @@ func (s *Server) Listen() (net.Listener, error) {
 		}
 	}

+	if tcpLn, ok := ln.(*net.TCPListener); ok {
+		ln = tcpKeepAliveListener{TCPListener: tcpLn}
+	}


@mastercactapus This is already done in the call to Serve() on (now-)line 249. Is there a reason we do this twice? I do not think this is desirable...

@mastercactapus Ready to merge other than this. :)

Pulled it out of Serve, since a TCPListener is not necessarily returned from Listen anymore.

mastercactapus · 2017-03-10T01:06:56Z

Cool, I'll take a look tonight. If I remember right, I'll probably take it out of the Serve call, since the tcplistener isn't returned by the listen functions anymore

…

On Thu, Mar 9, 2017, 6:20 PM Matt Holt ***@***.***> wrote: ***@***.**** commented on this pull request. In caddyhttp/httpserver/server.go <#1349 (comment)>: > @@ -215,9 +215,20 @@ func (s *Server) Listen() (net.Listener, error) { } } + if tcpLn, ok := ln.(*net.TCPListener); ok { + ln = tcpKeepAliveListener{TCPListener: tcpLn} + } @mastercactapus <https://github.com/mastercactapus> Ready to merge other than this. :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1349 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAkUQnsM6rsn2RlaMfNr9SNNVuGYfUXmks5rkJdAgaJpZM4LiS6i> .

mholt · 2017-03-10T01:48:01Z

Sounds good to me. Make it so!

This is now done in the Listen() function, along with other potential middleware.

mholt · 2017-03-10T05:31:55Z

Huzzah. Thanks so much for your persistence and flexibility, @mastercactapus ! Really looking forward to seeing sites using this in action. :)

mholt requested changes Jan 14, 2017

View reviewed changes

mholt changed the title ~~Add proxy protocol v1 support~~ Add proxy protocol v1 support and listener middleware plugin type Feb 4, 2017

mastercactapus added 2 commits February 5, 2017 15:30

Merge remote-tracking branch 'origin/go18shutdown' into listener-plug…

fa9f002

…ins-go18

add support for listener middleware

eaf987c

add proxyprotocol directive

a77f339

mastercactapus force-pushed the proxy_protocol branch from c94366f to a77f339 Compare February 14, 2017 05:30

make caddy.Listener interface required

9107466

mholt reviewed Feb 16, 2017

View reviewed changes

mholt changed the title ~~Add proxy protocol v1 support and listener middleware plugin type~~ Add proxyprotocol directive and listener middleware plugin type Feb 16, 2017

mholt mentioned this pull request Feb 21, 2017

Reverse proxy on ASP.NET sites using NTLM authentication is not working #663

Closed

mastercactapus added 2 commits March 4, 2017 09:12

Merge remote-tracking branch 'origin/master' into proxy_protocol

e93ed95

Merge remote-tracking branch 'origin/master' into proxy_protocol

a3918ce

mholt requested changes Mar 5, 2017

View reviewed changes

mastercactapus added 2 commits March 9, 2017 20:36

Merge branch 'master' into proxy_protocol

2042d1a

Remove tcpKeepAliveListener wrapper from Serve()

10c6de5

This is now done in the Listen() function, along with other potential middleware.

mholt approved these changes Mar 10, 2017

View reviewed changes

mholt merged commit 9e4eeb4 into caddyserver:master Mar 10, 2017

Add proxyprotocol directive and listener middleware plugin type #1349

Add proxyprotocol directive and listener middleware plugin type #1349

Conversation

mastercactapus commented Jan 12, 2017

CLAassistant commented Jan 12, 2017 • edited Loading

mholt left a 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

Choose a reason for hiding this comment

mastercactapus commented Jan 14, 2017

mholt commented Jan 17, 2017

mholt commented Jan 21, 2017

mastercactapus commented Jan 24, 2017

mastercactapus commented Jan 29, 2017

mholt commented Jan 29, 2017

mastercactapus commented Jan 31, 2017

mholt commented Feb 4, 2017

mastercactapus commented Feb 5, 2017

mholt commented Feb 5, 2017

mastercactapus commented Feb 5, 2017

mholt commented Feb 5, 2017

mholt commented Feb 16, 2017

mastercactapus commented Feb 16, 2017

mholt commented Feb 16, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mholt left a 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

mastercactapus commented Mar 10, 2017 via email

mholt commented Mar 10, 2017

mholt commented Mar 10, 2017

CLAassistant commented Jan 12, 2017 •

edited

Loading