Rewrite the inbound-SSH section: it described a state that no longer exists - #104
Merged
Conversation
…exists Two of the section's load-bearing claims were false, and the repo is heading for public visibility, so a reader would have been misled on both counts. "It still does" need 22/tcp open to the world — no longer true. Internet-facing SSH is closed on both hosts: each has a public-facing interface and a private one, security groups attach per-interface, and the public one now carries a group permitting 80/443 only. Verified from outside the VPN. "Do not try to solve this with a narrower CIDR — there isn't one" — the evidence for that was one observed public source address, which turned out to be an artifact. Staging's DEPLOY_HOST named the public FQDN, so its deploys were leaving over the internet rather than the VPN and presenting a public source. Both hosts on private addresses present sources inside the private range. The section spent four paragraphs arguing a conclusion that had already collapsed. Rewritten to say what is now true and what is left: narrow the rule to the private range and delete the world-facing one, after confirming the source across both runner clusters, and add our own rule rather than depending on another team's. The Calico reasoning is kept, demoted from "this is impossible" to "verify rather than assume", which is what it actually supports. SSM is kept as considered-and-declined with its reasoning intact, since the judgement could change. Also removes every internal address and resource identifier from the section, so it is publication-safe (part of #101). Details that need them live on #91. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8ZshwKmjD5fZ4Hs9dS6zh
There was a problem hiding this comment.
Pull request overview
Updates the native deployment documentation to reflect the current network/security posture for deploy-host SSH access, removing now-incorrect claims and clarifying the remaining hardening work.
Changes:
- Rewrites “Narrowing inbound SSH” to state that internet-facing SSH is closed via per-interface security groups and to explain the
DEPLOY_HOSTprivate-address requirement. - Reframes the CIDR-narrowing guidance as “verify then act” (Calico/SNAT variability) and records the prior failure mode accurately.
- Keeps SSM Session Manager as an explicitly considered (but currently declined) alternative end state.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The rewritten section referred to it as bare `deploy-native.yml` while lines 168-169 and 180 use a relative link. Matches them now. Scoped to the .yml files deliberately: `deploy.sh` appears bare in nine other places here and stays that way, so this follows the existing convention rather than inventing a new one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8ZshwKmjD5fZ4Hs9dS6zh
sea-snake
approved these changes
Jul 29, 2026
aterga
marked this pull request as ready for review
July 29, 2026 16:46
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
deploy/native/README.md:222
- This sentence claims the
DEPLOY_HOSTsecret description indeploy-native.yml“now says so”, but in the current workflow it still uses the old example (ec2-user@10.118.0.61) and doesn’t state the private-address requirement. If this PR can land before #102, make this wording non-time-dependent (or reference #102 explicitly) so the README stays accurate.
configured, unnoticed, until that change made it fail. The secret's description in
[`deploy-native.yml`](../../.github/workflows/deploy-native.yml) now says so.
aterga
added a commit
that referenced
this pull request
Jul 29, 2026
…exists (#104) * Rewrite the inbound-SSH section: it described a state that no longer exists Two of the section's load-bearing claims were false, and the repo is heading for public visibility, so a reader would have been misled on both counts. "It still does" need 22/tcp open to the world — no longer true. Internet-facing SSH is closed on both hosts: each has a public-facing interface and a private one, security groups attach per-interface, and the public one now carries a group permitting 80/443 only. Verified from outside the VPN. "Do not try to solve this with a narrower CIDR — there isn't one" — the evidence for that was one observed public source address, which turned out to be an artifact. Staging's DEPLOY_HOST named the public FQDN, so its deploys were leaving over the internet rather than the VPN and presenting a public source. Both hosts on private addresses present sources inside the private range. The section spent four paragraphs arguing a conclusion that had already collapsed. Rewritten to say what is now true and what is left: narrow the rule to the private range and delete the world-facing one, after confirming the source across both runner clusters, and add our own rule rather than depending on another team's. The Calico reasoning is kept, demoted from "this is impossible" to "verify rather than assume", which is what it actually supports. SSM is kept as considered-and-declined with its reasoning intact, since the judgement could change. Also removes every internal address and resource identifier from the section, so it is publication-safe (part of #101). Details that need them live on #91. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8ZshwKmjD5fZ4Hs9dS6zh * Link deploy-native.yml the way the rest of this README does The rewritten section referred to it as bare `deploy-native.yml` while lines 168-169 and 180 use a relative link. Matches them now. Scoped to the .yml files deliberately: `deploy.sh` appears bare in nine other places here and stays that way, so this follows the existing convention rather than inventing a new one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8ZshwKmjD5fZ4Hs9dS6zh --------- Co-authored-by: Claude <noreply@anthropic.com>
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.
Summary
deploy/native/README.md's "Narrowing inbound SSH" section had two load-bearing claims that are now false. With the repo heading for public visibility, a reader would be misled on both."It still does" need
22/tcpopen to the world. Not true as of today. Internet-facing SSH is closed on both hosts: each has a public-facing interface and a private one, security groups attach per-interface, and the public one now carries a group permitting80/443only. Verified from outside the VPN —22times out where it previously reachedsshd."Do not try to solve this with a narrower CIDR — there isn't one." The evidence for this was a single observed public source address, and it turned out to be an artifact. Staging's
DEPLOY_HOSTnamed the public FQDN, so its deploys left over the internet rather than the VPN and presented a public source. With both hosts on private addresses, both present sources inside the private range. The section spent four paragraphs arguing a conclusion that had already collapsed.Related issues
Part of #101 (publication scrub — the section now carries no internal addresses or resource identifiers). Supersedes the analysis in #91, which is being updated separately.
Changes
Rewritten to state what is true now and what is left to do:
DEPLOY_HOSTmust be the host's private address, with the failure mode spelled out. This is the same requirement Stop documenting a decommissioned host address as the DEPLOY_HOST example #102 adds to the secret's own description.22still open on the private interface, so the VPN and on-prem estate still reach SSH. Recorded as accepted rather than left implicit.::/0note and the$SSH_CLIENT/journalctlobservation guidance are kept — both still correct and still useful.Testing
git grepover the whole tree finds no internal IPs, VPC/subnet/security-group/ENI/instance identifiers, or IPv6 addresses.ship_runs_onand/version-assertion paragraphs that follow belong to the parent section and are untouched.cargo/npm— documentation only.Checklist
Sequencing
Best merged after #102. One sentence here says the requirement is stated in
deploy-native.yml's secret description, which is what #102 adds. Harmless if merged first, briefly forward-looking.Generated by Claude Code