From 455726eb095d92921fb172c13c542ba2fee7c8fb Mon Sep 17 00:00:00 2001
From: Jean-Paul Calderone <exarkun@twistedmatrix.com>
Date: Wed, 4 Mar 2020 09:40:21 -0500
Subject: [PATCH] Reject references that don't claim the expected storage
server interface
---
src/_zkapauthorizer/_storage_client.py | 42 +++++-
src/_zkapauthorizer/tests/_exception.py | 128 ++++++++++++++++++
src/_zkapauthorizer/tests/foolscap.py | 120 ++++++++++++++++
src/_zkapauthorizer/tests/matchers.py | 13 ++
src/_zkapauthorizer/tests/test_plugin.py | 84 ++++++++++--
.../tests/test_storage_protocol.py | 46 +------
6 files changed, 378 insertions(+), 55 deletions(-)
create mode 100644 src/_zkapauthorizer/tests/_exception.py
create mode 100644 src/_zkapauthorizer/tests/foolscap.py
diff --git a/src/_zkapauthorizer/_storage_client.py b/src/_zkapauthorizer/_storage_client.py
index 182a739..a51fbfd 100644
--- a/src/_zkapauthorizer/_storage_client.py
+++ b/src/_zkapauthorizer/_storage_client.py
@@ -44,6 +44,26 @@ from .storage_common import (
get_required_new_passes_for_mutable_write,
)
+
+class IncorrectStorageServerReference(Exception):
+ """
+ A Foolscap remote object which should reference a ZKAPAuthorizer storage
+ server instead references some other kind of object. This makes the
+ connection, and thus the configured storage server, unusable.
+ """
+ def __init__(self, furl, actual_name, expected_name):
+ self.furl = furl
+ self.actual_name = actual_name
+ self.expected_name = expected_name
+
+ def __str__(self):
+ return "RemoteReference via {} provides {} instead of {}".format(
+ self.furl,
+ self.actual_name,
+ self.expected_name,
+ )
+
+
@implementer(IStorageServer)
@attr.s
class ZKAPAuthorizerStorageClient(object):
@@ -69,12 +89,32 @@ class ZKAPAuthorizerStorageClient(object):
which they will be used. The second is an integer giving the number
of passes to request.
"""
+ _expected_remote_interface_name = (
+ "RIPrivacyPassAuthorizedStorageServer.tahoe.privatestorage.io"
+ )
+
_get_rref = attr.ib()
_get_passes = attr.ib()
@property
def _rref(self):
- return self._get_rref()
+ rref = self._get_rref()
+ # rref provides foolscap.ipb.IRemoteReference but in practice it is a
+ # foolscap.referenceable.RemoteReference instance. The interface
+ # doesn't give us enough functionality to verify that the reference is
+ # to the right sort of thing but the concrete type does.
+ #
+ # Foolscap development isn't exactly racing along and if we're lucky
+ # we'll switch to HTTP before too long anyway.
+ actual_name = rref.tracker.interfaceName
+ expected_name = self._expected_remote_interface_name
+ if actual_name != expected_name:
+ raise IncorrectStorageServerReference(
+ rref.tracker.getURL(),
+ actual_name,
+ expected_name,
+ )
+ return rref
def _get_encoded_passes(self, message, count):
"""
diff --git a/src/_zkapauthorizer/tests/_exception.py b/src/_zkapauthorizer/tests/_exception.py
new file mode 100644
index 0000000..504e383
--- /dev/null
+++ b/src/_zkapauthorizer/tests/_exception.py
@@ -0,0 +1,128 @@
+# Copyright (c) 2009-2012 testtools developers.
+#
+# Copyright 2019 PrivateStorage.io, LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+__all__ = [
+ 'MatchesExceptionType',
+ 'Raises',
+ 'raises',
+ ]
+
+import sys
+
+from testtools.matchers import (
+ Matcher,
+ Mismatch,
+)
+from testtools.content import (
+ TracebackContent,
+)
+
+
+def _is_exception(exc):
+ return isinstance(exc, BaseException)
+
+
+def _is_user_exception(exc):
+ return isinstance(exc, Exception)
+
+
+class MatchesExceptionType(Matcher):
+ """
+ Match an exc_info tuple against an exception type.
+ """
+
+ def __init__(self, exception_type):
+ """
+ Create a MatchesException that will match exc_info's for exception.
+
+ :param exception: An exception type.
+ """
+ Matcher.__init__(self)
+ self.expected = exception_type
+
+ def match(self, other):
+ if type(other) != tuple:
+ return Mismatch('{!r} is not an exc_info tuple'.format(other))
+ expected_class = self.expected
+ etype, evalue, etb = other
+ if not issubclass(etype, expected_class):
+ return Mismatch(
+ "{} is an instance of {}, expected an instance of {}.".format(
+ evalue,
+ etype,
+ expected_class,
+ ),
+ dict(
+ traceback=TracebackContent(other, None),
+ ),
+ )
+
+ def __str__(self):
+ return "MatchesExceptionType({!r})".format(self.expected)
+
+
+class Raises(Matcher):
+ """Match if the matchee raises an exception when called.
+
+ Exceptions which are not subclasses of Exception propogate out of the
+ Raises.match call unless they are explicitly matched.
+ """
+
+ def __init__(self, exception_matcher):
+ """
+ Create a Raises matcher.
+
+ :param exception_matcher: Validator for the exception raised by
+ matchee. The exc_info tuple for the exception raised is passed
+ into that matcher.
+ """
+ self.exception_matcher = exception_matcher
+
+ def match(self, matchee):
+ try:
+ result = matchee()
+ return Mismatch('%r returned %r' % (matchee, result))
+ # Catch all exceptions: Raises() should be able to match a
+ # KeyboardInterrupt or SystemExit.
+ except:
+ exc_info = sys.exc_info()
+ mismatch = self.exception_matcher.match(exc_info)
+ exc_type = exc_info[1]
+ # It's safer not to keep the traceback around.
+ del exc_info
+ if mismatch:
+ # The exception did not match, or no explicit matching logic was
+ # performed. If the exception is a non-user exception then
+ # propagate it.
+ if _is_exception(exc_type) and not _is_user_exception(exc_type):
+ raise
+ return mismatch
+ return None
+
+ def __str__(self):
+ return 'Raises()'
+
+
+def raises(exception_type):
+ """Make a matcher that checks that a callable raises an exception.
+
+ This is a convenience function, exactly equivalent to::
+
+ return Raises(MatchesExceptionType(exception_type))
+
+ See `Raises` and `MatchesExceptionType` for more information.
+ """
+ return Raises(MatchesExceptionType(exception_type))
diff --git a/src/_zkapauthorizer/tests/foolscap.py b/src/_zkapauthorizer/tests/foolscap.py
new file mode 100644
index 0000000..c9bb485
--- /dev/null
+++ b/src/_zkapauthorizer/tests/foolscap.py
@@ -0,0 +1,120 @@
+# Copyright 2019 PrivateStorage.io, LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""
+Testing helpers related to Foolscap.
+"""
+
+from __future__ import (
+ absolute_import,
+)
+
+from zope.interface import (
+ implementer,
+)
+
+import attr
+
+from twisted.internet.defer import (
+ execute,
+)
+
+from foolscap.api import (
+ RemoteInterface,
+)
+
+from allmydata.interfaces import (
+ RIStorageServer,
+)
+
+class RIStub(RemoteInterface):
+ pass
+
+@implementer(RIStorageServer)
+class StubStorageServer(object):
+ pass
+
+
+def get_anonymous_storage_server():
+ return StubStorageServer()
+
+
+@attr.s
+class DummyReferenceable(object):
+ _interface = attr.ib()
+
+ def getInterface(self):
+ return self._interface
+
+ def doRemoteCall(self, *a, **kw):
+ return None
+
+@attr.s
+class LocalTracker(object):
+ """
+ Pretend to be a tracker for a ``LocalRemote``.
+ """
+ interface = attr.ib()
+ interfaceName = attr.ib(default=None)
+
+ def __attrs_post_init__(self):
+ self.interfaceName = self.interface.__remote_name__
+
+ def getURL(self):
+ return b"pb://abcd@127.0.0.1:12345/efgh"
+
+
+@attr.s
+class LocalRemote(object):
+ """
+ Adapt a referenceable to behave as if it were a remote reference instead.
+
+ This is only a partial implementation of ``IRemoteReference`` so it
+ doesn't declare the interface.
+
+ ``foolscap.referenceable.LocalReferenceable`` is in many ways a better
+ adapter between these interfaces but it also uses ``eventually`` which
+ complicates matters immensely for testing.
+
+ :ivar foolscap.ipb.IReferenceable _referenceable: The object to which this
+ provides a simulated remote interface.
+ """
+ _referenceable = attr.ib()
+ check_args = attr.ib(default=True)
+ tracker = attr.ib(default=None)
+
+ def __attrs_post_init__(self):
+ self.tracker = LocalTracker(
+ self._referenceable.getInterface(),
+ )
+
+ def callRemote(self, methname, *args, **kwargs):
+ """
+ Call the given method on the wrapped object, passing the given arguments.
+
+ Arguments are checked for conformance to the remote interface but the
+ return value is not (because I don't know how -exarkun).
+
+ :return Deferred: The result of the call on the wrapped object.
+ """
+ schema = self._referenceable.getInterface()[methname]
+ if self.check_args:
+ schema.checkAllArgs(args, kwargs, inbound=False)
+ # TODO: Figure out how to call checkResults on the result.
+ return execute(
+ self._referenceable.doRemoteCall,
+ methname,
+ args,
+ kwargs,
+ )
diff --git a/src/_zkapauthorizer/tests/matchers.py b/src/_zkapauthorizer/tests/matchers.py
index b3730b7..09afa8d 100644
--- a/src/_zkapauthorizer/tests/matchers.py
+++ b/src/_zkapauthorizer/tests/matchers.py
@@ -16,6 +16,15 @@
Testtools matchers useful for the test suite.
"""
+__all__ = [
+ "Provides",
+ "raises",
+ "returns",
+ "matches_version_dictionary",
+ "between",
+ "leases_current",
+]
+
from datetime import (
datetime,
)
@@ -36,6 +45,10 @@ from testtools.matchers import (
AllMatch,
)
+from ._exception import (
+ raises,
+)
+
@attr.s
class Provides(object):
"""
diff --git a/src/_zkapauthorizer/tests/test_plugin.py b/src/_zkapauthorizer/tests/test_plugin.py
index 55c30a5..e9c8eec 100644
--- a/src/_zkapauthorizer/tests/test_plugin.py
+++ b/src/_zkapauthorizer/tests/test_plugin.py
@@ -27,8 +27,8 @@ from os import (
makedirs,
)
import tempfile
-from zope.interface import (
- implementer,
+from functools import (
+ partial,
)
from fixtures import (
@@ -96,12 +96,19 @@ from twisted.plugins.zkapauthorizer import (
storage_server,
)
+from ..foolscap import (
+ RIPrivacyPassAuthorizedStorageServer,
+)
from ..model import (
VoucherStore,
)
from ..controller import (
IssuerConfigurationMismatch,
)
+from .._storage_client import (
+ IncorrectStorageServerReference,
+)
+
from ..lease_maintenance import (
SERVICE_NAME,
)
@@ -123,23 +130,25 @@ from .strategies import (
)
from .matchers import (
Provides,
+ raises,
)
-
-SIGNING_KEY_PATH = FilePath(__file__).sibling(u"testing-signing.key")
+from .foolscap import (
+ LocalRemote,
+ get_anonymous_storage_server,
+ DummyReferenceable,
+)
-@implementer(RIStorageServer)
-class StubStorageServer(object):
- pass
+SIGNING_KEY_PATH = FilePath(__file__).sibling(u"testing-signing.key")
-def get_anonymous_storage_server():
- return StubStorageServer()
+def get_rref(interface=None):
+ if interface is None:
+ interface = RIPrivacyPassAuthorizedStorageServer
+ return LocalRemote(DummyReferenceable(interface))
-def get_rref():
- return LocalReferenceable(None)
class PluginTests(TestCase):
@@ -319,6 +328,59 @@ class ClientPluginTests(TestCase):
self.fail("get_storage_client didn't raise, returned: {}".format(result))
+ @given(
+ tahoe_configs(),
+ announcements(),
+ storage_indexes(),
+ lease_renew_secrets(),
+ lease_cancel_secrets(),
+ sharenum_sets(),
+ sizes(),
+ )
+ def test_mismatch_storage_server_furl(
+ self,
+ get_config,
+ announcement,
+ storage_index,
+ renew_secret,
+ cancel_secret,
+ sharenums,
+ size,
+
+ ):
+ """
+ If the ``get_rref`` passed to ``get_storage_client`` returns a reference
+ to something other than an ``RIPrivacyPassAuthorizedStorageServer``
+ provider then the storage methods of the client raise exceptions that
+ clearly indicate this.
+ """
+ tempdir = self.useFixture(TempDir())
+ node_config = get_config(
+ tempdir.join(b"node"),
+ b"tub.port",
+ )
+
+ storage_client = storage_server.get_storage_client(
+ node_config,
+ announcement,
+ partial(get_rref, RIStorageServer),
+ )
+
+ def use_it():
+ return storage_client.allocate_buckets(
+ storage_index,
+ renew_secret,
+ cancel_secret,
+ sharenums,
+ size,
+ LocalReferenceable(None),
+ )
+
+ self.assertThat(
+ use_it,
+ raises(IncorrectStorageServerReference),
+ )
+
@given(
tahoe_configs_with_dummy_redeemer,
datetimes(),
diff --git a/src/_zkapauthorizer/tests/test_storage_protocol.py b/src/_zkapauthorizer/tests/test_storage_protocol.py
index faa4f8f..7d9de02 100644
--- a/src/_zkapauthorizer/tests/test_storage_protocol.py
+++ b/src/_zkapauthorizer/tests/test_storage_protocol.py
@@ -20,8 +20,6 @@ from __future__ import (
absolute_import,
)
-import attr
-
from fixtures import (
MonkeyPatch,
)
@@ -59,9 +57,6 @@ from hypothesis.strategies import (
from twisted.python.filepath import (
FilePath,
)
-from twisted.internet.defer import (
- execute,
-)
from foolscap.referenceable import (
LocalReferenceable,
@@ -98,6 +93,9 @@ from .storage_common import (
cleanup_storage_server,
write_toy_shares,
)
+from .foolscap import (
+ LocalRemote,
+)
from ..api import (
ZKAPAuthorizerStorageServer,
ZKAPAuthorizerStorageClient,
@@ -114,44 +112,6 @@ from ..foolscap import (
ShareStat,
)
-@attr.s
-class LocalRemote(object):
- """
- Adapt a referenceable to behave as if it were a remote reference instead.
-
- This is only a partial implementation of ``IRemoteReference`` so it
- doesn't declare the interface.
-
- ``foolscap.referenceable.LocalReferenceable`` is in many ways a better
- adapter between these interfaces but it also uses ``eventually`` which
- complicates matters immensely for testing.
-
- :ivar foolscap.ipb.IReferenceable _referenceable: The object to which this
- provides a simulated remote interface.
- """
- _referenceable = attr.ib()
- check_args = attr.ib(default=True)
-
- def callRemote(self, methname, *args, **kwargs):
- """
- Call the given method on the wrapped object, passing the given arguments.
-
- Arguments are checked for conformance to the remote interface but the
- return value is not (because I don't know how -exarkun).
-
- :return Deferred: The result of the call on the wrapped object.
- """
- schema = self._referenceable.getInterface()[methname]
- if self.check_args:
- schema.checkAllArgs(args, kwargs, inbound=False)
- # TODO: Figure out how to call checkResults on the result.
- return execute(
- self._referenceable.doRemoteCall,
- methname,
- args,
- kwargs,
- )
-
class RequiredPassesTests(TestCase):
"""
--
GitLab