Add proxyprotocol directive and listener middleware plugin type #1349

Open
wants to merge 5 commits into
from

Projects

None yet

3 participants

@mastercactapus
Collaborator

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

From haproxy.org:

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
CLAassistant commented Jan 12, 2017 edited

CLA assistant check
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.

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)
@mholt
mholt Jan 14, 2017 Owner

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.)

@mastercactapus
mastercactapus Jan 14, 2017 Collaborator

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.

caddyhttp/httpserver/server_test.go
+
+func (f testHandleFunc) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) {
+ return f(w, r)
+}
caddyhttp/httpserver/server_test.go
+ return f(w, r)
+}
+
+func TestServer_Serve_ProxyProtocol(t *testing.T) {
@mholt
mholt Jan 14, 2017 Owner

TestServerServeProxyProtocol

caddyhttp/httpserver/proxyprotocol.go
+ subnets []*net.IPNet
+}
+
+func newFilteredPROXYListener(ln net.Listener, headerTimeout time.Duration, subnets string) (*filteredPROXYListener, error) {
@mholt
mholt Jan 14, 2017 Owner

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
Collaborator

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
Owner
mholt commented Jan 17, 2017

@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
Owner
mholt commented Jan 21, 2017

@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
Collaborator

@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

@mholt Replace our old faithful gracefulListener with Go 1.8's Shutdown()
139a3cf
@mastercactapus
Collaborator

@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
Owner
mholt commented Jan 29, 2017

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
Collaborator

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 mholt changed the title from Add proxy protocol v1 support to Add proxy protocol v1 support and listener middleware plugin type Feb 4, 2017
@mholt
Owner
mholt commented Feb 4, 2017

@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
Collaborator

@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
Owner
mholt commented Feb 5, 2017

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
Collaborator

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?

@mholt
Owner
mholt commented Feb 5, 2017

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.

@mastercactapus mastercactapus add proxyprotocol directive
a77f339
@mholt
Owner
mholt commented Feb 16, 2017

@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
Collaborator

@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
Owner
mholt commented Feb 16, 2017

@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().

@mastercactapus mastercactapus make caddy.Listener interface required
9107466
@@ -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
@mholt
mholt Feb 16, 2017 Owner

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?

@mastercactapus
mastercactapus Feb 16, 2017 Collaborator

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?

@mholt
mholt Feb 16, 2017 Owner

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.

@mastercactapus
mastercactapus Feb 16, 2017 Collaborator

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.

@mholt
mholt Feb 17, 2017 Owner

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.

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

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

@mastercactapus
mastercactapus Feb 16, 2017 Collaborator

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

@mholt
mholt Feb 16, 2017 Owner

Oh yeah, duh, because the middleware wrapper takes in a caddy.Listener too. πŸ‘

@mholt mholt changed the title from Add proxy protocol v1 support and listener middleware plugin type to Add proxyprotocol directive and listener middleware plugin type Feb 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment