fix(security): pin a TLS 1.2 floor for the Twitch connection - #29
Merged
Conversation
Closes CodeQL alert #13 (py/insecure-protocol, high). ssl.create_default_context() leaves minimum_version at the MINIMUM_SUPPORTED sentinel (-2), so TLS 1.0 and 1.1 stay permitted wherever the platform still enables them. This bot authenticates by sending its OAuth token in the IRC PASS command, so the negotiated floor is exactly what protects that token in transit — the comment two lines above already says the TLS wrap exists so the token 'travels encrypted rather than in plaintext', which is only as true as the weakest version allowed. Extracted _make_tls_context() so the floor is testable without opening a socket, and pinned minimum_version to TLSv1_2. Tests written first and confirmed red. The floor test carries a negative control asserting create_default_context() really does leave the floor below TLS 1.2 on this platform (verified: -2 < 771), so it cannot pass vacuously. A second test asserts hardening the floor did not weaken verification — verify_mode stays CERT_REQUIRED and check_hostname stays True, which is the thing most likely to be broken by fiddling with a context. Verified with the exact CI pin (ruff 0.15.22): check and format clean, 40 tests pass.
CI (Python 3.14) failed the negative control that passed locally on 3.12: ssl.create_default_context().minimum_version is the MINIMUM_SUPPORTED sentinel (-2) on 3.12 but already TLSv1_2 on 3.14, so asserting the platform default sits BELOW the floor is interpreter-dependent. The control now exhibits a context that fails the assertion outright rather than asserting anything about the platform's default, so it discriminates on every interpreter. Mutation-checked: reverting the helper to TLSv1 turns the test red. Worth recording rather than just fixing: the fix itself is a no-op on 3.14, where the default is already TLS 1.2 — but this package targets 3.10+, and on 3.10-3.13 the floor really is unpinned. The pin is what makes the guarantee independent of which interpreter a user runs.
The control used ssl.TLSVersion.TLSv1, which emits a DeprecationWarning. MINIMUM_SUPPORTED is the actual 'no floor' sentinel, expresses the intent more directly, and is not deprecated. Mutation-rechecked: removing the pin from the helper still turns the test red.
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
1 participant
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.
Closes CodeQL alert #13 (
py/insecure-protocol, high).ssl.create_default_context()leavesminimum_versionat the MINIMUM_SUPPORTED sentinel (-2), so TLS 1.0 and 1.1 remain permitted wherever the platform still enables them.That matters here specifically: the bot authenticates by sending its OAuth token in the IRC
PASScommand. The comment two lines above already says the TLS wrap exists so the token "travels encrypted rather than in plaintext" — which is only as true as the weakest version allowed.Change
Extracted
_make_tls_context()so the floor is testable without opening a socket, and pinnedminimum_version = ssl.TLSVersion.TLSv1_2.Verification
Tests written first and confirmed red.
create_default_context()really does leave the floor below TLS 1.2 on this platform (verified:-2 < 771), so it cannot pass vacuously.verify_modestaysCERT_REQUIRED,check_hostnamestaysTrue. That is the thing most likely to break when fiddling with an SSL context.Verified with the exact CI pin (ruff 0.15.22):
checkandformat --checkclean, 40 tests pass.