Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

onConnecting implementation #1170

Merged
merged 18 commits into from Apr 19, 2019
28 changes: 28 additions & 0 deletions autobahn/asyncio/util.py
Expand Up @@ -29,9 +29,37 @@
__all = (
'sleep',
'peer2str',
'transport_channel_id',
)


def transport_channel_id(transport, is_server, channel_id_type):
"""
Application-layer user authentication protocols are vulnerable to generic
credential forwarding attacks, where an authentication credential sent by
a client C to a server M may then be used by M to impersonate C at another
server S. To prevent such credential forwarding attacks, modern authentication
protocols rely on channel bindings. For example, WAMP-cryptosign can use
the tls-unique channel identifier provided by the TLS layer to strongly bind
authentication credentials to the underlying channel, so that a credential
received on one TLS channel cannot be forwarded on another.

"""
if channel_id_type is None:
return None

if channel_id_type not in [u'tls-unique']:
raise Exception("invalid channel ID type {}".format(channel_id_type))

ssl_obj = transport.get_extra_info('ssl_object')
if ssl_obj is None:
return None

if hasattr(ssl_obj, 'get_channel_binding'):
return ssl_obj.get_channel_binding(cb_type='tls-unique')
return None


def peer2str(peer):
if isinstance(peer, tuple):
ip_ver = 4 if len(peer) == 2 else 6
Expand Down
14 changes: 14 additions & 0 deletions autobahn/asyncio/websocket.py
Expand Up @@ -32,8 +32,10 @@
txaio.use_asyncio()

from autobahn.util import public
from autobahn.asyncio.util import transport_channel_id, peer2str
from autobahn.wamp import websocket
from autobahn.websocket import protocol
from autobahn.websocket.types import TransportDetails

try:
import asyncio
Expand Down Expand Up @@ -189,6 +191,18 @@ def get_channel_id(self):
self.log.debug('FIXME: transport channel binding not implemented for asyncio (autobahn-python issue #729)')
return None

def _create_transport_details(self):
"""
Internal helper.
Base class calls this to create a TransportDetails
"""
return TransportDetails(
peer=peer2str(self.transport.get_extra_info('peername')),
host=peer2str(self.transport.get_extra_info('sockname')),
is_secure=self.transport.get_extra_info('peercert', None) is not None,
secure_channel_id=transport_channel_id(self.transport, False, 'tls-unique'),
)

def registerProducer(self, producer, streaming):
raise Exception("not implemented")

Expand Down
21 changes: 21 additions & 0 deletions autobahn/test/__init__.py
Expand Up @@ -31,6 +31,12 @@ class FakeTransport(object):
_written = b""
_open = True

def __init__(self):
self._abort_calls = []

def abortConnection(self, *args, **kw):
self._abort_calls.append((args, kw))

def write(self, msg):
if not self._open:
raise Exception("Can't write to a closed connection")
Expand All @@ -46,3 +52,18 @@ def registerProducer(self, producer, streaming):
def unregisterProducer(self):
# do nothing is correct! until we fake implement registerProducer ..;)
pass

def getPeer(self):
# for Twisted, this would be an IAddress
class _FakePeer(object):
pass
return _FakePeer()

def getHost(self):
# for Twisted, this would be an IAddress
class _FakeHost(object):
pass
return _FakeHost()

def abort_called(self):
return len(self._abort_calls) > 0
6 changes: 4 additions & 2 deletions autobahn/twisted/test/test_component.py
Expand Up @@ -34,6 +34,7 @@
from zope.interface import directlyProvides
from autobahn.wamp.message import Welcome, Goodbye, Hello, Abort
from autobahn.wamp.serializer import JsonSerializer
from autobahn.test import FakeTransport
from twisted.internet.interfaces import IStreamClientEndpoint
from twisted.internet.defer import inlineCallbacks, succeed, Deferred
from twisted.internet.task import Clock
Expand Down Expand Up @@ -65,8 +66,9 @@ def joined(session, details):
component.on('join', joined)

def connect(factory, **kw):
proto = factory.buildProtocol('boom')
proto.makeConnection(Mock())
proto = factory.buildProtocol('ws://localhost/')
transport = FakeTransport()
proto.makeConnection(transport)

from autobahn.websocket.protocol import WebSocketProtocol
from base64 import b64encode
Expand Down
80 changes: 79 additions & 1 deletion autobahn/twisted/test/test_protocol.py
Expand Up @@ -26,15 +26,18 @@

from __future__ import absolute_import, print_function

import unittest
from mock import Mock

from autobahn.util import wildcards2patterns
from autobahn.twisted.websocket import WebSocketServerFactory
from autobahn.twisted.websocket import WebSocketServerProtocol
from autobahn.twisted.websocket import WebSocketClientProtocol
from autobahn.websocket.types import TransportDetails
from autobahn.websocket.types import ConnectingRequest
from twisted.python.failure import Failure
from twisted.internet.error import ConnectionDone, ConnectionAborted, \
ConnectionLost
from twisted.trial import unittest
from twisted.test.proto_helpers import StringTransport
from autobahn.test import FakeTransport

Expand Down Expand Up @@ -362,3 +365,78 @@ def test_trusted_addresses(self):
self.assertEquals(
self.proto.peer, "2.3.4.5",
"The second address in X-Forwarded-For should have been picked as the peer address")


class OnConnectingTests(unittest.TestCase):
"""
Tests related to onConnecting callback

These tests are testing generic behavior, but are somewhat tied to
'a framework' so we're just testing using Twisted-specifics here.
"""

def test_on_connecting_client_fails(self):

class TestProto(WebSocketClientProtocol):
state = None
wasClean = True
log = Mock()

def onConnecting(self, transport_details):
raise RuntimeError("bad stuff")

from autobahn.test import FakeTransport
proto = TestProto()
proto.transport = FakeTransport()
d = proto.startHandshake()
self.successResultOf(d) # error is ignored
# ... but error should be logged
self.assertTrue(len(proto.log.mock_calls) > 0)
self.assertIn(
"bad stuff",
str(proto.log.mock_calls[0]),
)

def test_on_connecting_client_success(self):

class TestProto(WebSocketClientProtocol):
state = None
wasClean = True
perMessageCompressionOffers = []
version = 18
openHandshakeTimeout = 5
log = Mock()

def onConnecting(self, transport_details):
return ConnectingRequest(
host="example.com",
port=443,
resource="/ws",
)

from autobahn.test import FakeTransport
proto = TestProto()
proto.transport = FakeTransport()
proto.factory = Mock()
proto._connectionMade()

d = proto.startHandshake()
req = self.successResultOf(d)
self.assertEqual("example.com", req.host)
self.assertEqual(443, req.port)
self.assertEqual("/ws", req.resource)

def test_str_transport(self):
details = TransportDetails(
host="example.com",
peer="example.com",
is_secure=False,
secure_channel_id=None,
)
# we can str() this and it doesn't fail
str(details)

def test_str_connecting(self):
req = ConnectingRequest(host="example.com", port="1234", resource="/ws")
# we can str() this and it doesn't fail
str(req)
27 changes: 25 additions & 2 deletions autobahn/twisted/websocket.py
Expand Up @@ -34,14 +34,16 @@
txaio.use_twisted()

import twisted.internet.protocol
from twisted.internet.interfaces import ITransport
from twisted.internet.interfaces import ITransport, ISSLTransport

from twisted.internet.error import ConnectionDone, ConnectionAborted, \
ConnectionLost

from autobahn.util import public
from autobahn.util import _is_tls_error, _maybe_tls_reason
from autobahn.wamp import websocket
from autobahn.websocket.types import ConnectionRequest, ConnectionResponse, ConnectionDeny
from autobahn.websocket.types import ConnectionRequest, ConnectionResponse, ConnectionDeny, \
TransportDetails
from autobahn.websocket import protocol
from autobahn.twisted.util import peer2str, transport_channel_id

Expand Down Expand Up @@ -232,6 +234,27 @@ def get_channel_id(self, channel_id_type=u'tls-unique'):
"""
return transport_channel_id(self.transport, is_server=False, channel_id_type=channel_id_type)

def _create_transport_details(self):
"""
Internal helper.
Base class calls this to create a TransportDetails
"""
# note that ITLSTransport exists too, which is "a TCP
# transport that *can be upgraded* to TLS" .. if it *is*
# upgraded to TLS, then the transport will implement
# ISSLTransport at that point according to Twisted
# documentation
return TransportDetails(
peer=peer2str(self.transport.getPeer()),
host=peer2str(self.transport.getHost()),
is_secure=ISSLTransport.providedBy(self.transport),
secure_channel_id=transport_channel_id(
transport=self.transport,
is_server=False,
channel_id_type=u"tls-unique",
),
)


class WebSocketAdapterFactory(object):
"""
Expand Down
16 changes: 16 additions & 0 deletions autobahn/websocket/interfaces.py
Expand Up @@ -417,6 +417,22 @@ def onConnect(self, request_or_response):
You can also return a Deferred/Future that resolves/rejects to the above.
"""

@public
@abc.abstractmethod
def onConnecting(self, transport_details):
"""
This method is called when we've connected, but before the handshake is done.

:param transport_details: information about the transport.
:type transport_details: :class:`autobahn.websocket.types.TransportDetails`

:returns: A
:class:`autobahn.websocket.types.ConnectingRequest`
instance is returned to indicate which options should be
used for this connection. If you wish to use the default
behavior, `None` may be returned (this is the default).
"""

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are all non-subclassing IWebSocketChannel implementations broken now, because they lack this method? Or is the only supported use through subclassing the ABC?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, people that implemented their own sub-classing stuff have broken classes now until they add the new callback (which is strictly expected by the calling library code). not sure if anyone has done that (wrote own classes for channel). adding dynamic attribute checking to work around that .. not sure if it is worth.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, but also the only way to "implement" an ABC is by subclassing it, isn't it? So then they'll get this empty method (which returns None and thus "asks for defaults"). I should double-check that, but I'm fairly certain that's how ABCs work...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, I think I'm wrong here ... "A class containing at least one method declared with this decorator that hasn't been overridden yet cannot be instantiated. Such methods may be called from the overriding method in the subclass (using super or direct invocation)."

That's from PEP3119. So, I think what that means is that yes, anyone who created their own classes from scratch and declared the IWebSocketChannel "interface" via subclassing will now be broken (because they haven't implemented onConnecting). A quick test in the repl confirms this -- you'll get TypeError: Can't instantiate abstract class Foo with abstract methods onClose, onConnect, onConnecting, onMessage, onOpen, onPing, onPong, sendClose, sendMessage, sendPing, sendPong.

@public
@abc.abstractmethod
def onOpen(self):
Expand Down