Skip to content

add SSL to server#99

Open
ad-astra-video wants to merge 6 commits into
mainfrom
add-ssl
Open

add SSL to server#99
ad-astra-video wants to merge 6 commits into
mainfrom
add-ssl

Conversation

@ad-astra-video
Copy link
Copy Markdown
Contributor

Adds ability to have SSL on runner with self-signed certs or cert files.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds HTTPS/SSL support to pytrickle’s StreamServer, including automatic self-signed certificate generation when certificate files aren’t provided, and exposes the configuration through StreamProcessor plus examples/tests.

Changes:

  • Add SSL context creation utilities with self-signed certificate generation fallback logic.
  • Extend StreamServer (and StreamProcessor.from_handlers) to accept ssl, ssl_certfile, and ssl_keyfile, and start aiohttp with an SSL context when enabled.
  • Add tests and example comments demonstrating SSL usage.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
pytrickle/server.py Wires SSL context into aiohttp.web.TCPSite and adds SSL-related config fields/method.
pytrickle/utils/ssl.py New helper module for creating an ssl.SSLContext and generating self-signed certs.
pytrickle/stream_processor.py Threads SSL configuration through StreamProcessor.from_handlers into StreamServer.
tests/test_stream_server.py Adds SSL configuration tests and a server start test for SSL mode.
examples/passthrough_example.py Documents how to enable SSL via commented configuration options.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/test_stream_server.py Outdated
Comment on lines +714 to +724
# Generate a real self-signed cert using openssl for the test
import subprocess
subprocess.run(
[
"openssl", "req", "-x509", "-newkey", "rsa:2048",
"-keyout", str(key_file), "-out", str(cert_file),
"-days", "1", "-nodes", "-subj", "/CN=test",
],
check=True,
capture_output=True,
)
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

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

test_ssl_enabled_with_provided_certs shells out to openssl unconditionally. This makes the test environment-dependent (and will fail on systems/containers without openssl, or with an openssl build that doesn’t support the flags used). Consider skipping the test when openssl isn’t available (e.g., via shutil.which), or generating the certificate via the same Python fallback path used by the production code (cryptography) to keep the test portable.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fixed in commit 00de998. Replaced the subprocess.run(["openssl", ...]) call with _generate_self_signed_cert_cryptography() from the production code, which uses the cryptography library and has no CLI dependency. Also removed the now-unused subprocess and ssl_utils imports and the unused tmp_path fixture parameter.

Comment on lines +771 to +790
async def test_ssl_server_starts_and_accepts_connections(self):
"""Test that the server can start with SSL and accept HTTPS connections."""
processor = MockFrameProcessor()
server = StreamServer(
frame_processor=processor,
port=0,
ssl=True,
capability_name="ssl-test",
)
processor.attach_state(server.state)
server.state.set_startup_complete()

runner = await server.start_server()
try:
# The server should be running on an HTTPS port
assert runner is not None
# Verify SSL context was created
ctx = server._setup_ssl()
assert ctx is not None
finally:
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

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

This test claims to verify that the SSL server “accepts HTTPS connections”, but it never actually performs a client connection/request against the started server. As written it only asserts that start_server() returns a runner and that _setup_ssl() can create a context; please add an actual HTTPS request (e.g., using aiohttp.ClientSession with verification disabled for the self-signed cert) so the test validates the behavior it describes.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fixed in commit 1285a98. The test now makes an actual HTTPS GET to /health using aiohttp.ClientSession with SSL verification disabled (self-signed cert). It also asserts server.port != 0 to confirm the OS-assigned port was captured.

Comment thread pytrickle/utils/ssl.py
Comment on lines +45 to +54
if certfile and keyfile:
if not os.path.isfile(certfile):
raise FileNotFoundError(f"SSL certificate file not found: {certfile}")
if not os.path.isfile(keyfile):
raise FileNotFoundError(f"SSL key file not found: {keyfile}")
ctx.load_cert_chain(certfile, keyfile)
logger.info("Loaded SSL certificate from %s", certfile)
else:
cert_path, key_path = generate_self_signed_cert()
try:
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

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

setup_ssl_context treats the “only certfile provided” or “only keyfile provided” cases the same as “no files provided” and silently generates a self-signed certificate. This is surprising and contradicts the server docstring that implies both files are required when providing your own cert. Please validate that certfile and keyfile are either both set or both unset, and raise a clear ValueError when only one is provided.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fixed in commit 1285a98. setup_ssl_context now raises ValueError("Both 'certfile' and 'keyfile' must be provided together, or neither.") when exactly one of the two is given. Two new tests (test_ssl_only_certfile_raises_value_error, test_ssl_only_keyfile_raises_value_error) cover both cases.

Comment thread pytrickle/utils/ssl.py Outdated
Comment thread pytrickle/server.py

from aiohttp import web
from .version import __version__
from .utils.ssl import setup_ssl_context
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

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

StreamServer introduces an import from .utils.ssl. In pyproject.toml, the setuptools packages list is currently explicitly enumerated and does not include pytrickle.utils, so this module (and the new pytrickle.utils.ssl) may be missing from built wheels/sdists even though it works in editable installs. Please ensure the packaging configuration includes pytrickle.utils (and other subpackages) so HTTPS works in installed distributions.

Suggested change
from .utils.ssl import setup_ssl_context
try:
from .utils.ssl import setup_ssl_context
except ImportError:
def setup_ssl_context(certfile: str, keyfile: str) -> ssl.SSLContext:
"""Create a server SSL context when the packaged helper is unavailable."""
context = ssl.create_default_context(ssl.Purpose.CLIENT_AUTH)
context.load_cert_chain(certfile=certfile, keyfile=keyfile)
return context

Copilot uses AI. Check for mistakes.
Comment thread tests/test_stream_server.py Outdated
Comment on lines +8 to +22
@@ -15,6 +19,7 @@
from pytrickle.test_utils import MockFrameProcessor, create_mock_client
from pytrickle.client import TrickleClient
from pytrickle.protocol import TrickleProtocol
from pytrickle.utils import ssl as ssl_utils
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

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

Unused imports at the top of the test module: os is never referenced, ssl_utils is never referenced, and the module-level subprocess import is unused because subprocess is re-imported inside the test. Please remove the unused imports and/or use the single module-level import consistently to avoid lint failures and keep the test file tidy.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The unused subprocess, ssl_utils, and top-level import os (only used inside the test) imports were removed in the previous commit (00de998). The test file now only imports what it uses.

Comment thread pytrickle/utils/ssl.py Outdated
Comment on lines +77 to +93
if shutil.which("openssl"):
return _generate_self_signed_cert_openssl()

# Fallback to cryptography library if available
try:
from cryptography import x509
from cryptography.x509.oid import NameOID
from cryptography.hazmat.primitives import hashes, serialization
from cryptography.hazmat.primitives.asymmetric import rsa
except ImportError:
raise RuntimeError(
"SSL is enabled but neither 'openssl' CLI nor 'cryptography' "
"library is available. Install cryptography "
"(pip install cryptography) or ensure openssl is on PATH."
)

return _generate_self_signed_cert_cryptography()
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

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

generate_self_signed_cert() always prefers the openssl CLI when present, but if the openssl invocation fails (e.g., due to an older/LibreSSL build not supporting -addext), the code raises and never attempts the cryptography fallback. To improve portability, consider falling back to the Python/cryptography implementation when the openssl path errors, not only when it’s missing from PATH.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fixed in commit 1285a98. generate_self_signed_cert now wraps _generate_self_signed_cert_openssl() in a try/except RuntimeError and falls back to the cryptography path when the CLI invocation fails, not just when openssl is absent from PATH.

Comment thread pytrickle/server.py Outdated
Comment thread pytrickle/server.py
Comment on lines +817 to +818
scheme = "https" if ssl_context else "http"
logger.info(f"Server started on {scheme}://{self.host}:{self.port}")
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

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

When starting on an ephemeral port (port=0), self.port remains 0 but the server binds to an OS-assigned port. Logging ...://{self.host}:{self.port} will therefore output :0, which is misleading. Consider logging the bound address/port from runner.addresses (or similar) after site.start() and/or updating self.port to the assigned port.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fixed in commit 1285a98. After site.start(), self.port is updated from runner.addresses[0][1] so the bound OS-assigned port is reflected in the log message and accessible to callers.

ad-astra-video and others added 3 commits April 26, 2026 19:20
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…d_with_provided_certs

Agent-Logs-Url: https://github.com/livepeer/pytrickle/sessions/3c3cabbe-8b65-4d3b-abe0-2867c6428fff

Co-authored-by: ad-astra-video <99882368+ad-astra-video@users.noreply.github.com>
…ssl fallback, port logging, HTTPS test

Agent-Logs-Url: https://github.com/livepeer/pytrickle/sessions/16cf59be-c72d-4cee-8a18-eb4253d47c5f

Co-authored-by: ad-astra-video <99882368+ad-astra-video@users.noreply.github.com>
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