Skip to content

[MRG] Add support for TLS v1.3 #890

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.

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

Merged
merged 5 commits into from
Nov 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .github/workflows/pr-pytest-apps.yml
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ jobs:
env:
PYTHON_VERSION: ${{ matrix.python-version }}
run: |
poetry run pytest --ignore=pynetdicom/tests
poetry run pytest -x --ignore=pynetdicom/tests

# pydicom-release:
# runs-on: ubuntu-latest
Expand Down
17 changes: 13 additions & 4 deletions .github/workflows/pr-pytest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,14 @@ jobs:
run: |
conda install pytest
conda install -c conda-forge pydicom
- name: Get OpenSSL version
run: |
python -c "import ssl; print('OpenSSL:', ssl.OPENSSL_VERSION_INFO)"
- name: Test with pytest
env:
PYTHON_VERSION: ${{ matrix.python-version }}
run: |
pytest --ignore=pynetdicom/apps
pytest -x --ignore=pynetdicom/apps

windows:
runs-on: windows-latest
Expand Down Expand Up @@ -65,11 +68,14 @@ jobs:
key: venv-${{ runner.os }}-${{ steps.setup-python.outputs.python-version }}-${{ hashFiles('**/poetry.lock') }}
- name: Install dependencies
run: poetry install --no-interaction --extras tests
- name: Get OpenSSL version
run: |
poetry run python -c "import ssl; print('OpenSSL:', ssl.OPENSSL_VERSION_INFO)"
- name: Test with pytest
env:
PYTHON_VERSION: ${{ matrix.python-version }}
run: |
poetry run pytest --cov pynetdicom --ignore=pynetdicom/apps &&
poetry run pytest -x --cov pynetdicom --ignore=pynetdicom/apps &&
poetry run coverage xml
- name: Send coverage results
if: ${{ success() }}
Expand Down Expand Up @@ -114,7 +120,7 @@ jobs:
env:
PYTHON_VERSION: ${{ matrix.python-version }}
run: |
poetry run pytest --cov pynetdicom --ignore=pynetdicom/apps &&
poetry run pytest -x --cov pynetdicom --ignore=pynetdicom/apps &&
poetry run coverage xml
- name: Send coverage results
if: ${{ success() }}
Expand Down Expand Up @@ -154,11 +160,14 @@ jobs:
run: |
poetry install --no-interaction --extras tests &&
poetry run pip list
- name: Get OpenSSL version
run: |
poetry run python -c "import ssl; print('OpenSSL:', ssl.OPENSSL_VERSION_INFO)"
- name: Test with pytest
env:
PYTHON_VERSION: ${{ matrix.python-version }}
run: |
poetry run pytest --cov pynetdicom --ignore=pynetdicom/apps &&
poetry run pytest -x --cov pynetdicom --ignore=pynetdicom/apps &&
poetry run coverage xml
- name: Send coverage results
if: ${{ success() }}
Expand Down
4 changes: 3 additions & 1 deletion docs/changelog/v2.1.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,6 @@ Changes
.......

* Dropped support for Python 3.7, 3.8 and 3.9
* Added support for Python 3.12
* Added support for Python 3.11 and 3.12
* With `Python 3.10 requiring OpenSSL v1.1.1 or newer
<https://peps.python.org/pep-0644/>`_, TLS v1.3 is now officially supported
77 changes: 47 additions & 30 deletions pynetdicom/tests/test_transport.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,8 @@ def test_close_connect(self):
sock.close()
assert sock.socket is None
# Tries to connect, sets to None if fails
# Ensure we fail if *something* is listening
self.assoc.connection_timeout = 1
request = A_ASSOCIATE()
request.called_presentation_address = ("123", 12)
sock.connect(T_CONNECT(request))
Expand Down Expand Up @@ -312,29 +314,28 @@ def handle_echo(event):
assert 2 == len(events)


@pytest.fixture
def server_context(request):
"""Return a good server SSLContext."""
# TLS v1.3 is not currently supported :(
# The actual available attributes/protocols depend on OS, OpenSSL version
# and Python version, ugh
if hasattr(ssl, "TLSVersion"):
# This is the current and future, but heavily depends on OpenSSL
# Python 3.7+, w/ OpenSSL 1.1.0g+
context = ssl.create_default_context(ssl.Purpose.CLIENT_AUTH)
context.verify_mode = ssl.CERT_REQUIRED
context.load_cert_chain(certfile=SERVER_CERT, keyfile=SERVER_KEY)
context.load_verify_locations(cafile=CLIENT_CERT)
context.maximum_version = ssl.TLSVersion.TLSv1_2

return context

# Should work with older Python and OpenSSL versions
# Python 3.6
context = ssl.SSLContext(protocol=ssl.PROTOCOL_TLSv1_2)
def server_context_v1_2():
"""Return a good TLs v1.2 server SSLContext."""
# Python 3.10 and PEP 644, the ssl module requires OpenSSL 1.1.1 or newer
context = ssl.create_default_context(ssl.Purpose.CLIENT_AUTH)
context.verify_mode = ssl.CERT_REQUIRED
context.load_cert_chain(certfile=SERVER_CERT, keyfile=SERVER_KEY)
context.load_verify_locations(cafile=CLIENT_CERT)
context.minimum_version = ssl.TLSVersion.TLSv1_2
context.maximum_version = ssl.TLSVersion.TLSv1_2

return context


def server_context_v1_3():
"""Return a good TLS v1.3 server SSLContext."""
# Python 3.10 and PEP 644, the ssl module requires OpenSSL 1.1.1 or newer
context = ssl.create_default_context(ssl.Purpose.CLIENT_AUTH)
context.verify_mode = ssl.CERT_REQUIRED
context.load_cert_chain(certfile=SERVER_CERT, keyfile=SERVER_KEY)
context.load_verify_locations(cafile=CLIENT_CERT)
context.minimum_version = ssl.TLSVersion.TLSv1_3
context.maximum_version = ssl.TLSVersion.TLSv1_3

return context

Expand All @@ -350,6 +351,14 @@ def client_context(request):
return context


TLS_SERVER_CONTEXTS = [(server_context_v1_2, "TLSv1.2")]
if ssl.OPENSSL_VERSION_INFO >= (1, 1, 1):
TLS_SERVER_CONTEXTS = [
(server_context_v1_2, "TLSv1.2"),
(server_context_v1_3, "TLSv1.3"),
]


class TestTLS:
"""Test using TLS to wrap the association."""

Expand Down Expand Up @@ -405,15 +414,16 @@ def test_tls_not_server_yes_client(self, client_context):

assert len(server.active_associations) == 0

def test_tls_yes_server_not_client(self, server_context, caplog):
@pytest.mark.parametrize("server_context, tls_version", TLS_SERVER_CONTEXTS)
def test_tls_yes_server_not_client(self, server_context, tls_version, caplog):
"""Test wrapping the acceptor socket with TLS (and not client)."""
with caplog.at_level(logging.ERROR, logger="pynetdicom"):
self.ae = ae = AE()
ae.add_supported_context("1.2.840.10008.1.1")
server = ae.start_server(
("localhost", 11112),
block=False,
ssl_context=server_context,
ssl_context=server_context(),
)

ae.add_requested_context("1.2.840.10008.1.1")
Expand All @@ -425,7 +435,10 @@ def test_tls_yes_server_not_client(self, server_context, caplog):
assert len(server.active_associations) == 0
assert "Connection closed before the entire PDU was received" in caplog.text

def test_tls_yes_server_yes_client(self, server_context, client_context):
@pytest.mark.parametrize("server_context, tls_version", TLS_SERVER_CONTEXTS)
def test_tls_yes_server_yes_client(
self, server_context, tls_version, client_context
):
"""Test associating with TLS on both ends."""
self.ae = ae = AE()
ae.acse_timeout = 5
Expand All @@ -435,13 +448,14 @@ def test_tls_yes_server_yes_client(self, server_context, client_context):
server = ae.start_server(
("localhost", 11112),
block=False,
ssl_context=server_context,
ssl_context=server_context(),
)

wait_for_server_socket(server, 1)

ae.add_requested_context("1.2.840.10008.1.1")
assoc = ae.associate("localhost", 11112, tls_args=(client_context, None))
assert assoc.dul.socket.socket.version() == tls_version
assert assoc.is_established
assoc.release()
assert assoc.is_released
Expand All @@ -450,7 +464,8 @@ def test_tls_yes_server_yes_client(self, server_context, client_context):

assert len(server.active_associations) == 0

def test_tls_transfer(self, server_context, client_context):
@pytest.mark.parametrize("server_context, tls_version", TLS_SERVER_CONTEXTS)
def test_tls_transfer(self, server_context, tls_version, client_context):
"""Test transferring data after associating with TLS."""
ds = []

Expand All @@ -469,7 +484,7 @@ def handle_store(event):
server = ae.start_server(
("localhost", 11112),
block=False,
ssl_context=server_context,
ssl_context=server_context(),
evt_handlers=handlers,
)

Expand Down Expand Up @@ -524,7 +539,8 @@ def test_no_ssl_scu(self):
with pytest.raises(RuntimeError, match=msg):
ae.associate("localhost", 11112, tls_args=(["random", "object"], None))

def test_multiple_pdu_req(self, server_context, client_context):
@pytest.mark.parametrize("server_context, tls_version", TLS_SERVER_CONTEXTS)
def test_multiple_pdu_req(self, server_context, tls_version, client_context):
"""Test what happens if two PDUs are sent before the select call."""
events = []

Expand All @@ -542,7 +558,7 @@ def handle_echo(event):
server = ae.start_server(
("localhost", 11112),
block=False,
ssl_context=server_context,
ssl_context=server_context(),
)

assoc = ae.associate(
Expand All @@ -567,7 +583,8 @@ def handle_echo(event):

assert assoc.is_released

def test_multiple_pdu_acc(self, server_context, client_context):
@pytest.mark.parametrize("server_context, tls_version", TLS_SERVER_CONTEXTS)
def test_multiple_pdu_acc(self, server_context, tls_version, client_context):
"""Test what happens if two PDUs are sent before the select call."""
events = []

Expand All @@ -585,7 +602,7 @@ def handle_echo(event):
server = ae.start_server(
("localhost", 11112),
block=False,
ssl_context=server_context,
ssl_context=server_context(),
evt_handlers=[(evt.EVT_C_ECHO, handle_echo)],
)

Expand Down
Loading