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

Use X-Forwared-For when behind reverse-proxy #174

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

mattikus
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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" [email protected] 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
Copy link
Contributor Author

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

@mattikus
Copy link
Contributor Author

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.

entry = {"address": address, "netmask": mask}
if ip_matches(client_address[0], entry):
client_address = (x_forwarded_for, client_address[1])
break
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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.

@mattikus mattikus force-pushed the reverse-proxy-fix branch 2 times, most recently from 787029d to d1c2a98 Compare October 24, 2014 23:54
This appears to not be necessary in the scope and could be a leftover.
If X-Forwarded-For header exists, use it as the source of the client rather
than the originating address.
This incorporates the configuration option allowed_proxy_from. It will take a list of hosts or CIDR ranges. If the setting contains anything and the connecting host is inside one of those ranges, allow the connection as if the contents of 'X-Forwarded-For' header were the actual host, otherwise just pass it through as if nothing happened.
@AlexanderS
Copy link
Member

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

@mattikus
Copy link
Contributor Author

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.

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