Use X-Forwared-For when behind reverse-proxy

[mattikus]

Allow the use of X-Forwarded-For if the header exists to allow reverse-proxies and loadbalancers to be set up in front of a bcfg2-server.

So far this diff only applies to the Builtin core. I could investigate applying it to the others as well, if there's interest in this PR.

[stpierre]

This would seem to be a pretty significant security risk. For instance, an attacker who had access to one of your Bcfg2 clients -- or even just to your Bcfg2 communication password -- could simply make requests for the configuration for any other host on your network by setting X-Forwarded-For. In order to mitigate the risk, I'd suggest adding an option that restricts usage of this feature to a list of hosts or IPs -- i.e., a list of the only machines that can forward requests for other hosts. If that option is not present, then the feature is completely disabled.

The other possibility would be to only permit this feature to be used with clients that have their own password set in clients.xml. That way an attacker could not spoof the X-Forwarded-For; they would need the password for the specific client, which securely authenticates that client, not merely the set of all clients. But this approach is rather restrictive and may make this feature more work than it's worth. I'm also not entirely sure how that'd be implemented, since the connection processing happens in SSLServer while the authentication happens in the core and is delegated to Metadata.

Whatever we do, we should add a note to the docs that setting per-server passwords is recommended if this feature is enabled.

[mattikus]

Oh yeah, totally agreed. I think an option is necessary. The whitelist would also be very useful to help determine what is a legitimate connection. This was more of a first draft approach to see if y'all would even be interested in the idea.

Do you have any suggestions on option naming? I would assume it would go in the server block.

Something like:

[server]
behind_proxy = True
proxy_whitelist = 10.0.0.1, 10.0.0.2

[stpierre]

I think you can just use one option; if it's empty, proxying is disabled.
Maybe:

[server]
allow_proxying_from = 10.0.0.0/30

The ClientACL plugin has some CIDR logic you can probably steal for this.
On May 22, 2014 11:46 AM, "Matt Kemp" notifications@github.com wrote:

Oh yeah, totally agreed. I think an option is necessary. The whitelist
would also be very useful to help determine what is a legitimate
connection. This was more of a first draft approach to see if y'all would
even be interested in the idea.

Do you have any suggestions on option naming? I would assume it would go
in the server block.

Something like:

[server]
behind_proxy = True
proxy_whitelist = 10.0.0.1, 10.0.0.2

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/174#issuecomment-43906169
.

[mattikus]

Yeah, that's much cleaner. I'll take a stab at it. Thanks.

[mattikus]

Apparently I have some problems with git and rebasing against master. Forgive any craziness.

Okay, so I took a stab at implementing an option under the [server] stanza and implementing a check based on the ClientACL plugin you pointed me to.

If this is starting to go in a direction you like, I think we should probably abstract out the ip2int and ip_matches functions from that module and into the Utils namespace.

I'll take a stab at writing up some docs as well once we agree on an implementation.

[stpierre]

Do we want to give descriptive error messages here if proxying is rejected either because it's not allowed at all, or because it's not allowed from the host trying to proxy?

[mattikus]

Possibly. I was thinking if it gets the header but it's from a host that isn't allowed to be proxied, it could just be spurious and it might actually be a real request from the client for a config.

I could go either way, though. If it gets a request, which isn't in the approved proxies, and the client doesn't exist in bcfg2 that it's being sent from, does that error out? Isn't that similar to receiving a request from a client that doesn't exist at all, but has the correct auth?

[stpierre]

If there's an X-Forwarded-For header, and proxying is disallowed (for whatever reason -- a single host disallowed or all proxying disallowed), I think we need to produce a sane error, for two reasons:

• It will make it easier to troubleshoot proxying;
• It will avoid any possibility of sending the configuration for one box to another. For instance, if box A requests its configuration via box B, which is not allowed to proxy, if we assume that this is just box B requesting its own configuration with an erroneous X-Forwarded-For, then in the end we return box B's configuration to box A, which is Bad.

[mattikus]

That makes sense. The only wrinkle in that is it changes behavior that is
already extant in current versions. Bcfg2 currently ignores any headers
besides standard ones like auth related ones. I don't know if that would
break current workflows, though I would it odd. I guess someone could have
connectivity only via one node in a cluster and are attempting to assign
the same configuration to all other nodes who use that node as a proxy.

What's the best way to trigger an error here? Just raise an exception and
have something higher catch it?

[stpierre]

Probably the best thing is to log an error and send a 4xx-series error code, e.g.:

msg = "So proxy much error wow"
self.logger.error(msg)
self.send_error(400, msg)

You might prefer 401 or another error. In the end XML-RPC isn't as neurotic about HTTP status codes as REST.

[stpierre]

I don't see anything in those functions that would prevent us from moving them into Bcfg2.Utils, so that sounds like a good approach to me.

[mattikus]

Cool, I'll refactor those functions into Util and import from there. I'll write up docs for the config and make sure they're included properly.

[mattikus]

I think this fixes up the reorganization and the error handling bits.

Still to do:

• Documentation
• Make it work with the cherrypy backend
• Test that it works at scale

Anything else you can think of?

[stpierre]

I have little confidence that the CherryPy backend works -- it hasn't seen any love since it was apparent that it was going to be a while before CherryPy deigned to fix the massive showstopping bug we hit -- so I'd suggest just adding a comment to it to suggest to whoever finally tunes it up adds your stuff in. Unless you a) are a glutton for punishment; or b) want to make the CherryPy backend work. :)

Other than that, sounds good.

[mattikus]

Sounds good to me. I can be a bit of a glutton and I already looked into it a little. It's not quite as simple as the SSLServer bits (which I think just work on multiprocessing mode since it still calls the same code), but it wasn't terribly awful, either. I just had a little bit of trouble getting the cherrypy backend working at all when I briefly looked at it.

As far as Docs go, where should it go? Do we want to just make it a blurb in the configuration options section for bcfg2.conf or should there more information? I think at first the former, just to get it out, then if I can prove that this works, maybe a doc on bcfg2 horizontal scaling that uses it?

[stpierre]

I think that sounds good. I'd probably put it in doc/server/configuration.txt, and link to it from doc/appendix/guides/{authentication,nat_howto}.txt.

In the medium term we desperately need a horizontal scaling document, but that doesn't need to be part of this change.

[AlexanderS]

Anything new on this topic? As far as I can see the TODO items from above are still open?

[mattikus]

Apologies, I no longer work somewhere where Bcfg2 is used and have since not had a need to continue work on this PR.

It was mainly meant for scaling the environment at my previous job. It probably needs refactoring and rebasing against the latest stable and some documentation on the parameters you'd pass.

Feel free to close this PR as well, as I do not believe I'll have the time or interest to finish it.