Offer only the deployment key when ssh-ing to instances - #67
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. Walkthrough
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change limits direct SSH connections to the intended deployment key while preserving SSH configuration directives, with targeted tests covering the behavior; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
|
@wayne - can you please sign the CLA, or re-trigger with |
|
OK should be an error as I signed before I'll retriever |
|
I think it's because this is my personal GH, re-signing |
|
Let's try that again I have 2 accounts, /eazycla |
Net::SSH fills in identity sources the caller does not name: every IdentityFile from ~/.ssh/config, which it tries ahead of :key_data, and every identity held by a reachable ssh-agent. On a workstation with a 'Host *' block or a loaded agent that is enough extra identities to exhaust the target sshd's MaxAuthTries, so the server disconnects and the example dies with 'Too many authentication failures'. Pin the identity set to the key BATs was given. :keys_only on its own does not do it: it filters agent identities but leaves the ones the config file contributed, so :keys has to be cleared as well. Parsing of ~/.ssh/config stays on, so directives such as ProxyJump and Port keep working.
8013da4 to
861ab8f
Compare
|
Figured it out, was a stupid Forking error 🤪 |
aramprice
moved this from Inbox
to Pending Review | Discussion
in Foundational Infrastructure Working Group
There was a problem hiding this comment.
Pull request overview
Restricts direct Net::SSH connections to the deployment-provided key, preventing authentication failures caused by extra SSH config and agent identities.
Changes:
- Clears configured identities and disables agent identities.
- Adds a regression test for the resulting SSH options.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
lib/bat/bosh_helper.rb |
Pins SSH authentication to the supplied key. |
spec/bat/bosh_helper_spec.rb |
Verifies exclusive identity options. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
github-project-automation
Bot
moved this from Pending Review | Discussion
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
The failure
Direct ssh examples (the
ostagged service configuration specs, and anything else going throughBoshHelper#ssh) die with:It looks intermittent, because whether it fires depends on how many identities happen to be loaded on the machine running BATs, and an RSA key is offered once per signature algorithm.
bosh sshis unaffected, since the CLI passes-o IdentitiesOnly=yes.The cause
BoshHelper#sshsets:key_dataand leaves every other identity source at its default, so Net::SSH adds two more:~/.ssh/config.Net::SSH.startmerges the parsed config under the caller's options, soIdentityFileentries in aHost *block land in:keys, which is offered before:key_data.The ssh-agent, which contributes every loaded identity.
SSH_AUTH_SOCKis not the only way it is found, so clearing that variable does not settle it either.Enough identities and the server hits
MaxAuthTriesand disconnects. Note:keys_onlyalone does not fix this. It gates agent identities only (key_manager.rb:134); the ones that came from the config file are still offered.The fix
Clear
:keys, set:keys_only, and turn the agent off, so the only identity offered is the key BATs was handed. Parsing of~/.ssh/configstays enabled, so directives such asProxyJump,Port, andHostNamekeep working; only the identity sources are pinned.The gateway path dups
ssh_optionsafter these lines, so it inherits them and still substitutes:gateway_private_keyfor:key_dataas before.Evidence
Counting the identities each option set would offer, using the real
KeyManageron a machine with aHost *block and a loaded agent:key_dataonly)keys_only: truekeys: []+keys_only: true+use_agent: false(this PR)And connecting to a real sshd with a key it accepts, all four in one run:
Variant C, which offers the same single identity as D but by disabling config parsing outright, is the form that ran the full suite green (47 examples, 0 failures, 11 pending) on a Proxmox VE lab: run report. D is submitted here instead because it fixes the same failure without silently dropping
~/.ssh/configdirectives that other users may depend on.Tests
Adds a
#sshexample asserting the identity set. It fails on master and passes with the fix.bundle exec rspec spec/bat: 42 examples, 0 failures.Related: #66 adds a Proxmox VE cloud config template. The two are independent; this fix stands on its own on any IaaS.