From aef0340027a76b657f02b18b359ce82452e72854 Mon Sep 17 00:00:00 2001 From: Jack Nagy Date: Mon, 5 Oct 2026 19:06:22 +0100 Subject: [PATCH 1/5] Answer a repeated cookie in either record framing _handle_record admitted a DTLS 1.0-framed HelloVerifyRequest only in sent_hello, while _handle_hello_verify_request answers one in sent_cookie_hello too. So a server that frames its challenges that way got its first answered and its second dropped, which is the OpenSSL behaviour that handler exists to avoid: both sides retransmit to the deadline with no alert. Found by @Jason-Morcos on #117. The four existing repeated-cookie tests call _handle_hello_verify_request directly, so none of them crossed the record layer where the gate sits. That is the same shape as the audit test which asserted the original repeated-HVR bug was correct. The version check also reads only the first message in the record, while _handle_handshake_fragment walks every message in it. A 1.0-framed record leading with a HelloVerifyRequest therefore carried a ServerHello straight into got_server_hello, which the framing test in test_dtls_psk_interop.py claims is refused -- true only while the ServerHello arrived in its own record. The exemption now travels with the record and admits one message. No peer is known to send a second 1.0-framed challenge: an OpenSSL server asked to re-challenge sends fatal alert 40 and one cookie, and the appliance frames 1.2. This is a consistency repair, not a measured break. Eight tests, five through the record layer; three fail without the fix and two pin behaviour it must not change. --- smartthings_local/protocol/_dtls_psk.py | 36 +++++-- tests/test_dtls_psk_hostile.py | 128 ++++++++++++++++++++++++ 2 files changed, 158 insertions(+), 6 deletions(-) diff --git a/smartthings_local/protocol/_dtls_psk.py b/smartthings_local/protocol/_dtls_psk.py index b6a8d57..18f16fd 100644 --- a/smartthings_local/protocol/_dtls_psk.py +++ b/smartthings_local/protocol/_dtls_psk.py @@ -490,6 +490,7 @@ def _handle_record(self, record: bytes) -> None: content_type = record[0] epoch = int.from_bytes(record[3:5], "big") fragment = record[_RECORD_HEADER:] + hello_verify_only = False if record[1:3] != _VERSION: # OpenSSL frames its HelloVerifyRequest as DTLS 1.0 under cookie # exchange; the interop tests pin that. The appliance does not: @@ -497,14 +498,24 @@ def _handle_record(self, record: bytes) -> None: # derives that version from the negotiated one, so other # generations are unmeasured rather than known to match. # - # Hence the exception, kept narrow. In this state a peer can - # already send a 1.2-framed HelloVerifyRequest, so accepting the - # same message in a second framing grants nothing further. + # Hence the exception, kept narrow. In either hello state a peer + # can already send a 1.2-framed HelloVerifyRequest, so accepting + # the same message in a second framing grants nothing further -- + # and both states have to be here, because a server that frames + # its first challenge this way frames its second one the same, + # and dropping that one leaves us retransmitting to the deadline: + # exactly the OpenSSL behaviour _handle_hello_verify_request + # exists to avoid. if not (record[1:3] == b"\xfe\xff" and epoch == 0 and content_type == _CT_HANDSHAKE - and self._state == "sent_hello" + and self._state in ("sent_hello", "sent_cookie_hello") and fragment[:1] == bytes([_HT_HELLO_VERIFY_REQUEST])): return + # The check above reads the first message in the record, while + # _handle_handshake_fragment walks every message in it, so the + # exemption has to travel with the record or anything packed + # behind the HelloVerifyRequest inherits it. + hello_verify_only = True seq = int.from_bytes(record[5:11], "big") expected_epoch = 1 if self._read_active else 0 @@ -529,7 +540,11 @@ def _handle_record(self, record: bytes) -> None: payload = fragment if content_type == _CT_HANDSHAKE: - self._handle_handshake_fragment(payload, authenticated=epoch > 0) + self._handle_handshake_fragment( + payload, + authenticated=epoch > 0, + hello_verify_only=hello_verify_only, + ) elif content_type == _CT_CHANGE_CIPHER_SPEC: if payload == b"\x01" and self._state == "sent_client_flight": self._read_active = True @@ -573,7 +588,11 @@ def _handle_alert(self, payload: bytes, *, authenticated: bool) -> None: self._fail(f"peer sent fatal alert {description}") def _handle_handshake_fragment( - self, payload: bytes, *, authenticated: bool = False + self, + payload: bytes, + *, + authenticated: bool = False, + hello_verify_only: bool = False, ) -> None: if self._state == "established" and authenticated: # Renegotiation is disabled on the appliance @@ -583,6 +602,11 @@ def _handle_handshake_fragment( offset = 0 while offset + _HANDSHAKE_HEADER <= len(payload): msg_type = payload[offset] + if hello_verify_only and msg_type != _HT_HELLO_VERIFY_REQUEST: + # A DTLS 1.0-framed record is admitted for one message only. + # Abandon the rest of it rather than reading a ServerHello + # out of a framing the state machine never accepted. + return length = int.from_bytes(payload[offset + 1:offset + 4], "big") msg_seq = int.from_bytes(payload[offset + 4:offset + 6], "big") frag_off = int.from_bytes(payload[offset + 6:offset + 9], "big") diff --git a/tests/test_dtls_psk_hostile.py b/tests/test_dtls_psk_hostile.py index 16a1765..d5db8ad 100644 --- a/tests/test_dtls_psk_hostile.py +++ b/tests/test_dtls_psk_hostile.py @@ -39,6 +39,35 @@ def record(content_type: int, epoch: int, seq: int, payload: bytes) -> bytes: ) +def framed_record( + content_type: int, epoch: int, seq: int, payload: bytes, version: bytes +) -> bytes: + """``record()`` with the record-header version left to the caller.""" + return ( + bytes([content_type]) + + version + + struct.pack("!H", epoch) + + seq.to_bytes(6, "big") + + struct.pack("!H", len(payload)) + + payload + ) + + +def handshake_message(msg_type: int, msg_seq: int, body: bytes) -> bytes: + length = len(body).to_bytes(3, "big") + return ( + bytes([msg_type]) + length + struct.pack("!H", msg_seq) + + (0).to_bytes(3, "big") + length + body + ) + + +def hello_verify(msg_seq: int, cookie: bytes) -> bytes: + """One HelloVerifyRequest message, its body always framing DTLS 1.2.""" + return handshake_message( + 3, msg_seq, b"\xfe\xfd" + bytes([len(cookie)]) + cookie + ) + + def peer_of(key_block: bytes) -> _Transform: return _Transform( key_block[32:64] + key_block[0:32] + key_block[80:96] + key_block[64:80] @@ -228,6 +257,105 @@ def test_identical_cookie_does_not_renumber_the_handshake(client): assert client._next_msg_seq == after_first +# -- the DTLS 1.0 framing exemption, driven through the record layer ------- +# +# The four tests above call _handle_hello_verify_request directly, which is +# how a record-layer admission bug survived them: the engine answered a +# repeated cookie framed as DTLS 1.2 and dropped the same message framed as +# DTLS 1.0, because the exemption was gated on `sent_hello` alone. Found by +# @Jason-Morcos on PR #117. Anything asserting which challenges are answered +# has to cross the record layer to mean it. + +FRAMINGS = [b"\xfe\xfd", b"\xfe\xff"] + + +@pytest.mark.parametrize("version", FRAMINGS) +def test_second_cookie_is_answered_under_either_framing(client, version): + client.bio_write(framed_record(22, 0, 0, hello_verify(0, b"\xAA" * 4), version)) + with pytest.raises(WantRead): + client.do_handshake() + assert (client._state, client._cookie) == ("sent_cookie_hello", b"\xAA" * 4) + client.bio_read() + + client.bio_write(framed_record(22, 0, 1, hello_verify(1, b"\xBB" * 4), version)) + with pytest.raises(WantRead): + client.do_handshake() + assert client._cookie == b"\xBB" * 4 + assert client._hello_verify_count == 1 + assert client._next_recv_msg_seq == 2 + # The replacement hello carries the new cookie, so it has to be on the wire. + assert b"\xBB" * 4 in client.bio_read() + + +@pytest.mark.parametrize("version", FRAMINGS) +def test_repeated_cookie_does_not_renumber_under_either_framing(client, version): + message = hello_verify(0, b"\xAA" * 4) + client.bio_write(framed_record(22, 0, 0, message, version)) + with pytest.raises(WantRead): + client.do_handshake() + after_first = client._next_msg_seq + + client.bio_write(framed_record(22, 0, 1, hello_verify(1, b"\xAA" * 4), version)) + with pytest.raises(WantRead): + client.do_handshake() + assert client._next_msg_seq == after_first + + +@pytest.mark.parametrize("version", FRAMINGS) +def test_cookie_retry_cap_holds_under_either_framing(client, version): + """A server that only ever re-challenges must not hold us open.""" + for index in range(40): + cookie = bytes([index, index, index, index]) + client.bio_write( + framed_record(22, 0, index, hello_verify(index, cookie), version) + ) + try: + client.do_handshake() + except WantRead: + continue + except DtlsError: + break + assert client._failed is not None + + +def test_dtls10_record_admits_nothing_behind_the_hello_verify_request(client): + """The exemption covers one message, not the record it arrived in. + + _handle_handshake_fragment walks every message in a record while the + version check reads only the first, so a 1.0-framed record leading with + a HelloVerifyRequest carried a ServerHello straight into the state + machine -- the framing test in test_dtls_psk_interop.py asserts that is + refused, and it was, only while the ServerHello arrived in its own + record. + """ + server_hello = handshake_message( + 2, 1, b"\xfe\xfd" + bytes(range(32)) + b"\x00" + + struct.pack("!H", 0xC037) + b"\x00" + ) + client.bio_write( + framed_record( + 22, 0, 0, hello_verify(0, b"\xAA" * 4) + server_hello, b"\xfe\xff" + ) + ) + with pytest.raises(WantRead): + client.do_handshake() + assert client._state == "sent_cookie_hello" + assert client._server_random == b"" + + +def test_dtls10_record_not_leading_with_hello_verify_is_dropped(client): + """The first-message check is what admits the record at all.""" + server_hello = handshake_message( + 2, 0, b"\xfe\xfd" + bytes(range(32)) + b"\x00" + + struct.pack("!H", 0xC037) + b"\x00" + ) + client.bio_write(framed_record(22, 0, 0, server_hello, b"\xfe\xff")) + with pytest.raises(WantRead): + client.do_handshake() + assert client._state == "sent_hello" + assert client._server_random == b"" + + def test_zero_length_fragments_terminate(client): payload = ( bytes([2]) + (40).to_bytes(3, "big") + struct.pack("!H", 9) From 974d1b5ff3857f28cd1dff54c34f547e29f99531 Mon Sep 17 00:00:00 2001 From: Jack Nagy Date: Mon, 5 Oct 2026 19:20:43 +0100 Subject: [PATCH 2/5] Carry a PSK credential through the library's own DTLS client PskAuth now answers the connection factory the previous commit added, and a session reaches the pure-Python ECDHE-PSK engine instead of OpenSSL. The credential class that was 100% unusable here is the one that gains it: an OwnerPSK whose identity happens to contain a zero byte. The guard moves rather than going away. 475178e measured the reason it exists -- OpenSSL 4.0.0 puts a 16-byte identity with a NUL at byte 8 on the wire as 8 bytes, raising nothing locally -- and that is a fact about OpenSSL's callback, not about the credential. So configure_context refuses such an identity explicitly and names the path that does carry it, while validate_identity stops refusing it and goes back to being pure: a caller showing a user why their credential was rejected cannot have that answer move with the environment, which is the mistake the closed #115 made by checking for a shared library on disk. No OpenSSL callback is built for an unpresentable identity at all, so the guard is the absence of the thing that would truncate rather than a flag beside it. The measured figures stay in the docstring: ~6% of uniformly random 16-byte identities, ~5% of UUIDv4s. Credential material still reaches no attribute, public or private. The engine factory is a closure captured in __init__, the same way the OpenSSL callback already was, because test_psk_auth asserts even _identity and _key are absent. getattr dispatch on a private name is the existing house pattern here, not a new one: connect() already looks up _authenticated_server_identity that way. A session's PSK path is now covered end to end against a real OpenSSL DTLS server behind a fake socket, with and without cookie exchange, carrying an identity that leads with a zero byte: the full flight, the identity read back out of the ClientKeyExchange in the clear, application data both ways, and one record per datagram as TizenRT requires. Those tests fail if the factory is removed. Public API change, so docs/api.md, README.md and the contract test move with it. Downstream still has its own copy of the restriction -- localthings credentials.py raises PskIdentityZeroByte -- so a user does not see this until that goes too. --- README.md | 6 +- docs/api.md | 2 +- smartthings_local/protocol/auth.py | 98 ++++++++++++---- tests/test_dtls_probe.py | 28 ++++- tests/test_psk_auth.py | 73 ++++++++---- tests/test_psk_session_integration.py | 162 ++++++++++++++++++++++++++ tests/test_public_api_contract.py | 7 +- 7 files changed, 324 insertions(+), 52 deletions(-) create mode 100644 tests/test_psk_session_integration.py diff --git a/README.md b/README.md index c87da42..a0a4340 100644 --- a/README.md +++ b/README.md @@ -392,9 +392,11 @@ sess = DtlsCoapSession("192.0.2.100", 49154, auth=auth) The identity must be the raw 16-byte OCF UUID and the key exactly 16 or 32 bytes. `PskAuth` selects only `ECDHE-PSK-AES128-CBC-SHA256` and does not acquire, derive, provision, rotate, or persist credentials. Ownership transfer and credential discovery are outside this package. -An identity containing a zero byte is rejected, and that limit is OpenSSL's rather than the appliance's. An OCF device takes the identity as bytes with an explicit length, so a zero byte means nothing to it, but OpenSSL's DTLS 1.2 PSK client callback returns the identity as a C string. Measured against OpenSSL 4.0.0, a 16-byte identity with a NUL at byte 8 reaches the wire as 8 bytes and the handshake raises nothing locally, so the appliance answers a truncated identity it has never seen. DTLS 1.2 offers no length-carrying PSK callback, so such a credential is unusable here: roughly 6% of uniformly random 16-byte identities, and about 5% of UUIDv4s, which have two fixed bytes. +An identity containing a zero byte works in a session. A `DtlsCoapSession` carries a PSK credential through this package's own DTLS 1.2 ECDHE-PSK client, which frames the identity with an explicit length, exactly as an OCF device reads it. -Code holding a credential can check it, and report why, before building a provider or storing anything: +One path cannot carry such an identity, and that limit is OpenSSL's rather than the appliance's: `PskAuth.configure_context` raises for it instead of truncating it. OpenSSL's DTLS 1.2 PSK client callback returns the identity as a C string, and measured against OpenSSL 4.0.0 a 16-byte identity with a NUL at byte 8 reaches the wire as 8 bytes with nothing raising locally, so the appliance would answer a truncated identity it has never seen. DTLS 1.2 offers no length-carrying PSK callback. That affects roughly 6% of uniformly random 16-byte identities, and about 5% of UUIDv4s, which have two fixed bytes -- so code that configures a context itself, rather than handing the provider to a session, should expect the refusal. + +Code holding a credential can check it, and report why, before building a provider or storing anything. The check is pure -- it reads the identity and nothing else, so the answer does not move with the environment: ```python try: diff --git a/docs/api.md b/docs/api.md index 4c2a74f..8704097 100644 --- a/docs/api.md +++ b/docs/api.md @@ -157,7 +157,7 @@ PskAuth(*, identity: bytes, key: bytes) *class*: DTLS authentication using an existing OCF PSK credential. - `configure_context(context: OpenSSL.SSL.Context) -> None`: Configure one context for the narrow Samsung OCF PSK profile. -- `validate_identity(identity: bytes) -> None`: Raise unless `identity` is one OpenSSL can put on the wire. +- `validate_identity(identity: bytes) -> None`: Raise unless `identity` is one a session can put on the wire. #### `SamsungServerProfile` diff --git a/smartthings_local/protocol/auth.py b/smartthings_local/protocol/auth.py index e2fb505..d98b89b 100644 --- a/smartthings_local/protocol/auth.py +++ b/smartthings_local/protocol/auth.py @@ -14,6 +14,8 @@ from cryptography.x509.oid import ExtensionOID from OpenSSL import SSL, _util, crypto +from ._dtls_psk import DtlsPskClient + logger = logging.getLogger(__name__) _OCF_ROOT_CA = str(Path(__file__).with_name("ocf_root_ca.pem")) @@ -556,45 +558,47 @@ def _authenticated_server_identity(self, connection) -> UUID | None: class PskAuth: """DTLS authentication using an existing OCF PSK credential. - The identity must be a raw 16-byte OCF UUID that OpenSSL can present, as - described on :meth:`validate_identity`. The key must contain 16 or 32 - bytes. Credential material is intentionally not exposed as public + The identity must be a raw 16-byte OCF UUID; the key must contain 16 or + 32 bytes. Credential material is intentionally not exposed as public attributes and is never included in this provider's representation. A configured context must not outlive this provider; ``DtlsCoapSession`` enforces that lifetime by retaining its provider. + + A session reaches this credential through the library's own DTLS 1.2 + ECDHE-PSK client, which carries the identity with an explicit length and + so takes any 16 bytes. :meth:`configure_context` is the OpenSSL path, + kept for direct callers, and it cannot carry an identity containing a + zero byte -- see :meth:`validate_identity`. """ - __slots__ = ("_callback",) + __slots__ = ("_callback", "_engine") @staticmethod def validate_identity(identity: bytes) -> None: - """Raise unless ``identity`` is one OpenSSL can put on the wire. + """Raise unless ``identity`` is one a session can put on the wire. A caller holding a credential can check it here, and report the - reason, before building a provider or storing anything. - - An OCF appliance takes the identity as bytes with an explicit length, - so a zero byte is unremarkable to the device. OpenSSL's DTLS 1.2 PSK - client callback returns the identity as a C string, which leaves no - way to express one: a NUL truncates the identity on the wire, and the - handshake then fails against the truncated value with no local error - to point at the cause. Roughly 6% of uniformly random 16-byte - identities carry a zero byte, and about 5% of UUIDv4s, whose version - and variant bytes can never be zero. There is no length-carrying PSK - callback for DTLS 1.2 to fall back on, so such a credential cannot be - used through this library. + reason, before building a provider or storing anything. This is a + pure check: it depends on the identity and nothing else. + + A zero byte is accepted. An OCF appliance takes the identity as + bytes with an explicit length, so a zero byte is unremarkable to the + device, and the library's own DTLS client frames it the same way. + What cannot carry it is OpenSSL: its DTLS 1.2 PSK client callback + returns the identity as a C string, so a NUL truncates it on the + wire and the handshake fails against the truncated value with no + local error to point at the cause. That constraint belongs to one + backend rather than to the credential, so :meth:`configure_context` + refuses such an identity explicitly and a session does not. + + The size of that difference, measured: roughly 6% of uniformly + random 16-byte identities carry a zero byte, and about 5% of + UUIDv4s, whose version and variant bytes can never be zero. """ if type(identity) is not bytes: raise TypeError("identity must be bytes") if len(identity) != 16: raise ValueError("identity must be a raw 16-byte OCF UUID") - if b"\x00" in identity: - raise ValueError( - "identity cannot contain a NUL byte: OpenSSL presents a " - "DTLS 1.2 PSK identity as a C string, so a NUL truncates it " - "and the appliance would be sent a shorter identity than the " - "one supplied" - ) def __init__(self, *, identity: bytes, key: bytes) -> None: if type(identity) is not bytes or type(key) is not bytes: @@ -603,6 +607,22 @@ def __init__(self, *, identity: bytes, key: bytes) -> None: if len(key) not in (16, 32): raise ValueError("key must be 16 or 32 bytes") + def engine(*, mtu: int) -> DtlsPskClient: + return DtlsPskClient(identity, key, mtu) + + # Held in a closure rather than an attribute, the same way the + # OpenSSL callback below is: no attribute, public or private, holds + # credential material, and test_psk_auth pins that. + object.__setattr__(self, "_engine", engine) + + if b"\x00" in identity: + # OpenSSL would truncate this identity at the zero byte, so no + # callback is built for it at all and configure_context refuses. + # The engine a session uses carries an explicit length and is + # unaffected; see validate_identity. + object.__setattr__(self, "_callback", None) + return + ffi = _util.ffi @ffi.callback(_PSK_CLIENT_CALLBACK_CDEF) @@ -644,8 +664,36 @@ def __repr__(self) -> str: """Return a representation that never includes credential material.""" return "PskAuth()" + def _create_dtls_connection(self, *, mtu: int): + """Return a DTLS connection carrying this credential. + + Private, and found by ``DtlsCoapSession`` and the diagnostic through + ``getattr`` rather than through ``AuthenticationProvider``: that + Protocol is ``runtime_checkable`` and gated on with ``isinstance``, + so a required method here would reject third-party providers. + + The engine speaks one ciphersuite and no X.509, and it frames the + identity with an explicit length, which is the whole reason this + path exists alongside :meth:`configure_context`. + """ + return self._engine(mtu=mtu) + def configure_context(self, context: SSL.Context) -> None: - """Configure one context for the narrow Samsung OCF PSK profile.""" + """Configure one context for the narrow Samsung OCF PSK profile. + + Raises when the identity contains a zero byte. OpenSSL presents a + DTLS 1.2 PSK identity as a C string, so it would send a shorter + identity than the one supplied and nothing local would say so. A + session does not come through here and has no such limit. + """ + if self._callback is None: + raise ValueError( + "this identity contains a NUL byte and cannot be presented " + "through OpenSSL, which carries a DTLS 1.2 PSK identity as a " + "C string: it truncates there, and the appliance would be " + "sent a shorter identity than the one supplied. A " + "DtlsCoapSession carries this credential without OpenSSL" + ) setter = getattr(_util.lib, "SSL_CTX_set_psk_client_callback", None) if setter is None: raise RuntimeError( diff --git a/tests/test_dtls_probe.py b/tests/test_dtls_probe.py index a7a6454..927d378 100644 --- a/tests/test_dtls_probe.py +++ b/tests/test_dtls_probe.py @@ -838,11 +838,11 @@ def test_cli_refuses_a_certificate_and_a_psk_together(capsys): def test_cli_reports_an_unusable_psk_credential_without_a_traceback(capsys): - # A NUL in the identity is rejected by PskAuth, and the CLI has to render - # that as a usage error rather than an exception. + # A malformed credential is rejected by PskAuth, and the CLI has to + # render that as a usage error rather than an exception. result = p._main([ '127.0.0.1', '5684', '--diagnostic', - '--psk-identity', '0102030405060708000a0b0c0d0e0f10', + '--psk-identity', '0102030405060708', '--psk-key', '00' * 16, ]) @@ -850,6 +850,28 @@ def test_cli_reports_an_unusable_psk_credential_without_a_traceback(capsys): assert 'invalid PSK credential' in capsys.readouterr().out +def test_cli_accepts_an_identity_containing_a_zero_byte(monkeypatch, capsys): + # It used to be a usage error, because OpenSSL could not carry it. The + # diagnostic now reaches the same engine a session uses, so the + # credential is valid and the run has to proceed to the appliance. + diagnosed = {} + + def record(host, port, **kwargs): + diagnosed['auth'] = kwargs.get('auth') + return p.ProbeResult(host, port) + + monkeypatch.setattr(p, 'diagnose_dtls_handshake', record) + result = p._main([ + '127.0.0.1', '5684', '--diagnostic', + '--psk-identity', '0102030405060708000a0b0c0d0e0f10', + '--psk-key', '00' * 16, + ]) + + assert 'invalid PSK credential' not in capsys.readouterr().out + assert result != 2 + assert diagnosed['auth'] is not None + + def test_encrypted_records_do_not_invent_handshake_messages(monkeypatch): # A handshake record after ChangeCipherSpec is encrypted, so its first # byte is ciphertext. Read as a message type it names whatever it diff --git a/tests/test_psk_auth.py b/tests/test_psk_auth.py index fbc7bdb..e18aaed 100644 --- a/tests/test_psk_auth.py +++ b/tests/test_psk_auth.py @@ -84,25 +84,43 @@ def test_psk_auth_rejects_invalid_identity_lengths(identity_length): PskAuth(identity=b"i" * identity_length, key=_KEY) -def test_psk_auth_rejects_identity_with_nul_byte(): - with pytest.raises(ValueError, match="cannot contain a NUL"): - PskAuth(identity=b"i" * 15 + b"\x00", key=_KEY) +def test_a_nul_identity_builds_a_provider_and_a_session_connection(): + # The restriction is OpenSSL's, not the credential's: a session reaches + # this credential through the library's own DTLS client, which carries + # the identity with an explicit length. So the provider exists and can + # produce a connection, and only configure_context refuses. + identity = b"i" * 15 + b"\x00" + provider = PskAuth(identity=identity, key=_KEY) + assert PskAuth.validate_identity(identity) is None + assert provider._create_dtls_connection(mtu=1200) is not None -def test_nul_rejection_explains_the_truncation_it_prevents(): + +def test_openssl_path_refuses_a_nul_identity_rather_than_truncating_it(): # Measured against OpenSSL 4.0.0: a 16-byte identity with a NUL at byte 8 - # goes on the wire as 8 bytes and the handshake raises nothing locally, so - # the guard is the only thing standing between a caller and a silently - # wrong identity. The message has to carry that, because an appliance - # answers the truncated value with unknown_psk_identity and nothing else - # points back here. + # goes on the wire as 8 bytes and the handshake raises nothing locally, + # so this guard is the only thing standing between a direct + # configure_context caller and a silently wrong identity. The message has + # to carry that, because an appliance answers the truncated value with + # unknown_psk_identity and nothing else points back here. + provider = PskAuth(identity=b"i" * 15 + b"\x00", key=_KEY) + with pytest.raises(ValueError) as raised: - PskAuth(identity=b"i" * 15 + b"\x00", key=_KEY) + provider.configure_context(MagicMock()) message = str(raised.value) assert "C string" in message assert "truncates" in message assert "shorter identity" in message + # And it names the path that does carry the credential. + assert "DtlsCoapSession" in message + + +def test_no_openssl_callback_is_built_for_an_unpresentable_identity(): + # Nothing that could truncate the identity is constructed at all, so the + # guard is the absence of the callback rather than a flag beside it. + assert PskAuth(identity=b"i" * 15 + b"\x00", key=_KEY)._callback is None + assert PskAuth(identity=_IDENTITY, key=_KEY)._callback is not None def test_validate_identity_checks_a_credential_before_one_is_assembled(): @@ -111,13 +129,26 @@ def test_validate_identity_checks_a_credential_before_one_is_assembled(): # and raises what the constructor raises. assert PskAuth.validate_identity(_IDENTITY) is None - with pytest.raises(ValueError, match="cannot contain a NUL"): - PskAuth.validate_identity(b"i" * 15 + b"\x00") + with pytest.raises(ValueError, match="raw 16-byte OCF UUID"): + PskAuth.validate_identity(b"i" * 15) + + +def test_validate_identity_is_a_pure_check(): + """It must not depend on anything but the identity. + + The closed #115 made this environment-dependent, so a staticmethod + raised or not depending on whether a shared library was on disk. A + caller showing a user the reason their credential was refused cannot + have that answer move underneath them. + """ + identity = b"i" * 15 + b"\x00" + assert PskAuth.validate_identity(identity) is None + assert PskAuth.validate_identity(identity) is None @pytest.mark.parametrize( "identity", - [b"i" * 15 + b"\x00", b"i" * 15, b"i" * 17, b""], + [b"i" * 15, b"i" * 17, b""], ) def test_validate_identity_rejects_what_the_constructor_rejects(identity): # One code path, so the reason a caller can show a user is the same @@ -401,8 +432,6 @@ def test_session_accepts_psk_provider_without_legacy_certificate_material(): def test_psk_handshake_rejection_does_not_expose_credentials(): provider = PskAuth(identity=_IDENTITY, key=_KEY) session = DtlsCoapSession("appliance.invalid", 49154, auth=provider) - context = MagicMock() - context._context = object() connection = MagicMock() connection.do_handshake.side_effect = SSL.Error() udp_socket = MagicMock() @@ -410,8 +439,11 @@ def test_psk_handshake_rejection_does_not_expose_credentials(): with ( patch.object(auth_module, "_util", _fake_openssl_util(lambda *_: None)), - patch.object(session_module.SSL, "Context", return_value=context), - patch.object(session_module.SSL, "Connection", return_value=connection), + patch.object( + PskAuth, + "_create_dtls_connection", + lambda _self, *, mtu: connection, + ), patch.object( session_module, "open_host_filtered_udp_socket", @@ -528,7 +560,8 @@ def test_openssl_sends_a_clean_identity_whole(): def test_a_nul_identity_would_reach_the_wire_truncated(): - # The reason PskAuth refuses this rather than passing it through. + # The reason configure_context refuses this rather than passing it + # through. A session does not come this way and has no such limit. # OpenSSL's DTLS 1.2 PSK client callback returns the identity as a # C string and takes its strlen, so everything from the NUL onward is # dropped and nothing raises. An appliance would be asked to @@ -544,5 +577,5 @@ def test_a_nul_identity_would_reach_the_wire_truncated(): assert declared == 8 assert wire == identity[:8] - with pytest.raises(ValueError, match="cannot contain a NUL"): - PskAuth(identity=identity, key=_KEY) + with pytest.raises(ValueError, match="truncates"): + PskAuth(identity=identity, key=_KEY).configure_context(MagicMock()) diff --git a/tests/test_psk_session_integration.py b/tests/test_psk_session_integration.py new file mode 100644 index 0000000..60b9a6f --- /dev/null +++ b/tests/test_psk_session_integration.py @@ -0,0 +1,162 @@ +"""A PSK credential driven end to end through DtlsCoapSession. + +The seam, the handshake driver and the pure-Python engine together, against +a real OpenSSL DTLS PSK server. No appliance and no credential of our own: +OpenSSL plays the server over memory BIOs behind a fake socket, which is how +the rest of this suite avoids real sockets, so these run in CI. + +The identity used throughout contains a zero byte. That is the whole point +of the engine -- OpenSSL's client callback would truncate it -- so a test +that proves the wiring has to carry one. +""" + +from __future__ import annotations + +import socket + +import pytest +from OpenSSL import SSL, _util + +from smartthings_local.protocol import dtls_session +from smartthings_local.protocol.auth import PskAuth +from smartthings_local.protocol.coap import split_dtls as _split_dtls +from smartthings_local.protocol.dtls_session import DtlsCoapSession +from smartthings_local.protocol.endpoint import ResolvedUdpEndpoint + +IDENTITY = bytes(range(16)) # leads with 0x00 +KEY = b"k" * 16 + + +def _psk_server(*, cookies: bool): + context = SSL.Context(SSL.DTLS_METHOD) + context.set_cipher_list(b"ECDHE-PSK-AES128-CBC-SHA256:@SECLEVEL=0") + seen = [] + + @_util.ffi.callback( + "unsigned int(*)(SSL *, const char *, unsigned char *, unsigned int)" + ) + def psk_callback(_ssl, _identity, output, capacity): + if capacity < len(KEY): + return 0 + _util.ffi.memmove(output, KEY, len(KEY)) + return len(KEY) + + _util.lib.SSL_CTX_set_psk_server_callback(context._context, psk_callback) + if cookies: + context.set_cookie_generate_callback(lambda _c: b"offline-cookie") + context.set_cookie_verify_callback( + lambda _c, cookie: cookie == b"offline-cookie" + ) + context.set_options(SSL.OP_COOKIE_EXCHANGE) + server = SSL.Connection(context, None) + server.set_accept_state() + # The callbacks must outlive the context, as they do on a provider. + server._test_keepalive = (psk_callback, context) + return server, seen + + +class _ServerSocket: + """A socket whose peer is an in-process OpenSSL DTLS server.""" + + def __init__(self, server): + self.server = server + self.inbound: list[bytes] = [] + self.sent: list[bytes] = [] + self.closed = False + self.completed = False + + def send(self, record): + self.sent.append(record) + self.server.bio_write(record) + try: + self.server.do_handshake() + self.completed = True + except SSL.WantReadError: + pass + try: + self.inbound.extend(_split_dtls(self.server.bio_read(65535))) + except SSL.WantReadError: + pass + return len(record) + + def recv(self, _size=65535): + if not self.inbound: + raise socket.timeout() + return self.inbound.pop(0) + + def settimeout(self, _timeout): + return None + + def close(self): + self.closed = True + + # Deliberately no fileno(): connect() documents the timeout-driven path + # for "structural socket adapters that intentionally expose no file + # descriptor", and taking it keeps the handshake off select(). + + +def _install(monkeypatch, data_socket): + endpoint = ResolvedUdpEndpoint(socket.AF_INET, ("192.0.2.10", 49154)) + monkeypatch.setattr( + dtls_session, + "open_host_filtered_udp_socket", + lambda *_a, **_k: (data_socket, endpoint), + ) + return endpoint + + +@pytest.mark.parametrize("cookies", [False, True]) +def test_session_completes_a_psk_handshake_with_a_nul_identity( + monkeypatch, cookies +): + server, _ = _psk_server(cookies=cookies) + data_socket = _ServerSocket(server) + _install(monkeypatch, data_socket) + session = DtlsCoapSession("appliance.invalid", 49154, + auth=PskAuth(identity=IDENTITY, key=KEY)) + + session.connect(timeout=5.0) + + assert data_socket.completed, "the OpenSSL server never finished" + assert session.conn is not None + assert session.server_certificate_identity is None + assert not data_socket.closed + + # The credential really crossed the wire whole, in the clear, ahead of + # ChangeCipherSpec: a 16-byte identity whose first byte is a NUL. + client_key_exchanges = [ + record for record in data_socket.sent + if record[0] == 22 and record[3:5] == b"\0\0" and record[13] == 16 + ] + assert len(client_key_exchanges) == 1 + body = client_key_exchanges[0][13 + 12:] + assert body[:2] == b"\0\x10" + assert body[2:18] == IDENTITY + + # Each record went out as its own datagram, which TizenRT requires. + for record in data_socket.sent: + assert len(_split_dtls(record)) == 1 + + +def test_application_data_round_trips_over_the_session(monkeypatch): + server, _ = _psk_server(cookies=False) + data_socket = _ServerSocket(server) + _install(monkeypatch, data_socket) + session = DtlsCoapSession("appliance.invalid", 49154, + auth=PskAuth(identity=IDENTITY, key=KEY)) + session.connect(timeout=5.0) + + session.conn.send(b"coap request") + data_socket.server.bio_write(session.conn.bio_read(65535)) + assert data_socket.server.recv(65535) == b"coap request" + + data_socket.server.send(b"coap response") + session.conn.bio_write(data_socket.server.bio_read(65535)) + assert session.conn.recv(65535) == b"coap response" + + +def test_a_session_still_refuses_to_configure_an_openssl_context(): + """The engine lifting the restriction must not lift it for OpenSSL.""" + provider = PskAuth(identity=IDENTITY, key=KEY) + with pytest.raises(ValueError, match="truncates"): + provider.configure_context(SSL.Context(SSL.DTLS_METHOD)) diff --git a/tests/test_public_api_contract.py b/tests/test_public_api_contract.py index b6e2aeb..8d6a691 100644 --- a/tests/test_public_api_contract.py +++ b/tests/test_public_api_contract.py @@ -263,7 +263,12 @@ def test_psk_identity_validation_is_reachable_without_a_key(): assert PskAuth.validate_identity(b"i" * 16) is None with pytest.raises(ValueError): - PskAuth.validate_identity(b"i" * 15 + b"\x00") + PskAuth.validate_identity(b"i" * 15) + + # A zero byte is accepted here: it is OpenSSL that cannot carry one, and + # a session does not go through OpenSSL. configure_context refuses it, + # which test_psk_auth pins. + assert PskAuth.validate_identity(b"i" * 15 + b"\x00") is None def test_owner_psk_derivation_keeps_every_security_input_explicit(): From 30807b16659c39d6dc232eb8d8f091156e75697f Mon Sep 17 00:00:00 2001 From: Jack Nagy Date: Mon, 5 Oct 2026 19:24:37 +0100 Subject: [PATCH 3/5] Pin the cryptography floor the package now declares The engine commit added cryptography>=38.0 to dependencies, but the floor job pins cbor2, pyOpenSSL and pytest and then installs with --no-deps, so nothing ever resolved against that floor: cryptography arrived as whatever pyOpenSSL 23.1.0 happened to pull, around 40.x. A declared floor no job exercises is not a floor. Verified at 38.0.0 on Python 3.11 with the pinned cbor2 and pyOpenSSL: the whole suite passes, which is unsurprising -- AES-CBC, HMAC, EC key generation and from_encoded_point all long predate it. --- .github/workflows/ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5dea2da..b006071 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,6 +60,7 @@ jobs: run: >- python -m pip install "cbor2==5.6.0" + "cryptography==38.0.0" "pyOpenSSL==23.1.0" "pytest==8.0.0" - if: matrix.mode == 'floor' From 70d949594f6b766da5341d4c07eefb433b2dcfc6 Mon Sep 17 00:00:00 2001 From: Jack Nagy Date: Mon, 5 Oct 2026 19:53:37 +0100 Subject: [PATCH 4/5] Pin that close() still releases the peer through the engine The close machinery stays above the seam, which was the explicit ask, but nothing exercised it on this path. It needs to: close_notify matters more for a PSK session than for a certificate one. CAdecryptSsl runs SetupCipher only for an endpoint with no existing peer entry (ca_adapter_net_ssl.c:2177-2191), and the suite list it builds is one file-static array (:323) rebuilt in place (:1542) and handed to mbedtls_ssl_conf_ciphersuites as a pointer (:1586), which ssl_srv.c reads live while parsing a ClientHello. So a ClientHello landing on a reused peer entry is matched against whatever the last SetupCipher left there. Our ClientHello offers exactly one suite, so that is binary: present and selected, or handshake_failure with nothing naming the stale entry as the cause. A certificate ClientHello offers a list and degrades more gently. Verified: one datagram, one alert record, epoch 1 so encrypted under the session keys, and the OpenSSL server reports an orderly close rather than our own byte inspection of it. --- tests/test_psk_session_integration.py | 42 +++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/test_psk_session_integration.py b/tests/test_psk_session_integration.py index 60b9a6f..8be7cc6 100644 --- a/tests/test_psk_session_integration.py +++ b/tests/test_psk_session_integration.py @@ -160,3 +160,45 @@ def test_a_session_still_refuses_to_configure_an_openssl_context(): provider = PskAuth(identity=IDENTITY, key=KEY) with pytest.raises(ValueError, match="truncates"): provider.configure_context(SSL.Context(SSL.DTLS_METHOD)) + + +def test_close_releases_the_peer_through_the_engine(monkeypatch): + """close() must still emit an encrypted close_notify, as its own datagram. + + This is the machinery that has to stay above the seam, and it matters + more on this path than on the certificate one. The appliance frees a + peer entry on close_notify, and its CAdecryptSsl only runs SetupCipher + for an endpoint with no existing peer entry + (ca_adapter_net_ssl.c:2177-2191), so a reused peer entry keeps whatever + the shared g_cipherSuitesList (:323, rebuilt in place at :1542 and + handed to mbedtls_ssl_conf_ciphersuites at :1586) last held. Our + ClientHello offers exactly one suite, so a stale entry without + ECDHE-PSK in that list is a handshake_failure with nothing naming the + cause. A clean close is what prevents it. + """ + server, _ = _psk_server(cookies=True) + data_socket = _ServerSocket(server) + _install(monkeypatch, data_socket) + session = DtlsCoapSession("appliance.invalid", 49154, + auth=PskAuth(identity=IDENTITY, key=KEY)) + session.connect(timeout=5.0) + before = len(data_socket.sent) + + session.close() + + closing = data_socket.sent[before:] + records = [record for datagram in closing + for record in _split_dtls(datagram)] + alerts = [record for record in records if record[0] == 21] + assert len(alerts) == 1, records + # Epoch 1: encrypted under the session keys, which is the only form the + # appliance can authenticate. + assert int.from_bytes(alerts[0][3:5], "big") == 1 + # One record per datagram, all the way through the close path. + for datagram in closing: + assert len(_split_dtls(datagram)) == 1 + assert data_socket.closed + + # The server's own verdict, not just our byte inspection. + with pytest.raises(SSL.ZeroReturnError): + server.recv(65535) From 3417c382df15bde5afe036e51002919e99dbb22a Mon Sep 17 00:00:00 2001 From: Jack Nagy Date: Mon, 5 Oct 2026 20:17:56 +0100 Subject: [PATCH 5/5] Validate the session's mtu under the rule the probe already had DtlsCoapSession took mtu straight onto an attribute without checking it, and OpenSSL consults the value only when it has to fragment a flight, so an unusable number was invisible: measured here, set_ciphertext_mtu accepts 100 and 70000 alike and the ClientHello goes out at 212 bytes either way. That stopped being harmless when a session gained a second backend. DtlsPskClient validates 256-65535 of its own, so mtu=100 constructed and then raised from connect() on a PSK provider while a certificate provider carried on -- the same argument meaning two different things depending on the credential, reported from the wrong place. dtls_probe has validated 576-16384 since it was written, with the message "mtu is outside the safe UDP range", so the project already had an answer and the session was the outlier. That check moves to dtls_handshake, which both callers already import from, and the session uses it: one rule, three callers, the failure at the constructor where the argument was supplied. Nothing in the repository or the bridge passes a non-default mtu, and the range the session now enforces is inside the engine's, so the engine's own check is unreachable from a session and stays as a guard for direct use. --- smartthings_local/protocol/dtls_handshake.py | 21 ++++++++++ smartthings_local/protocol/dtls_probe.py | 7 +--- smartthings_local/protocol/dtls_session.py | 2 + tests/test_dtls_connection_seam.py | 40 ++++++++++++++++++++ 4 files changed, 65 insertions(+), 5 deletions(-) diff --git a/smartthings_local/protocol/dtls_handshake.py b/smartthings_local/protocol/dtls_handshake.py index ec5be7b..35ef73a 100644 --- a/smartthings_local/protocol/dtls_handshake.py +++ b/smartthings_local/protocol/dtls_handshake.py @@ -22,6 +22,27 @@ _DTLS_EPOCH_ZERO = b'\x00\x00' _DTLS_VERSIONS = frozenset((b'\xfe\xff', b'\xfe\xfd')) _MAX_CLEANUP_TRANSCRIPT_RECORDS = 32 +# The probe has validated this range since it was written; the session never +# did, and the two have to agree now that a session can carry an engine with +# a range of its own. +_MIN_MTU = 576 +_MAX_MTU = 16384 + + +def _validate_mtu(mtu): + """Raise unless ``mtu`` is a datagram size every DTLS backend here takes. + + One rule rather than one per caller. OpenSSL silently accepts anything + and only consults the value when it has to fragment a flight, so an + out-of-range mtu used to be invisible until something large went out. + The pure-Python PSK engine validates its own argument, which would + otherwise make the same number raise on one provider and pass on + another, from connect() rather than from where it was supplied. + """ + if isinstance(mtu, bool) or not isinstance(mtu, int): + raise TypeError('mtu must be an integer') + if not _MIN_MTU <= mtu <= _MAX_MTU: + raise ValueError('mtu is outside the safe UDP range') def _complete_epoch_zero_handshake_types(record): diff --git a/smartthings_local/protocol/dtls_probe.py b/smartthings_local/protocol/dtls_probe.py index 041ef4c..bc0216a 100644 --- a/smartthings_local/protocol/dtls_probe.py +++ b/smartthings_local/protocol/dtls_probe.py @@ -42,7 +42,7 @@ from ..errors import ProbeError from .auth import PskAuth, _DTLS_CIPHERS, _OCF_ROOT_CA, _load_pem_chain from .coap import split_dtls -from .dtls_handshake import _drive_dtls_handshake +from .dtls_handshake import _drive_dtls_handshake, _validate_mtu from .endpoint import open_host_filtered_udp_socket # DTLS record content types (RFC 6347 ยง4.1) @@ -187,10 +187,7 @@ def _validate_liveness_options(port, retries, timeout, mtu): raise TypeError('timeout must be a number') if not math.isfinite(timeout) or not 0 < timeout <= 30: raise ValueError('timeout must be greater than zero and at most 30') - if isinstance(mtu, bool) or not isinstance(mtu, int): - raise TypeError('mtu must be an integer') - if not 576 <= mtu <= 16384: - raise ValueError('mtu is outside the safe UDP range') + _validate_mtu(mtu) def _validate_probe_family(family): diff --git a/smartthings_local/protocol/dtls_session.py b/smartthings_local/protocol/dtls_session.py index c30468d..50ee659 100644 --- a/smartthings_local/protocol/dtls_session.py +++ b/smartthings_local/protocol/dtls_session.py @@ -95,6 +95,7 @@ _HvrPeerCleanupTranscript, _drive_dtls_handshake, _HandshakeCancelled, + _validate_mtu, ) from .endpoint import open_host_filtered_udp_socket @@ -570,6 +571,7 @@ def __init__(self, host, port, cert_path=None, key_path=None, *, # cannot express which delivery answered the register CON, nor # which query-qualified relation it belongs to. self.on_observe_delivery = on_observe_delivery + _validate_mtu(mtu) self.mtu = mtu self._min_req_interval = 1.0 / rate_limit_rps self._write_max_attempts = max(1, int(write_max_attempts)) diff --git a/tests/test_dtls_connection_seam.py b/tests/test_dtls_connection_seam.py index 37f1f04..6a069f7 100644 --- a/tests/test_dtls_connection_seam.py +++ b/tests/test_dtls_connection_seam.py @@ -248,3 +248,43 @@ def test_port_probe_flight_stays_on_openssl(): parameters = inspect.signature(dtls_probe._client_hello_flight).parameters assert set(parameters) == {"mtu"} + + +# -- one mtu rule, so a number cannot mean different things per provider ---- + + +@pytest.mark.parametrize("mtu", [100, 575, 16385, 70000]) +def test_out_of_range_mtu_is_refused_for_every_provider(mtu): + """The divergence this closes: the session never validated mtu. + + DtlsPskClient requires 256-65535 and OpenSSL silently accepts anything, + consulting it only when it has to fragment. So mtu=100 used to construct, + then raise from connect() on a PSK provider while a certificate provider + carried on. Now both are refused where the argument was supplied, under + the range dtls_probe has validated since it was written. + """ + for auth in (_ContextAuth(), _EngineAuth(_Connection())): + with pytest.raises(ValueError, match="safe UDP range"): + DtlsCoapSession("device.example", 5684, auth=auth, mtu=mtu) + + +@pytest.mark.parametrize("mtu", [576, 1200, 16384]) +def test_in_range_mtu_is_accepted_for_every_provider(mtu): + for auth in (_ContextAuth(), _EngineAuth(_Connection())): + assert DtlsCoapSession( + "device.example", 5684, auth=auth, mtu=mtu).mtu == mtu + + +@pytest.mark.parametrize("mtu", [True, 1200.0, "1200", None]) +def test_mtu_must_be_an_integer(mtu): + with pytest.raises(TypeError, match="mtu must be an integer"): + DtlsCoapSession("device.example", 5684, auth=_ContextAuth(), mtu=mtu) + + +def test_the_session_and_the_probe_share_one_mtu_rule(): + """Two copies of a range drift. The probe's message is the shared one.""" + from smartthings_local.protocol.dtls_handshake import _validate_mtu + + for module_validator in (dtls_probe._validate_mtu, + dtls_session._validate_mtu): + assert module_validator is _validate_mtu