crossbario / crossbar

Crossbar.io - WAMP application router
https://crossbar.io/
Other
2.05k stars 274 forks source link

cryptosign-proxy is broken with static config #1962

Closed om26er closed 2 years ago

om26er commented 2 years ago

If a router worker is using cryptosign-proxy with static authentication(i.e. principals defined in crossbar config), then when the proxy worker tries to authenticate, it fails with an exception here https://github.com/crossbario/crossbar/blob/5c3d0e44e7e2b72357b7364ab8e945e08a994b27/crossbar/router/auth/cryptosign.py#L383

because the static authenticator either returns a types.Deny or types.Challenge instance and our code tries to deal with as a 'future' so below error is thrown

2022-03-20T20:54:47+0500 [Router     1153300] internal error: 'Challenge' object has no attribute 'addCallbacks'
Traceback (most recent call last):
  File "/home/om26er/Documents/inrouter-comp/venv/lib/python3.8/site-packages/autobahn/twisted/rawsocket.py", line 141, in stringReceived
    self._session.onMessage(msg)
  File "/home/om26er/Documents/inrouter-comp/venv/lib/python3.8/site-packages/crossbar/router/session.py", line 501, in onMessage
    d = txaio.as_future(self.onHello, msg.realm, details)
  File "/home/om26er/Documents/inrouter-comp/venv/lib/python3.8/site-packages/txaio/tx.py", line 369, in as_future
    return maybeDeferred(fun, *args, **kwargs)
  File "/home/om26er/Documents/inrouter-comp/venv/lib/python3.8/site-packages/twisted/internet/defer.py", line 190, in maybeDeferred
    result = f(*args, **kwargs)
--- <exception caught here> ---
  File "/home/om26er/Documents/inrouter-comp/venv/lib/python3.8/site-packages/crossbar/router/session.py", line 893, in onHello
    return self._pending_auth.hello(realm, details)
  File "/home/om26er/Documents/inrouter-comp/venv/lib/python3.8/site-packages/crossbar/router/auth/cryptosign.py", line 388, in hello
    txaio.add_callbacks(f, assign, error)
  File "/home/om26er/Documents/inrouter-comp/venv/lib/python3.8/site-packages/txaio/tx.py", line 446, in add_callbacks
    future.addCallbacks(callback, errback)
builtins.AttributeError: 'Challenge' object has no attribute 'addCallbacks
om26er commented 2 years ago

So here is how a fix could look like.

If below call returns types.Deny, we should just return it as-is https://github.com/crossbario/crossbar/blob/5c3d0e44e7e2b72357b7364ab8e945e08a994b27/crossbar/router/auth/cryptosign.py#L353

If that call returns a type.Challenge, then we need to assign the principal and fill that with proxy_* variants and return the Challenge object

I'll do a PR later tonight

om26er commented 2 years ago

Took a stab at it here https://github.com/crossbario/crossbar/pull/1963

oberstet commented 2 years ago

easiest is just wrapping it with txaio.as_future

however, also, IMO we should take the chance and nail/define the (internal) interface of authenticators including either sync/async or async-always ...

om26er commented 2 years ago

@oberstet I did try that first, since types.Deny and types.Challenge are not callable(s), we can't really pass that to txaio.as_future, so the proposed solution in my PR just extends the same approach that PendingAuthCryptosign uses (for non-proxy auth) and brings that over to PendingAuthCryptosignProxy

om26er commented 2 years ago

Here is the error that the router worker gets from the proxy worker

2022-03-21T18:02:57+0500 [Proxy      1716298] WampRawSocketClientProtocol.stringReceived: WAMP InvalidUriError - aborting connection!
'reason' in ABORT: invalid value "Internal error: [Failure instance: Traceback: <class 'TypeError'>: 'Deny' object is not callable
/home/om26er/scm/crossbario/crossbar/venv/lib/python3.8/site-packages/twisted/internet/defer.py:190:maybeDeferred
/home/om26er/scm/crossbario/crossbar/crossbar/router/session.py:905:onHello
/home/om26er/scm/crossbario/crossbar/crossbar/router/auth/cryptosign.py:389:hello
/home/om26er/scm/crossbario/crossbar/venv/lib/python3.8/site-packages/txaio/tx.py:369:as_future
--- <exception caught here> ---
/home/om26er/scm/crossbario/crossbar/venv/lib/python3.8/site-packages/twisted/internet/defer.py:190:maybeDeferred
]" for URI (did not match pattern "^([^\s\.#]+\.)*([^\s\.#]+)$" with options strict=False, allow_empty_components=False, allow_last_empty=False, allow_none=False)
2022-03-21T18:02:57+0500 [Proxy      1716298] Stopping factory <autobahn.twisted.rawsocket.WampRawSocketClientFactory object at 0x7f596e31edf0>
2022-03-21T18:02:57+0500 [Proxy      1716298] <crossbar.worker.proxy.ProxyFrontendSession.onClose>(wasClean=True)
oberstet commented 2 years ago
f = txaio.as_future(super(PendingAuthCryptosignProxy, self).hello, realm, details))

should work?

rgd your PR: this looks a hack-on-top-of-a-hack;) it conceils its actual intention. as_future is simple and explicit

also: I haven't ever run into the problem (if there is one) anyways. and I am hesitating to merge code for which I don't have a need nor a CI test case ...

om26er commented 2 years ago

should work?

It did work, I was doing it wrong. I have updated the PR, which is now a one-liner

om26er commented 2 years ago

Regarding adding tests for bug fixes and features: that makes sense. I will be working on that area next, starting with rlinks. I hope that will make me somewhat familiar with crossbar testing