Report the ship runner's source address, and fix the dual-stack guidance - #90
Merged
Merged
Conversation
Two corrections drawn from bringing production up, both of which cost real deploys. Revoking 22/tcp from 0.0.0.0/0 broke staging and nearly broke the production release. The reasoning was that the security group already permits 22 from 10.0.0.0/8 and the ship job reaches the host on an RFC1918 address, so the world-facing rule was redundant. Reaching an RFC1918 destination says nothing about the source: the pods present something outside 10/8, so that rule was the only thing allowing them. The runner cannot answer this for itself either, since `curl ifconfig.me` reports public internet egress and this path does not use it. So ask the host. The ship job now reads $SSH_CLIENT over the connection it just made and prints the address the host sees, every run, non-fatally. Whoever narrows the rule needs that value, and needs more than one sample of it: the pools are ephemeral pods across more than one cluster, so consecutive deploys can differ. The README says so, and records that the group permitting 22 from 10.0.0.0/8 carries no IPv6 entry. Separately, the dual-stack section said to publish both an A and an AAAA record. The families are not symmetric and that framing hid which one matters. AAAA-only is the dangerous case, and both hosts were in it unnoticed because every observer had IPv6. A-only is fine — IPv4 reaches effectively everyone and IPv6-only clients arrive via NAT64/DNS64 — and production is deliberately A-only now. Say that, and say to verify IPv4 from a network without v6 rather than from a VPN or a cloud shell that would mask a dead v4 path. 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
This PR improves deploy operability by making the ship runner’s actual SSH source address observable (as seen by the target host) and correcting the documentation guidance around dual-stack DNS reachability and runner/SSH assumptions.
Changes:
- Adds a non-fatal “Report source address as seen by the host” SSH step in the
shipjob to print$SSH_CLIENTas observed by the host. - Updates
deploy/native/README.mdto clarify that IPv4 reachability is the critical requirement (warning onAAAA-only), and adds guidance on narrowing inbound SSH by measuring real source addresses. - Updates the build/ship split wording to remove “on-prem” framing and reflect the current runner model.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
.github/workflows/deploy-native.yml |
Adds a diagnostic SSH step to print the runner’s source address as seen by the host during deploy. |
deploy/native/README.md |
Updates dual-stack guidance and documents how/why inbound SSH narrowing still requires observed runner source ranges. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The diagnostic step discarded ssh's stderr, which defeated much of its purpose: "Connection timed out", "Permission denied (publickey)" and a host key mismatch are entirely different diagnoses, and a step whose whole job is to explain SSH problems should say which one happened. Capture stderr and fold it into the annotation, collapsed to one line since ::warning:: is line-oriented. Verified against stubs for all four outcomes. The dual-stack paragraph asserted that IPv6-only clients reach IPv4-only services via NAT64/DNS64. That holds only where the client's access network provides the translation — it is not a property this deployment supplies, and a v6-only client without it cannot reach an A-only host at all. Since the paragraph is arguing that A-only is acceptable, overstating the fallback makes the case look stronger than it is. Attribute it to the client network and keep recommending an AAAA wherever the v6 path genuinely works. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8ZshwKmjD5fZ4Hs9dS6zh
The section told the reader to obtain the runners' source ranges and narrow the rule to them. That goal turns out not to exist. Calico source-NATs each pod to its worker node's IP, and traffic to this VPC leaves over a routed IPsec tunnel rather than a NATting gateway, so the node address arrives unchanged. Node IPs are provisioned dynamically rather than declared, the two clusters are separate sites with separate ranges and either can run the job, and the SNAT-to-node-IP behaviour is an unpinned Calico default rather than a contract. A prefix that fits today can stop fitting after cluster maintenance, and it would surface as a deploy timeout. So say that, and point at the fix that removes the requirement instead of narrowing it: reach the host over SSM Session Manager, which needs no inbound port. ssh works over it through a ProxyCommand, so deploy.sh is unaffected and only the workflow's SSH setup changes. Also name the two circular alternatives, since both look reasonable until you notice a job cannot discover the address it would need to allow, and a bastion inherits the same problem. Record that the source is outside 10.0.0.0/8 as an established fact rather than a guess: both hosts carry two independent rules permitting 22 from that range, and removing only the 0.0.0.0/0 rule still broke the deploy. Also note the ::/0 half of the rule is now gone, since the tunnels are IPv4-only and it permitted nothing but internet-sourced attempts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8ZshwKmjD5fZ4Hs9dS6zh
aterga
marked this pull request as ready for review
July 28, 2026 15:41
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
.github/workflows/deploy-native.yml:213
- The
::warning::...workflow command includes rawsshstderr. GitHub Actions command parsing requires escaping%(and CR/LF) in command data; otherwise percent sequences like%0Acan be interpreted/decoded by the runner, which can lead to workflow-command injection or malformed annotations. Since this content comes from the network, it should be sanitized before emitting the::warning::line.
else
echo "::warning::could not read SSH_CLIENT from the host: $(tr '\n' ' ' < "$err" 2>/dev/null)"
fi
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
Two corrections from bringing production up, both of which cost real deploys.
Revoking
22/tcpfrom0.0.0.0/0broke staging and nearly broke the production release. The reasoning was thatsg-0cbc9aa49683725f0already permits22from10.0.0.0/8and the ship job reaches the host on an RFC1918 address, so the world-facing rule looked redundant. It isn't: reaching an RFC1918 destination says nothing about the source, and the ship pods present something outside10/8, so that rule was the only thing allowing them. Staging's deploy failed atssh: connect to host *** port 22: Connection timed out, and the production release only survived because the rule went back before its ship job started.The runner can't answer this for itself —
curl ifconfig.mereports public internet egress, which this path doesn't use. So ask the host: the ship job now reads$SSH_CLIENTover the connection it just made and prints the address the host sees. Every run, non-fatally.The dual-stack guidance was framed wrongly. It said to publish both an
Aand anAAAArecord, which obscures which one matters.AAAA-only is the dangerous case — and both hosts sat in it unnoticed, because every observer had IPv6.A-only is fine: IPv4 reaches effectively everyone, and IPv6-only clients arrive via NAT64/DNS64. Production is deliberatelyA-only now, so the README contradicted reality as well as being misleading.Related issues
mcp.internetcomputer.orgbehind Cloudflare, which needs an origin certificate and an SSE compatibility test.Does not close the SSH question. This makes the missing input observable; narrowing the rule still needs the documented source ranges from whoever operates the runner pools.
Changes
deploy-native.yml: add a Report source address as seen by the host step toship, betweenConfigure SSHandDeploy. Reads$SSH_CLIENTover SSH and prints the source the host observes. Warns rather than failing if SSH is unavailable, sinceDeployreports that properly a step later.deploy/native/README.md: new Narrowing inbound SSH subsection explaining why the rule is still open, why10.0.0.0/8doesn't cover it, how to read the source from the host ($SSH_CLIENT, orjournalctl -u sshd | grep 'Accepted publickey'), that one sample is insufficient because the pools are ephemeral pods across multiple clusters, and that the10.0.0.0/8group has no IPv6 entry.deploy/native/README.md: rewrite the dual-stack paragraph. Keeps theAAAA-only warning, notes it actually happened, states thatA-only is acceptable and why, and says to verify IPv4 from a network without v6 rather than from a VPN or cloud shell that would mask a dead v4 path.dind-smallpool, not an on-prem runner.Testing
deploy-native.ymlparses as YAML;ship's step order verified as Checkout → Download binary → Restore executable bit → Configure SSH → Report source address → Deploy → Verify deployed version.sshfor all three paths: reachable host prints the address,sshfailing exits non-zero and takes the warning branch, and an empty$SSH_CLIENTalso takes the warning branch. The job continues in every case.mainor a release tag. The staging deploy that follows this merge is what exercises it, and its output is the value needed for Put mcp.internetcomputer.org behind Cloudflare: origin cert plus an SSE compatibility test #89's sibling work on the SSH rule.cargo build/cargo test/clippy— no Rust source changed.npm test --prefix monitoring/mcp-status— no dashboard code changed; the workflow runs it anyway.Checklist
Note
The step costs one extra SSH round trip per deploy and prints a private address into the run log. That address is already visible in the same log via the arch-guard output, so it discloses nothing new — but it is worth knowing that is where the answer will appear.
Generated by Claude Code