Repository: borgbackup/borgstore

Issue:
SFTP backend does not use local SSH-client's config
Coming from duplicity - which AFAIK also uses paramiko for accessing SFTP endpoints - I expected to be able to log into the SFTP backend the same (or similar) way.

For duplicity using the SFTP backend my URL is rather simple: `sftp://bck-host//storage/backup/duplicity`.

User, Port, IdentityKeyFile, etc. are supplemented from respective host defined in `~/.ssh/config`.

Example ssh-config-file:

```
Host bck-host
	IdentitiesOnly yes
	Hostname %h
	User bck
	IdentityFile ~/.ssh/id_ed25519_bck
	HashKnownHosts no
	Ciphers chacha20-poly1305@openssh.com
```

While it would be nice for borgstore to also use the local SSH-client's config (I figure paramiko already provides functionality for that), I'd at least like to have the possibility / to know how to pass an IdentityFile for login to the SFTP-server.

Is there (already) a way for doing so?

Discussion:
mirko: Works for me, thanks a lot!
Some nit-picking:
 - there could be multiple `IdentityKey`s 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
 - 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. Keep in mind, that lists (e.g. `identityfile` for paramiko >= 1.9.1). Question  popping up: does ~/.ssh/config extend or override options from /etc/ssh/config which are allowed to be specified multiple times?
mirko: 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: 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: I'll squash everything together after review is finished, so we can have a 0.0.3 release soon.
mirko: 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 do document behaviour as well.
ThomasWaldmann: 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: Not tested, but looking through the changeset it's how I'd expect it to be(have)
ThomasWaldmann: I did some sftp tests (with the test suite, but it only works with my keys), works.
mirko: I can test with my setup and assumptions in a few hours if that provides any benefits

Patch:
diff --git a/pyproject.toml b/pyproject.toml
index 1daaf02..2f187d2 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -23,7 +23,7 @@ classifiers = [
 license = {text="BSD"}
 requires-python = ">=3.9"
 dependencies = [
-    "paramiko",
+    "paramiko >= 1.9.1",  # 1.9.1+ supports multiple IdentityKey entries in .ssh/config
 ]
 
 [project.urls]
diff --git a/src/borgstore/backends/sftp.py b/src/borgstore/backends/sftp.py
index 1d25e0c..c5fe86a 100644
--- a/src/borgstore/backends/sftp.py
+++ b/src/borgstore/backends/sftp.py
@@ -26,21 +26,60 @@ def get_sftp_backend(url):
     """
     m = re.match(sftp_regex, url, re.VERBOSE)
     if m:
-        return Sftp(username=m["username"], hostname=m["hostname"], port=int(m["port"] or "22"), path=m["path"])
+        return Sftp(username=m["username"], hostname=m["hostname"], port=int(m["port"] or "0"), path=m["path"])
 
 
 class Sftp(BackendBase):
-    def __init__(self, hostname: str, path: str, port: int = 22, username: Optional[str] = None):
+    def __init__(self, hostname: str, path: str, port: int = 0, username: Optional[str] = None):
         self.username = username
         self.hostname = hostname
         self.port = port
         self.base_path = path
         self.opened = False
 
+    def _get_host_config_from_file(self, path: str, hostname: str):
+        """lookup the configuration for hostname in path (ssh config file)"""
+        config_path = Path(path).expanduser()
+        try:
+            ssh_config = paramiko.SSHConfig.from_path(config_path)
+        except FileNotFoundError:
+            return paramiko.SSHConfigDict()  # empty dict
+        else:
+            return ssh_config.lookup(hostname)
+
+    def _get_host_config(self):
+        """assemble all given and configured host config values"""
+        host_config = paramiko.SSHConfigDict()
+        # self.hostname might be an alias/shortcut (with real hostname given in configuration),
+        # but there might be also nothing in the configs at all for self.hostname:
+        host_config["hostname"] = self.hostname
+        # first process system-wide ssh config, then override with user ssh config:
+        host_config.update(self._get_host_config_from_file("/etc/ssh/ssh_config", self.hostname))
+        # note: no support yet for /etc/ssh/ssh_config.d/*
+        host_config.update(self._get_host_config_from_file("~/.ssh/config", self.hostname))
+        # now override configured values with given values
+        if self.username is not None:
+            host_config.update({"user": self.username})
+        if self.port != 0:
+            host_config.update({"port": self.port})
+        # make sure port is present and is an int
+        host_config["port"] = int(host_config.get("port") or 22)
+        return host_config
+
     def _connect(self):
         ssh = paramiko.SSHClient()
-        ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy())
-        ssh.connect(hostname=self.hostname, username=self.username, port=self.port, allow_agent=True)
+        # note: we do not deal with unknown hosts and ssh.set_missing_host_key_policy here,
+        # the user shall just make "first contact" to any new host using ssh or sftp cli command
+        # and interactively verify remote host fingerprints.
+        ssh.load_system_host_keys()  # this is documented to load the USER's known_hosts file
+        host_config = self._get_host_config()
+        ssh.connect(
+            hostname=host_config["hostname"],
+            username=host_config.get("user"),  # if None, paramiko will use current user
+            port=host_config["port"],
+            key_filename=host_config.get("identityfile"),  # list of keys, ~ is already expanded
+            allow_agent=True,
+        )
         self.client = ssh.open_sftp()
 
     def _disconnect(self):


Test patch:
diff --git a/tests/test_backends.py b/tests/test_backends.py
index 7a15ed4..e48305d 100644
--- a/tests/test_backends.py
+++ b/tests/test_backends.py
@@ -2,6 +2,7 @@
 Generic testing for the misc. backend implementations.
 """
 
+import os
 from pathlib import Path
 
 import pytest
@@ -32,12 +33,11 @@ def posixfs_backend_created(tmp_path):
 
 
 def _get_sftp_backend():
-    # needs an authorized key loaded into the ssh agent. pytest works, tox doesn't:
-    # return Sftp(username="tw", hostname="localhost", path="/Users/tw/w/borgstore/temp-store")
-    # for tests with higher latency:
-    return Sftp(
-        username="twaldmann", hostname="shell.ipv4.thinkmo.de", port=2222, path="/home/twaldmann/borgstore/temp-store"
-    )
+    # export BORGSTORE_TEST_SFTP_URL="sftp://user@host:port/home/user/borgstore/temp-store"
+    # needs an authorized key loaded into the ssh agent. pytest works, tox doesn't.
+    url = os.environ.get("BORGSTORE_TEST_SFTP_URL")
+    if url:
+        return get_sftp_backend(url)
 
 
 def check_sftp_available():
@@ -46,7 +46,7 @@ def check_sftp_available():
         be = _get_sftp_backend()
         be.create()  # first sftp activity happens here
     except Exception:
-        return False
+        return False  # use "raise" here for debugging sftp store issues
     else:
         be.destroy()
         return True
@@ -96,8 +96,8 @@ def test_file_url(url, path):
     "url,username,hostname,port,path",
     [
         ("sftp://username@hostname:2222/some/path", "username", "hostname", 2222, "/some/path"),
-        ("sftp://username@hostname/some/path", "username", "hostname", 22, "/some/path"),
-        ("sftp://hostname/some/path", None, "hostname", 22, "/some/path"),
+        ("sftp://username@hostname/some/path", "username", "hostname", 0, "/some/path"),
+        ("sftp://hostname/some/path", None, "hostname", 0, "/some/path"),
     ],
 )
 def test_sftp_url(url, username, hostname, port, path):
@@ -105,7 +105,7 @@ def test_sftp_url(url, username, hostname, port, path):
     assert isinstance(backend, Sftp)
     assert backend.username == username
     assert backend.hostname == hostname
-    assert backend.port == port
+    assert backend.port == port  # note: 0 means "not given" (and will usually mean 22 in the end)
     assert backend.base_path == path