Say why a NUL in a PSK identity is refused - #85
Merged
Merged
Conversation
PskAuth rejected an identity containing a zero byte with a bare "identity cannot contain a NUL byte", which reads as this library's own fussiness. It is OpenSSL's constraint, and the consequence of lifting it is worse than the refusal. Measured on OpenSSL 4.0.0 over a loopback ECDHE-PSK handshake, reading psk_identity out of the ClientKeyExchange: a 16-byte identity with a NUL at byte 8 reaches the wire as 8 bytes, and nothing raises locally. An appliance takes the identity with an explicit length and would have been fine with the zero byte, so lifting the guard means silently presenting a different, shorter identity and getting unknown_psk_identity back with nothing pointing at the cause. DTLS 1.2 offers no length-carrying PSK callback, so the credential is unreachable here: about 6% of uniformly random 16-byte identities, and 5% of UUIDv4s. So the guard stays and the message now carries the cause. validate_identity exposes the same check to a caller holding a credential before it has a key to pair with it, which lets an import flow refuse at import time with a reason rather than failing later inside a handshake.
The reason for the guard was measured in a scratch script, which left the claim in the commit message unreproducible from the repo. Move it into the suite. Two OpenSSL endpoints complete a real DTLS 1.2 ECDHE-PSK handshake over memory BIOs, relayed by hand so the client's records can be read, and the psk_identity is parsed out of the ClientKeyExchange, which precedes ChangeCipherSpec and is therefore in the clear. The client callback is byte for byte what PskAuth installs, so this measures OpenSSL rather than a straw man. A clean identity arrives whole. One with a NUL at byte 8 arrives as 8 bytes, and nothing raises. The truncation test then asserts that PskAuth refuses exactly that identity, so the guard and its justification move together. Confirmed on both CI dependency sets, which carry different OpenSSL builds: 4.0.0 under pyOpenSSL 26.2.0, and 3.1.0 under the 23.1.0 floor. If the full 16 bytes ever arrive, OpenSSL has gained a length-carrying path and the guard can be revisited, which the test says in place.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PskAuthrefused an identity containing a zero byte with a bareidentity cannot contain a NUL byte, which reads as this library being fussy. It is OpenSSL's constraint, and I went in expecting to lift it. Measuring it showed the opposite: the guard is the only thing standing between a caller and a silently wrong identity on the wire.localthings#435 records this as gap 4, described there as "luck of the draw for an OwnerPSK". The measurement below is what that entry needs, and a follow-up comment there will carry it.
The measurement
Two tests in the suite carry this, so a reviewer can rerun it. Two OpenSSL endpoints complete a real DTLS 1.2 ECDHE-PSK handshake over memory BIOs, relayed by hand so the client's records can be read, and
psk_identityis parsed out of the ClientKeyExchange, which precedes ChangeCipherSpec and is therefore in the clear. The client callback is byte for byte whatPskAuthinstalls, so what it measures is OpenSSL itself.0102…0f10, 16 bytes, no NUL01…08000a…10, 16 bytes, NUL at byte 8OpenSSL's DTLS 1.2 PSK client callback hands the identity back as a
char *and takesstrlen()of it, so there is no way to express a length. The truncated identity goes out and the handshake completes locally without complaint.Both CI dependency sets carry different OpenSSL builds, 4.0.0 under pyOpenSSL 26.2.0 and 3.1.0 under the 23.1.0 floor, and both truncate. The truncation test also asserts that
PskAuthrefuses that exact identity, so the guard and its justification move together. Should the full 16 bytes ever arrive, OpenSSL has gained a length-carrying path and the guard can be revisited, which the test says in place.The appliance side is fine with a zero byte, and two independent OCF stacks agree on that. Samsung's RT-OCF takes
rt_sec_cred_get_psk(const uint8_t *uuid, size_t uuid_len, ...), and mbedTLS hands it(*p, n). iotivity-lite, the maintained reference stack, builds its client identity asmemcpy(identity_hint, device_id->id, 16)withidentity_hint_len = 16formbedtls_ssl_conf_psk, and itsget_psk_cb(..., const unsigned char *identity, size_t identity_len)casts that tooc_uuid_t *. Counted bytes throughout, on both. So this is a client-library limit, and lifting the guard would mean presenting a different, shorter identity and readingunknown_psk_identityback with nothing pointing at the cause.TLS 1.3's
psk_find_sessionandpsk_use_sessioncallbacks do carry arbitrary-length identities, and both setters are present in the binding, but they are TLS 1.3 only. These appliances are DTLS 1.2, so the credential is unreachable here: roughly 6% of uniformly random 16-byte identities, and about 5% of UUIDv4s, whose version and variant bytes can never be zero.What changed
The guard stays. The message now names the cause and the consequence, so it survives being surfaced to a user who has to act on it.
PskAuth.validate_identity(identity)exposes the same check to a caller holding a credential before it has a key to pair with it. An import flow can then refuse at import time and show the reason, at the point where a user can still act on it. The constructor delegates to it, so there is one code path and one message.The constructor still reports
identity and key must be bytes, since it checks both together.validate_identitytakes one argument and saysidentity must be bytes. A test holds that distinction, so sharing the path keeps both messages accurate.One boundary that came out of reading iotivity-lite: its random-PIN OTM uses a 33-byte identity,
"oic.sec.doxm.rdp:"followed by the 16 raw UUID bytes, so an OCF identity is not always 16 bytes.PskAuthrequires exactly 16, which is right for a runtime credential and cannot express the PIN-OTM shape. Onboarding is out of scope, so I read that as a scope decision, though anyone reaching for the RDP path later will meet this length check.Validation
780 tests pass on both CI dependency sets: cbor2 5.6.0 / pyOpenSSL 23.1.0 / pytest 8.0.0, and current releases.
check_share_safety.pyis clean.One overlap with #84: both branches add
import pytesttotests/test_public_api_contract.py. Whichever merges second may want a one-line rebase.