Merge pull request #1166 from tahoe-lafs/3848.http-api-start-immutables

HTTP API: secrets infrastructure

Fixes ticket:3848

docs/proposed/http-storage-node-protocol.rst

@@ -369,6 +369,19 @@ The authentication *type* used is ``Tahoe-LAFS``.
 The swissnum from the NURL used to locate the storage service is used as the *credentials*.
 If credentials are not presented or the swissnum is not associated with a storage service then no storage processing is performed and the request receives an ``401 UNAUTHORIZED`` response.
 
+There are also, for some endpoints, secrets sent via ``X-Tahoe-Authorization`` headers.
+If these are:
+
+1. Missing.
+2. The wrong length.
+3. Not the expected kind of secret.
+4. They are otherwise unparseable before they are actually semantically used.
+
+the server will respond with ``400 BAD REQUEST``.
+401 is not used because this isn't an authorization problem, this is a "you sent garbage and should know better" bug.
+
+If authorization using the secret fails, then a ``401 UNAUTHORIZED`` response should be sent.
+
 General
 ~~~~~~~
 

src/allmydata/storage/http_client.py

@@ -19,7 +19,7 @@ else:
     from typing import Union
     from treq.testing import StubTreq
 
-import base64
+from base64 import b64encode
 
 # TODO Make sure to import Python version?
 from cbor2 import loads
@@ -44,7 +44,7 @@ def _decode_cbor(response):
 
 def swissnum_auth_header(swissnum):  # type: (bytes) -> bytes
     """Return value for ``Authentication`` header."""
-    return b"Tahoe-LAFS " + base64.b64encode(swissnum).strip()
+    return b"Tahoe-LAFS " + b64encode(swissnum).strip()
 
 
 class StorageClient(object):
@@ -68,12 +68,25 @@ class StorageClient(object):
         )
         return headers
 
+    def _request(self, method, url, secrets, **kwargs):
+        """
+        Like ``treq.request()``, but additional argument of secrets mapping
+        ``http_server.Secret`` to the bytes value of the secret.
+        """
+        headers = self._get_headers()
+        for key, value in secrets.items():
+            headers.addRawHeader(
+                "X-Tahoe-Authorization",
+                b"%s %s" % (key.value.encode("ascii"), b64encode(value).strip())
+            )
+        return self._treq.request(method, url, headers=headers, **kwargs)
+
     @inlineCallbacks
     def get_version(self):
         """
         Return the version metadata for the server.
         """
         url = self._base_url.click("/v1/version")
-        response = yield self._treq.get(url, headers=self._get_headers())
+        response = yield self._request("GET", url, {})
         decoded_response = yield _decode_cbor(response)
         returnValue(decoded_response)

src/allmydata/storage/http_server.py

@@ -13,8 +13,12 @@ if PY2:
     # fmt: off
     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
     # fmt: on
+else:
+    from typing import Dict, List, Set
 
 from functools import wraps
+from enum import Enum
+from base64 import b64decode
 
 from klein import Klein
 from twisted.web import http
@@ -24,40 +28,95 @@ from cbor2 import dumps
 
 from .server import StorageServer
 from .http_client import swissnum_auth_header
+from ..util.hashutil import timing_safe_compare
 
 
-def _authorization_decorator(f):
+class Secrets(Enum):
+    """Different kinds of secrets the client may send."""
+
+    LEASE_RENEW = "lease-renew-secret"
+    LEASE_CANCEL = "lease-cancel-secret"
+    UPLOAD = "upload-secret"
+
+
+class ClientSecretsException(Exception):
+    """The client did not send the appropriate secrets."""
+
+
+def _extract_secrets(
+    header_values, required_secrets
+):  # type: (List[str], Set[Secrets]) -> Dict[Secrets, bytes]
     """
-    Check the ``Authorization`` header, and (TODO: in later revision of code)
-    extract ``X-Tahoe-Authorization`` headers and pass them in.
+    Given list of values of ``X-Tahoe-Authorization`` headers, and required
+    secrets, return dictionary mapping secrets to decoded values.
+
+    If too few secrets were given, or too many, a ``ClientSecretsException`` is
+    raised.
+    """
+    string_key_to_enum = {e.value: e for e in Secrets}
+    result = {}
+    try:
+        for header_value in header_values:
+            string_key, string_value = header_value.strip().split(" ", 1)
+            key = string_key_to_enum[string_key]
+            value = b64decode(string_value)
+            if key in (Secrets.LEASE_CANCEL, Secrets.LEASE_RENEW) and len(value) != 32:
+                raise ClientSecretsException("Lease secrets must be 32 bytes long")
+            result[key] = value
+    except (ValueError, KeyError):
+        raise ClientSecretsException("Bad header value(s): {}".format(header_values))
+    if result.keys() != required_secrets:
+        raise ClientSecretsException(
+            "Expected {} secrets, got {}".format(required_secrets, result.keys())
+        )
+    return result
+
+
+def _authorization_decorator(required_secrets):
+    """
+    Check the ``Authorization`` header, and extract ``X-Tahoe-Authorization``
+    headers and pass them in.
     """
 
-    @wraps(f)
-    def route(self, request, *args, **kwargs):
-        if request.requestHeaders.getRawHeaders("Authorization", [None])[0] != str(
-            swissnum_auth_header(self._swissnum), "ascii"
-        ):
-            request.setResponseCode(http.UNAUTHORIZED)
-            return b""
-        # authorization = request.requestHeaders.getRawHeaders("X-Tahoe-Authorization", [])
-        # For now, just a placeholder:
-        authorization = None
-        return f(self, request, authorization, *args, **kwargs)
+    def decorator(f):
+        @wraps(f)
+        def route(self, request, *args, **kwargs):
+            if not timing_safe_compare(
+                request.requestHeaders.getRawHeaders("Authorization", [None])[0].encode(
+                    "utf-8"
+                ),
+                swissnum_auth_header(self._swissnum),
+            ):
+                request.setResponseCode(http.UNAUTHORIZED)
+                return b""
+            authorization = request.requestHeaders.getRawHeaders(
+                "X-Tahoe-Authorization", []
+            )
+            try:
+                secrets = _extract_secrets(authorization, required_secrets)
+            except ClientSecretsException:
+                request.setResponseCode(400)
+                return b""
+            return f(self, request, secrets, *args, **kwargs)
 
-    return route
+        return route
+
+    return decorator
 
 
-def _authorized_route(app, *route_args, **route_kwargs):
+def _authorized_route(app, required_secrets, *route_args, **route_kwargs):
     """
     Like Klein's @route, but with additional support for checking the
     ``Authorization`` header as well as ``X-Tahoe-Authorization`` headers.  The
-    latter will (TODO: in later revision of code) get passed in as second
-    argument to wrapped functions.
+    latter will get passed in as second argument to wrapped functions, a
+    dictionary mapping a ``Secret`` value to the uploaded secret.
+
+    :param required_secrets: Set of required ``Secret`` types.
     """
 
     def decorator(f):
         @app.route(*route_args, **route_kwargs)
-        @_authorization_decorator
+        @_authorization_decorator(required_secrets)
         def handle_route(*args, **kwargs):
             return f(*args, **kwargs)
 
@@ -89,6 +148,9 @@ class HTTPServer(object):
         # TODO if data is big, maybe want to use a temporary file eventually...
         return dumps(data)
 
-    @_authorized_route(_app, "/v1/version", methods=["GET"])
+    ##### Generic APIs #####
+
+    @_authorized_route(_app, set(), "/v1/version", methods=["GET"])
     def version(self, request, authorization):
+        """Return version information."""
         return self._cbor(request, self._storage_server.get_version())

src/allmydata/test/test_storage_http.py

@@ -14,36 +14,217 @@ 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
     # fmt: on
 
-from unittest import SkipTest
+from base64 import b64encode
 
-from twisted.trial.unittest import TestCase
 from twisted.internet.defer import inlineCallbacks
 
+from hypothesis import assume, given, strategies as st
+from fixtures import Fixture, TempDir
 from treq.testing import StubTreq
+from klein import Klein
 from hyperlink import DecodedURL
 
+from .common import AsyncTestCase, SyncTestCase
 from ..storage.server import StorageServer
-from ..storage.http_server import HTTPServer
+from ..storage.http_server import (
+    HTTPServer,
+    _extract_secrets,
+    Secrets,
+    ClientSecretsException,
+    _authorized_route,
+)
 from ..storage.http_client import StorageClient, ClientException
 
 
-class HTTPTests(TestCase):
+def _post_process(params):
+    secret_types, secrets = params
+    secrets = {t: s for (t, s) in zip(secret_types, secrets)}
+    headers = [
+        "{} {}".format(
+            secret_type.value, str(b64encode(secrets[secret_type]), "ascii").strip()
+        )
+        for secret_type in secret_types
+    ]
+    return secrets, headers
+
+
+# Creates a tuple of ({Secret enum value: secret_bytes}, [http headers with secrets]).
+SECRETS_STRATEGY = (
+    st.sets(st.sampled_from(Secrets))
+    .flatmap(
+        lambda secret_types: st.tuples(
+            st.just(secret_types),
+            st.lists(
+                st.binary(min_size=32, max_size=32),
+                min_size=len(secret_types),
+                max_size=len(secret_types),
+            ),
+        )
+    )
+    .map(_post_process)
+)
+
+
+class ExtractSecretsTests(SyncTestCase):
     """
-    Tests of HTTP client talking to the HTTP server.
+    Tests for ``_extract_secrets``.
     """
 
     def setUp(self):
         if PY2:
-            raise SkipTest("Not going to bother supporting Python 2")
-        self.storage_server = StorageServer(self.mktemp(), b"\x00" * 20)
-        # TODO what should the swissnum _actually_ be?
-        self._http_server = HTTPServer(self.storage_server, b"abcd")
+            self.skipTest("Not going to bother supporting Python 2")
+        super(ExtractSecretsTests, self).setUp()
+
+    @given(secrets_to_send=SECRETS_STRATEGY)
+    def test_extract_secrets(self, secrets_to_send):
+        """
+        ``_extract_secrets()`` returns a dictionary with the extracted secrets
+        if the input secrets match the required secrets.
+        """
+        secrets, headers = secrets_to_send
+
+        # No secrets needed, none given:
+        self.assertEqual(_extract_secrets(headers, secrets.keys()), secrets)
+
+    @given(
+        secrets_to_send=SECRETS_STRATEGY,
+        secrets_to_require=st.sets(st.sampled_from(Secrets)),
+    )
+    def test_wrong_number_of_secrets(self, secrets_to_send, secrets_to_require):
+        """
+        If the wrong number of secrets are passed to ``_extract_secrets``, a
+        ``ClientSecretsException`` is raised.
+        """
+        secrets_to_send, headers = secrets_to_send
+        assume(secrets_to_send.keys() != secrets_to_require)
+
+        with self.assertRaises(ClientSecretsException):
+            _extract_secrets(headers, secrets_to_require)
+
+    def test_bad_secret_missing_value(self):
+        """
+        Missing value in ``_extract_secrets`` result in
+        ``ClientSecretsException``.
+        """
+        with self.assertRaises(ClientSecretsException):
+            _extract_secrets(["lease-renew-secret"], {Secrets.LEASE_RENEW})
+
+    def test_bad_secret_unknown_prefix(self):
+        """
+        Missing value in ``_extract_secrets`` result in
+        ``ClientSecretsException``.
+        """
+        with self.assertRaises(ClientSecretsException):
+            _extract_secrets(["FOO eA=="], {})
+
+    def test_bad_secret_not_base64(self):
+        """
+        A non-base64 value in ``_extract_secrets`` result in
+        ``ClientSecretsException``.
+        """
+        with self.assertRaises(ClientSecretsException):
+            _extract_secrets(["lease-renew-secret x"], {Secrets.LEASE_RENEW})
+
+    def test_bad_secret_wrong_length_lease_renew(self):
+        """
+        Lease renewal secrets must be 32-bytes long.
+        """
+        with self.assertRaises(ClientSecretsException):
+            _extract_secrets(["lease-renew-secret eA=="], {Secrets.LEASE_RENEW})
+
+    def test_bad_secret_wrong_length_lease_cancel(self):
+        """
+        Lease cancel secrets must be 32-bytes long.
+        """
+        with self.assertRaises(ClientSecretsException):
+            _extract_secrets(["lease-cancel-secret eA=="], {Secrets.LEASE_RENEW})
+
+
+SWISSNUM_FOR_TEST = b"abcd"
+
+
+class TestApp(object):
+    """HTTP API for testing purposes."""
+
+    _app = Klein()
+    _swissnum = SWISSNUM_FOR_TEST  # Match what the test client is using
+
+    @_authorized_route(_app, {Secrets.UPLOAD}, "/upload_secret", methods=["GET"])
+    def validate_upload_secret(self, request, authorization):
+        if authorization == {Secrets.UPLOAD: b"MAGIC"}:
+            return "GOOD SECRET"
+        else:
+            return "BAD: {}".format(authorization)
+
+
+class RoutingTests(AsyncTestCase):
+    """
+    Tests for the HTTP routing infrastructure.
+    """
+
+    def setUp(self):
+        if PY2:
+            self.skipTest("Not going to bother supporting Python 2")
+        super(RoutingTests, self).setUp()
+        # Could be a fixture, but will only be used in this test class so not
+        # going to bother:
+        self._http_server = TestApp()
         self.client = StorageClient(
             DecodedURL.from_text("http://127.0.0.1"),
-            b"abcd",
-            treq=StubTreq(self._http_server.get_resource()),
+            SWISSNUM_FOR_TEST,
+            treq=StubTreq(self._http_server._app.resource()),
         )
 
+    @inlineCallbacks
+    def test_authorization_enforcement(self):
+        """
+        The requirement for secrets is enforced; if they are not given, a 400
+        response code is returned.
+        """
+        # Without secret, get a 400 error.
+        response = yield self.client._request(
+            "GET", "http://127.0.0.1/upload_secret", {}
+        )
+        self.assertEqual(response.code, 400)
+
+        # With secret, we're good.
+        response = yield self.client._request(
+            "GET", "http://127.0.0.1/upload_secret", {Secrets.UPLOAD: b"MAGIC"}
+        )
+        self.assertEqual(response.code, 200)
+        self.assertEqual((yield response.content()), b"GOOD SECRET")
+
+
+class HttpTestFixture(Fixture):
+    """
+    Setup HTTP tests' infrastructure, the storage server and corresponding
+    client.
+    """
+
+    def _setUp(self):
+        self.tempdir = self.useFixture(TempDir())
+        self.storage_server = StorageServer(self.tempdir.path, b"\x00" * 20)
+        # TODO what should the swissnum _actually_ be?
+        self.http_server = HTTPServer(self.storage_server, SWISSNUM_FOR_TEST)
+        self.client = StorageClient(
+            DecodedURL.from_text("http://127.0.0.1"),
+            SWISSNUM_FOR_TEST,
+            treq=StubTreq(self.http_server.get_resource()),
+        )
+
+
+class GenericHTTPAPITests(AsyncTestCase):
+    """
+    Tests of HTTP client talking to the HTTP server, for generic HTTP API
+    endpoints and concerns.
+    """
+
+    def setUp(self):
+        if PY2:
+            self.skipTest("Not going to bother supporting Python 2")
+        super(GenericHTTPAPITests, self).setUp()
+        self.http = self.useFixture(HttpTestFixture())
+
     @inlineCallbacks
     def test_bad_authentication(self):
         """
@@ -53,7 +234,7 @@ class HTTPTests(TestCase):
         client = StorageClient(
             DecodedURL.from_text("http://127.0.0.1"),
             b"something wrong",
-            treq=StubTreq(self._http_server.get_resource()),
+            treq=StubTreq(self.http.http_server.get_resource()),
         )
         with self.assertRaises(ClientException) as e:
             yield client.get_version()
@@ -67,14 +248,14 @@ class HTTPTests(TestCase):
         We ignore available disk space and max immutable share size, since that
         might change across calls.
         """
-        version = yield self.client.get_version()
+        version = yield self.http.client.get_version()
         version[b"http://allmydata.org/tahoe/protocols/storage/v1"].pop(
             b"available-space"
         )
         version[b"http://allmydata.org/tahoe/protocols/storage/v1"].pop(
             b"maximum-immutable-share-size"
         )
-        expected_version = self.storage_server.get_version()
+        expected_version = self.http.storage_server.get_version()
         expected_version[b"http://allmydata.org/tahoe/protocols/storage/v1"].pop(
             b"available-space"
         )