fix(network): state the TLS floor instead of inheriting it from OpenSSL - #31
Merged
Conversation
Two of the four Code of Conduct files pointed at GitHub features that are not real: - devrepro-doctor: report "via GitHub private message". GitHub has no private messaging. - local-ai-hardware-bench: report by "opening a private issue tagged `conduct`". GitHub has private *vulnerability reports*; it has no private issues. api-verity-lab gave a profile URL rather than a contact channel. Only tooltrace-bench named an address that works. Someone reporting harassment is the worst possible person to hand a dead end, so all four now carry the same wording: the maintainer's noreply address plus GitHub's real report-abuse form. Worse, and found while checking the above: all four SECURITY.md files direct reporters to GitHub's private vulnerability reporting, and it was **disabled on all four repositories**. Every documented security-disclosure path in this family of projects led to a page the reporter could not use. It is enabled now (a repository setting, so not visible in this diff). The disclosure SLAs also disagreed for the same solo maintainer -- 72h, 72h+7d, 7d, and 7d+30d. Standardized on 7 days to acknowledge and 30 to update, the most conservative of the four, and said plainly why: promising 72 hours when nobody is on call is a promise, not a policy. tests/test_contact_channels_exist.py pins this. It asserts the working address and the report-abuse form are present, that SECURITY.md still names private vulnerability reporting and still warns against public issues, and -- the point of the exercise -- that no document mentions "private message", "private issue" or "report-user functionality" again. Verified by reintroducing the devrepro-doctor wording and watching it fail.
CodeQL reported three `py/insecure-protocol` alerts (high) against `ssl.create_default_context()` in check_tls, _check_endpoint and _clock_skew_seconds: TLS 1.0 and 1.1 "allowed". Judged rather than auto-patched, because the obvious reading was wrong. The guess was that a diagnostics tool might deliberately offer old protocols to report what a host negotiates -- it does not. All three sites are DevRepro acting as a *client*, with full certificate verification, and nothing here probes a remote endpoint's version support. So raising the floor cannot mask a finding, and there is nothing to suppress. What CodeQL is actually pointing at is real, if narrow: `create_default_context()` leaves `minimum_version` to whatever the OpenSSL build decides. On a current build that is already 1.2. But this tool runs on whatever machine it is asked to diagnose, and a machine with an old OpenSSL is precisely the machine someone runs a doctor on. Depending on the environment to choose the floor is the wrong default for this program in particular. Three lines, stating what the code already meant.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
CodeQL reported three
py/insecure-protocolalerts (high) againstssl.create_default_context()incheck_tls,_check_endpointand_clock_skew_seconds: TLS 1.0 and 1.1 "allowed".Judged, not auto-patched — and the obvious reading was wrong
The plausible hypothesis was that a diagnostics tool might deliberately offer old protocols in order to report what a host negotiates, which would make this a false positive to dismiss with justification.
It isn't. All three sites are DevRepro acting as a client, with full certificate verification, and nothing here probes a remote endpoint's version support. Raising the floor cannot mask a finding, so there is nothing to suppress.
What the alert is actually pointing at
create_default_context()leavesminimum_versionto whatever the OpenSSL build decides. On a current build that is already TLS 1.2 — so in practice the alert is conservative. But this tool runs on whatever machine it is asked to diagnose, and a machine with an old OpenSSL is precisely the machine someone runs a doctor on. Depending on the environment to pick the floor is the wrong default for this program in particular.Three lines, stating what the code already meant.
Verification
ruff check/ruff format --checkmypypytest