Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sftp: add support for ~/.ssh/config, fixes #37 #38

Merged
merged 3 commits into from
Sep 17, 2024

Conversation

ThomasWaldmann
Copy link
Member

No description provided.

@mirko
Copy link

mirko commented Sep 15, 2024

Works for me, thanks a lot!

Some nit-picking:

  • there could be multiple IdentityKeys specified. From paramiko 1.9.1 onwards paramiko.SSHConfig() returns identitykey as list, no matter how many (incl. only 1) - before (<=1.9.0) it only ever returned it as string (not supporting multiple). Given that you don't specify the required version of paramiko, you might want to either do so, or handle both cases (if not isinstance(cfg["identityfile"], list): cfg["identityfile"] = [cfg["identityfile"]] or alike.
  • var name scd - what does the d stand for? :) scd immediately triggers me in regard of the infamous smartcard-daemon, which slightly confused me at first (EDIT: ah, probably SshConfigDict!)
  • you might still want to also parse /etc/ssh/ssh_client instead of only ~/.ssh/config and override/supplement options from the former with ones from the latter. Question popping up: does ~/.ssh/config extend or override options from /etc/ssh/config which are allowed to be specified multiple times?

Anyway, here a convenient pointer to how duplicity uses paramiko and deals with the quirks: https://git.launchpad.net/duplicity/tree/duplicity/backends/ssh_paramiko_backend.py#n210

@mirko
Copy link

mirko commented Sep 15, 2024

Oh, and maybe also still parse respective parts from the URL provided.

I think it's more than reasonable to assume that, if I specify sftp://USER_X@HOST:55555/ it's connecting to port 55555 as USER_X - no matter what's defined in ~/.ssh/config in section HOST.

So:

  • /etc/ssh/ssh_config [*]
  • ~/.ssh/config
  • elements specified as part of the URL

[*] Of course there's also /etc/ssh/ssh_config.d/* nowadays. Just for the record.

@ThomasWaldmann
Copy link
Member Author

I didn't find any docs about how to parse multiple ssh configs (like user and system, and includes), so I just called parse multiple times: first user, then system.

Guess we might need another PR to improve that.

@ThomasWaldmann
Copy link
Member Author

I'll squash everything together after review is finished, so we can have a 0.0.3 release soon.

@mirko
Copy link

mirko commented Sep 15, 2024

I think we could do it as simple as duplicity does it: https://git.launchpad.net/duplicity/tree/duplicity/backends/ssh_paramiko_backend.py#n210

Parse system config and put it into a dict.
Parse user config and update the dict.
Parse URL and update the dict.

Yes, that would replace instead of adding options to keys which are allowed to be specified multiple times, but I think that's more predictable and easier to document behaviour as well.

I didn't find any docs about how to parse multiple ssh configs (like user and system, and includes), so I just called parse multiple times

that's also how duplicity does it

first user, then system.

shouldn't it be the other way round? First system and override with duplicates specified in user [and then override with elements from the URL]?

@ThomasWaldmann ThomasWaldmann force-pushed the ssh-config-support branch 2 times, most recently from e007950 to 330e026 Compare September 16, 2024 12:04
@ThomasWaldmann ThomasWaldmann force-pushed the ssh-config-support branch 2 times, most recently from 5ff7a6a to a8ba7de Compare September 16, 2024 15:17
@ThomasWaldmann
Copy link
Member Author

OK, I'll check the duplicity way now. I first parsed user then system due to the documented "first match counts" behavior when dealing with the whole configs.

@mirko
Copy link

mirko commented Sep 16, 2024

Not tested, but looking through the changeset it's how I'd expect it to be(have) now

@ThomasWaldmann
Copy link
Member Author

I did some sftp tests (with the test suite, but it only works with my keys), works.

@mirko
Copy link

mirko commented Sep 16, 2024

I can test with my setup and assumptions in a few hours if that provides any benefits

- support configured IdentityFiles
- parse user and system ssh config
- prefer values in url over config
- add note about missing ssh_config.d/* support
export BORGSTORE_TEST_SFTP_URL="sftp://user@host:port/home/user/borgstore/temp-store"
@ThomasWaldmann
Copy link
Member Author

@mirko Now testing sftp is much easier, see latest commit.

@mirko
Copy link

mirko commented Sep 17, 2024

Didn't use the test suite - but tested all possible and impossible combinations.

What was to be expected: IdentityFile specified in /etc/ssh/config gets overridden and not supplemented by / if also specified in ~/.ssh/config.

Everything else set / replaced as intended.

@ThomasWaldmann ThomasWaldmann merged commit 0ef31fe into borgbackup:master Sep 17, 2024
7 checks passed
@ThomasWaldmann ThomasWaldmann deleted the ssh-config-support branch September 17, 2024 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants