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.

Sign up for GitHub

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
+65 −33
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f1fa21b
Removed extraneous client_address assignment.
mattikus May 22, 2014
70f5006
Use X-Forwarded-For header for RPC calls.
mattikus May 22, 2014
f75d885
Add option for allowing reverse proxies.
mattikus Jun 10, 2014
20a67ba
Check for allowed reverse proxies.
mattikus Jun 10, 2014
37843c5
Add error check in case proxying disallowed by server.
mattikus Jun 21, 2014
f55766e
Refactor slightly for better flow.
mattikus Jun 21, 2014
9e6e3d8
Move ip_matches and ip2int to Bcfg2.Utils.
mattikus Jun 22, 2014
debf4f4
Add blank lines to make tests pass.
mattikus Jun 22, 2014
ac284cc
One extra line too many for tests passing.
mattikus Jun 22, 2014
d4e05b5
Ensure session stickiness when using a load balanced configuration.
mattikus Oct 24, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/lib/Bcfg2/Client/Proxy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ def __init__(self, key=None, cert=None, ca=None,
self.scns = scns
self.timeout = timeout
self.protocol = protocol
self.cookie = None

def make_connection(self, host):
host, self._extra_headers = self.get_host_info(host)[0:2]
Expand All @@ -258,6 +259,8 @@ def request(self, host, handler, request_body, verbose=0):
try:
conn = self.send_request(host, handler, request_body, False)
response = conn.getresponse()
if not self.cookie:
self.cookie = response.getheader('Set-Cookie')
errcode = response.status
errmsg = response.reason
headers = response.msg
Expand Down Expand Up @@ -285,6 +288,8 @@ def send_request(self, host, handler, request_body, debug):
xmlrpclib.Transport.send_request(self, conn, handler, request_body)
self.send_host(conn, host)
self.send_user_agent(conn)
if self.cookie:
conn.putheader('Cookie', self.cookie)
self.send_content(conn, request_body)
return conn
# pylint: enable=E1101
Expand Down
6 changes: 5 additions & 1 deletion src/lib/Bcfg2/Server/Core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1393,7 +1393,11 @@ class NetworkCore(Core):
Bcfg2.Options.Option(
cf=('server', 'group'), default=0, dest='daemon_gid',
type=Bcfg2.Options.Types.groupname,
help="Group to run the server daemon as")]
help="Group to run the server daemon as"),
Bcfg2.Options.Option(
cf=('server', 'allow_proxying_from'), default=[],
dest='daemon_allowed_proxies', type=Bcfg2.Options.Types.comma_list,
help="Hosts or CIDR ranges that are allowed as reverse proxies")]

def __init__(self):
Core.__init__(self)
Expand Down
28 changes: 1 addition & 27 deletions src/lib/Bcfg2/Server/Plugins/ACL.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
""" Support for client ACLs based on IP address and client metadata """

import os
import struct
import socket
import Bcfg2.Server.Plugin
from Bcfg2.Utils import ip_matches


def rmi_names_equal(first, second):
Expand Down Expand Up @@ -35,31 +34,6 @@ def rmi_names_equal(first, second):
return True


def ip2int(ip):
""" convert a dotted-quad IP address into an integer
representation of the same """
return struct.unpack('>L', socket.inet_pton(socket.AF_INET, ip))[0]


def ip_matches(ip, entry):
""" Return True if the given IP matches the IP or IP and netmask
in the given ACL entry; False otherwise """
if entry.get("netmask"):
try:
mask = int("1" * int(entry.get("netmask")) +
"0" * (32 - int(entry.get("netmask"))), 2)
except ValueError:
mask = ip2int(entry.get("netmask"))
return ip2int(ip) & mask == ip2int(entry.get("address")) & mask
elif entry.get("address") is None:
# no address, no netmask -- match all
return True
elif ip == entry.get("address"):
# just a plain ip address
return True
return False


class IPACLFile(Bcfg2.Server.Plugin.XMLFileBacked):
""" representation of ACL ip.xml, for IP-based ACLs """
__identifier__ = None
Expand Down
30 changes: 26 additions & 4 deletions src/lib/Bcfg2/Server/SSLServer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@
import ssl
import threading
import time
import Bcfg2.Options
from Bcfg2.Compat import xmlrpclib, SimpleXMLRPCServer, SocketServer, \
b64decode
from Bcfg2.Utils import ip_matches


class XMLRPCACLCheckException(Exception):
Expand Down Expand Up @@ -237,6 +239,28 @@ def parse_request(self):
return True

def do_POST(self):
client_address = self.client_address

# check for allowed reverse proxies
allowed_proxies = Bcfg2.Options.setup.daemon_allowed_proxies
if "X-Forwarded-For" in self.headers:
x_forwarded_for = self.headers["X-Forwarded-For"].split(",")[0]

if not allowed_proxies:
msg = "X-Forwarded-For header specified but proxying disallowed"
self.logger.error(msg)
self.send_error(400, msg)
return

for proxy in allowed_proxies:
try:
address, mask = proxy.split("/", 1)
except ValueError:
address, mask = (proxy, None)
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.

try:
max_chunk_size = 10 * 1024 * 1024
size_remaining = int(self.headers["content-length"])
Expand All @@ -252,16 +276,15 @@ def do_POST(self):
if data is None:
return # response has been sent

response = self.server._marshaled_dispatch(self.client_address,
data)
response = self.server._marshaled_dispatch(client_address, data)
if sys.hexversion >= 0x03000000:
response = response.encode('utf-8')
except XMLRPCACLCheckException:
self.send_error(401, self.responses[401][0])
self.end_headers()
except: # pylint: disable=W0702
self.logger.error("Unexpected dispatch error for %s: %s" %
(self.client_address, sys.exc_info()[1]))
(client_address, sys.exc_info()[1]))
try:
self.send_response(500)
self.send_header("Content-length", "0")
Expand All @@ -273,7 +296,6 @@ def do_POST(self):
raise
else:
# got a valid XML RPC response
client_address = self.request.getpeername()
try:
self.send_response(200)
self.send_header("Content-type", "text/xml")
Expand Down
29 changes: 28 additions & 1 deletion src/lib/Bcfg2/Utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@
import re
import select
import shlex
import sys
import socket
import struct
import subprocess
import sys
import threading
from Bcfg2.Compat import input, any # pylint: disable=W0622

Expand Down Expand Up @@ -330,3 +332,28 @@ def __init__(self, getter):

def __get__(self, instance, owner):
return self.getter(owner)


def ip2int(ip):
""" convert a dotted-quad IP address into an integer
representation of the same """
return struct.unpack('>L', socket.inet_pton(socket.AF_INET, ip))[0]


def ip_matches(ip, entry):
""" Return True if the given IP matches the IP or IP and netmask
in the given ACL entry; False otherwise """
if entry.get("netmask"):
try:
mask = int("1" * int(entry.get("netmask")) +
"0" * (32 - int(entry.get("netmask"))), 2)
except ValueError:
mask = ip2int(entry.get("netmask"))
return ip2int(ip) & mask == ip2int(entry.get("address")) & mask
elif entry.get("address") is None:
# no address, no netmask -- match all
return True
elif ip == entry.get("address"):
# just a plain ip address
return True
return False