diff --git a/CHANGELOG.md b/CHANGELOG.md index 3491b4c6..9c80e6d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,14 @@ and this project adheres to [Semantic Versioning 2.0.0](https://semver.org/spec/ Entries are case-insensitive, apply to all versions, and take effect without a restart. Server startup creates a commented template with no default exceptions; other installation security checks still apply. +- Verify the TLS certificate when the legacy UI fetches its notice from GitHub; + verification 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). ### Fixed diff --git a/comfyui_manager/legacy/manager_server.py b/comfyui_manager/legacy/manager_server.py index 2f181423..7da62e73 100644 --- a/comfyui_manager/legacy/manager_server.py +++ b/comfyui_manager/legacy/manager_server.py @@ -1851,46 +1851,50 @@ 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'
([\s\S]*?)
') - match = pattern.search(html_content) + pattern = re.compile(r'
([\s\S]*?)
') + match = pattern.search(html_content) - if match: - # Remote page content lands in innerHTML on the client: - # keep formatting markup only. - markdown_content = html_utils.sanitize_html_fragment(match.group(1)) - version_tag = os.environ.get('__COMFYUI_DESKTOP_VERSION__') - if version_tag is not None: - markdown_content += f"
ComfyUI: {version_tag} [Desktop]" - else: - version_tag = context.get_comfyui_tag() - if version_tag is None: - markdown_content += f"
ComfyUI: {core.comfy_ui_revision}[{comfy_ui_hash[:6]}]({core.comfy_ui_commit_datetime.date()})" + if match: + # Remote page content lands in innerHTML on the client: + # keep formatting markup only. + markdown_content = html_utils.sanitize_html_fragment(match.group(1)) + version_tag = os.environ.get('__COMFYUI_DESKTOP_VERSION__') + if version_tag is not None: + markdown_content += f"
ComfyUI: {version_tag} [Desktop]" else: - markdown_content += (f"
ComfyUI: {version_tag}
" - f"         ({core.comfy_ui_commit_datetime.date()})") - # markdown_content += f"
         ()" - markdown_content += f"
Manager: {core.version_str}" + version_tag = context.get_comfyui_tag() + if version_tag is None: + markdown_content += f"
ComfyUI: {core.comfy_ui_revision}[{comfy_ui_hash[:6]}]({core.comfy_ui_commit_datetime.date()})" + else: + markdown_content += (f"
ComfyUI: {version_tag}
" + f"         ({core.comfy_ui_commit_datetime.date()})") + # markdown_content += f"
         ()" + markdown_content += f"
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 = '

Your ComfyUI isn\'t git repo.

' + markdown_content - elif core.comfy_ui_required_commit_datetime.date() > core.comfy_ui_commit_datetime.date(): - markdown_content = '

Your ComfyUI is too OUTDATED!!!

' + markdown_content - except Exception: - pass + try: + if '__COMFYUI_DESKTOP_VERSION__' not in os.environ: + if core.comfy_ui_commit_datetime == datetime(1900, 1, 1, 0, 0, 0): + markdown_content = '

Your ComfyUI isn\'t git repo.

' + markdown_content + elif core.comfy_ui_required_commit_datetime.date() > core.comfy_ui_commit_datetime.date(): + markdown_content = '

Your ComfyUI is too OUTDATED!!!

' + markdown_content + except Exception: + pass - return web.Response(text=markdown_content, status=200) + return web.Response(text=markdown_content, status=200) + else: + 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) # legacy /manager/notice diff --git a/tests/test_notice_html_sanitizer.py b/tests/test_notice_html_sanitizer.py index cc27b6e2..69b9d3dc 100644 --- a/tests/test_notice_html_sanitizer.py +++ b/tests/test_notice_html_sanitizer.py @@ -123,6 +123,7 @@ def test_notice_endpoint_sanitizes_remote_content_and_retains_local_version_foot 'routes': web.RouteTableDef(), 're': re, 'web': web, 'html_utils': load_renderer()['html_utils'], 'aiohttp': SimpleNamespace(ClientSession=lambda **kwargs: client, TCPConnector=lambda **kwargs: None), + 'manager_util': SimpleNamespace(bypass_ssl=False), 'os': SimpleNamespace(environ={'__COMFYUI_DESKTOP_VERSION__': '1.0'}), 'core': SimpleNamespace(version_str='4.2.1'), }) diff --git a/tests/test_notice_tls.py b/tests/test_notice_tls.py new file mode 100644 index 00000000..da010486 --- /dev/null +++ b/tests/test_notice_tls.py @@ -0,0 +1,218 @@ +"""The legacy 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 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 legacy_html_testutil import PACKAGE, load_functions, load_renderer + +NOTICE_PATH = "/ltdrdata/ltdrdata.github.io/wiki/News" +NOTICE_HTML = ('

Served by the test server ' + 'notes

') +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): + return load_functions(PACKAGE / "legacy/manager_server.py", {"get_notice"}, { + "routes": web.RouteTableDef(), "aiohttp": aiohttp, "asyncio": asyncio, "logging": logging, + "re": re, "web": web, "os": os, + "html_utils": load_renderer()["html_utils"], + "manager_util": SimpleNamespace(bypass_ssl=bypass_ssl), + "core": SimpleNamespace(version_str="4.test"), + })["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 "the test server" 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 "the test server" in result["text"] + assert 'href="https://example.com/notes"' in result["text"] + assert "