fix(install.ps1): SET the TLS 1.2 floor, don't OR it onto the default - #532
Conversation
The floor bitwise-OR-ed Tls12 onto [Net.ServicePointManager]::SecurityProtocol, which on PowerShell 5.1 already advertises SSL3/TLS1.0/1.1 — so those stay on and a fetch of the binary or the cosign verifier can still negotiate down, the exact downgrade the floor's own comment says it prevents (cli#528 Bugbot, Medium). Assign the protocol to Tls12 (dropping the weak ones), adding Tls13 only where the runtime defines the enum member (absent on older 5.1 hosts, where naming it throws). New install-ps1-verify assertion fails on the OR-onto-default form (mutation-proved); collapses newlines first since the old form spanned two lines. install-ps1-verify 6/6, behavioural tier 22/22. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3a00545. Configure here.
Bugbot (Medium) on the first push: [Enum]::IsDefined([Net.SecurityProtocolType], 'Tls13') is true on .NET 4.8 even where Schannel cannot negotiate TLS 1.3 (Win10 21H1, Server 2019). Assigning Tls12 -bor Tls13 then THROWS, the empty catch swallows it, and SecurityProtocol is never set — so the Tls13 decoration could defeat the very Tls12 floor it was meant to extend. Assign Tls12 alone: it is the floor, always negotiable, and secure for these fetches. Verify assertion now pins the direct Tls12 assignment; still fails on the OR-onto-default form (mutation-proved). 6/6 verify, 22/22 behavioural. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
aptracebloc
left a comment
There was a problem hiding this comment.
Approving. The code change is correct, and both failure modes of the OR→assign swap are cleanly avoided.
The defect is real as described. The old form only ever added TLS 1.2 to whatever the process already advertised, so SSL3 / TLS 1.0 / 1.1 stayed on the table — directly contradicting the comment above it about not negotiating down. The straight assignment is the right shape for a floor.
Dropping TLS 1.3 costs nothing here. The assignment is Tls12 alone, which does narrow a host that had 1.3 advertised — but ServicePointManager.SecurityProtocol only has effect on Windows PowerShell 5.1 / .NET Framework, which never shipped Schannel negotiation for TLS 1.3 (the 4.8 enum member exists; the capability doesn't). On PS7 the setting is inert entirely, since Invoke-WebRequest goes through SocketsHttpHandler and ignores ServicePointManager. So the existing "PS7+ already defaults higher; setting it is harmless there" comment still holds.
And the throw hazard is already gone. Worth recording why the current shape is right: the first commit on this branch named Tls13 behind an [Enum]::IsDefined guard, and Bugbot's Medium was correct — IsDefined returns true on .NET 4.8 even where Schannel can't negotiate 1.3 (Win10 21H1, Server 2019), the assignment throws, and the empty catch { } swallows it, leaving the floor entirely unset. Dropping Tls13 in 45c6656 was the right call rather than tightening the guard.
Placement verified: the floor is at line 77, preceded only by Set-StrictMode, $ErrorActionPreference, variable assignments and function definitions; the first Invoke-WebRequest is at 121, the rest at 122/198/234/237/322/323. No network call precedes it. No SCHANNEL or SystemDefaultTlsVersions registry writes anywhere in the file — the only registry work is user-scope PATH at the end. No manifest or checksum in the repo covers scripts/install.ps1, so nothing to regenerate.
Non-blocking, but assertion #5 doesn't guard what it's for
scripts/tests/install-ps1-verify.sh:80-91 passes four of the five mutations it exists to catch. Replicated verbatim against mutated copies of the head installer:
| mutation | expected | actual |
|---|---|---|
| head as-is | ok | ok |
develop's OR form |
FAIL | FAIL |
operands swapped (= Tls12 -bor [ServicePointManager]::SecurityProtocol) |
FAIL | ok |
= Tls12 -bor Ssl3 -bor Tls (weak protocols re-added) |
FAIL | ok |
= Tls12 -bor Tls13 (the exact hazard the comment forbids) |
FAIL | ok |
line deleted, only a # was: ... comment left |
FAIL | ok |
Two causes: the bad-branch regex hardcodes one operand order, and the good-branch regex isn't end-anchored, so anything OR-ed after Tls12 sails through. And because it greps the flattened file including comments, deleting the floor outright still passes as long as a comment mentions it.
This matters more than usual because the behavioural tier (install-ps1-functions.tests.ps1) extracts functions by AST, and the TLS floor is top-level — assertion #5 is its only coverage. Someone "improves" this to Tls12 -bor Tls13 in six months, CI stays green, and the installer silently reverts to no floor at all on Server 2019 via that empty catch. End-anchoring the good-branch match and asserting the absence of -bor on the line would close it.
Two smaller things:
- The body's "## Fix" section still describes the abandoned approach — "adding
Tls13only when the runtime defines the enum member … hence the[Enum]::IsDefinedguard" — which45c6656removed. The regenerated Cursor summary below it correctly says 1.3 is not combined in, so the body contradicts itself, and it becomes the squash-merge message. - Since the documented entry point is
irm <url> | iex, this narrows the caller's live session and persists after the installer exits; the old-borform could only ever add protocols. Marginal, arguably the intended hardening, but it's a behaviour change in the other direction and worth a line in the comment.
One bound on the security claim, unfixable from inside the script: the most privileged fetch of all — install.ps1 itself via irm — happens at the session's default protocol, before line 77 exists.
— drafted with Claude Code
saadqbal
left a comment
There was a problem hiding this comment.
Late to this one — it merged while I was reading. Agreeing with the approval anyway: assigning Tls12 is the right shape for a floor, and on the only runtime where ServicePointManager.SecurityProtocol actually has effect (Windows PowerShell 5.1 / .NET Framework) there is no TLS 1.3 to lose, so the narrowing is theoretical rather than a downgrade. On PS7 the setting is inert — Invoke-WebRequest goes through SocketsHttpHandler, which ignores ServicePointManager. Placement checks out: floor at line 77, first Invoke-WebRequest at 121, nothing fetches before it.
Everything I'd have flagged on assertion #5 — unanchored good-branch regex, comments counted as code because of the tr '\n' ' ', hardcoded operand order — is already in @aptracebloc's review, so I won't restate it. Worth actually landing that follow-up though: #5 is the floor's only coverage, since the behavioural tier extracts functions by AST and this is top-level.
Two things not yet said.
nit: the rationale at install.ps1:69-75 says assigning Tls13 "THROWS" and the empty catch swallows it, leaving the floor unset. On .NET 4.8 — the case where [Enum]::IsDefined is true — the enum member exists and the setter accepts it; the Schannel incompatibility surfaces later, at handshake time inside Invoke-WebRequest, outside the try/catch, as a download failure rather than a silently-unset floor. Same conclusion (skip 1.3, 1.2 is fine here), but the next person weighing 1.3 will reason from a mechanism that doesn't hold. Worth a re-word rather than a code change.
Out of diff: tracebloc/client's scripts/install-k8s.ps1:323-324 still carries the exact -bor-onto-the-default form this PR removes, and its comment ("OR-in, don't clobber a higher floor") argues for it deliberately — the same downgrade window, with no equivalent guard in that repo. install-k8s.ps1:108 and client/scripts/install.ps1:230,589 already assign, so it's just that one site.
|
/fr-pass Best-effort FR passed (triage; behavioral evidence limited while e2e journey red — backend#2206). Advancing to Ready for prod. |

Resolves the Bugbot Medium on the cli develop→staging mirror (#528),
scripts/install.ps1:70.Bug
The TLS floor did
SecurityProtocol = SecurityProtocol -bor Tls12. On PowerShell 5.1 the default already advertises SSL3/TLS1.0/1.1, so OR-ing Tls12 on leaves them enabled — a fetch of the binary or the cosign verifier can still negotiate down. The comment right above it says the opposite ("neither may negotiate down").Fix
Assign the protocol to
Tls12(weak protocols dropped), addingTls13only when the runtime defines the enum member (absent on older 5.1 hosts, where naming it throws — hence the[Enum]::IsDefinedguard).Guard
New assertion #5 in
install-ps1-verify.shfails if the floor OR-s onto the default[Net.ServicePointManager]::SecurityProtocol(collapses newlines first — the old form spanned two lines). Mutation-proved: reverting to the OR form reddens it.install-ps1-verify6/6, behavioural tier 22/22.On
developfor the staging-hop re-prepare.Note
Medium Risk
Touches transport security for installer fetches of binaries and the cosign verifier; the change tightens protocol negotiation but affects all Windows install paths.
Overview
Fixes the Windows installer TLS setup so downloads of the CLI and cosign verifier cannot negotiate down to SSL3/TLS 1.0/1.1 on PowerShell 5.1.
install.ps1now assigns[Net.SecurityProtocolType]::Tls12toSecurityProtocolinstead of OR-ingTls12onto the existing default (which left weak protocols advertised). Comments document why TLS 1.3 is not combined in—assigning it can throw on some Schannel hosts and the emptycatchwould leave the floor unset.install-ps1-verify.shadds assertion #5 that fails if the installer OR-s onto the defaultSecurityProtocoland passes only when TLS 1.2 is assigned directly (with newlines collapsed so multi-line-borforms are caught).Reviewed by Cursor Bugbot for commit 45c6656. Bugbot is set up for automated code reviews on this repo. Configure here.