fix: confine keys to rootdir; verify host keys by default - #8
Merged
Merged
Conversation
- Keys that are absolute or whose `..` segments leave rootdir now raise KeyError in getitem/setitem/delitem/mkdir (and `in` returns False). Keyword-only `allow_escape=True` restores the old behaviour. - Host keys are checked against known_hosts (system, user, and the ssh config's UserKnownHostsFile). Unknown keys are refused unless the host's ssh config sets StrictHostKeyChecking no/accept-new, or the caller passes keyword-only `missing_host_key_policy` (e.g. paramiko.AutoAddPolicy). Closes #7 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- known_hosts is parsed line by line; marker (@cert-authority/@Revoked), unsupported, malformed or undecodable lines are skipped instead of aborting (or crashing) the connection. - GlobalKnownHostsFile/UserKnownHostsFile replace the defaults, as in OpenSSH (/dev/null or none = no file). - Keys recorded under HostKeyAlias or the lower-cased host name are matched. - Subdirectory instances (new connections) are pinned to the host key their parent connected with. - Keys containing NUL are refused; clearer refusal message. - Integration tests fail instead of skipping when SSH_TEST_HOST is set explicitly (as in CI) and the connection fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hable configured test server Co-Authored-By: Claude Opus 5 <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.
Closes #7
What changes
Keys confined to
rootdir. A key that is absolute, or whose..segments leaverootdirafter normalisation, raisesKeyErrorin__getitem__,__setitem__,__delitem__andmkdir, andinreturnsFalse. Keys that stay inside (a/../b,./a) behave as before. Keyword-onlyallow_escape=Truerestores the old behaviour. Symbolic links on the server are not resolved by this check (documented).Host keys verified by default. The client used
AutoAddPolicy, accepting any host key. It now readsknown_hoststhe way OpenSSH picks the files (GlobalKnownHostsFile/UserKnownHostsFilefrom~/.ssh/configreplace the defaults;/dev/nullmeans none), parsing leniently (marker, unsupported or malformed lines are skipped, so@revokedis not honoured), matchingHostKeyAliasand lower-cased names. For a host not found there it:StrictHostKeyChecking no/off/accept-newaccept the key for the session;ssh, set the ssh config option, or pass the policy);missing_host_key_policy=(a paramiko policy class or instance) overrides both.A changed key for a known host is always refused (paramiko behaviour). Subdirectory instances (which open new connections) are pinned to the key their parent connected with. Keys containing NUL are refused.
Release note (behaviour change)
known_hostsand has noStrictHostKeyCheckingsetting now fails instead of silently trusting the key. Fix:ssh <host>once, or passmissing_host_key_policy=paramiko.AutoAddPolicy.rootdirnow raiseKeyError; passallow_escape=Trueif you relied on that.No fleet dependents.
Review
Independent refute-review by a sub-agent: first round found one blocker (a known_hosts file with
@cert-authority/malformed lines could crash the constructor) and should-fixes (UserKnownHostsFile should replace defaults, silent test skips, subdirectory re-trust, HostKeyAlias/case); all fixed. Re-review: no blockers; its performance note (quadratic key loading) and the remaining silent-skip path were fixed too.Tests
sshdol/tests/test_confinement.py(no server needed) plus doctests. The existing integration tests run in hosted CI against a localhost sshd whose ssh config setsStrictHostKeyChecking no, which exercises the config-following path.wads ci-localpasses (integration tests skipped locally: no test server). WhenSSH_TEST_HOSTis set explicitly (as in CI), a failing connection now fails the tests instead of skipping them.🤖 Generated with Claude Code