Skip to content

Commit

Permalink
Merge branch 'regressionfix/xff-issue' into staging/bugfix
Browse files Browse the repository at this point in the history
  • Loading branch information
foosel committed May 13, 2024
2 parents 8783267 + c1f8810 commit 5afbec8
Show file tree
Hide file tree
Showing 19 changed files with 410 additions and 100 deletions.
14 changes: 7 additions & 7 deletions docs/configuration/config_yaml.rst
Original file line number Diff line number Diff line change
Expand Up @@ -917,18 +917,18 @@ Use the following settings to configure the server:
# (X-Forwarded-Host by default, see above) with forwarded requests.
hostFallback:
# List of trusted downstream servers for which to ignore the IP address when trying to determine
# the connecting client's IP address. If you have OctoPrint behind more than one reverse proxy
# you should add their IPs here so that they won't be interpreted as the client's IP. One reverse
# proxy will be handled correctly by default.
# List of trusted proxy servers for which to ignore the IP address when trying to determine
# the connecting client's IP address. A reverse proxy on the same machine as OctoPrint (e.g. as
# found on OctoPi) will be handled correctly by the default setting of 127.0.0.1 and ::1, further
# proxies in front of that you'll have to add yourself.
trustedDownstream:
- 192.168.1.254
- 192.168.23.42
- 127.0.0.1
- "::1"
# Whether to allow OctoPrint to be embedded in a frame or not. Note that depending on your setup you might
# have to set SameSite to None, Secure to true and serve OctoPrint through a reverse proxy that enables https
# for cookies and thus logging in to work
allowFraming: true
allowFraming: false
# Settings for further configuration of the cookies that OctoPrint sets (login, remember me, ...)
cookies:
Expand Down
6 changes: 1 addition & 5 deletions src/octoprint/plugins/appkeys/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -356,12 +356,8 @@ def on_api_command(self, command, data):
# deprecated key based revoke?
from flask import request

from octoprint.server.util import get_remote_address

self._logger.warning(
"Deprecated key based revoke command sent to /api/plugin/appkeys by {}, should be migrated to use app id/user tuple".format(
get_remote_address(request)
)
f"Deprecated key based revoke command sent to /api/plugin/appkeys by {request.remote_addr}, should be migrated to use app id/user tuple"
)

else:
Expand Down
3 changes: 3 additions & 0 deletions src/octoprint/schema/config/access_control.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ class AccessControlConfig(BaseModel):
autologinAs: Optional[str] = None
"""The name of the user to automatically log on clients originating from `localNetworks` as. Must be the name of one of your configured users."""

autologinHeadsupAcknowledged: bool = False
"""Whether the user has acknowledged the heads-up about the importance of a correct reverse proxy configuration in the presence of autologin."""

trustBasicAuthentication: bool = False
"""Whether to trust Basic Authentication headers. If you have setup Basic Authentication in front of OctoPrint and the user names you use there match OctoPrint accounts, by setting this to true users will be logged into OctoPrint as the user during Basic Authentication. **ONLY ENABLE THIS** if your OctoPrint instance is only accessible through a connection locked down through Basic Authentication!"""

Expand Down
4 changes: 2 additions & 2 deletions src/octoprint/schema/config/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ class ReverseProxyConfig(BaseModel):

portFallback: Optional[str] = None

trustedDownstream: List[str] = []
"""List of trusted downstream servers for which to ignore the IP address when trying to determine the connecting client's IP address. If you have OctoPrint behind more than one reverse proxy you should add their IPs here so that they won't be interpreted as the client's IP. One reverse proxy will be handled correctly by default."""
trustedDownstream: List[str] = ["127.0.0.1", "::1"]
"""List of trusted downstream servers for which to ignore the IP address when trying to determine the connecting client's IP address. A reverse proxy on the same machine as OctoPrint (e.g. as found on OctoPi) will be handled correctly by default, further proxies in front of that you'll have to add yourself."""


@with_attrs_docs
Expand Down
1 change: 1 addition & 0 deletions src/octoprint/server/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,7 @@ def run(self):
util.tornado.fix_json_encode()
util.tornado.fix_websocket_check_origin()
util.tornado.enable_per_message_deflate_extension()
util.tornado.fix_tornado_xheader_handling()

self._setup_mimetypes()

Expand Down
7 changes: 3 additions & 4 deletions src/octoprint/server/api/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
)
from octoprint.server.util.flask import (
get_json_command_from_request,
get_remote_address,
limit,
no_firstrun_access,
passive_login,
Expand Down Expand Up @@ -302,7 +301,7 @@ def login():
if "user" in data and "pass" in data:
username = data["user"]
password = data["pass"]
remote_addr = get_remote_address(request)
remote_addr = request.remote_addr

if "remember" in data and data["remember"] in valid_boolean_trues:
remember = True
Expand Down Expand Up @@ -398,7 +397,7 @@ def logout():

if username:
eventManager().fire(Events.USER_LOGGED_OUT, payload={"username": username})
auth_log(f"Logging out user {username} from {get_remote_address(request)}")
auth_log(f"Logging out user {username} from {request.remote_addr}")

return r

Expand Down Expand Up @@ -774,7 +773,7 @@ def _test_address(data):

remote_addr = data.get("address")
if not remote_addr:
remote_addr = get_remote_address(request)
remote_addr = request.remote_addr

remote_addr = sanitize_address(remote_addr)
ip = netaddr.IPAddress(remote_addr)
Expand Down
23 changes: 20 additions & 3 deletions src/octoprint/server/api/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,11 @@ def getSettings():

data = {
"api": {
"key": s.get(["api", "key"])
if Permissions.ADMIN.can() and credentials_checked_recently()
else None,
"key": (
s.get(["api", "key"])
if Permissions.ADMIN.can() and credentials_checked_recently()
else None
),
"allowCrossOrigin": s.get(["api", "allowCrossOrigin"]),
},
"appearance": {
Expand Down Expand Up @@ -410,6 +412,14 @@ def getSettings():
else:
data["webcam"] = {}

if Permissions.ADMIN.can():
data["accessControl"] = {
"autologinLocal": s.getBoolean(["accessControl", "autologinLocal"]),
"autologinHeadsupAcknowledged": s.getBoolean(
["accessControl", "autologinHeadsupAcknowledged"]
),
}

return jsonify(data)


Expand Down Expand Up @@ -557,6 +567,13 @@ def _saveSettings(data):
if "allowCrossOrigin" in data["api"]:
s.setBoolean(["api", "allowCrossOrigin"], data["api"]["allowCrossOrigin"])

if "accessControl" in data:
if "autologinHeadsupAcknowledged" in data["accessControl"]:
s.setBoolean(
["accessControl", "autologinHeadsupAcknowledged"],
data["accessControl"]["autologinHeadsupAcknowledged"],
)

if "appearance" in data:
if "name" in data["appearance"]:
s.set(["appearance", "name"], data["appearance"]["name"])
Expand Down
6 changes: 2 additions & 4 deletions src/octoprint/server/api/system.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
from octoprint.plugin import plugin_manager
from octoprint.server import NO_CONTENT
from octoprint.server.api import api
from octoprint.server.util.flask import get_remote_address, no_firstrun_access
from octoprint.server.util.flask import no_firstrun_access
from octoprint.settings import settings as s
from octoprint.systemcommands import system_command_manager
from octoprint.util.commandline import CommandlineCaller
Expand Down Expand Up @@ -96,9 +96,7 @@ def _usageForFolders():
@Permissions.SYSTEM.require(403)
def performSystemAction():
logging.getLogger(__name__).warning(
"Deprecated API call to /api/system made by {}, should be migrated to use /system/commands/custom/<action>".format(
get_remote_address(request)
)
f"Deprecated API call to /api/system made by {request.remote_addr}, should be migrated to use /system/commands/custom/<action>"
)

data = request.get_json(silent=True)
Expand Down
59 changes: 54 additions & 5 deletions src/octoprint/server/util/flask.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
import threading
import time
from datetime import datetime, timedelta, timezone
from typing import Any, Union
from typing import Any, Dict, List, Union

import flask
import flask.json
Expand All @@ -29,6 +29,7 @@
from flask import current_app
from flask_login import COOKIE_NAME as REMEMBER_COOKIE_NAME
from flask_login.utils import decode_cookie, encode_cookie
from pydantic import BaseModel
from werkzeug.local import LocalProxy
from werkzeug.utils import cached_property

Expand Down Expand Up @@ -666,7 +667,7 @@ def passive_login():

user = flask_login.current_user

remote_address = get_remote_address(flask.request)
remote_address = flask.request.remote_addr
ip_check_enabled = settings().getBoolean(["server", "ipCheck", "enabled"])
ip_check_trusted = settings().get(["server", "ipCheck", "trustedSubnets"])

Expand Down Expand Up @@ -1713,10 +1714,11 @@ def decorated_view(*args, **kwargs):
return decorated_view


@deprecated(
"get_remote_address is no longer required and deprecated, you can just use flask.request.remote_addr instead",
since="1.10.0",
)
def get_remote_address(request):
forwardedFor = request.headers.get("X-Forwarded-For", None)
if forwardedFor is not None:
return forwardedFor.split(",")[0]
return request.remote_addr


Expand Down Expand Up @@ -2012,3 +2014,50 @@ def session_signature(user, session):
def validate_session_signature(sig, user, session):
user_sig = session_signature(user, session)
return len(user_sig) == len(sig) and hmac.compare_digest(sig, user_sig)


##~~ Reverse proxy info


class ReverseProxyInfo(BaseModel):
client_ip: str
server_protocol: str
server_name: str
server_port: int
server_path: str
cookie_suffix: str
trusted_proxies: List[str] = []
headers: Dict[str, str] = {}


def get_reverse_proxy_info():
headers = {}
for header in sorted(
(
"X-Forwarded-For",
"X-Forwarded-Protocol",
"X-Scheme",
"X-Forwarded-Host",
"X-Forwarded-Port",
"X-Forwarded-Server",
"Host",
"X-Script-Name",
)
):
if header in flask.request.headers:
headers[header] = flask.request.headers[header]

trusted_downstreams = settings().get(["server", "reverseProxy", "trustedDownstream"])
if not trusted_downstreams:
trusted_downstreams = []

return ReverseProxyInfo(
client_ip=flask.request.remote_addr,
server_protocol=flask.request.environ.get("wsgi.url_scheme"),
server_name=flask.request.environ.get("SERVER_NAME"),
server_port=int(flask.request.environ.get("SERVER_PORT")),
server_path=flask.request.script_root if flask.request.script_root else "/",
cookie_suffix=get_cookie_suffix(flask.request),
trusted_proxies=trusted_downstreams,
headers=headers,
)
35 changes: 21 additions & 14 deletions src/octoprint/server/util/sockjs.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,14 +85,14 @@ class PrinterStateConnection(

_event_payload_processors = {
Events.CLIENT_OPENED: [
lambda user, payload: payload
if user.has_permission(Permissions.ADMIN)
else {}
lambda user, payload: (
payload if user.has_permission(Permissions.ADMIN) else {}
)
],
Events.CLIENT_AUTHED: [
lambda user, payload: payload
if user.has_permission(Permissions.ADMIN)
else {}
lambda user, payload: (
payload if user.has_permission(Permissions.ADMIN) else {}
)
],
"*": [],
}
Expand All @@ -101,10 +101,12 @@ class PrinterStateConnection(
_emit_permissions = {
"connected": [],
"reauthRequired": [],
"plugin": lambda payload: []
if payload.get("plugin") in ("backup", "softwareupdate")
and settings().getBoolean(["server", "firstRun"])
else [Permissions.STATUS],
"plugin": lambda payload: (
[]
if payload.get("plugin") in ("backup", "softwareupdate")
and settings().getBoolean(["server", "firstRun"])
else [Permissions.STATUS]
),
"*": [Permissions.STATUS],
}

Expand Down Expand Up @@ -179,10 +181,15 @@ def __init__(

@staticmethod
def _get_remote_address(info):
forwarded_for = info.headers.get("X-Forwarded-For")
if forwarded_for is not None:
return forwarded_for.split(",")[0]
return info.ip
from octoprint.util.net import get_http_client_ip

trusted_proxies = settings().get(["server", "reverseProxy", "trustedUpstream"])
if not isinstance(trusted_proxies, list):
trusted_proxies = ["127.0.0.1"]

return get_http_client_ip(
info.ip, info.headers.get("X-Forwarded-For"), trusted_proxies
)

def _keep_alive_callback(self):
if not self._authed:
Expand Down
32 changes: 32 additions & 0 deletions src/octoprint/server/util/tornado.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import tornado.httpserver
import tornado.httputil
import tornado.iostream
import tornado.netutil
import tornado.tcpserver
import tornado.util
import tornado.web
Expand All @@ -26,6 +27,7 @@
from zipstream.ng import ZIP_DEFLATED, ZipStream

import octoprint.util
import octoprint.util.net


def fix_json_encode():
Expand Down Expand Up @@ -93,6 +95,36 @@ def get_check_tuple(urlstring):
tornado.websocket.WebSocketHandler.check_origin = patched_check_origin


def fix_tornado_xheader_handling():
"""
This fixes tornado.httpserver._HTTPRequestContext._apply_xheaders to only use "X-Forwarded-For" header
for rewriting the ``remote_ip`` field, utilizing the set of trusted downstreams, instead of blindly
trusting ``X-Real-Ip``, and to also fetch the scheme from "X-Forwarded-Proto" if available.
"""

def patched_apply_xheaders(self, headers: "tornado.httputil.HTTPHeaders") -> None:
"""Rewrite the ``remote_ip`` and ``protocol`` fields."""

# other than the default implementation, we only use "X-Forwarded-For" here, not "X-Real-Ip"
ip = octoprint.util.net.get_http_client_ip(
self.remote_ip, headers.get("X-Forwarded-For"), self.trusted_downstream
)
if tornado.netutil.is_valid_ip(ip):
self.remote_ip = ip

# also fetch scheme from "X-Forwarded-Proto" if available
proto_header = headers.get("X-Scheme", headers.get("X-Forwarded-Proto"))
if proto_header:
# use only the last proto entry if there is more than one
proto_header = proto_header.split(",")[-1].strip()
if proto_header in ("http", "https"):
self.protocol = proto_header

import tornado.httpserver

tornado.httpserver._HTTPRequestContext._apply_xheaders = patched_apply_xheaders


# ~~ More sensible logging


Expand Down
Loading

0 comments on commit 5afbec8

Please sign in to comment.