From 054af4b76ee918eb4eb27e775d53bb814f48ff0f Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 7 Jan 2021 11:25:26 -0500 Subject: [PATCH 01/25] Sketch of where SFTP setup needs to happen. --- integration/conftest.py | 12 ++++++++++++ integration/util.py | 21 +++++++++++---------- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/integration/conftest.py b/integration/conftest.py index f37ec9353..cc4ffaa08 100644 --- a/integration/conftest.py +++ b/integration/conftest.py @@ -7,6 +7,7 @@ from os import mkdir, listdir, environ from os.path import join, exists from tempfile import mkdtemp, mktemp from functools import partial +from json import loads from foolscap.furl import ( decode_furl, @@ -37,6 +38,8 @@ from util import ( _tahoe_runner_optional_coverage, await_client_ready, TahoeProcess, + cli, + _run_node, ) @@ -350,6 +353,15 @@ def alice(reactor, temp_dir, introducer_furl, flog_gatherer, storage_nodes, requ ) ) await_client_ready(process) + cli(process, "create-alias", "test") + rwcap = loads(cli(process, "list-aliases", "--json"))["test"]["readwrite"] + # TODO at this point we need to: + # 1. configure sftpd + # 2. add an sftp access file with username, password, and rwcap + # 3. eventually, add sftp access with public key + process.kill() + pytest_twisted.blockon(_run_node(reactor, process.node_dir, request, None)) + await_client_ready(process) return process diff --git a/integration/util.py b/integration/util.py index eed073225..60d96a214 100644 --- a/integration/util.py +++ b/integration/util.py @@ -5,6 +5,7 @@ from os import mkdir, environ from os.path import exists, join from six.moves import StringIO from functools import partial +from subprocess import check_output from twisted.python.filepath import ( FilePath, @@ -175,6 +176,10 @@ class TahoeProcess(object): u"portnum", ) + def kill(self): + """Kill the process, block until it's done.""" + _cleanup_tahoe_process(self.transport, self.transport.exited) + def __str__(self): return "".format(self._node_dir) @@ -249,7 +254,7 @@ def _create_node(reactor, request, temp_dir, introducer_furl, flog_gatherer, nam '--helper', ] if not storage: - args.append('--no-storage') + args.append('--no-storage') args.append(node_dir) _tahoe_runner_optional_coverage(done_proto, reactor, request, args) @@ -390,17 +395,13 @@ def await_file_vanishes(path, timeout=10): raise FileShouldVanishException(path, timeout) -def cli(request, reactor, node_dir, *argv): +def cli(node, *argv): """ - Run a tahoe CLI subcommand for a given node, optionally running - under coverage if '--coverage' was supplied. + Run a tahoe CLI subcommand for a given node in a blocking manner, returning + the output. """ - proto = _CollectOutputProtocol() - _tahoe_runner_optional_coverage( - proto, reactor, request, - ['--node-directory', node_dir] + list(argv), - ) - return proto.done + arguments = ["tahoe", '--node-directory', node.node_dir] + return check_output(arguments + list(argv)) def node_url(node_dir, uri_fragment): From 3b29a5f70725299b0783dd0e5fb54227e868bd31 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 7 Jan 2021 11:59:23 -0500 Subject: [PATCH 02/25] Work with new Unicode configs. --- src/allmydata/frontends/sftpd.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/frontends/sftpd.py b/src/allmydata/frontends/sftpd.py index b25ac0270..65a01b322 100644 --- a/src/allmydata/frontends/sftpd.py +++ b/src/allmydata/frontends/sftpd.py @@ -2012,5 +2012,5 @@ class SFTPServer(service.MultiService): f = SSHFactory() f.portal = p - s = strports.service(sftp_portstr, f) + s = strports.service(six.ensure_str(sftp_portstr), f) s.setServiceParent(self) From a536a1a970a6455dc17f89e248acacfd50cfd9a8 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 7 Jan 2021 12:50:31 -0500 Subject: [PATCH 03/25] First passing end-to-end test of SFTP --- integration/conftest.py | 37 +++++++++++++++++++++++++++++++++---- integration/test_sftp.py | 37 +++++++++++++++++++++++++++++++++++++ setup.py | 1 + 3 files changed, 71 insertions(+), 4 deletions(-) create mode 100644 integration/test_sftp.py diff --git a/integration/conftest.py b/integration/conftest.py index cc4ffaa08..28e364cb6 100644 --- a/integration/conftest.py +++ b/integration/conftest.py @@ -8,6 +8,7 @@ from os.path import join, exists from tempfile import mkdtemp, mktemp from functools import partial from json import loads +from subprocess import check_call from foolscap.furl import ( decode_furl, @@ -342,6 +343,12 @@ def storage_nodes(reactor, temp_dir, introducer, introducer_furl, flog_gatherer, return nodes +def generate_ssh_key(path): + """Create a new SSH private/public key pair.""" + check_call(["ckeygen", "--type", "rsa", "--no-passphrase", "--bits", "512", + "--file", path]) + + @pytest.fixture(scope='session') @log_call(action_type=u"integration:alice", include_args=[], include_result=False) def alice(reactor, temp_dir, introducer_furl, flog_gatherer, storage_nodes, request): @@ -353,14 +360,36 @@ def alice(reactor, temp_dir, introducer_furl, flog_gatherer, storage_nodes, requ ) ) await_client_ready(process) + + # 1. Create a new RW directory cap: cli(process, "create-alias", "test") rwcap = loads(cli(process, "list-aliases", "--json"))["test"]["readwrite"] - # TODO at this point we need to: - # 1. configure sftpd - # 2. add an sftp access file with username, password, and rwcap - # 3. eventually, add sftp access with public key + + # 2. Enable SFTP on the node: + ssh_key_path = join(process.node_dir, "private", "ssh_host_rsa_key") + accounts_path = join(process.node_dir, "private", "accounts") + with open(join(process.node_dir, "tahoe.cfg"), "a") as f: + f.write("""\ +[sftpd] +enabled = true +port = tcp:8022:interface=127.0.0.1 +host_pubkey_file = {ssh_key_path}.pub +host_privkey_file = {ssh_key_path} +accounts.file = {accounts_path} +""".format(ssh_key_path=ssh_key_path, accounts_path=accounts_path)) + generate_ssh_key(ssh_key_path) + + # 3. Add a SFTP access file with username, password, and rwcap. + with open(accounts_path, "w") as f: + f.write("""\ +alice password {} +""".format(rwcap)) + # TODO add sftp access with public key + + # 4. Restart the node with new SFTP config. process.kill() pytest_twisted.blockon(_run_node(reactor, process.node_dir, request, None)) + await_client_ready(process) return process diff --git a/integration/test_sftp.py b/integration/test_sftp.py new file mode 100644 index 000000000..f308ddf5a --- /dev/null +++ b/integration/test_sftp.py @@ -0,0 +1,37 @@ +""" +It's possible to create/rename/delete files and directories in Tahoe-LAFS using +SFTP. +""" + +from __future__ import unicode_literals +from __future__ import absolute_import +from __future__ import division +from __future__ import print_function + +from future.utils import PY2 +if PY2: + from future.builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, bytes, dict, list, object, range, str, max, min # noqa: F401 + +from paramiko import SSHClient +from paramiko.client import AutoAddPolicy +from paramiko.sftp_client import SFTPClient + + +def test_read_write_files(alice): + """It's possible to upload and download files.""" + client = SSHClient() + client.set_missing_host_key_policy(AutoAddPolicy) + client.connect( + "localhost", username="alice", password="password", port=8022, + look_for_keys=False + ) + sftp = SFTPClient.from_transport(client.get_transport()) + f = sftp.file("myfile", "wb") + f.write(b"abc") + f.write(b"def") + f.close() + f = sftp.file("myfile", "rb") + assert f.read(4) == b"abcd" + assert f.read(2) == b"ef" + assert f.read(1) == b"" + f.close() diff --git a/setup.py b/setup.py index 0e5a43dba..32581e293 100644 --- a/setup.py +++ b/setup.py @@ -399,6 +399,7 @@ setup(name="tahoe-lafs", # also set in __init__.py "html5lib", "junitxml", "tenacity", + "paramiko", ] + tor_requires + i2p_requires, "tor": tor_requires, "i2p": i2p_requires, From b8879916b2618c810724693e4232d3aa996c7547 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 7 Jan 2021 13:27:34 -0500 Subject: [PATCH 04/25] More SFTP integration tests. --- integration/test_sftp.py | 80 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 73 insertions(+), 7 deletions(-) diff --git a/integration/test_sftp.py b/integration/test_sftp.py index f308ddf5a..eeeb8b0b7 100644 --- a/integration/test_sftp.py +++ b/integration/test_sftp.py @@ -12,20 +12,53 @@ from future.utils import PY2 if PY2: from future.builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, bytes, dict, list, object, range, str, max, min # noqa: F401 +from posixpath import join +from stat import S_ISDIR + from paramiko import SSHClient from paramiko.client import AutoAddPolicy from paramiko.sftp_client import SFTPClient +from paramiko.ssh_exception import AuthenticationException + +import pytest + + +def connect_sftp(alice, username="alice", password="password"): + """Create an SFTP client.""" + client = SSHClient() + client.set_missing_host_key_policy(AutoAddPolicy) + client.connect( + "localhost", username=username, password=password, port=8022, + look_for_keys=False + ) + sftp = SFTPClient.from_transport(client.get_transport()) + + def rmdir(path, delete_root=True): + for f in sftp.listdir_attr(path=path): + childpath = join(path, f.filename) + if S_ISDIR(f.st_mode): + rmdir(childpath) + else: + sftp.remove(childpath) + if delete_root: + sftp.rmdir(path) + + # Delete any files left over from previous tests :( + rmdir("/", delete_root=False) + + return sftp + + +def test_bad_account_password(alice): + """Can't login with unknown username or wrong password.""" + for u, p in [("alice", "wrong"), ("someuser", "password")]: + with pytest.raises(AuthenticationException): + connect_sftp(alice, u, p) def test_read_write_files(alice): """It's possible to upload and download files.""" - client = SSHClient() - client.set_missing_host_key_policy(AutoAddPolicy) - client.connect( - "localhost", username="alice", password="password", port=8022, - look_for_keys=False - ) - sftp = SFTPClient.from_transport(client.get_transport()) + sftp = connect_sftp(alice) f = sftp.file("myfile", "wb") f.write(b"abc") f.write(b"def") @@ -35,3 +68,36 @@ def test_read_write_files(alice): assert f.read(2) == b"ef" assert f.read(1) == b"" f.close() + + +def test_directories(alice): + """ + It's possible to create, list directories, and create and remove files in + them. + """ + sftp = connect_sftp(alice) + assert sftp.listdir() == [] + + sftp.mkdir("childdir") + assert sftp.listdir() == ["childdir"] + + with sftp.file("myfile", "wb") as f: + f.write(b"abc") + assert sorted(sftp.listdir()) == ["childdir", "myfile"] + + sftp.chdir("childdir") + assert sftp.listdir() == [] + + with sftp.file("myfile2", "wb") as f: + f.write(b"def") + assert sftp.listdir() == ["myfile2"] + + sftp.chdir(None) # root + with sftp.file("childdir/myfile2", "rb") as f: + assert f.read() == b"def" + + sftp.remove("myfile") + assert sftp.listdir() == ["childdir"] + + sftp.rmdir("childdir") + assert sftp.listdir() == [] From 3764e3b6b1d75fbdba105b9cb2699c842b1c38b2 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 7 Jan 2021 13:59:57 -0500 Subject: [PATCH 05/25] A (so far failing) test for SSH public key authentication. --- integration/conftest.py | 30 +++++++++++++------------ integration/test_sftp.py | 47 +++++++++++++++++++++++++++++++--------- integration/util.py | 8 ++++++- 3 files changed, 60 insertions(+), 25 deletions(-) diff --git a/integration/conftest.py b/integration/conftest.py index 28e364cb6..4ae22deee 100644 --- a/integration/conftest.py +++ b/integration/conftest.py @@ -8,7 +8,6 @@ from os.path import join, exists from tempfile import mkdtemp, mktemp from functools import partial from json import loads -from subprocess import check_call from foolscap.furl import ( decode_furl, @@ -41,6 +40,7 @@ from util import ( TahoeProcess, cli, _run_node, + generate_ssh_key ) @@ -343,12 +343,6 @@ def storage_nodes(reactor, temp_dir, introducer, introducer_furl, flog_gatherer, return nodes -def generate_ssh_key(path): - """Create a new SSH private/public key pair.""" - check_call(["ckeygen", "--type", "rsa", "--no-passphrase", "--bits", "512", - "--file", path]) - - @pytest.fixture(scope='session') @log_call(action_type=u"integration:alice", include_args=[], include_result=False) def alice(reactor, temp_dir, introducer_furl, flog_gatherer, storage_nodes, request): @@ -366,7 +360,7 @@ def alice(reactor, temp_dir, introducer_furl, flog_gatherer, storage_nodes, requ rwcap = loads(cli(process, "list-aliases", "--json"))["test"]["readwrite"] # 2. Enable SFTP on the node: - ssh_key_path = join(process.node_dir, "private", "ssh_host_rsa_key") + host_ssh_key_path = join(process.node_dir, "private", "ssh_host_rsa_key") accounts_path = join(process.node_dir, "private", "accounts") with open(join(process.node_dir, "tahoe.cfg"), "a") as f: f.write("""\ @@ -376,15 +370,23 @@ port = tcp:8022:interface=127.0.0.1 host_pubkey_file = {ssh_key_path}.pub host_privkey_file = {ssh_key_path} accounts.file = {accounts_path} -""".format(ssh_key_path=ssh_key_path, accounts_path=accounts_path)) - generate_ssh_key(ssh_key_path) +""".format(ssh_key_path=host_ssh_key_path, accounts_path=accounts_path)) + generate_ssh_key(host_ssh_key_path) - # 3. Add a SFTP access file with username, password, and rwcap. + # 3. Add a SFTP access file with username/password and SSH key auth. + + # The client SSH key path is typically going to be somewhere else (~/.ssh, + # typically), but for convenience sake for testing we'll put it inside node. + client_ssh_key_path = join(process.node_dir, "private", "ssh_client_rsa_key") + generate_ssh_key(client_ssh_key_path) + # Pub key format is "ssh-rsa ". We want the key. + ssh_public_key = open(client_ssh_key_path + ".pub").read().strip().split()[1] with open(accounts_path, "w") as f: f.write("""\ -alice password {} -""".format(rwcap)) - # TODO add sftp access with public key +alice password {rwcap} + +alice2 ssh-rsa {ssh_public_key} {rwcap} +""".format(rwcap=rwcap, ssh_public_key=ssh_public_key)) # 4. Restart the node with new SFTP config. process.kill() diff --git a/integration/test_sftp.py b/integration/test_sftp.py index eeeb8b0b7..f9a7830ac 100644 --- a/integration/test_sftp.py +++ b/integration/test_sftp.py @@ -23,14 +23,11 @@ from paramiko.ssh_exception import AuthenticationException import pytest -def connect_sftp(alice, username="alice", password="password"): +def connect_sftp(connect_args={"username": "alice", "password": "password"}): """Create an SFTP client.""" client = SSHClient() client.set_missing_host_key_policy(AutoAddPolicy) - client.connect( - "localhost", username=username, password=password, port=8022, - look_for_keys=False - ) + client.connect("localhost", port=8022, look_for_keys=False, **connect_args) sftp = SFTPClient.from_transport(client.get_transport()) def rmdir(path, delete_root=True): @@ -49,16 +46,30 @@ def connect_sftp(alice, username="alice", password="password"): return sftp -def test_bad_account_password(alice): - """Can't login with unknown username or wrong password.""" +def test_bad_account_password_ssh_key(alice): + """ + Can't login with unknown username, wrong password, or wrong SSH pub key. + """ for u, p in [("alice", "wrong"), ("someuser", "password")]: with pytest.raises(AuthenticationException): - connect_sftp(alice, u, p) + connect_sftp(connect_args={ + "username": u, "password": p, + }) + # TODO bad pubkey + + +def test_ssh_key_auth(alice): + """It's possible to login authenticating with SSH public key.""" + key_filename = join(alice.node_dir, "private", "ssh_client_rsa_key") + sftp = connect_sftp(connect_args={ + "username": "alice2", "key_filename": key_filename + }) + assert sftp.listdir() == [] def test_read_write_files(alice): """It's possible to upload and download files.""" - sftp = connect_sftp(alice) + sftp = connect_sftp() f = sftp.file("myfile", "wb") f.write(b"abc") f.write(b"def") @@ -75,7 +86,7 @@ def test_directories(alice): It's possible to create, list directories, and create and remove files in them. """ - sftp = connect_sftp(alice) + sftp = connect_sftp() assert sftp.listdir() == [] sftp.mkdir("childdir") @@ -101,3 +112,19 @@ def test_directories(alice): sftp.rmdir("childdir") assert sftp.listdir() == [] + + +def test_rename(alice): + """Directories and files can be renamed.""" + sftp = connect_sftp() + sftp.mkdir("dir") + + filepath = join("dir", "file") + with sftp.file(filepath, "wb") as f: + f.write(b"abc") + + sftp.rename(filepath, join("dir", "file2")) + sftp.rename("dir", "dir2") + + with sftp.file(join("dir2", "file2"), "rb") as f: + assert f.read() == b"abc" diff --git a/integration/util.py b/integration/util.py index 60d96a214..0e8fea2be 100644 --- a/integration/util.py +++ b/integration/util.py @@ -5,7 +5,7 @@ from os import mkdir, environ from os.path import exists, join from six.moves import StringIO from functools import partial -from subprocess import check_output +from subprocess import check_output, check_call from twisted.python.filepath import ( FilePath, @@ -506,3 +506,9 @@ def await_client_ready(tahoe, timeout=10, liveness=60*2): tahoe, ) ) + + +def generate_ssh_key(path): + """Create a new SSH private/public key pair.""" + check_call(["ckeygen", "--type", "rsa", "--no-passphrase", "--bits", "512", + "--file", path]) From 7a15f7e11dd1e68e5e9d0b7e2e350b20a83752fa Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 8 Jan 2021 13:32:11 -0500 Subject: [PATCH 06/25] Switch to modern (circa 2014!) Conch API. --- src/allmydata/frontends/auth.py | 37 +++++++-------------------------- 1 file changed, 8 insertions(+), 29 deletions(-) diff --git a/src/allmydata/frontends/auth.py b/src/allmydata/frontends/auth.py index 1bd481321..943db27c8 100644 --- a/src/allmydata/frontends/auth.py +++ b/src/allmydata/frontends/auth.py @@ -6,6 +6,7 @@ from twisted.internet import defer from twisted.cred import error, checkers, credentials from twisted.conch import error as conch_error from twisted.conch.ssh import keys +from twisted.conch.checkers import SSHPublicKeyChecker, InMemorySSHKeyDB from allmydata.util import base32 from allmydata.util.fileutil import abspath_expanduser_unicode @@ -29,7 +30,7 @@ class AccountFileChecker(object): def __init__(self, client, accountfile): self.client = client self.passwords = {} - self.pubkeys = {} + pubkeys = {} self.rootcaps = {} with open(abspath_expanduser_unicode(accountfile), "r") as f: for line in f: @@ -40,12 +41,14 @@ class AccountFileChecker(object): if passwd.startswith("ssh-"): bits = rest.split() keystring = " ".join([passwd] + bits[:-1]) + key = keys.Key.fromString(keystring) rootcap = bits[-1] - self.pubkeys[name] = keystring + pubkeys[name] = [key] else: self.passwords[name] = passwd rootcap = rest self.rootcaps[name] = rootcap + self._pubkeychecker = SSHPublicKeyChecker(InMemorySSHKeyDB(pubkeys)) def _avatarId(self, username): return FTPAvatarID(username, self.rootcaps[username]) @@ -57,11 +60,9 @@ class AccountFileChecker(object): def requestAvatarId(self, creds): if credentials.ISSHPrivateKey.providedBy(creds): - # Re-using twisted.conch.checkers.SSHPublicKeyChecker here, rather - # than re-implementing all of the ISSHPrivateKey checking logic, - # would be better. That would require Twisted 14.1.0 or newer, - # though. - return self._checkKey(creds) + d = defer.maybeDeferred(self._pubkeychecker.requestAvatarId, creds) + d.addCallback(self._avatarId) + return d elif credentials.IUsernameHashedPassword.providedBy(creds): return self._checkPassword(creds) elif credentials.IUsernamePassword.providedBy(creds): @@ -86,28 +87,6 @@ class AccountFileChecker(object): d.addCallback(self._cbPasswordMatch, str(creds.username)) return d - def _checkKey(self, creds): - """ - Determine whether some key-based credentials correctly authenticates a - user. - - Returns a Deferred that fires with the username if so or with an - UnauthorizedLogin failure otherwise. - """ - - # Is the public key indicated by the given credentials allowed to - # authenticate the username in those credentials? - if creds.blob == self.pubkeys.get(creds.username): - if creds.signature is None: - return defer.fail(conch_error.ValidPublicKey()) - - # Is the signature in the given credentials the correct - # signature for the data in those credentials? - key = keys.Key.fromString(creds.blob) - if key.verify(creds.signature, creds.sigData): - return defer.succeed(self._avatarId(creds.username)) - - return defer.fail(error.UnauthorizedLogin()) @implementer(checkers.ICredentialsChecker) class AccountURLChecker(object): From 2589737e1e39b93f516cae84624ac1cc1cbf4d5a Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 8 Jan 2021 13:33:22 -0500 Subject: [PATCH 07/25] Public key auth test passes. --- integration/test_sftp.py | 8 +++++--- integration/util.py | 4 ++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/integration/test_sftp.py b/integration/test_sftp.py index f9a7830ac..51d3f15c3 100644 --- a/integration/test_sftp.py +++ b/integration/test_sftp.py @@ -19,6 +19,7 @@ from paramiko import SSHClient from paramiko.client import AutoAddPolicy from paramiko.sftp_client import SFTPClient from paramiko.ssh_exception import AuthenticationException +from paramiko.rsakey import RSAKey import pytest @@ -27,7 +28,8 @@ def connect_sftp(connect_args={"username": "alice", "password": "password"}): """Create an SFTP client.""" client = SSHClient() client.set_missing_host_key_policy(AutoAddPolicy) - client.connect("localhost", port=8022, look_for_keys=False, **connect_args) + client.connect("localhost", port=8022, look_for_keys=False, + allow_agent=False, **connect_args) sftp = SFTPClient.from_transport(client.get_transport()) def rmdir(path, delete_root=True): @@ -60,9 +62,9 @@ def test_bad_account_password_ssh_key(alice): def test_ssh_key_auth(alice): """It's possible to login authenticating with SSH public key.""" - key_filename = join(alice.node_dir, "private", "ssh_client_rsa_key") + key = RSAKey(filename=join(alice.node_dir, "private", "ssh_client_rsa_key")) sftp = connect_sftp(connect_args={ - "username": "alice2", "key_filename": key_filename + "username": "alice2", "pkey": key }) assert sftp.listdir() == [] diff --git a/integration/util.py b/integration/util.py index 0e8fea2be..d4c09d073 100644 --- a/integration/util.py +++ b/integration/util.py @@ -510,5 +510,5 @@ def await_client_ready(tahoe, timeout=10, liveness=60*2): def generate_ssh_key(path): """Create a new SSH private/public key pair.""" - check_call(["ckeygen", "--type", "rsa", "--no-passphrase", "--bits", "512", - "--file", path]) + check_call(["ckeygen", "--type", "rsa", "--no-passphrase", "--bits", "2048", + "--file", path, "--private-key-subtype", "v1"]) From e986e864314589c4c8dbca6395cfb86b3a089a19 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 8 Jan 2021 13:41:04 -0500 Subject: [PATCH 08/25] Test failure to auth. --- integration/test_sftp.py | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/integration/test_sftp.py b/integration/test_sftp.py index 51d3f15c3..f1cf92eab 100644 --- a/integration/test_sftp.py +++ b/integration/test_sftp.py @@ -23,6 +23,8 @@ from paramiko.rsakey import RSAKey import pytest +from .util import generate_ssh_key + def connect_sftp(connect_args={"username": "alice", "password": "password"}): """Create an SFTP client.""" @@ -48,16 +50,33 @@ def connect_sftp(connect_args={"username": "alice", "password": "password"}): return sftp -def test_bad_account_password_ssh_key(alice): +def test_bad_account_password_ssh_key(alice, tmpdir): """ Can't login with unknown username, wrong password, or wrong SSH pub key. """ + # Wrong password, wrong username: for u, p in [("alice", "wrong"), ("someuser", "password")]: with pytest.raises(AuthenticationException): connect_sftp(connect_args={ "username": u, "password": p, }) - # TODO bad pubkey + + another_key = join(str(tmpdir), "ssh_key") + generate_ssh_key(another_key) + good_key = RSAKey(filename=join(alice.node_dir, "private", "ssh_client_rsa_key")) + bad_key = RSAKey(filename=another_key) + + # Wrong key: + with pytest.raises(AuthenticationException): + connect_sftp(connect_args={ + "username": "alice2", "pkey": bad_key, + }) + + # Wrong username: + with pytest.raises(AuthenticationException): + connect_sftp(connect_args={ + "username": "someoneelse", "pkey": good_key, + }) def test_ssh_key_auth(alice): From f71dcfe9fcb13d1de226a79f9aaf3fe79b87c639 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 8 Jan 2021 13:42:10 -0500 Subject: [PATCH 09/25] Lint. --- src/allmydata/frontends/auth.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/allmydata/frontends/auth.py b/src/allmydata/frontends/auth.py index 943db27c8..de406d604 100644 --- a/src/allmydata/frontends/auth.py +++ b/src/allmydata/frontends/auth.py @@ -4,7 +4,6 @@ from zope.interface import implementer from twisted.web.client import getPage from twisted.internet import defer from twisted.cred import error, checkers, credentials -from twisted.conch import error as conch_error from twisted.conch.ssh import keys from twisted.conch.checkers import SSHPublicKeyChecker, InMemorySSHKeyDB From 6f3b3d07fdb76f53e1a64555481deaf38d40c76f Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 8 Jan 2021 13:43:23 -0500 Subject: [PATCH 10/25] News file. --- newsfragments/3584.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/3584.bugfix diff --git a/newsfragments/3584.bugfix b/newsfragments/3584.bugfix new file mode 100644 index 000000000..73650f40b --- /dev/null +++ b/newsfragments/3584.bugfix @@ -0,0 +1 @@ +SFTP public key auth likely works better, and SFTP in general was broken in the prerelease. \ No newline at end of file From 6b2a999f8d28d467320b6fd0384ff23751ae332d Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Mon, 11 Jan 2021 14:02:45 -0500 Subject: [PATCH 11/25] Replace ckeygen with Paramiko library calls, since ckeygen doesn't work on Windows. --- integration/util.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/integration/util.py b/integration/util.py index d4c09d073..39ec36a38 100644 --- a/integration/util.py +++ b/integration/util.py @@ -16,6 +16,8 @@ from twisted.internet.error import ProcessExitedAlready, ProcessDone import requests +from paramiko.rsakey import RSAKey + from allmydata.util.configutil import ( get_config, set_config, @@ -510,5 +512,7 @@ def await_client_ready(tahoe, timeout=10, liveness=60*2): def generate_ssh_key(path): """Create a new SSH private/public key pair.""" - check_call(["ckeygen", "--type", "rsa", "--no-passphrase", "--bits", "2048", - "--file", path, "--private-key-subtype", "v1"]) + key = RSAKey.generate(2048) + key.write_private_key_file(path) + with open(path + ".pub", "wb") as f: + f.write(b"%s %s" % (key.get_name(), key.get_base64())) From 6107e52f96249ff44726da6f19c0eb430f82f030 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Mon, 11 Jan 2021 15:26:38 -0500 Subject: [PATCH 12/25] Fix flake. --- integration/util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/util.py b/integration/util.py index 39ec36a38..64f02a446 100644 --- a/integration/util.py +++ b/integration/util.py @@ -5,7 +5,7 @@ from os import mkdir, environ from os.path import exists, join from six.moves import StringIO from functools import partial -from subprocess import check_output, check_call +from subprocess import check_output from twisted.python.filepath import ( FilePath, From 3489e381be45b094c6dfe7b35102d7cf65bebe55 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 12 Jan 2021 11:16:45 -0500 Subject: [PATCH 13/25] Get rid of finalizer which, I suspect, is keeping tests from shutting down on Windows. --- integration/conftest.py | 3 +++ integration/util.py | 10 ++++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/integration/conftest.py b/integration/conftest.py index 4ae22deee..27404d9e8 100644 --- a/integration/conftest.py +++ b/integration/conftest.py @@ -351,6 +351,9 @@ def alice(reactor, temp_dir, introducer_furl, flog_gatherer, storage_nodes, requ reactor, request, temp_dir, introducer_furl, flog_gatherer, "alice", web_port="tcp:9980:interface=localhost", storage=False, + # We're going to kill this ourselves, so no need for finalizer to + # do it: + finalize=False, ) ) await_client_ready(process) diff --git a/integration/util.py b/integration/util.py index 64f02a446..f5f7029d8 100644 --- a/integration/util.py +++ b/integration/util.py @@ -186,7 +186,7 @@ class TahoeProcess(object): return "".format(self._node_dir) -def _run_node(reactor, node_dir, request, magic_text): +def _run_node(reactor, node_dir, request, magic_text, finalize=True): """ Run a tahoe process from its node_dir. @@ -210,7 +210,8 @@ def _run_node(reactor, node_dir, request, magic_text): ) transport.exited = protocol.exited - request.addfinalizer(partial(_cleanup_tahoe_process, transport, protocol.exited)) + if finalize: + request.addfinalizer(partial(_cleanup_tahoe_process, transport, protocol.exited)) # XXX abusing the Deferred; should use .when_magic_seen() pattern @@ -229,7 +230,8 @@ def _create_node(reactor, request, temp_dir, introducer_furl, flog_gatherer, nam magic_text=None, needed=2, happy=3, - total=4): + total=4, + finalize=True): """ Helper to create a single node, run it and return the instance spawnProcess returned (ITransport) @@ -277,7 +279,7 @@ def _create_node(reactor, request, temp_dir, introducer_furl, flog_gatherer, nam d = Deferred() d.callback(None) d.addCallback(lambda _: created_d) - d.addCallback(lambda _: _run_node(reactor, node_dir, request, magic_text)) + d.addCallback(lambda _: _run_node(reactor, node_dir, request, magic_text, finalize=finalize)) return d From b74ec6919dc695e661e2523ef8c528535e13332e Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 12 Jan 2021 13:24:42 -0500 Subject: [PATCH 14/25] Don't blow up just because irrelevant cleanup complains. --- integration/conftest.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/integration/conftest.py b/integration/conftest.py index 27404d9e8..1679bd9f9 100644 --- a/integration/conftest.py +++ b/integration/conftest.py @@ -536,7 +536,13 @@ def tor_network(reactor, temp_dir, chutney, request): path=join(chutney_dir), env=env, ) - pytest_twisted.blockon(proto.done) + try: + pytest_twisted.blockon(proto.done) + except ProcessTerminated: + # If this doesn't exit cleanly, that's fine, that shouldn't fail + # the test suite. + pass + request.addfinalizer(cleanup) return chut From dfcd75f20de629723b091c9d5b8ca2aa1b194718 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 12 Jan 2021 13:58:28 -0500 Subject: [PATCH 15/25] Infinite blocking is bad. --- integration/conftest.py | 9 +++++---- integration/util.py | 9 ++++++++- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/integration/conftest.py b/integration/conftest.py index 1679bd9f9..533cbdb67 100644 --- a/integration/conftest.py +++ b/integration/conftest.py @@ -40,7 +40,8 @@ from util import ( TahoeProcess, cli, _run_node, - generate_ssh_key + generate_ssh_key, + block_with_timeout, ) @@ -156,7 +157,7 @@ def flog_gatherer(reactor, temp_dir, flog_binary, request): ) print("Waiting for flogtool to complete") try: - pytest_twisted.blockon(flog_protocol.done) + block_with_timeout(flog_protocol.done, reactor) except ProcessTerminated as e: print("flogtool exited unexpectedly: {}".format(str(e))) print("Flogtool completed") @@ -297,7 +298,7 @@ log_gatherer.furl = {log_furl} def cleanup(): try: transport.signalProcess('TERM') - pytest_twisted.blockon(protocol.exited) + block_with_timeout(protocol.exited, reactor) except ProcessExitedAlready: pass request.addfinalizer(cleanup) @@ -537,7 +538,7 @@ def tor_network(reactor, temp_dir, chutney, request): env=env, ) try: - pytest_twisted.blockon(proto.done) + block_with_timeout(proto.done, reactor) except ProcessTerminated: # If this doesn't exit cleanly, that's fine, that shouldn't fail # the test suite. diff --git a/integration/util.py b/integration/util.py index f5f7029d8..3d1708bae 100644 --- a/integration/util.py +++ b/integration/util.py @@ -28,6 +28,12 @@ from allmydata import client import pytest_twisted +def block_with_timeout(deferred, reactor, timeout=10): + """Block until Deferred has result, but timeout instead of waiting forever.""" + deferred.addTimeout(timeout, reactor) + return pytest_twisted.blockon(deferred) + + class _ProcessExitedProtocol(ProcessProtocol): """ Internal helper that .callback()s on self.done when the process @@ -126,11 +132,12 @@ def _cleanup_tahoe_process(tahoe_transport, exited): :return: After the process has exited. """ + from twisted.internet import reactor try: print("signaling {} with TERM".format(tahoe_transport.pid)) tahoe_transport.signalProcess('TERM') print("signaled, blocking on exit") - pytest_twisted.blockon(exited) + block_with_timeout(exited, reactor) print("exited, goodbye") except ProcessExitedAlready: pass From 20e90b4b65cbe67a5bda54fb4a1a52fcf2de5a3e Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 13 Jan 2021 10:21:00 -0500 Subject: [PATCH 16/25] Set --- tox.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tox.ini b/tox.ini index c61331885..110be7a51 100644 --- a/tox.ini +++ b/tox.ini @@ -77,7 +77,7 @@ setenv = COVERAGE_PROCESS_START=.coveragerc commands = # NOTE: 'run with "py.test --keep-tempdir -s -v integration/" to debug failures' - py.test --coverage -v {posargs:integration} + py.test --timeout=1800 --coverage -v {posargs:integration} coverage combine coverage report From 9ca17d780ebfc972cf5b6b319d5303dab0a3c4ce Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 13 Jan 2021 10:21:06 -0500 Subject: [PATCH 17/25] Add some overall timeout, and timeout on specific test that seems to be the issue somehow. --- integration/test_web.py | 3 +++ setup.py | 1 + 2 files changed, 4 insertions(+) diff --git a/integration/test_web.py b/integration/test_web.py index fe2137ff3..a16bf2e71 100644 --- a/integration/test_web.py +++ b/integration/test_web.py @@ -21,6 +21,8 @@ import requests import html5lib from bs4 import BeautifulSoup +import pytest + def test_index(alice): """ @@ -175,6 +177,7 @@ def test_deep_stats(alice): time.sleep(.5) +@pytest.mark.timeout(60) def test_status(alice): """ confirm we get something sensible from /status and the various sub-types diff --git a/setup.py b/setup.py index 952a921bc..5dc68d367 100644 --- a/setup.py +++ b/setup.py @@ -396,6 +396,7 @@ setup(name="tahoe-lafs", # also set in __init__.py "junitxml", "tenacity", "paramiko", + "pytest-timeout", ] + tor_requires + i2p_requires, "tor": tor_requires, "i2p": i2p_requires, From db22291660d3a73a8bda617c33299e6e98ca21b7 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 21 Jan 2021 13:54:22 -0500 Subject: [PATCH 18/25] Try to minimally workaround issues causing Windows to block when writing logs. --- integration/test_sftp.py | 7 ++++++- integration/test_web.py | 1 + integration/util.py | 20 ++++++++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/integration/test_sftp.py b/integration/test_sftp.py index f1cf92eab..0b1c600d1 100644 --- a/integration/test_sftp.py +++ b/integration/test_sftp.py @@ -23,7 +23,7 @@ from paramiko.rsakey import RSAKey import pytest -from .util import generate_ssh_key +from .util import generate_ssh_key, run_in_thread def connect_sftp(connect_args={"username": "alice", "password": "password"}): @@ -50,6 +50,7 @@ def connect_sftp(connect_args={"username": "alice", "password": "password"}): return sftp +@run_in_thread def test_bad_account_password_ssh_key(alice, tmpdir): """ Can't login with unknown username, wrong password, or wrong SSH pub key. @@ -79,6 +80,7 @@ def test_bad_account_password_ssh_key(alice, tmpdir): }) +@run_in_thread def test_ssh_key_auth(alice): """It's possible to login authenticating with SSH public key.""" key = RSAKey(filename=join(alice.node_dir, "private", "ssh_client_rsa_key")) @@ -88,6 +90,7 @@ def test_ssh_key_auth(alice): assert sftp.listdir() == [] +@run_in_thread def test_read_write_files(alice): """It's possible to upload and download files.""" sftp = connect_sftp() @@ -102,6 +105,7 @@ def test_read_write_files(alice): f.close() +@run_in_thread def test_directories(alice): """ It's possible to create, list directories, and create and remove files in @@ -135,6 +139,7 @@ def test_directories(alice): assert sftp.listdir() == [] +@run_in_thread def test_rename(alice): """Directories and files can be renamed.""" sftp = connect_sftp() diff --git a/integration/test_web.py b/integration/test_web.py index a16bf2e71..fb45807b3 100644 --- a/integration/test_web.py +++ b/integration/test_web.py @@ -178,6 +178,7 @@ def test_deep_stats(alice): @pytest.mark.timeout(60) +@util.run_in_thread def test_status(alice): """ confirm we get something sensible from /status and the various sub-types diff --git a/integration/util.py b/integration/util.py index 3d1708bae..2b8f587f2 100644 --- a/integration/util.py +++ b/integration/util.py @@ -13,10 +13,12 @@ from twisted.python.filepath import ( from twisted.internet.defer import Deferred, succeed from twisted.internet.protocol import ProcessProtocol from twisted.internet.error import ProcessExitedAlready, ProcessDone +from twisted.internet.threads import deferToThread import requests from paramiko.rsakey import RSAKey +from boltons.funcutils import wraps from allmydata.util.configutil import ( get_config, @@ -525,3 +527,21 @@ def generate_ssh_key(path): key.write_private_key_file(path) with open(path + ".pub", "wb") as f: f.write(b"%s %s" % (key.get_name(), key.get_base64())) + + +def run_in_thread(f): + """Decorator for integration tests that runs code in a thread. + + Because we're using pytest_twisted, tests are expected to return a Deferred + so reactor can run. If the reactor doesn't run, reads from nodes' stdout + and stderr don't happen. eventually the buffers fill up, and the nodes + block when they try to flush logs. + + We can switch to Twisted APIs (treq instead of requests etc.), but + sometimes it's easier or expedient to just have a block test. So this runs + the test in a thread in a way that still lets the reactor run. + """ + @wraps(f) + def test(*args, **kwargs): + return deferToThread(lambda: f(*args, **kwargs)) + return test From 3b893a56f94dfd57debccc51f2039e0db2e5567f Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 26 Jan 2021 09:55:13 -0500 Subject: [PATCH 19/25] Just rely on global timeout. --- integration/test_web.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/integration/test_web.py b/integration/test_web.py index e89837d30..aab11412f 100644 --- a/integration/test_web.py +++ b/integration/test_web.py @@ -21,8 +21,6 @@ import requests import html5lib from bs4 import BeautifulSoup -import pytest - def test_index(alice): """ @@ -177,7 +175,6 @@ def test_deep_stats(alice): time.sleep(.5) -@pytest.mark.timeout(60) @util.run_in_thread def test_status(alice): """ From 0424ba2a487bdc6b3ba6499f6f864e3ffad3e92d Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 26 Jan 2021 09:57:11 -0500 Subject: [PATCH 20/25] Fix indent. --- integration/util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/util.py b/integration/util.py index 2b8f587f2..e8064934f 100644 --- a/integration/util.py +++ b/integration/util.py @@ -267,7 +267,7 @@ def _create_node(reactor, request, temp_dir, introducer_furl, flog_gatherer, nam '--helper', ] if not storage: - args.append('--no-storage') + args.append('--no-storage') args.append(node_dir) _tahoe_runner_optional_coverage(done_proto, reactor, request, args) From cc8e613fe3bb1cdec3f84860cfc7c89dee7fb610 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 26 Jan 2021 10:00:50 -0500 Subject: [PATCH 21/25] Rephrase. --- newsfragments/3584.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/newsfragments/3584.bugfix b/newsfragments/3584.bugfix index 73650f40b..faf57713b 100644 --- a/newsfragments/3584.bugfix +++ b/newsfragments/3584.bugfix @@ -1 +1 @@ -SFTP public key auth likely works better, and SFTP in general was broken in the prerelease. \ No newline at end of file +SFTP public key auth likely works more consistently, and SFTP in general was previously broken. \ No newline at end of file From e7ab792c4c81bd33e989499b304e2fdd212e4715 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 26 Jan 2021 10:06:17 -0500 Subject: [PATCH 22/25] Explain why Paramiko. --- integration/test_sftp.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/integration/test_sftp.py b/integration/test_sftp.py index 0b1c600d1..ed5b37a31 100644 --- a/integration/test_sftp.py +++ b/integration/test_sftp.py @@ -1,6 +1,13 @@ """ It's possible to create/rename/delete files and directories in Tahoe-LAFS using SFTP. + +These tests use Paramiko, rather than Twisted's Conch, because: + + 1. It's a different implementation, so we're not testing Conch against + itself. + + 2. Its API is much simpler to use. """ from __future__ import unicode_literals From 4e89ab2e66f3fb78c92e3060507f1a50a5a74d69 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 26 Jan 2021 10:06:57 -0500 Subject: [PATCH 23/25] Context manager. --- integration/test_sftp.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/integration/test_sftp.py b/integration/test_sftp.py index ed5b37a31..6171c7413 100644 --- a/integration/test_sftp.py +++ b/integration/test_sftp.py @@ -101,15 +101,14 @@ def test_ssh_key_auth(alice): def test_read_write_files(alice): """It's possible to upload and download files.""" sftp = connect_sftp() - f = sftp.file("myfile", "wb") - f.write(b"abc") - f.write(b"def") - f.close() - f = sftp.file("myfile", "rb") - assert f.read(4) == b"abcd" - assert f.read(2) == b"ef" - assert f.read(1) == b"" - f.close() + with sftp.file("myfile", "wb") as f: + f.write(b"abc") + f.write(b"def") + + with sftp.file("myfile", "rb") as f: + assert f.read(4) == b"abcd" + assert f.read(2) == b"ef" + assert f.read(1) == b"" @run_in_thread From 6c04ea74977d745fc4649e871c85bc690e9f9263 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 26 Jan 2021 10:14:14 -0500 Subject: [PATCH 24/25] Explanatory comment is better now. --- integration/util.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/integration/util.py b/integration/util.py index e8064934f..444c8b05e 100644 --- a/integration/util.py +++ b/integration/util.py @@ -532,14 +532,21 @@ def generate_ssh_key(path): def run_in_thread(f): """Decorator for integration tests that runs code in a thread. - Because we're using pytest_twisted, tests are expected to return a Deferred - so reactor can run. If the reactor doesn't run, reads from nodes' stdout - and stderr don't happen. eventually the buffers fill up, and the nodes - block when they try to flush logs. + Because we're using pytest_twisted, tests that rely on the reactor are + expected to return a Deferred and use async APIs so the reactor can run. + + In the case of the integration test suite, it launches nodes in the + background using Twisted APIs. The nodes stdout and stderr is read via + Twisted code. If the reactor doesn't run, reads don't happen, and + eventually the buffers fill up, and the nodes block when they try to flush + logs. We can switch to Twisted APIs (treq instead of requests etc.), but - sometimes it's easier or expedient to just have a block test. So this runs - the test in a thread in a way that still lets the reactor run. + sometimes it's easier or expedient to just have a blocking test. So this + decorator allows you to run the test in a thread, and the reactor can keep + running in the main thread. + + See https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3597 for tracking bug. """ @wraps(f) def test(*args, **kwargs): From d25a0f1ce29d82efd02d1e845f23caa09a2dac93 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 26 Jan 2021 12:40:39 -0500 Subject: [PATCH 25/25] Increase timeout, just to be on the safe side. --- integration/util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/util.py b/integration/util.py index 444c8b05e..256fd68c1 100644 --- a/integration/util.py +++ b/integration/util.py @@ -30,7 +30,7 @@ from allmydata import client import pytest_twisted -def block_with_timeout(deferred, reactor, timeout=10): +def block_with_timeout(deferred, reactor, timeout=120): """Block until Deferred has result, but timeout instead of waiting forever.""" deferred.addTimeout(timeout, reactor) return pytest_twisted.blockon(deferred)