Merge remote-tracking branch 'origin/master' into 3596.test-web-python-3-even-more

.github/CONTRIBUTING.rst

@@ -17,4 +17,4 @@ Examples of contributions include:
 * `Patch reviews <https://tahoe-lafs.org/trac/tahoe-lafs/wiki/PatchReviewProcess>`_
 
 Before authoring or reviewing a patch,
-please familiarize yourself with the `coding standard <https://tahoe-lafs.org/trac/tahoe-lafs/wiki/CodingStandards>`_.
+please familiarize yourself with the `Coding Standards <https://tahoe-lafs.org/trac/tahoe-lafs/wiki/CodingStandards>`_ and the `Contributor Code of Conduct <../docs/CODE_OF_CONDUCT.md>`_.

docs/INSTALL.rst

@@ -173,7 +173,9 @@ from PyPI with ``venv/bin/pip install tahoe-lafs``. After installation, run
 Install From a Source Tarball
 -----------------------------
 
-You can also install directly from the source tarball URL::
+You can also install directly from the source tarball URL. To verify
+signatures, first see verifying_signatures_ and replace the URL in the
+following instructions with the local filename.
 
  % virtualenv venv
  New python executable in ~/venv/bin/python2.7
@@ -189,6 +191,40 @@ You can also install directly from the source tarball URL::
  tahoe-lafs: 1.14.0
  ...
 
+.. _verifying_signatures:
+
+Verifying Signatures
+--------------------
+
+First download the source tarball and then any signatures. There are several
+developers who are able to produce signatures for a release. A release may
+have multiple signatures. All should be valid and you should confirm at least
+one of them (ideally, confirm all).
+
+This statement, signed by the existing Tahoe release-signing key, attests to
+those developers authorized to sign a Tahoe release:
+
+.. include:: developer-release-signatures
+   :code:
+
+Signatures are made available beside the release. So for example, a release
+like ``https://tahoe-lafs.org/downloads/tahoe-lafs-1.16.0.tar.bz2`` might
+have signatures ``tahoe-lafs-1.16.0.tar.bz2.meejah.asc`` and
+``tahoe-lafs-1.16.0.tar.bz2.warner.asc``.
+
+To verify the signatures using GnuPG::
+
+  % gpg --verify tahoe-lafs-1.16.0.tar.bz2.meejah.asc tahoe-lafs-1.16.0.tar.bz2
+  gpg: Signature made XXX
+  gpg:                using RSA key 9D5A2BD5688ECB889DEBCD3FC2602803128069A7
+  gpg: Good signature from "meejah <meejah@meejah.ca>" [full]
+  % gpg --verify tahoe-lafs-1.16.0.tar.bz2.warner.asc tahoe-lafs-1.16.0.tar.bz2
+  gpg: Signature made XXX
+  gpg:                using RSA key 967EFE06699872411A77DF36D43B4C9C73225AAF
+  gpg: Good signature from "Brian Warner <warner@lothar.com>" [full]
+
+
+
 Extras
 ------
 

docs/conf.py

@@ -28,7 +28,7 @@ import os
 # Add any Sphinx extension module names here, as strings. They can be
 # extensions coming with Sphinx (named 'sphinx.ext.*') or your custom
 # ones.
-extensions = []
+extensions = ['recommonmark']
 
 # Add any paths that contain templates here, relative to this directory.
 templates_path = ['_templates']
@@ -36,7 +36,7 @@ templates_path = ['_templates']
 # The suffix(es) of source filenames.
 # You can specify multiple suffix as a list of string:
 # source_suffix = ['.rst', '.md']
-source_suffix = '.rst'
+source_suffix = ['.rst', '.md']
 
 # The encoding of source files.
 #source_encoding = 'utf-8-sig'

docs/contributing.rst

@@ -0,0 +1 @@
+.. include:: ../.github/CONTRIBUTING.rst

docs/developer-release-signatures

@@ -0,0 +1,42 @@
+-----BEGIN PGP SIGNED MESSAGE-----
+Hash: SHA512
+
+
+January 20, 2021
+
+Any of the following core Tahoe contributers may sign a release. Each
+release MUST be signed by at least one developer but MAY have
+additional signatures. Each developer independently produces a
+signature which is made available beside Tahoe releases after 1.15.0
+
+This statement is signed by the existing Tahoe release key. Any future
+such statements may be signed by it OR by any two developers (for
+example, to add or remove developers from the list).
+
+meejah
+0xC2602803128069A7
+9D5A 2BD5 688E CB88 9DEB CD3F C260 2803 1280 69A7
+https://meejah.ca/meejah.asc
+
+jean-paul calderone (exarkun)
+0xE27B085EDEAA4B1B
+96B9 C5DA B2EA 9EB6 7941 9DB7 E27B 085E DEAA 4B1B
+https://twistedmatrix.com/~exarkun/E27B085EDEAA4B1B.asc
+
+brian warner (lothar)
+0x863333C265497810
+5810 F125 7F8C F753 7753  895A 8633 33C2 6549 7810
+https://www.lothar.com/warner-gpg.html
+
+
+-----BEGIN PGP SIGNATURE-----
+
+iQEzBAEBCgAdFiEE405i0G0Oac/KQXn/veDTHWhmanoFAmAHIyIACgkQveDTHWhm
+anqhqQf/YSbMXL+gwFhAZsjX39EVlbr/Ik7WPPkJW7v1oHybTnwFpFIc52COU1x/
+sqRfk4OyYtz9IBgOPXoWgXu9R4qdK6vYKxEsekcGT9C5l0OyDz8YWXEWgbGK5mvI
+aEub9WucD8r2uOQnnW6DtznFuEpvOjtf/+2BU767+bvLsbViW88ocbuLfCqLdOgD
+WZT9j3M+Y2Dc56DAJzP/4fkrUSVIofZStYp5u9HBjburgcYIp0g/cyc4xXRoi6Mp
+lFTRFv3MIjmoamzSQseoIgP6fi8QRqPrffPrsyqAp+06mJnPhxxFqxtO/ZErmpSa
++BGrLBxdWa8IF9U1A4Fs5nuAzAKMEg==
+=E9J+
+-----END PGP SIGNATURE-----

docs/frontends/webapi.rst

@@ -2032,10 +2032,11 @@ potential for surprises when the file store structure is changed.
 
 Tahoe-LAFS provides a mutable file store, but the ways that the store can
 change are limited. The only things that can change are:
- * the mapping from child names to child objects inside mutable directories
+
+* the mapping from child names to child objects inside mutable directories
   (by adding a new child, removing an existing child, or changing an
   existing child to point to a different object)
- * the contents of mutable files
+* the contents of mutable files
 
 Obviously if you query for information about the file store and then act
 to change it (such as by getting a listing of the contents of a mutable

docs/index.rst

@@ -23,8 +23,9 @@ Contents:
    frontends/download-status
 
    known_issues
-   ../.github/CONTRIBUTING
+   contributing
    CODE_OF_CONDUCT
+   release-checklist
 
    servers
    helper

docs/release-checklist.rst

@@ -40,23 +40,31 @@ Create Branch and Apply Updates
 - Create a branch for release-candidates (e.g. `XXXX.release-1.15.0.rc0`)
 - run `tox -e news` to produce a new NEWS.txt file (this does a commit)
 - create the news for the release
+
   - newsfragments/<ticket number>.minor
   - commit it
+
 - manually fix NEWS.txt
+
   - proper title for latest release ("Release 1.15.0" instead of "Release ...post1432")
   - double-check date (maybe release will be in the future)
   - spot-check the release notes (these come from the newsfragments
     files though so don't do heavy editing)
   - commit these changes
+
 - update "relnotes.txt"
+
   - update all mentions of 1.14.0 -> 1.15.0
   - update "previous release" statement and date
   - summarize major changes
   - commit it
+
 - update "CREDITS"
+
   - are there any new contributors in this release?
   - one way: git log release-1.14.0.. | grep Author | sort | uniq
   - commit it
+
 - update "docs/known_issues.rst" if appropriate
 - update "docs/INSTALL.rst" references to the new release
 - Push the branch to github
@@ -82,21 +90,32 @@ they will need to evaluate which contributors' signatures they trust.
 
 - (all steps above are completed)
 - sign the release
+
   - git tag -s -u 0xE34E62D06D0E69CFCA4179FFBDE0D31D68666A7A -m "release Tahoe-LAFS-1.15.0rc0" tahoe-lafs-1.15.0rc0
   - (replace the key-id above with your own)
+
 - build all code locally
   - these should all pass:
+
     - tox -e py27,codechecks,docs,integration
+
   - these can fail (ideally they should not of course):
+
     - tox -e deprecations,upcoming-deprecations
+
 - build tarballs
+
   - tox -e tarballs
   - confirm it at least exists:
   - ls dist/ | grep 1.15.0rc0
+
 - inspect and test the tarballs
+
   - install each in a fresh virtualenv
   - run `tahoe` command
+
 - when satisfied, sign the tarballs:
+
   - gpg --pinentry=loopback --armor --detach-sign dist/tahoe_lafs-1.15.0rc0-py2-none-any.whl
   - gpg --pinentry=loopback --armor --detach-sign dist/tahoe_lafs-1.15.0rc0.tar.bz2
   - gpg --pinentry=loopback --armor --detach-sign dist/tahoe_lafs-1.15.0rc0.tar.gz
@@ -118,6 +137,12 @@ Did anyone contribute a hack since the last release? If so, then
 https://tahoe-lafs.org/hacktahoelafs/ needs to be updated.
 
 
+Sign Git Tag
+````````````
+
+- git tag -s -u 0xE34E62D06D0E69CFCA4179FFBDE0D31D68666A7A -m "release Tahoe-LAFS-X.Y.Z" tahoe-lafs-X.Y.Z
+
+
 Upload Artifacts
 ````````````````
 
@@ -129,6 +154,7 @@ need to be uploaded to https://tahoe-lafs.org in `~source/downloads`
   https://tahoe-lafs.org/downloads/ on the Web.
 - scp dist/*1.15.0* username@tahoe-lafs.org:/home/source/downloads
 - the following developers have access to do this:
+
   - exarkun
   - meejah
   - warner
@@ -137,8 +163,9 @@ For the actual release, the tarball and signature files need to be
 uploaded to PyPI as well.
 
 - how to do this?
-- (original guide says only "twine upload dist/*")
+- (original guide says only `twine upload dist/*`)
 - the following developers have access to do this:
+
   - warner
   - exarkun (partial?)
   - meejah (partial?)

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,10 @@ from util import (
     _tahoe_runner_optional_coverage,
     await_client_ready,
     TahoeProcess,
+    cli,
+    _run_node,
+    generate_ssh_key,
+    block_with_timeout,
 )
 
 
@@ -152,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")
@@ -293,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)
@@ -347,8 +352,50 @@ 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)
+
+    # 1. Create a new RW directory cap:
+    cli(process, "create-alias", "test")
+    rwcap = loads(cli(process, "list-aliases", "--json"))["test"]["readwrite"]
+
+    # 2. Enable SFTP on the node:
+    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("""\
+[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=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 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 <thekey> <username>". 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 {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()
+    pytest_twisted.blockon(_run_node(reactor, process.node_dir, request, None))
+
     await_client_ready(process)
     return process
 
@@ -490,7 +537,13 @@ def tor_network(reactor, temp_dir, chutney, request):
             path=join(chutney_dir),
             env=env,
         )
-        pytest_twisted.blockon(proto.done)
+        try:
+            block_with_timeout(proto.done, reactor)
+        except ProcessTerminated:
+            # If this doesn't exit cleanly, that's fine, that shouldn't fail
+            # the test suite.
+            pass
+
     request.addfinalizer(cleanup)
 
     return chut

integration/test_sftp.py

@@ -0,0 +1,162 @@
+"""
+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 __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 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
+from paramiko.rsakey import RSAKey
+
+import pytest
+
+from .util import generate_ssh_key, run_in_thread
+
+
+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,
+                   allow_agent=False, **connect_args)
+    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
+
+
+@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.
+    """
+    # Wrong password, wrong username:
+    for u, p in [("alice", "wrong"), ("someuser", "password")]:
+        with pytest.raises(AuthenticationException):
+            connect_sftp(connect_args={
+                "username": u, "password": p,
+            })
+
+    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,
+        })
+
+
+@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"))
+    sftp = connect_sftp(connect_args={
+        "username": "alice2", "pkey": key
+    })
+    assert sftp.listdir() == []
+
+
+@run_in_thread
+def test_read_write_files(alice):
+    """It's possible to upload and download files."""
+    sftp = connect_sftp()
+    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
+def test_directories(alice):
+    """
+    It's possible to create, list directories, and create and remove files in
+    them.
+    """
+    sftp = connect_sftp()
+    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() == []
+
+
+@run_in_thread
+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"

integration/test_web.py

@@ -175,6 +175,7 @@ def test_deep_stats(alice):
         time.sleep(.5)
 
 
+@util.run_in_thread
 def test_status(alice):
     """
     confirm we get something sensible from /status and the various sub-types

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,
@@ -12,9 +13,13 @@ 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,
     set_config,
@@ -25,6 +30,12 @@ from allmydata import client
 import pytest_twisted
 
 
+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)
+
+
 class _ProcessExitedProtocol(ProcessProtocol):
     """
     Internal helper that .callback()s on self.done when the process
@@ -123,11 +134,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
@@ -175,11 +187,15 @@ 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 "<TahoeProcess in '{}'>".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.
 
@@ -203,6 +219,7 @@ def _run_node(reactor, node_dir, request, magic_text):
     )
     transport.exited = protocol.exited
 
+    if finalize:
         request.addfinalizer(partial(_cleanup_tahoe_process, transport, protocol.exited))
 
     # XXX abusing the Deferred; should use .when_magic_seen() pattern
@@ -222,7 +239,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)
@@ -270,7 +288,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
 
 
@@ -390,17 +408,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
+    Run a tahoe CLI subcommand for a given node in a blocking manner, returning
-    under coverage if '--coverage' was supplied.
+    the output.
     """
-    proto = _CollectOutputProtocol()
+    arguments = ["tahoe", '--node-directory', node.node_dir]
-    _tahoe_runner_optional_coverage(
+    return check_output(arguments + list(argv))
-        proto, reactor, request,
-        ['--node-directory', node_dir] + list(argv),
-    )
-    return proto.done
 
 
 def node_url(node_dir, uri_fragment):
@@ -505,3 +519,36 @@ def await_client_ready(tahoe, timeout=10, liveness=60*2):
             tahoe,
         )
     )
+
+
+def generate_ssh_key(path):
+    """Create a new SSH private/public key pair."""
+    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()))
+
+
+def run_in_thread(f):
+    """Decorator for integration tests that runs code in a thread.
+
+    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 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):
+        return deferToThread(lambda: f(*args, **kwargs))
+    return test

newsfragments/3584.bugfix

@@ -0,0 +1 @@
+SFTP public key auth likely works more consistently, and SFTP in general was previously broken.

setup.py

@@ -395,6 +395,8 @@ setup(name="tahoe-lafs", # also set in __init__.py
               "html5lib",
               "junitxml",
               "tenacity",
+              "paramiko",
+              "pytest-timeout",
           ] + tor_requires + i2p_requires,
           "tor": tor_requires,
           "i2p": i2p_requires,

src/allmydata/frontends/auth.py

@@ -4,8 +4,8 @@ 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 allmydata.util import base32
 from allmydata.util.fileutil import abspath_expanduser_unicode
@@ -29,7 +29,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 +40,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 +59,9 @@ class AccountFileChecker(object):
 
     def requestAvatarId(self, creds):
         if credentials.ISSHPrivateKey.providedBy(creds):
-            # Re-using twisted.conch.checkers.SSHPublicKeyChecker here, rather
+            d = defer.maybeDeferred(self._pubkeychecker.requestAvatarId, creds)
-            # than re-implementing all of the ISSHPrivateKey checking logic,
+            d.addCallback(self._avatarId)
-            # would be better.  That would require Twisted 14.1.0 or newer,
+            return d
-            # though.
-            return self._checkKey(creds)
         elif credentials.IUsernameHashedPassword.providedBy(creds):
             return self._checkPassword(creds)
         elif credentials.IUsernamePassword.providedBy(creds):
@@ -86,28 +86,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):

src/allmydata/frontends/sftpd.py

@@ -1,5 +1,17 @@
+"""
+Ported to Python 3.
+"""
+from __future__ import absolute_import
+from __future__ import division
+from __future__ import print_function
+from __future__ import unicode_literals
+
+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
+
 import six
-import heapq, traceback, array, stat, struct
+import heapq, traceback, stat, struct
 from stat import S_IFREG, S_IFDIR
 from time import time, strftime, localtime
 
@@ -44,6 +56,17 @@ from allmydata.util.log import NOISY, OPERATIONAL, WEIRD, \
 if six.PY3:
     long = int
 
+
+def createSFTPError(errorCode, errorMessage):
+    """
+    SFTPError that can accept both Unicode and bytes.
+
+    Twisted expects _native_ strings for the SFTPError message, but we often do
+    Unicode by default even on Python 2.
+    """
+    return SFTPError(errorCode, six.ensure_str(errorMessage))
+
+
 def eventually_callback(d):
     return lambda res: eventually(d.callback, res)
 
@@ -52,9 +75,9 @@ def eventually_errback(d):
 
 
 def _utf8(x):
-    if isinstance(x, unicode):
-        return x.encode('utf-8')
     if isinstance(x, str):
+        return x.encode('utf-8')
+    if isinstance(x, bytes):
         return x
     return repr(x)
 
@@ -63,7 +86,7 @@ def _to_sftp_time(t):
     """SFTP times are unsigned 32-bit integers representing UTC seconds
     (ignoring leap seconds) since the Unix epoch, January 1 1970 00:00 UTC.
     A Tahoe time is the corresponding float."""
-    return long(t) & long(0xFFFFFFFF)
+    return int(t) & int(0xFFFFFFFF)
 
 
 def _convert_error(res, request):
@@ -72,7 +95,7 @@ def _convert_error(res, request):
 
     if not isinstance(res, Failure):
         logged_res = res
-        if isinstance(res, str): logged_res = "<data of length %r>" % (len(res),)
+        if isinstance(res, (bytes, str)): logged_res = "<data of length %r>" % (len(res),)
         logmsg("SUCCESS %r %r" % (request, logged_res,), level=OPERATIONAL)
         return res
 
@@ -91,10 +114,10 @@ def _convert_error(res, request):
         raise err
     if err.check(NoSuchChildError):
         childname = _utf8(err.value.args[0])
-        raise SFTPError(FX_NO_SUCH_FILE, childname)
+        raise createSFTPError(FX_NO_SUCH_FILE, childname)
     if err.check(NotWriteableError) or err.check(ChildOfWrongTypeError):
         msg = _utf8(err.value.args[0])
-        raise SFTPError(FX_PERMISSION_DENIED, msg)
+        raise createSFTPError(FX_PERMISSION_DENIED, msg)
     if err.check(ExistingChildError):
         # Versions of SFTP after v3 (which is what twisted.conch implements)
         # define a specific error code for this case: FX_FILE_ALREADY_EXISTS.
@@ -103,16 +126,16 @@ def _convert_error(res, request):
         # to translate the error to the equivalent of POSIX EEXIST, which is
         # necessary for some picky programs (such as gedit).
         msg = _utf8(err.value.args[0])
-        raise SFTPError(FX_FAILURE, msg)
+        raise createSFTPError(FX_FAILURE, msg)
     if err.check(NotImplementedError):
-        raise SFTPError(FX_OP_UNSUPPORTED, _utf8(err.value))
+        raise createSFTPError(FX_OP_UNSUPPORTED, _utf8(err.value))
     if err.check(EOFError):
-        raise SFTPError(FX_EOF, "end of file reached")
+        raise createSFTPError(FX_EOF, "end of file reached")
     if err.check(defer.FirstError):
         _convert_error(err.value.subFailure, request)
 
     # We assume that the error message is not anonymity-sensitive.
-    raise SFTPError(FX_FAILURE, _utf8(err.value))
+    raise createSFTPError(FX_FAILURE, _utf8(err.value))
 
 
 def _repr_flags(flags):
@@ -145,7 +168,7 @@ def _lsLine(name, attrs):
     # Since we now depend on Twisted v10.1, consider calling Twisted's version.
 
     mode = st_mode
-    perms = array.array('c', '-'*10)
+    perms = ["-"] * 10
     ft = stat.S_IFMT(mode)
     if   stat.S_ISDIR(ft): perms[0] = 'd'
     elif stat.S_ISREG(ft): perms[0] = '-'
@@ -164,7 +187,7 @@ def _lsLine(name, attrs):
     if mode&stat.S_IXOTH: perms[9] = 'x'
     # suid/sgid never set
 
-    l = perms.tostring()
+    l = "".join(perms)
     l += str(st_nlink).rjust(5) + ' '
     un = str(st_uid)
     l += un.ljust(9)
@@ -181,6 +204,7 @@ def _lsLine(name, attrs):
         l += strftime("%b %d  %Y ", localtime(st_mtime))
     else:
         l += strftime("%b %d %H:%M ", localtime(st_mtime))
+    l = l.encode("utf-8")
     l += name
     return l
 
@@ -222,7 +246,7 @@ def _populate_attrs(childnode, metadata, size=None):
         if childnode and size is None:
             size = childnode.get_size()
         if size is not None:
-            _assert(isinstance(size, (int, long)) and not isinstance(size, bool), size=size)
+            _assert(isinstance(size, int) and not isinstance(size, bool), size=size)
             attrs['size'] = size
         perms = S_IFREG | 0o666
 
@@ -254,7 +278,7 @@ def _attrs_to_metadata(attrs):
 
     for key in attrs:
         if key == "mtime" or key == "ctime" or key == "createtime":
-            metadata[key] = long(attrs[key])
+            metadata[key] = int(attrs[key])
         elif key.startswith("ext_"):
             metadata[key] = str(attrs[key])
 
@@ -266,7 +290,7 @@ def _attrs_to_metadata(attrs):
 
 
 def _direntry_for(filenode_or_parent, childname, filenode=None):
-    precondition(isinstance(childname, (unicode, type(None))), childname=childname)
+    precondition(isinstance(childname, (str, type(None))), childname=childname)
 
     if childname is None:
         filenode_or_parent = filenode
@@ -274,7 +298,7 @@ def _direntry_for(filenode_or_parent, childname, filenode=None):
     if filenode_or_parent:
         rw_uri = filenode_or_parent.get_write_uri()
         if rw_uri and childname:
-            return rw_uri + "/" + childname.encode('utf-8')
+            return rw_uri + b"/" + childname.encode('utf-8')
         else:
             return rw_uri
 
@@ -326,7 +350,7 @@ class OverwriteableFileConsumer(PrefixingLogMixin):
         if size < self.current_size or size < self.downloaded:
             self.f.truncate(size)
         if size > self.current_size:
-            self.overwrite(self.current_size, "\x00" * (size - self.current_size))
+            self.overwrite(self.current_size, b"\x00" * (size - self.current_size))
         self.current_size = size
 
         # make the invariant self.download_size <= self.current_size be true again
@@ -334,7 +358,7 @@ class OverwriteableFileConsumer(PrefixingLogMixin):
             self.download_size = size
 
         if self.downloaded >= self.download_size:
-            self.download_done("size changed")
+            self.download_done(b"size changed")
 
     def registerProducer(self, p, streaming):
         if noisy: self.log(".registerProducer(%r, streaming=%r)" % (p, streaming), level=NOISY)
@@ -409,21 +433,21 @@ class OverwriteableFileConsumer(PrefixingLogMixin):
                 milestone = end
 
         while len(self.milestones) > 0:
-            (next, d) = self.milestones[0]
+            (next_, d) = self.milestones[0]
-            if next > milestone:
+            if next_ > milestone:
                 return
-            if noisy: self.log("MILESTONE %r %r" % (next, d), level=NOISY)
+            if noisy: self.log("MILESTONE %r %r" % (next_, d), level=NOISY)
             heapq.heappop(self.milestones)
-            eventually_callback(d)("reached")
+            eventually_callback(d)(b"reached")
 
         if milestone >= self.download_size:
-            self.download_done("reached download size")
+            self.download_done(b"reached download size")
 
     def overwrite(self, offset, data):
         if noisy: self.log(".overwrite(%r, <data of length %r>)" % (offset, len(data)), level=NOISY)
         if self.is_closed:
             self.log("overwrite called on a closed OverwriteableFileConsumer", level=WEIRD)
-            raise SFTPError(FX_BAD_MESSAGE, "cannot write to a closed file handle")
+            raise createSFTPError(FX_BAD_MESSAGE, "cannot write to a closed file handle")
 
         if offset > self.current_size:
             # Normally writing at an offset beyond the current end-of-file
@@ -434,7 +458,7 @@ class OverwriteableFileConsumer(PrefixingLogMixin):
             # the gap between the current EOF and the offset.
 
             self.f.seek(self.current_size)
-            self.f.write("\x00" * (offset - self.current_size))
+            self.f.write(b"\x00" * (offset - self.current_size))
             start = self.current_size
         else:
             self.f.seek(offset)
@@ -454,7 +478,7 @@ class OverwriteableFileConsumer(PrefixingLogMixin):
         if noisy: self.log(".read(%r, %r), current_size = %r" % (offset, length, self.current_size), level=NOISY)
         if self.is_closed:
             self.log("read called on a closed OverwriteableFileConsumer", level=WEIRD)
-            raise SFTPError(FX_BAD_MESSAGE, "cannot read from a closed file handle")
+            raise createSFTPError(FX_BAD_MESSAGE, "cannot read from a closed file handle")
 
         # Note that the overwrite method is synchronous. When a write request is processed
         # (e.g. a writeChunk request on the async queue of GeneralSFTPFile), overwrite will
@@ -508,7 +532,7 @@ class OverwriteableFileConsumer(PrefixingLogMixin):
         return d
 
     def download_done(self, res):
-        _assert(isinstance(res, (str, Failure)), res=res)
+        _assert(isinstance(res, (bytes, Failure)), res=res)
         # Only the first call to download_done counts, but we log subsequent calls
         # (multiple calls are normal).
         if self.done_status is not None:
@@ -525,8 +549,8 @@ class OverwriteableFileConsumer(PrefixingLogMixin):
         eventually_callback(self.done)(None)
 
         while len(self.milestones) > 0:
-            (next, d) = self.milestones[0]
+            (next_, d) = self.milestones[0]
-            if noisy: self.log("MILESTONE FINISH %r %r %r" % (next, d, res), level=NOISY)
+            if noisy: self.log("MILESTONE FINISH %r %r %r" % (next_, d, res), level=NOISY)
             heapq.heappop(self.milestones)
             # The callback means that the milestone has been reached if
             # it is ever going to be. Note that the file may have been
@@ -540,7 +564,7 @@ class OverwriteableFileConsumer(PrefixingLogMixin):
                 self.f.close()
             except Exception as e:
                 self.log("suppressed %r from close of temporary file %r" % (e, self.f), level=WEIRD)
-        self.download_done("closed")
+        self.download_done(b"closed")
         return self.done_status
 
     def unregisterProducer(self):
@@ -564,7 +588,7 @@ class ShortReadOnlySFTPFile(PrefixingLogMixin):
         PrefixingLogMixin.__init__(self, facility="tahoe.sftp", prefix=userpath)
         if noisy: self.log(".__init__(%r, %r, %r)" % (userpath, filenode, metadata), level=NOISY)
 
-        precondition(isinstance(userpath, str) and IFileNode.providedBy(filenode),
+        precondition(isinstance(userpath, bytes) and IFileNode.providedBy(filenode),
                      userpath=userpath, filenode=filenode)
         self.filenode = filenode
         self.metadata = metadata
@@ -576,7 +600,7 @@ class ShortReadOnlySFTPFile(PrefixingLogMixin):
         self.log(request, level=OPERATIONAL)
 
         if self.closed:
-            def _closed(): raise SFTPError(FX_BAD_MESSAGE, "cannot read from a closed file handle")
+            def _closed(): raise createSFTPError(FX_BAD_MESSAGE, "cannot read from a closed file handle")
             return defer.execute(_closed)
 
         d = defer.Deferred()
@@ -593,7 +617,7 @@ class ShortReadOnlySFTPFile(PrefixingLogMixin):
             # i.e. we respond with an EOF error iff offset is already at EOF.
 
             if offset >= len(data):
-                eventually_errback(d)(Failure(SFTPError(FX_EOF, "read at or past end of file")))
+                eventually_errback(d)(Failure(createSFTPError(FX_EOF, "read at or past end of file")))
             else:
                 eventually_callback(d)(data[offset:offset+length])  # truncated if offset+length > len(data)
             return data
@@ -604,7 +628,7 @@ class ShortReadOnlySFTPFile(PrefixingLogMixin):
     def writeChunk(self, offset, data):
         self.log(".writeChunk(%r, <data of length %r>) denied" % (offset, len(data)), level=OPERATIONAL)
 
-        def _denied(): raise SFTPError(FX_PERMISSION_DENIED, "file handle was not opened for writing")
+        def _denied(): raise createSFTPError(FX_PERMISSION_DENIED, "file handle was not opened for writing")
         return defer.execute(_denied)
 
     def close(self):
@@ -618,7 +642,7 @@ class ShortReadOnlySFTPFile(PrefixingLogMixin):
         self.log(request, level=OPERATIONAL)
 
         if self.closed:
-            def _closed(): raise SFTPError(FX_BAD_MESSAGE, "cannot get attributes for a closed file handle")
+            def _closed(): raise createSFTPError(FX_BAD_MESSAGE, "cannot get attributes for a closed file handle")
             return defer.execute(_closed)
 
         d = defer.execute(_populate_attrs, self.filenode, self.metadata)
@@ -627,7 +651,7 @@ class ShortReadOnlySFTPFile(PrefixingLogMixin):
 
     def setAttrs(self, attrs):
         self.log(".setAttrs(%r) denied" % (attrs,), level=OPERATIONAL)
-        def _denied(): raise SFTPError(FX_PERMISSION_DENIED, "file handle was not opened for writing")
+        def _denied(): raise createSFTPError(FX_PERMISSION_DENIED, "file handle was not opened for writing")
         return defer.execute(_denied)
 
 
@@ -648,7 +672,7 @@ class GeneralSFTPFile(PrefixingLogMixin):
         if noisy: self.log(".__init__(%r, %r = %r, %r, <convergence censored>)" %
                            (userpath, flags, _repr_flags(flags), close_notify), level=NOISY)
 
-        precondition(isinstance(userpath, str), userpath=userpath)
+        precondition(isinstance(userpath, bytes), userpath=userpath)
         self.userpath = userpath
         self.flags = flags
         self.close_notify = close_notify
@@ -667,11 +691,11 @@ class GeneralSFTPFile(PrefixingLogMixin):
         # not be set before then.
         self.consumer = None
 
-    def open(self, parent=None, childname=None, filenode=None, metadata=None):
+    def open(self, parent=None, childname=None, filenode=None, metadata=None):  # noqa: F811
         self.log(".open(parent=%r, childname=%r, filenode=%r, metadata=%r)" %
                  (parent, childname, filenode, metadata), level=OPERATIONAL)
 
-        precondition(isinstance(childname, (unicode, type(None))), childname=childname)
+        precondition(isinstance(childname, (str, type(None))), childname=childname)
         precondition(filenode is None or IFileNode.providedBy(filenode), filenode=filenode)
         precondition(not self.closed, sftpfile=self)
 
@@ -688,7 +712,7 @@ class GeneralSFTPFile(PrefixingLogMixin):
         if (self.flags & FXF_TRUNC) or not filenode:
             # We're either truncating or creating the file, so we don't need the old contents.
             self.consumer = OverwriteableFileConsumer(0, tempfile_maker)
-            self.consumer.download_done("download not needed")
+            self.consumer.download_done(b"download not needed")
         else:
             self.async_.addCallback(lambda ignored: filenode.get_best_readable_version())
 
@@ -702,7 +726,7 @@ class GeneralSFTPFile(PrefixingLogMixin):
                 d = version.read(self.consumer, 0, None)
                 def _finished(res):
                     if not isinstance(res, Failure):
-                        res = "download finished"
+                        res = b"download finished"
                     self.consumer.download_done(res)
                 d.addBoth(_finished)
                 # It is correct to drop d here.
@@ -722,7 +746,7 @@ class GeneralSFTPFile(PrefixingLogMixin):
     def rename(self, new_userpath, new_parent, new_childname):
         self.log(".rename(%r, %r, %r)" % (new_userpath, new_parent, new_childname), level=OPERATIONAL)
 
-        precondition(isinstance(new_userpath, str) and isinstance(new_childname, unicode),
+        precondition(isinstance(new_userpath, bytes) and isinstance(new_childname, str),
                      new_userpath=new_userpath, new_childname=new_childname)
         self.userpath = new_userpath
         self.parent = new_parent
@@ -750,11 +774,11 @@ class GeneralSFTPFile(PrefixingLogMixin):
         self.log(request, level=OPERATIONAL)
 
         if not (self.flags & FXF_READ):
-            def _denied(): raise SFTPError(FX_PERMISSION_DENIED, "file handle was not opened for reading")
+            def _denied(): raise createSFTPError(FX_PERMISSION_DENIED, "file handle was not opened for reading")
             return defer.execute(_denied)
 
         if self.closed:
-            def _closed(): raise SFTPError(FX_BAD_MESSAGE, "cannot read from a closed file handle")
+            def _closed(): raise createSFTPError(FX_BAD_MESSAGE, "cannot read from a closed file handle")
             return defer.execute(_closed)
 
         d = defer.Deferred()
@@ -772,11 +796,11 @@ class GeneralSFTPFile(PrefixingLogMixin):
         self.log(".writeChunk(%r, <data of length %r>)" % (offset, len(data)), level=OPERATIONAL)
 
         if not (self.flags & FXF_WRITE):
-            def _denied(): raise SFTPError(FX_PERMISSION_DENIED, "file handle was not opened for writing")
+            def _denied(): raise createSFTPError(FX_PERMISSION_DENIED, "file handle was not opened for writing")
             return defer.execute(_denied)
 
         if self.closed:
-            def _closed(): raise SFTPError(FX_BAD_MESSAGE, "cannot write to a closed file handle")
+            def _closed(): raise createSFTPError(FX_BAD_MESSAGE, "cannot write to a closed file handle")
             return defer.execute(_closed)
 
         self.has_changed = True
@@ -892,7 +916,7 @@ class GeneralSFTPFile(PrefixingLogMixin):
         self.log(request, level=OPERATIONAL)
 
         if self.closed:
-            def _closed(): raise SFTPError(FX_BAD_MESSAGE, "cannot get attributes for a closed file handle")
+            def _closed(): raise createSFTPError(FX_BAD_MESSAGE, "cannot get attributes for a closed file handle")
             return defer.execute(_closed)
 
         # Optimization for read-only handles, when we already know the metadata.
@@ -916,16 +940,16 @@ class GeneralSFTPFile(PrefixingLogMixin):
         self.log(request, level=OPERATIONAL)
 
         if not (self.flags & FXF_WRITE):
-            def _denied(): raise SFTPError(FX_PERMISSION_DENIED, "file handle was not opened for writing")
+            def _denied(): raise createSFTPError(FX_PERMISSION_DENIED, "file handle was not opened for writing")
             return defer.execute(_denied)
 
         if self.closed:
-            def _closed(): raise SFTPError(FX_BAD_MESSAGE, "cannot set attributes for a closed file handle")
+            def _closed(): raise createSFTPError(FX_BAD_MESSAGE, "cannot set attributes for a closed file handle")
             return defer.execute(_closed)
 
         size = attrs.get("size", None)
-        if size is not None and (not isinstance(size, (int, long)) or size < 0):
+        if size is not None and (not isinstance(size, int) or size < 0):
-            def _bad(): raise SFTPError(FX_BAD_MESSAGE, "new size is not a valid nonnegative integer")
+            def _bad(): raise createSFTPError(FX_BAD_MESSAGE, "new size is not a valid nonnegative integer")
             return defer.execute(_bad)
 
         d = defer.Deferred()
@@ -1011,7 +1035,7 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
     def logout(self):
         self.log(".logout()", level=OPERATIONAL)
 
-        for files in self._heisenfiles.itervalues():
+        for files in self._heisenfiles.values():
             for f in files:
                 f.abandon()
 
@@ -1038,7 +1062,7 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
         request = "._abandon_any_heisenfiles(%r, %r)" % (userpath, direntry)
         self.log(request, level=OPERATIONAL)
 
-        precondition(isinstance(userpath, str), userpath=userpath)
+        precondition(isinstance(userpath, bytes), userpath=userpath)
 
         # First we synchronously mark all heisenfiles matching the userpath or direntry
         # as abandoned, and remove them from the two heisenfile dicts. Then we .sync()
@@ -1087,8 +1111,8 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
                    (from_userpath, from_parent, from_childname, to_userpath, to_parent, to_childname, overwrite))
         self.log(request, level=OPERATIONAL)
 
-        precondition((isinstance(from_userpath, str) and isinstance(from_childname, unicode) and
+        precondition((isinstance(from_userpath, bytes) and isinstance(from_childname, str) and
-                      isinstance(to_userpath, str) and isinstance(to_childname, unicode)),
+                      isinstance(to_userpath, bytes) and isinstance(to_childname, str)),
                      from_userpath=from_userpath, from_childname=from_childname, to_userpath=to_userpath, to_childname=to_childname)
 
         if noisy: self.log("all_heisenfiles = %r\nself._heisenfiles = %r" % (all_heisenfiles, self._heisenfiles), level=NOISY)
@@ -1117,7 +1141,7 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
         # does not mean that they were not committed; it is used to determine whether
         # a NoSuchChildError from the rename attempt should be suppressed). If overwrite
         # is False and there were already heisenfiles at the destination userpath or
-        # direntry, we return a Deferred that fails with SFTPError(FX_PERMISSION_DENIED).
+        # direntry, we return a Deferred that fails with createSFTPError(FX_PERMISSION_DENIED).
 
         from_direntry = _direntry_for(from_parent, from_childname)
         to_direntry = _direntry_for(to_parent, to_childname)
@@ -1126,7 +1150,7 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
                            (from_direntry, to_direntry, len(all_heisenfiles), len(self._heisenfiles), request), level=NOISY)
 
         if not overwrite and (to_userpath in self._heisenfiles or to_direntry in all_heisenfiles):
-            def _existing(): raise SFTPError(FX_PERMISSION_DENIED, "cannot rename to existing path " + to_userpath)
+            def _existing(): raise createSFTPError(FX_PERMISSION_DENIED, "cannot rename to existing path " + str(to_userpath, "utf-8"))
             if noisy: self.log("existing", level=NOISY)
             return defer.execute(_existing)
 
@@ -1160,7 +1184,7 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
         request = "._update_attrs_for_heisenfiles(%r, %r, %r)" % (userpath, direntry, attrs)
         self.log(request, level=OPERATIONAL)
 
-        _assert(isinstance(userpath, str) and isinstance(direntry, str),
+        _assert(isinstance(userpath, bytes) and isinstance(direntry, bytes),
                 userpath=userpath, direntry=direntry)
 
         files = []
@@ -1193,7 +1217,7 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
         request = "._sync_heisenfiles(%r, %r, ignore=%r)" % (userpath, direntry, ignore)
         self.log(request, level=OPERATIONAL)
 
-        _assert(isinstance(userpath, str) and isinstance(direntry, (str, type(None))),
+        _assert(isinstance(userpath, bytes) and isinstance(direntry, (bytes, type(None))),
                 userpath=userpath, direntry=direntry)
 
         files = []
@@ -1218,7 +1242,7 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
     def _remove_heisenfile(self, userpath, parent, childname, file_to_remove):
         if noisy: self.log("._remove_heisenfile(%r, %r, %r, %r)" % (userpath, parent, childname, file_to_remove), level=NOISY)
 
-        _assert(isinstance(userpath, str) and isinstance(childname, (unicode, type(None))),
+        _assert(isinstance(userpath, bytes) and isinstance(childname, (str, type(None))),
                 userpath=userpath, childname=childname)
 
         direntry = _direntry_for(parent, childname)
@@ -1245,7 +1269,7 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
                            (existing_file, userpath, flags, _repr_flags(flags), parent, childname, filenode, metadata),
                            level=NOISY)
 
-        _assert((isinstance(userpath, str) and isinstance(childname, (unicode, type(None))) and
+        _assert((isinstance(userpath, bytes) and isinstance(childname, (str, type(None))) and
                  (metadata is None or 'no-write' in metadata)),
                 userpath=userpath, childname=childname, metadata=metadata)
 
@@ -1279,17 +1303,17 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
 
         if not (flags & (FXF_READ | FXF_WRITE)):
             def _bad_readwrite():
-                raise SFTPError(FX_BAD_MESSAGE, "invalid file open flags: at least one of FXF_READ and FXF_WRITE must be set")
+                raise createSFTPError(FX_BAD_MESSAGE, "invalid file open flags: at least one of FXF_READ and FXF_WRITE must be set")
             return defer.execute(_bad_readwrite)
 
         if (flags & FXF_EXCL) and not (flags & FXF_CREAT):
             def _bad_exclcreat():
-                raise SFTPError(FX_BAD_MESSAGE, "invalid file open flags: FXF_EXCL cannot be set without FXF_CREAT")
+                raise createSFTPError(FX_BAD_MESSAGE, "invalid file open flags: FXF_EXCL cannot be set without FXF_CREAT")
             return defer.execute(_bad_exclcreat)
 
         path = self._path_from_string(pathstring)
         if not path:
-            def _emptypath(): raise SFTPError(FX_NO_SUCH_FILE, "path cannot be empty")
+            def _emptypath(): raise createSFTPError(FX_NO_SUCH_FILE, "path cannot be empty")
             return defer.execute(_emptypath)
 
         # The combination of flags is potentially valid.
@@ -1348,20 +1372,20 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
         def _got_root(root_and_path):
             (root, path) = root_and_path
             if root.is_unknown():
-                raise SFTPError(FX_PERMISSION_DENIED,
+                raise createSFTPError(FX_PERMISSION_DENIED,
                                 "cannot open an unknown cap (or child of an unknown object). "
                                 "Upgrading the gateway to a later Tahoe-LAFS version may help")
             if not path:
                 # case 1
                 if noisy: self.log("case 1: root = %r, path[:-1] = %r" % (root, path[:-1]), level=NOISY)
                 if not IFileNode.providedBy(root):
-                    raise SFTPError(FX_PERMISSION_DENIED,
+                    raise createSFTPError(FX_PERMISSION_DENIED,
                                     "cannot open a directory cap")
                 if (flags & FXF_WRITE) and root.is_readonly():
-                    raise SFTPError(FX_PERMISSION_DENIED,
+                    raise createSFTPError(FX_PERMISSION_DENIED,
                                     "cannot write to a non-writeable filecap without a parent directory")
                 if flags & FXF_EXCL:
-                    raise SFTPError(FX_FAILURE,
+                    raise createSFTPError(FX_FAILURE,
                                     "cannot create a file exclusively when it already exists")
 
                 # The file does not need to be added to all_heisenfiles, because it is not
@@ -1388,7 +1412,7 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
                 def _got_parent(parent):
                     if noisy: self.log("_got_parent(%r)" % (parent,), level=NOISY)
                     if parent.is_unknown():
-                        raise SFTPError(FX_PERMISSION_DENIED,
+                        raise createSFTPError(FX_PERMISSION_DENIED,
                                         "cannot open a child of an unknown object. "
                                         "Upgrading the gateway to a later Tahoe-LAFS version may help")
 
@@ -1403,13 +1427,13 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
                         # which is consistent with what might happen on a POSIX filesystem.
 
                         if parent_readonly:
-                            raise SFTPError(FX_FAILURE,
+                            raise createSFTPError(FX_FAILURE,
                                             "cannot create a file exclusively when the parent directory is read-only")
 
                         # 'overwrite=False' ensures failure if the link already exists.
                         # FIXME: should use a single call to set_uri and return (child, metadata) (#1035)
 
-                        zero_length_lit = "URI:LIT:"
+                        zero_length_lit = b"URI:LIT:"
                         if noisy: self.log("%r.set_uri(%r, None, readcap=%r, overwrite=False)" %
                                            (parent, zero_length_lit, childname), level=NOISY)
                         d3.addCallback(lambda ign: parent.set_uri(childname, None, readcap=zero_length_lit,
@@ -1435,14 +1459,14 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
                         metadata['no-write'] = _no_write(parent_readonly, filenode, current_metadata)
 
                         if filenode.is_unknown():
-                            raise SFTPError(FX_PERMISSION_DENIED,
+                            raise createSFTPError(FX_PERMISSION_DENIED,
                                             "cannot open an unknown cap. Upgrading the gateway "
                                             "to a later Tahoe-LAFS version may help")
                         if not IFileNode.providedBy(filenode):
-                            raise SFTPError(FX_PERMISSION_DENIED,
+                            raise createSFTPError(FX_PERMISSION_DENIED,
                                             "cannot open a directory as if it were a file")
                         if (flags & FXF_WRITE) and metadata['no-write']:
-                            raise SFTPError(FX_PERMISSION_DENIED,
+                            raise createSFTPError(FX_PERMISSION_DENIED,
                                             "cannot open a non-writeable file for writing")
 
                         return self._make_file(file, userpath, flags, parent=parent, childname=childname,
@@ -1452,10 +1476,10 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
                         f.trap(NoSuchChildError)
 
                         if not (flags & FXF_CREAT):
-                            raise SFTPError(FX_NO_SUCH_FILE,
+                            raise createSFTPError(FX_NO_SUCH_FILE,
                                             "the file does not exist, and was not opened with the creation (CREAT) flag")
                         if parent_readonly:
-                            raise SFTPError(FX_PERMISSION_DENIED,
+                            raise createSFTPError(FX_PERMISSION_DENIED,
                                             "cannot create a file when the parent directory is read-only")
 
                         return self._make_file(file, userpath, flags, parent=parent, childname=childname)
@@ -1494,9 +1518,9 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
             (to_parent, to_childname) = to_pair
 
             if from_childname is None:
-                raise SFTPError(FX_NO_SUCH_FILE, "cannot rename a source object specified by URI")
+                raise createSFTPError(FX_NO_SUCH_FILE, "cannot rename a source object specified by URI")
             if to_childname is None:
-                raise SFTPError(FX_NO_SUCH_FILE, "cannot rename to a destination specified by URI")
+                raise createSFTPError(FX_NO_SUCH_FILE, "cannot rename to a destination specified by URI")
 
             # <http://tools.ietf.org/html/draft-ietf-secsh-filexfer-02#section-6.5>
             # "It is an error if there already exists a file with the name specified
@@ -1511,7 +1535,7 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
                 d2.addCallback(lambda ign: to_parent.get(to_childname))
                 def _expect_fail(res):
                     if not isinstance(res, Failure):
-                        raise SFTPError(FX_PERMISSION_DENIED, "cannot rename to existing path " + to_userpath)
+                        raise createSFTPError(FX_PERMISSION_DENIED, "cannot rename to existing path " + str(to_userpath, "utf-8"))
 
                     # It is OK if we fail for errors other than NoSuchChildError, since that probably
                     # indicates some problem accessing the destination directory.
@@ -1536,7 +1560,7 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
                     if not isinstance(err, Failure) or (renamed and err.check(NoSuchChildError)):
                         return None
                     if not overwrite and err.check(ExistingChildError):
-                        raise SFTPError(FX_PERMISSION_DENIED, "cannot rename to existing path " + to_userpath)
+                        raise createSFTPError(FX_PERMISSION_DENIED, "cannot rename to existing path " + str(to_userpath, "utf-8"))
 
                     return err
                 d3.addBoth(_check)
@@ -1554,7 +1578,7 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
         path = self._path_from_string(pathstring)
         metadata = _attrs_to_metadata(attrs)
         if 'no-write' in metadata:
-            def _denied(): raise SFTPError(FX_PERMISSION_DENIED, "cannot create a directory that is initially read-only")
+            def _denied(): raise createSFTPError(FX_PERMISSION_DENIED, "cannot create a directory that is initially read-only")
             return defer.execute(_denied)
 
         d = self._get_root(path)
@@ -1566,7 +1590,7 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
     def _get_or_create_directories(self, node, path, metadata):
         if not IDirectoryNode.providedBy(node):
             # TODO: provide the name of the blocking file in the error message.
-            def _blocked(): raise SFTPError(FX_FAILURE, "cannot create directory because there "
+            def _blocked(): raise createSFTPError(FX_FAILURE, "cannot create directory because there "
                                                         "is a file in the way") # close enough
             return defer.execute(_blocked)
 
@@ -1604,7 +1628,7 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
         def _got_parent(parent_and_childname):
             (parent, childname) = parent_and_childname
             if childname is None:
-                raise SFTPError(FX_NO_SUCH_FILE, "cannot remove an object specified by URI")
+                raise createSFTPError(FX_NO_SUCH_FILE, "cannot remove an object specified by URI")
 
             direntry = _direntry_for(parent, childname)
             d2 = defer.succeed(False)
@@ -1635,18 +1659,18 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
         d.addCallback(_got_parent_or_node)
         def _list(dirnode):
             if dirnode.is_unknown():
-                raise SFTPError(FX_PERMISSION_DENIED,
+                raise createSFTPError(FX_PERMISSION_DENIED,
                                 "cannot list an unknown cap as a directory. Upgrading the gateway "
                                 "to a later Tahoe-LAFS version may help")
             if not IDirectoryNode.providedBy(dirnode):
-                raise SFTPError(FX_PERMISSION_DENIED,
+                raise createSFTPError(FX_PERMISSION_DENIED,
                                 "cannot list a file as if it were a directory")
 
             d2 = dirnode.list()
             def _render(children):
                 parent_readonly = dirnode.is_readonly()
                 results = []
-                for filename, (child, metadata) in children.iteritems():
+                for filename, (child, metadata) in list(children.items()):
                     # The file size may be cached or absent.
                     metadata['no-write'] = _no_write(parent_readonly, child, metadata)
                     attrs = _populate_attrs(child, metadata)
@@ -1726,7 +1750,7 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
         if "size" in attrs:
             # this would require us to download and re-upload the truncated/extended
             # file contents
-            def _unsupported(): raise SFTPError(FX_OP_UNSUPPORTED, "setAttrs wth size attribute unsupported")
+            def _unsupported(): raise createSFTPError(FX_OP_UNSUPPORTED, "setAttrs wth size attribute unsupported")
             return defer.execute(_unsupported)
 
         path = self._path_from_string(pathstring)
@@ -1743,7 +1767,7 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
                 if childname is None:
                     if updated_heisenfiles:
                         return None
-                    raise SFTPError(FX_NO_SUCH_FILE, userpath)
+                    raise createSFTPError(FX_NO_SUCH_FILE, userpath)
                 else:
                     desired_metadata = _attrs_to_metadata(attrs)
                     if noisy: self.log("desired_metadata = %r" % (desired_metadata,), level=NOISY)
@@ -1766,7 +1790,7 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
     def readLink(self, pathstring):
         self.log(".readLink(%r)" % (pathstring,), level=OPERATIONAL)
 
-        def _unsupported(): raise SFTPError(FX_OP_UNSUPPORTED, "readLink")
+        def _unsupported(): raise createSFTPError(FX_OP_UNSUPPORTED, "readLink")
         return defer.execute(_unsupported)
 
     def makeLink(self, linkPathstring, targetPathstring):
@@ -1775,7 +1799,7 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
         # If this is implemented, note the reversal of arguments described in point 7 of
         # <http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/PROTOCOL?rev=1.15>.
 
-        def _unsupported(): raise SFTPError(FX_OP_UNSUPPORTED, "makeLink")
+        def _unsupported(): raise createSFTPError(FX_OP_UNSUPPORTED, "makeLink")
         return defer.execute(_unsupported)
 
     def extendedRequest(self, extensionName, extensionData):
@@ -1784,8 +1808,8 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
         # We implement the three main OpenSSH SFTP extensions; see
         # <http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/PROTOCOL?rev=1.15>
 
-        if extensionName == 'posix-rename@openssh.com':
+        if extensionName == b'posix-rename@openssh.com':
-            def _bad(): raise SFTPError(FX_BAD_MESSAGE, "could not parse posix-rename@openssh.com request")
+            def _bad(): raise createSFTPError(FX_BAD_MESSAGE, "could not parse posix-rename@openssh.com request")
 
             if 4 > len(extensionData): return defer.execute(_bad)
             (fromPathLen,) = struct.unpack('>L', extensionData[0:4])
@@ -1802,11 +1826,11 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
             # an error, or an FXP_EXTENDED_REPLY. But it happens to do the right thing
             # (respond with an FXP_STATUS message) if we return a Failure with code FX_OK.
             def _succeeded(ign):
-                raise SFTPError(FX_OK, "request succeeded")
+                raise createSFTPError(FX_OK, "request succeeded")
             d.addCallback(_succeeded)
             return d
 
-        if extensionName == 'statvfs@openssh.com' or extensionName == 'fstatvfs@openssh.com':
+        if extensionName == b'statvfs@openssh.com' or extensionName == b'fstatvfs@openssh.com':
             # f_bsize and f_frsize should be the same to avoid a bug in 'df'
             return defer.succeed(struct.pack('>11Q',
                 1024,         # uint64  f_bsize     /* file system block size */
@@ -1822,7 +1846,7 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
                 65535,        # uint64  f_namemax   /* maximum filename length */
                 ))
 
-        def _unsupported(): raise SFTPError(FX_OP_UNSUPPORTED, "unsupported %r request <data of length %r>" %
+        def _unsupported(): raise createSFTPError(FX_OP_UNSUPPORTED, "unsupported %r request <data of length %r>" %
                                                                (extensionName, len(extensionData)))
         return defer.execute(_unsupported)
 
@@ -1837,29 +1861,29 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
     def _path_from_string(self, pathstring):
         if noisy: self.log("CONVERT %r" % (pathstring,), level=NOISY)
 
-        _assert(isinstance(pathstring, str), pathstring=pathstring)
+        _assert(isinstance(pathstring, bytes), pathstring=pathstring)
 
         # The home directory is the root directory.
-        pathstring = pathstring.strip("/")
+        pathstring = pathstring.strip(b"/")
-        if pathstring == "" or pathstring == ".":
+        if pathstring == b"" or pathstring == b".":
             path_utf8 = []
         else:
-            path_utf8 = pathstring.split("/")
+            path_utf8 = pathstring.split(b"/")
 
         # <http://tools.ietf.org/html/draft-ietf-secsh-filexfer-02#section-6.2>
         # "Servers SHOULD interpret a path name component ".." as referring to
         #  the parent directory, and "." as referring to the current directory."
         path = []
         for p_utf8 in path_utf8:
-            if p_utf8 == "..":
+            if p_utf8 == b"..":
                 # ignore excess .. components at the root
                 if len(path) > 0:
                     path = path[:-1]
-            elif p_utf8 != ".":
+            elif p_utf8 != b".":
                 try:
                     p = p_utf8.decode('utf-8', 'strict')
                 except UnicodeError:
-                    raise SFTPError(FX_NO_SUCH_FILE, "path could not be decoded as UTF-8")
+                    raise createSFTPError(FX_NO_SUCH_FILE, "path could not be decoded as UTF-8")
                 path.append(p)
 
         if noisy: self.log(" PATH %r" % (path,), level=NOISY)
@@ -1978,9 +2002,9 @@ class SFTPServer(service.MultiService):
 
     def __init__(self, client, accountfile, accounturl,
                  sftp_portstr, pubkey_file, privkey_file):
-        precondition(isinstance(accountfile, (unicode, type(None))), accountfile)
+        precondition(isinstance(accountfile, (str, type(None))), accountfile)
-        precondition(isinstance(pubkey_file, unicode), pubkey_file)
+        precondition(isinstance(pubkey_file, str), pubkey_file)
-        precondition(isinstance(privkey_file, unicode), privkey_file)
+        precondition(isinstance(privkey_file, str), privkey_file)
         service.MultiService.__init__(self)
 
         r = Dispatcher(client)
@@ -2011,5 +2035,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)

src/allmydata/immutable/upload.py

@@ -13,19 +13,30 @@ if PY2:
 from past.builtins import long, unicode
 from six import ensure_str
 
+try:
+    from typing import List
+except ImportError:
+    pass
+
 import os, time, weakref, itertools
+
+import attr
+
 from zope.interface import implementer
 from twisted.python import failure
 from twisted.internet import defer
 from twisted.application import service
-from foolscap.api import Referenceable, Copyable, RemoteCopy, fireEventually
+from foolscap.api import Referenceable, Copyable, RemoteCopy
 
 from allmydata.crypto import aes
 from allmydata.util.hashutil import file_renewal_secret_hash, \
      file_cancel_secret_hash, bucket_renewal_secret_hash, \
      bucket_cancel_secret_hash, plaintext_hasher, \
      storage_index_hash, plaintext_segment_hasher, convergence_hasher
-from allmydata.util.deferredutil import timeout_call
+from allmydata.util.deferredutil import (
+    timeout_call,
+    until,
+)
 from allmydata import hashtree, uri
 from allmydata.storage.server import si_b2a
 from allmydata.immutable import encode
@@ -900,13 +911,45 @@ class Tahoe2ServerSelector(log.PrefixingLogMixin):
         raise UploadUnhappinessError(msg)
 
 
+@attr.s
+class _Accum(object):
+    """
+    Accumulate up to some known amount of ciphertext.
+
+    :ivar remaining: The number of bytes still expected.
+    :ivar ciphertext: The bytes accumulated so far.
+    """
+    remaining = attr.ib(validator=attr.validators.instance_of(int)) # type: int
+    ciphertext = attr.ib(default=attr.Factory(list))                # type: List[bytes]
+
+    def extend(self,
+               size,           # type: int
+               ciphertext,     # type: List[bytes]
+    ):
+        """
+        Accumulate some more ciphertext.
+
+        :param size: The amount of data the new ciphertext represents towards
+            the goal.  This may be more than the actual size of the given
+            ciphertext if the source has run out of data.
+
+        :param ciphertext: The new ciphertext to accumulate.
+        """
+        self.remaining -= size
+        self.ciphertext.extend(ciphertext)
+
+
 @implementer(IEncryptedUploadable)
 class EncryptAnUploadable(object):
     """This is a wrapper that takes an IUploadable and provides
     IEncryptedUploadable."""
     CHUNKSIZE = 50*1024
 
-    def __init__(self, original, log_parent=None, progress=None):
+    def __init__(self, original, log_parent=None, progress=None, chunk_size=None):
+        """
+        :param chunk_size: The number of bytes to read from the uploadable at a
+            time, or None for some default.
+        """
         precondition(original.default_params_set,
                      "set_default_encoding_parameters not called on %r before wrapping with EncryptAnUploadable" % (original,))
         self.original = IUploadable(original)
@@ -920,6 +963,8 @@ class EncryptAnUploadable(object):
         self._ciphertext_bytes_read = 0
         self._status = None
         self._progress = progress
+        if chunk_size is not None:
+            self.CHUNKSIZE = chunk_size
 
     def set_upload_status(self, upload_status):
         self._status = IUploadStatus(upload_status)
@@ -1026,47 +1071,53 @@ class EncryptAnUploadable(object):
         # and size
         d.addCallback(lambda ignored: self.get_size())
         d.addCallback(lambda ignored: self._get_encryptor())
-        # then fetch and encrypt the plaintext. The unusual structure here
+
-        # (passing a Deferred *into* a function) is needed to avoid
+        accum = _Accum(length)
-        # overflowing the stack: Deferreds don't optimize out tail recursion.
+
-        # We also pass in a list, to which _read_encrypted will append
+        def action():
-        # ciphertext.
+            """
-        ciphertext = []
+            Read some bytes into the accumulator.
-        d2 = defer.Deferred()
+            """
-        d.addCallback(lambda ignored:
+            return self._read_encrypted(accum, hash_only)
-                      self._read_encrypted(length, ciphertext, hash_only, d2))
+
-        d.addCallback(lambda ignored: d2)
+        def condition():
+            """
+            Check to see if the accumulator has all the data.
+            """
+            return accum.remaining == 0
+
+        d.addCallback(lambda ignored: until(action, condition))
+        d.addCallback(lambda ignored: accum.ciphertext)
         return d
 
-    def _read_encrypted(self, remaining, ciphertext, hash_only, fire_when_done):
+    def _read_encrypted(self,
-        if not remaining:
+                        ciphertext_accum,  # type: _Accum
-            fire_when_done.callback(ciphertext)
+                        hash_only,         # type: bool
-            return None
+    ):
+        # type: (...) -> defer.Deferred
+        """
+        Read the next chunk of plaintext, encrypt it, and extend the accumulator
+        with the resulting ciphertext.
+        """
         # tolerate large length= values without consuming a lot of RAM by
         # reading just a chunk (say 50kB) at a time. This only really matters
         # when hash_only==True (i.e. resuming an interrupted upload), since
         # that's the case where we will be skipping over a lot of data.
-        size = min(remaining, self.CHUNKSIZE)
+        size = min(ciphertext_accum.remaining, self.CHUNKSIZE)
-        remaining = remaining - size
+
         # read a chunk of plaintext..
         d = defer.maybeDeferred(self.original.read, size)
-        # N.B.: if read() is synchronous, then since everything else is
-        # actually synchronous too, we'd blow the stack unless we stall for a
-        # tick. Once you accept a Deferred from IUploadable.read(), you must
-        # be prepared to have it fire immediately too.
-        d.addCallback(fireEventually)
         def _good(plaintext):
             # and encrypt it..
             # o/' over the fields we go, hashing all the way, sHA! sHA! sHA! o/'
             ct = self._hash_and_encrypt_plaintext(plaintext, hash_only)
-            ciphertext.extend(ct)
+            # Intentionally tell the accumulator about the expected size, not
-            self._read_encrypted(remaining, ciphertext, hash_only,
+            # the actual size.  If we run out of data we still want remaining
-                                 fire_when_done)
+            # to drop otherwise it will never reach 0 and the loop will never
-        def _err(why):
+            # end.
-            fire_when_done.errback(why)
+            ciphertext_accum.extend(size, ct)
         d.addCallback(_good)
-        d.addErrback(_err)
+        return d
-        return None
 
     def _hash_and_encrypt_plaintext(self, data, hash_only):
         assert isinstance(data, (tuple, list)), type(data)

src/allmydata/test/test_deferredutil.py

@@ -74,3 +74,58 @@ class DeferredUtilTests(unittest.TestCase, deferredutil.WaitForDelayedCallsMixin
         d = defer.succeed(None)
         d.addBoth(self.wait_for_delayed_calls)
         return d
+
+
+class UntilTests(unittest.TestCase):
+    """
+    Tests for ``deferredutil.until``.
+    """
+    def test_exception(self):
+        """
+        If the action raises an exception, the ``Deferred`` returned by ``until``
+        fires with a ``Failure``.
+        """
+        self.assertFailure(
+            deferredutil.until(lambda: 1/0, lambda: True),
+            ZeroDivisionError,
+        )
+
+    def test_stops_on_condition(self):
+        """
+        The action is called repeatedly until ``condition`` returns ``True``.
+        """
+        calls = []
+        def action():
+            calls.append(None)
+
+        def condition():
+            return len(calls) == 3
+
+        self.assertIs(
+            self.successResultOf(
+                deferredutil.until(action, condition),
+            ),
+            None,
+        )
+        self.assertEqual(3, len(calls))
+
+    def test_waits_for_deferred(self):
+        """
+        If the action returns a ``Deferred`` then it is called again when the
+        ``Deferred`` fires.
+        """
+        counter = [0]
+        r1 = defer.Deferred()
+        r2 = defer.Deferred()
+        results = [r1, r2]
+        def action():
+            counter[0] += 1
+            return results.pop(0)
+
+        def condition():
+            return False
+
+        deferredutil.until(action, condition)
+        self.assertEqual([1], counter)
+        r1.callback(None)
+        self.assertEqual([2], counter)

src/allmydata/test/test_upload.py

@@ -14,6 +14,17 @@ if PY2:
 
 import os, shutil
 from io import BytesIO
+from base64 import (
+    b64encode,
+)
+
+from hypothesis import (
+    given,
+)
+from hypothesis.strategies import (
+    just,
+    integers,
+)
 
 from twisted.trial import unittest
 from twisted.python.failure import Failure
@@ -2029,6 +2040,91 @@ class EncodingParameters(GridTestMixin, unittest.TestCase, SetDEPMixin,
         f.close()
         return None
 
+
+class EncryptAnUploadableTests(unittest.TestCase):
+    """
+    Tests for ``EncryptAnUploadable``.
+    """
+    def test_same_length(self):
+        """
+        ``EncryptAnUploadable.read_encrypted`` returns ciphertext of the same
+        length as the underlying plaintext.
+        """
+        plaintext = b"hello world"
+        uploadable = upload.FileHandle(BytesIO(plaintext), None)
+        uploadable.set_default_encoding_parameters({
+            # These values shouldn't matter.
+            "k": 3,
+            "happy": 5,
+            "n": 10,
+            "max_segment_size": 128 * 1024,
+        })
+        encrypter = upload.EncryptAnUploadable(uploadable)
+        ciphertext = b"".join(self.successResultOf(encrypter.read_encrypted(1024, False)))
+        self.assertEqual(len(ciphertext), len(plaintext))
+
+    @given(just(b"hello world"), integers(min_value=0, max_value=len(b"hello world")))
+    def test_known_result(self, plaintext, split_at):
+        """
+        ``EncryptAnUploadable.read_encrypted`` returns a known-correct ciphertext
+        string for certain inputs.  The ciphertext is independent of the read
+        sizes.
+        """
+        convergence = b"\x42" * 16
+        uploadable = upload.FileHandle(BytesIO(plaintext), convergence)
+        uploadable.set_default_encoding_parameters({
+            # The convergence key is a function of k, n, and max_segment_size
+            # (among other things).  The value for happy doesn't matter
+            # though.
+            "k": 3,
+            "happy": 5,
+            "n": 10,
+            "max_segment_size": 128 * 1024,
+        })
+        encrypter = upload.EncryptAnUploadable(uploadable)
+        def read(n):
+            return b"".join(self.successResultOf(encrypter.read_encrypted(n, False)))
+
+        # Read the string in one or two pieces to make sure underlying state
+        # is maintained properly.
+        first = read(split_at)
+        second = read(len(plaintext) - split_at)
+        third = read(1)
+        ciphertext = first + second + third
+
+        self.assertEqual(
+            b"Jd2LHCRXozwrEJc=",
+            b64encode(ciphertext),
+        )
+
+    def test_large_read(self):
+        """
+        ``EncryptAnUploadable.read_encrypted`` succeeds even when the requested
+        data length is much larger than the chunk size.
+        """
+        convergence = b"\x42" * 16
+        # 4kB of plaintext
+        plaintext = b"\xde\xad\xbe\xef" * 1024
+        uploadable = upload.FileHandle(BytesIO(plaintext), convergence)
+        uploadable.set_default_encoding_parameters({
+            "k": 3,
+            "happy": 5,
+            "n": 10,
+            "max_segment_size": 128 * 1024,
+        })
+        # Make the chunk size very small so we don't have to operate on a huge
+        # amount of data to exercise the relevant codepath.
+        encrypter = upload.EncryptAnUploadable(uploadable, chunk_size=1)
+        d = encrypter.read_encrypted(len(plaintext), False)
+        ciphertext = self.successResultOf(d)
+        self.assertEqual(
+            list(map(len, ciphertext)),
+            # Chunk size was specified as 1 above so we will get the whole
+            # plaintext in one byte chunks.
+            [1] * len(plaintext),
+        )
+
+
 # TODO:
 #  upload with exactly 75 servers (shares_of_happiness)
 #  have a download fail

src/allmydata/util/_python3.py

@@ -37,6 +37,7 @@ PORTED_MODULES = [
     "allmydata.crypto.util",
     "allmydata.deep_stats",
     "allmydata.dirnode",
+    "allmydata.frontends.sftpd",
     "allmydata.hashtree",
     "allmydata.immutable.checker",
     "allmydata.immutable.downloader",
@@ -170,6 +171,7 @@ PORTED_TEST_MODULES = [
     "allmydata.test.test_pipeline",
     "allmydata.test.test_python3",
     "allmydata.test.test_repairer",
+    "allmydata.test.test_sftp",
     "allmydata.test.test_spans",
     "allmydata.test.test_statistics",
     "allmydata.test.test_stats",

src/allmydata/util/deferredutil.py

@@ -15,7 +15,18 @@ if PY2:
 
 import time
 
+try:
+    from typing import (
+        Callable,
+        Any,
+    )
+except ImportError:
+    pass
+
 from foolscap.api import eventually
+from eliot.twisted import (
+    inline_callbacks,
+)
 from twisted.internet import defer, reactor, error
 from twisted.python.failure import Failure
 
@@ -201,3 +212,22 @@ class WaitForDelayedCallsMixin(PollMixin):
         d.addErrback(log.err, "error while waiting for delayed calls")
         d.addBoth(lambda ign: res)
         return d
+
+@inline_callbacks
+def until(
+        action,     # type: Callable[[], defer.Deferred[Any]]
+        condition,  # type: Callable[[], bool]
+):
+    # type: (...) -> defer.Deferred[None]
+    """
+    Run a Deferred-returning function until a condition is true.
+
+    :param action: The action to run.
+    :param condition: The predicate signaling stop.
+
+    :return: A Deferred that fires after the condition signals stop.
+    """
+    while True:
+        yield action()
+        if condition():
+            break

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
 
@@ -221,6 +221,7 @@ commands =
 deps =
      sphinx
      docutils==0.12
+     recommonmark
 # normal install is not needed for docs, and slows things down
 skip_install = True
 commands =