Verify the TLS certificate when fetching the notice (#3424)
The notice route fetched the News page from GitHub with verify_ssl=False, so certificate verification was off for every user: whoever sat on the connection could present any certificate for github.com and choose the notice text and links. The sanitizer limits the markup, but it cannot tell whose content it is. - Turn verification off for the notice only when bypass_ssl is set, as manager_util.get_data does - Answer a certificate or connection failure with "Unable to retrieve Notice" where it raised and the notice area showed the server's 500 page - Add tests that run get_notice against a local HTTPS server behind a local CONNECT proxy: an untrusted certificate and a trusted certificate for another host are rejected; bypass_ssl accepts the untrusted certificate; a CA trusted through SSL_CERT_FILE is accepted; a closed proxy, a proxy answering 502 and a proxy that never answers end in the unavailable notice, each with one warning naming the cause - Add a CHANGELOG entry Fixes #3302
This commit is contained in:
Perfect Johny
@@ -2,6 +2,16 @@
|
||||
|
||||
## Unreleased
|
||||
|
||||
### Notice retrieval verifies the TLS certificate
|
||||
|
||||
The notice in the Manager menu is fetched from GitHub with certificate
|
||||
verification on; it was off for every user. A response whose certificate is not
|
||||
trusted is discarded and the notice area shows `Unable to retrieve Notice`. It
|
||||
shows the same when the connection fails, where it used to show a server error.
|
||||
Behind a proxy that re-signs TLS traffic, point the `SSL_CERT_FILE` environment
|
||||
variable at a CA bundle that includes the proxy's CA, or set `bypass_ssl = True`
|
||||
in `config.ini`, which turns verification off for this fetch too (#3302).
|
||||
|
||||
### Security policy: dedicated install flags (`allow_git_url_install` / `allow_pip_install`)
|
||||
|
||||
Two new boolean keys in `config.ini` (`[default]` section), both defaulting to
|
||||
|
||||
+48
-44
@@ -1869,55 +1869,59 @@ async def get_notice(request):
|
||||
url = "github.com"
|
||||
path = "/ltdrdata/ltdrdata.github.io/wiki/News"
|
||||
|
||||
async with aiohttp.ClientSession(trust_env=True, connector=aiohttp.TCPConnector(verify_ssl=False)) as session:
|
||||
async with session.get(f"https://{url}{path}") as response:
|
||||
if response.status == 200:
|
||||
# html_content = response.read().decode('utf-8')
|
||||
html_content = await response.text()
|
||||
try:
|
||||
async with aiohttp.ClientSession(trust_env=True, connector=aiohttp.TCPConnector(verify_ssl=not manager_util.bypass_ssl)) as session:
|
||||
async with session.get(f"https://{url}{path}") as response:
|
||||
if response.status == 200:
|
||||
# html_content = response.read().decode('utf-8')
|
||||
html_content = await response.text()
|
||||
|
||||
pattern = re.compile(r'<div class="markdown-body">([\s\S]*?)</div>')
|
||||
match = pattern.search(html_content)
|
||||
pattern = re.compile(r'<div class="markdown-body">([\s\S]*?)</div>')
|
||||
match = pattern.search(html_content)
|
||||
|
||||
if match:
|
||||
# Sanitize remote HTML before appending Manager's version text.
|
||||
markdown_content = manager_util.sanitize_html_fragment(match.group(1))
|
||||
version_tag = os.environ.get('__COMFYUI_DESKTOP_VERSION__')
|
||||
if version_tag is not None:
|
||||
markdown_content += f"<HR>ComfyUI: {version_tag} [Desktop]"
|
||||
if match:
|
||||
# Sanitize remote HTML before appending Manager's version text.
|
||||
markdown_content = manager_util.sanitize_html_fragment(match.group(1))
|
||||
version_tag = os.environ.get('__COMFYUI_DESKTOP_VERSION__')
|
||||
if version_tag is not None:
|
||||
markdown_content += f"<HR>ComfyUI: {version_tag} [Desktop]"
|
||||
else:
|
||||
version_tag = core.get_comfyui_tag()
|
||||
if version_tag is None:
|
||||
markdown_content += f"<HR>ComfyUI: {core.comfy_ui_revision}[{comfy_ui_hash[:6]}]({core.comfy_ui_commit_datetime.date()})"
|
||||
else:
|
||||
markdown_content += (f"<HR>ComfyUI: {version_tag}<BR>"
|
||||
f" ({core.comfy_ui_commit_datetime.date()})")
|
||||
# markdown_content += f"<BR> ()"
|
||||
markdown_content += f"<BR>Manager: {core.version_str}"
|
||||
|
||||
try:
|
||||
if '__COMFYUI_DESKTOP_VERSION__' not in os.environ:
|
||||
if core.comfy_ui_commit_datetime == datetime(1900, 1, 1, 0, 0, 0):
|
||||
markdown_content = '<P style="text-align: center; color:red; background-color:white; font-weight:bold">Your ComfyUI isn\'t git repo.</P>' + markdown_content
|
||||
elif core.comfy_ui_required_commit_datetime.date() > core.comfy_ui_commit_datetime.date():
|
||||
markdown_content = '<P style="text-align: center; color:red; background-color:white; font-weight:bold">Your ComfyUI is too OUTDATED!!!</P>' + markdown_content
|
||||
except:
|
||||
pass
|
||||
|
||||
# Prepend startup notices from manager_migration
|
||||
for message, level in reversed(manager_migration.startup_notices):
|
||||
if level == 'error':
|
||||
style = 'color:red; background-color:white; font-weight:bold'
|
||||
elif level == 'warning':
|
||||
style = 'color:orange; background-color:white; font-weight:bold'
|
||||
else:
|
||||
style = 'color:blue; background-color:white'
|
||||
markdown_content = f'<P style="{style}">{message}</P>' + markdown_content
|
||||
|
||||
return web.Response(text=markdown_content, status=200)
|
||||
else:
|
||||
version_tag = core.get_comfyui_tag()
|
||||
if version_tag is None:
|
||||
markdown_content += f"<HR>ComfyUI: {core.comfy_ui_revision}[{comfy_ui_hash[:6]}]({core.comfy_ui_commit_datetime.date()})"
|
||||
else:
|
||||
markdown_content += (f"<HR>ComfyUI: {version_tag}<BR>"
|
||||
f" ({core.comfy_ui_commit_datetime.date()})")
|
||||
# markdown_content += f"<BR> ()"
|
||||
markdown_content += f"<BR>Manager: {core.version_str}"
|
||||
|
||||
try:
|
||||
if '__COMFYUI_DESKTOP_VERSION__' not in os.environ:
|
||||
if core.comfy_ui_commit_datetime == datetime(1900, 1, 1, 0, 0, 0):
|
||||
markdown_content = '<P style="text-align: center; color:red; background-color:white; font-weight:bold">Your ComfyUI isn\'t git repo.</P>' + markdown_content
|
||||
elif core.comfy_ui_required_commit_datetime.date() > core.comfy_ui_commit_datetime.date():
|
||||
markdown_content = '<P style="text-align: center; color:red; background-color:white; font-weight:bold">Your ComfyUI is too OUTDATED!!!</P>' + markdown_content
|
||||
except:
|
||||
pass
|
||||
|
||||
# Prepend startup notices from manager_migration
|
||||
for message, level in reversed(manager_migration.startup_notices):
|
||||
if level == 'error':
|
||||
style = 'color:red; background-color:white; font-weight:bold'
|
||||
elif level == 'warning':
|
||||
style = 'color:orange; background-color:white; font-weight:bold'
|
||||
else:
|
||||
style = 'color:blue; background-color:white'
|
||||
markdown_content = f'<P style="{style}">{message}</P>' + markdown_content
|
||||
|
||||
return web.Response(text=markdown_content, status=200)
|
||||
return web.Response(text="Unable to retrieve Notice", status=200)
|
||||
else:
|
||||
return web.Response(text="Unable to retrieve Notice", status=200)
|
||||
else:
|
||||
return web.Response(text="Unable to retrieve Notice", status=200)
|
||||
except (aiohttp.ClientError, asyncio.TimeoutError) as e:
|
||||
logging.warning(f"[ComfyUI-Manager] Unable to retrieve Notice: {type(e).__name__}: {e}")
|
||||
return web.Response(text="Unable to retrieve Notice", status=200)
|
||||
|
||||
|
||||
@routes.get("/manager/startup_alerts")
|
||||
|
||||
@@ -0,0 +1,226 @@
|
||||
"""The notice route verifies the upstream certificate and survives a failed fetch (#3302).
|
||||
|
||||
Each case runs the real `get_notice` in a child process. The child points HTTPS_PROXY at a
|
||||
local CONNECT proxy that hands the tunnel for github.com:443 to a local HTTPS server, which
|
||||
holds a "github.com" certificate from a throwaway CA: a machine in the middle, unless the
|
||||
case trusts that CA through SSL_CERT_FILE. Nothing leaves 127.0.0.1. A child is needed
|
||||
because aiohttp builds its verifying SSL context when it is imported.
|
||||
"""
|
||||
import ast
|
||||
import asyncio
|
||||
import json
|
||||
import logging
|
||||
import os
|
||||
import re
|
||||
import shutil
|
||||
import ssl
|
||||
import subprocess
|
||||
import sys
|
||||
from types import SimpleNamespace
|
||||
|
||||
import aiohttp
|
||||
import pytest
|
||||
from aiohttp import web
|
||||
|
||||
from manager_test_utils import GLOB_DIR, load_manager_util
|
||||
|
||||
NOTICE_PATH = "/ltdrdata/ltdrdata.github.io/wiki/News"
|
||||
NOTICE_HTML = ('<div class="markdown-body"><p>Served by <b>the test server</b> '
|
||||
'<a href="https://example.com/notes">notes</a><script>x()</script></p></div>')
|
||||
UNAVAILABLE = "Unable to retrieve Notice"
|
||||
TUNNEL = "CONNECT github.com:443 HTTP/1.1"
|
||||
OTHER_HOST = "notice-test.example"
|
||||
|
||||
OPENSSL_CONFIG = """
|
||||
[req]
|
||||
distinguished_name = dn
|
||||
prompt = no
|
||||
[dn]
|
||||
CN = ComfyUI-Manager notice test CA
|
||||
[ca]
|
||||
basicConstraints = critical, CA:TRUE
|
||||
keyUsage = critical, keyCertSign, cRLSign
|
||||
subjectKeyIdentifier = hash
|
||||
"""
|
||||
LEAF_EXTENSIONS = """
|
||||
[leaf]
|
||||
basicConstraints = critical, CA:FALSE
|
||||
keyUsage = critical, digitalSignature, keyEncipherment
|
||||
extendedKeyUsage = serverAuth
|
||||
subjectAltName = DNS:{host}
|
||||
subjectKeyIdentifier = hash
|
||||
authorityKeyIdentifier = keyid, issuer
|
||||
"""
|
||||
|
||||
|
||||
def load_get_notice(bypass_ssl):
|
||||
source = (GLOB_DIR / "manager_server.py").read_text(encoding="utf-8")
|
||||
nodes = [node for node in ast.parse(source).body
|
||||
if isinstance(node, ast.AsyncFunctionDef) and node.name == "get_notice"]
|
||||
assert len(nodes) == 1
|
||||
manager_util = load_manager_util()
|
||||
manager_util.bypass_ssl = bypass_ssl
|
||||
namespace = {
|
||||
"routes": web.RouteTableDef(), "aiohttp": aiohttp, "asyncio": asyncio, "logging": logging,
|
||||
"re": re, "web": web, "os": os, "manager_util": manager_util,
|
||||
"core": SimpleNamespace(version_str="3.test"),
|
||||
"manager_migration": SimpleNamespace(startup_notices=[]),
|
||||
}
|
||||
exec(compile(ast.Module(body=nodes, type_ignores=[]), "manager_server.py", "exec"), namespace)
|
||||
return namespace["get_notice"]
|
||||
|
||||
|
||||
async def _pipe(reader, writer):
|
||||
try:
|
||||
while data := await reader.read(65536):
|
||||
writer.write(data)
|
||||
await writer.drain()
|
||||
finally:
|
||||
writer.close()
|
||||
|
||||
|
||||
async def fetch_notice(certdir, host, bypass_ssl, proxy_mode):
|
||||
"""Child process: what `get_notice` answers and logs, and the tunnels the proxy was asked for."""
|
||||
async def notice_page(request):
|
||||
return web.Response(text=NOTICE_HTML, content_type="text/html")
|
||||
|
||||
upstream = web.Application()
|
||||
upstream.router.add_get(NOTICE_PATH, notice_page)
|
||||
runner = web.AppRunner(upstream)
|
||||
await runner.setup()
|
||||
server_context = ssl.create_default_context(ssl.Purpose.CLIENT_AUTH)
|
||||
server_context.load_cert_chain(os.path.join(certdir, host + ".pem"), os.path.join(certdir, host + ".key"))
|
||||
await web.TCPSite(runner, "127.0.0.1", 0, ssl_context=server_context).start()
|
||||
upstream_port = runner.addresses[0][1]
|
||||
|
||||
tunnels = []
|
||||
answered = asyncio.Event()
|
||||
|
||||
async def connect_proxy(reader, writer):
|
||||
tunnels.append((await reader.readuntil(b"\r\n\r\n")).split(b"\r\n", 1)[0].decode())
|
||||
if proxy_mode == "bad_gateway":
|
||||
writer.write(b"HTTP/1.1 502 Bad Gateway\r\nContent-Length: 0\r\n\r\n")
|
||||
await writer.drain()
|
||||
if proxy_mode == "silent":
|
||||
await answered.wait()
|
||||
if proxy_mode != "tunnel":
|
||||
writer.close()
|
||||
return
|
||||
upstream_reader, upstream_writer = await asyncio.open_connection("127.0.0.1", upstream_port)
|
||||
writer.write(b"HTTP/1.1 200 Connection established\r\n\r\n")
|
||||
await writer.drain()
|
||||
await asyncio.gather(_pipe(reader, upstream_writer), _pipe(upstream_reader, writer), return_exceptions=True)
|
||||
|
||||
proxy = await asyncio.start_server(connect_proxy, "127.0.0.1", 0)
|
||||
proxy_port = proxy.sockets[0].getsockname()[1]
|
||||
if proxy_mode == "closed":
|
||||
proxy.close()
|
||||
await proxy.wait_closed()
|
||||
if proxy_mode == "silent":
|
||||
aiohttp.client.DEFAULT_TIMEOUT = aiohttp.ClientTimeout(total=0.5)
|
||||
os.environ["HTTPS_PROXY"] = f"http://127.0.0.1:{proxy_port}"
|
||||
|
||||
# Only what get_notice logs: aiohttp warns through its own loggers, for example about .netrc.
|
||||
warnings = []
|
||||
handler = logging.Handler(logging.WARNING)
|
||||
handler.emit = lambda record: record.name == "root" and warnings.append(record.getMessage())
|
||||
logging.getLogger().addHandler(handler)
|
||||
|
||||
response = await load_get_notice(bypass_ssl)(None)
|
||||
answered.set()
|
||||
proxy.close()
|
||||
await runner.cleanup()
|
||||
return {"status": response.status, "text": response.text, "tunnels": tunnels, "warnings": warnings}
|
||||
|
||||
|
||||
@pytest.fixture(scope="module")
|
||||
def certdir(tmp_path_factory):
|
||||
if shutil.which("openssl") is None:
|
||||
pytest.skip("the openssl command is needed to create the test certificates")
|
||||
path = tmp_path_factory.mktemp("notice-tls")
|
||||
config = path / "openssl.cnf"
|
||||
config.write_text(OPENSSL_CONFIG, encoding="utf-8")
|
||||
|
||||
def openssl(*args):
|
||||
subprocess.run(["openssl", *args], cwd=path, check=True, capture_output=True)
|
||||
|
||||
openssl("req", "-x509", "-newkey", "rsa:2048", "-nodes", "-days", "2", "-config", str(config),
|
||||
"-extensions", "ca", "-keyout", "ca.key", "-out", "ca.pem")
|
||||
for serial, host in enumerate(("github.com", OTHER_HOST), start=1):
|
||||
(path / f"{host}.ext").write_text(LEAF_EXTENSIONS.format(host=host), encoding="utf-8")
|
||||
openssl("req", "-newkey", "rsa:2048", "-nodes", "-subj", f"/CN={host}", "-config", str(config),
|
||||
"-keyout", f"{host}.key", "-out", f"{host}.csr")
|
||||
openssl("x509", "-req", "-in", f"{host}.csr", "-CA", "ca.pem", "-CAkey", "ca.key",
|
||||
"-set_serial", str(serial), "-days", "2", "-extfile", f"{host}.ext", "-extensions", "leaf",
|
||||
"-out", f"{host}.pem")
|
||||
return path
|
||||
|
||||
|
||||
def fetch(certdir, host="github.com", trust_ca=False, bypass_ssl=False, proxy_mode="tunnel"):
|
||||
env = {key: value for key, value in os.environ.items()
|
||||
if not key.lower().endswith("_proxy") and key not in ("SSL_CERT_FILE", "SSL_CERT_DIR")}
|
||||
env["__COMFYUI_DESKTOP_VERSION__"] = "test"
|
||||
if trust_ca:
|
||||
env["SSL_CERT_FILE"] = str(certdir / "ca.pem")
|
||||
child = subprocess.run(
|
||||
[sys.executable, __file__, str(certdir), host, str(int(bypass_ssl)), proxy_mode],
|
||||
env=env, capture_output=True, text=True, timeout=60)
|
||||
assert child.returncode == 0, "get_notice raised instead of answering:\n" + child.stderr[-2000:]
|
||||
return json.loads(child.stdout.strip().splitlines()[-1])
|
||||
|
||||
|
||||
def assert_unavailable(result, *cause):
|
||||
assert (result["status"], result["text"]) == (200, UNAVAILABLE)
|
||||
assert len(result["warnings"]) == 1
|
||||
for text in cause:
|
||||
assert text in result["warnings"][0]
|
||||
|
||||
|
||||
def test_untrusted_certificate_is_rejected_before_its_content_is_used(certdir):
|
||||
result = fetch(certdir)
|
||||
|
||||
assert result["tunnels"] == [TUNNEL]
|
||||
assert_unavailable(result, "ClientConnectorCertificateError", "CERTIFICATE_VERIFY_FAILED")
|
||||
|
||||
|
||||
def test_bypass_ssl_accepts_the_untrusted_certificate(certdir):
|
||||
result = fetch(certdir, bypass_ssl=True)
|
||||
|
||||
assert result["tunnels"] == [TUNNEL]
|
||||
assert result["status"] == 200
|
||||
assert "<b>the test server</b>" in result["text"]
|
||||
assert result["warnings"] == []
|
||||
|
||||
|
||||
def test_trusted_certificate_for_another_host_is_rejected(certdir):
|
||||
result = fetch(certdir, host=OTHER_HOST, trust_ca=True)
|
||||
|
||||
assert result["tunnels"] == [TUNNEL]
|
||||
assert_unavailable(result, "ClientConnectorCertificateError", "not valid for 'github.com'")
|
||||
|
||||
|
||||
def test_ca_trusted_through_ssl_cert_file_is_accepted(certdir):
|
||||
result = fetch(certdir, trust_ca=True)
|
||||
|
||||
assert result["tunnels"] == [TUNNEL]
|
||||
assert result["status"] == 200
|
||||
assert "<b>the test server</b>" in result["text"]
|
||||
assert 'href="https://example.com/notes"' in result["text"]
|
||||
assert "<script" not in result["text"]
|
||||
assert result["warnings"] == []
|
||||
assert "Manager: 3.test" in result["text"]
|
||||
|
||||
|
||||
@pytest.mark.parametrize("proxy_mode, error", [
|
||||
("closed", "ClientProxyConnectionError"),
|
||||
("bad_gateway", "ClientHttpProxyError"),
|
||||
("silent", "TimeoutError"),
|
||||
], ids=["proxy_closed", "proxy_answers_502", "proxy_silent"])
|
||||
def test_failed_fetch_answers_with_the_unavailable_notice(certdir, proxy_mode, error):
|
||||
result = fetch(certdir, trust_ca=True, proxy_mode=proxy_mode)
|
||||
|
||||
assert_unavailable(result, error)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
print(json.dumps(asyncio.run(fetch_notice(sys.argv[1], sys.argv[2], sys.argv[3] == "1", sys.argv[4]))))
|
||||
Reference in New Issue
Block a user