Decode JWT token for validation #1480

roshii · 2023-04-24T21:12:25Z

Implement jmclient.auth module to manage JWT.

Upon successful authentication (e.g. unlock wallet), response includes both a token and a refresh_token. The former can be used for call authentication, valid for 30 min. After expiration, user can call /token/refresh endpoint with his expired access token in header and refresh token in POST call payload to get both a new access and refresh token. Refresh token is valid for 4 hours.

Anytime a new access token is issued, refresh token signature key is re-initialized, invalidating any previously issued token.
Tokens are scoped to a specific wallet_name and a generic walletrpc category, and should allow future upgrades such as authorization granularity.

Fixes #1297

roshii · 2023-04-29T05:05:38Z

Tests are passing locally but require Werkzeug<2.3.0

Pending #1479 resolution to have CI test passing and dependencies updated

roshii · 2023-04-29T05:14:45Z

Now thinking that I did not consider the case when the token expires: how will client request a new token and authenticate itself?
To be continued

AdamISZ · 2023-04-29T16:31:07Z

Add cookie_secret_key and cookie_signature_algorithm to JMWalletDaemon. Decode token with set secret key, automatically validating expiry time with 10 s tolerance. Simplify set_token method by removing wallet_name parameter, reading it from property instead

Fixes #1297

Thanks a lot for working on this, it was badly needed.

roshii · 2023-05-08T15:37:10Z

Now thinking that I did not consider the case when the token expires: how will client request a new token and authenticate itself?

After analysis, I think token issuance responsibility should be moved to RPC clients. I did start working on an implementation in which a JWT (public) key parameter can be added to JMWalletDaemon. This key can later be used to verify token signature, would a token be expired, client will received a 409 which they have to deal with by issuing a new token.
For the sake of compatibility, if a secret is not provided JMWalletDaemon would create a token with a random secret, with 30 min validity a no way to refresh

roshii · 2023-05-08T20:26:28Z

I think token issuance responsibility should be moved to RPC clients.

Maybe not a good idea... I will rather refine server side implementation and address access token refresh issue with refresh tokens as described here.

AdamISZ · 2023-05-08T22:55:47Z

I think token issuance responsibility should be moved to RPC clients.

Maybe not a good idea... I will rather refine server side implementation and address access token refresh issue with refresh tokens as described here.

From a brief read, it does look like an interesting idea, though rather complex. Are you thinking about something like this? https://stackoverflow.com/a/71900761 .. if so, how would it work client-side? I didn't have a clear idea about this expiration issue myself, when we first (half-) implemented this.

roshii · 2023-05-09T10:52:58Z

Idea is as follows, for a typical flow:

Client unlock wallet -> receives an access token and a refresh token in the payload, and stores refresh token for later use
Client access authenticated endpoint with expired access token -> receives an error response with "invalid_token" error code
Client makes a refresh request with its expired access token and its refresh token, the latter in the payload -> server validates access token without expiration time verification, validates refresh token, and finally sends a new access and refresh token to client

AdamISZ · 2023-05-09T11:06:49Z

Yes that's approximately what I understood, it is a bit complex, and I guess we don't get out of the box with the PyJWT package right? But from a logic perspective it seems entirely fine.

roshii · 2023-05-09T15:20:22Z

Refresh logic requires a custom implementation indeed, PyJWT will solely help validating tokens

roshii · 2023-05-14T16:07:00Z

I have just refined token validation implementation, it should now be straight forward to add a refresh token endpoint.

roshii · 2023-05-19T16:25:13Z

Added token/refresh endpoint

roshii · 2023-05-19T16:46:16Z

Updated PR comment

AdamISZ · 2023-07-30T17:58:31Z

Looks like really good work, I haven't yet found any issues in testing it out. I'll keep looking at it for a while.

You need to update https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/master/docs/api/wallet-rpc.yaml with the new /refresh endpoint (and also update other endpoints like /unlock with the new refresh_token field). I'd also add some details in the /refresh endpoint that gives clear instructions to client coders on how to handle the refresh operation.

It really does seem a shame that we need such a large amount of code here to do stuff that really feels like it should be handled by an underlying library ... but it's certainly better to have sensible logic instead of basically broken (or, charitably "very incomplete") logic for tokens that we had before.

jmclient/jmclient/auth.py

jmclient/jmclient/websocketserver.py

AdamISZ · 2023-08-03T15:15:39Z

OK should be ready after adding yaml and squashing.

roshii · 2023-08-04T10:51:23Z

Updated OpenAPI doc and squashed

roshii · 2023-08-04T14:02:22Z

It really does seem a shame that we need such a large amount of code here to do stuff that really feels like it should be handled by an underlying library ... but it's certainly better to have sensible logic instead of basically broken (or, charitably "very incomplete") logic for tokens that we had before.

The issue is with the authorization logic that cannot really be delegated to a library imho. Considering every application will have different authorization needs, importing a "flexible library" would add many more lines of code than implementing a custom logic, which we do here with less than hundred lines.

AdamISZ · 2023-08-14T22:27:57Z

Two things that are not hugely needed but worth considering:

Add a note to https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/master/docs/JSON-RPC-API-using-jmwalletd.md#rules-about-making-requests about the token refresh scheme.
Perhaps mention there the algorithm chosen for the token signing (not really needed for clients, but, just to doc it). Relatedly, consider adding a test case where the signing algorithm is the wrong one, to verify that it is rejected as expected.

AdamISZ · 2023-08-14T22:37:36Z

jmclient/jmclient/auth.py

+        arguments and wallet_name property.
+        """
+        token_type = "refresh" if is_refresh else "access"
+        claims = jwt.decode(


Afaict from looking at the pyjwt code, this will throw ExpiredSignatureError if we passed the expiration. I think there's a value in catching that to bubble up an error message for the caller? So they can distinguish between a rejected token that is just invalid vs one that is expired?

Sometimes there's an argument in computer security for never telling the caller the reason for the failure, since it can allow probing attacks. That doesn't seem to apply here.

Afaict from looking at the pyjwt code, this will throw ExpiredSignatureError if we passed the expiration. I think there's a value in catching that to bubble up an error message for the caller? So they can distinguish between a rejected token that is just invalid vs one that is expired?

ExnpiredSignatureError can indeed be caught and bubble up to the caller in error description.

Sometimes there's an argument in computer security for never telling the caller the reason for the failure, since it can allow probing attacks. That doesn't seem to apply here.

That was my argument as a matter of facts, but thinking about it, expiry date is already readable from token. I will adjust code.

AdamISZ · 2023-08-17T15:51:30Z

@roshii if you're not inclined to make further updates, I'm pretty much fine with merging as-is. (Minor exception here but whatever, I can edit that later if needed).

roshii · 2023-08-19T21:02:59Z

@AdamISZ you'll have my comments shortly. We may want to modify a few things.

roshii · 2023-08-22T20:39:15Z

@roshii if you're not inclined to make further updates, I'm pretty much fine with merging as-is. (Minor exception here but whatever, I can edit that later if needed).

Reading OAuth 2.1 draft specs but I came to realize that my implementation differs from it on a few point.

Endpoint should be /token - not /token/refresh
Endpoint should accept POST method only
refresh grant should be specified in HTTP request content
Response should have an access_token parameter but since we already use token I propose to keep the latter.

Since changes are rather straight forward, I would like to align to standards before merging.

AdamISZ · 2023-08-22T22:14:56Z

refresh grant should be specified in HTTP request content

Could you explain point 3? I don't understand what that means.

roshii · 2023-08-23T05:12:42Z

refresh grant should be specified in HTTP request content

Could you explain point 3? I don't understand what that means.

An example will speak by itself:

My implementation implies the following call:
curl -X GET https://localhost/api/v1/token/refresh --data 'refresh_token=a7f1...'
OAuth recommends the following:
curl -X POST https://localhost/api/v1/token --data 'grant_type=refresh&refresh_token=a7f1...'

roshii · 2023-08-29T19:35:32Z

Rebased on master
Addressed review comments
Align to OAuth specs, with two exceptions (in response key to maintain compatibility with joinmarket implementation

AdamISZ · 2023-08-29T20:49:34Z

I'll make it a priority to review it as I can over the next day or so. @theborakompanioni any feedback from you (or other JAM people) would be great.

jmclient/jmclient/wallet_rpc.py

AdamISZ · 2023-09-01T16:49:44Z

Have reviewed the update and I believe it all makes sense as far as I have checked. Also tested it out, though only using the test suite. I just have that one question above for now, though I will probably try to check a couple more things.

AdamISZ · 2023-09-04T17:50:59Z

@roshii

OK so I tested this in as realistic a way as I could (using POSTMAN) to call /token and I found an error:

In the refresh() method in JMWalletDaemon, you are calling get_POST_body() twice, but the first one has already read the byestream object, so the second call returns b'' from the request.content.read(). Hence calling /token results in an error (because get_POST_body just returns False and you subscript that return value).

I understand that the intention was to have it be dynamic, i.e. that the grant_type field defines the key name of the field containing the actual "thing" which is currently just "refresh_token" and nothing else. That doesn't quite fit with how `get_POST_body was written. So for now I'm going to take the "executive decision" to merge this then add the small patch that fixes that error by just only allowing a "refresh_token" field; but you're more than welcome to just propose another change if you like, after.

The JWT validation update in PR 1480 contained a small error: by calling the function get_POST_body() twice, the content of the request was unavailable in the second call. This calls only once, but has the downside of requiring a specific set of keys in the json request data. Hence this might be fixed to be more flexible later, see comments in the PR.

AdamISZ · 2023-09-04T17:58:37Z

@theborakompanioni please see the follow up commit directly after ( a847df9 ), and let us know how this goes for JAM clients, as I can well imagine this is a non trivial extra bit of coding client side.

roshii · 2023-09-04T19:33:51Z

I understand that the intention was to have it be dynamic, i.e. that the grant_type field defines the key name of the field containing the actual "thing" which is currently just "refresh_token" and nothing else. That doesn't quite fit with how `get_POST_body was written. So for now I'm going to take the "executive decision" to merge this then add the small patch that fixes that error by just only allowing a "refresh_token" field; but you're more than welcome to just propose another change if you like, after.

Thanks for the fix. I will address the dynamic behavior of this method while implementing authentication/credentials (#3) which will most likely need to manage another grant type.

AdamISZ · 2023-09-04T20:59:23Z

I understand that the intention was to have it be dynamic, i.e. that the grant_type field defines the key name of the field containing the actual "thing" which is currently just "refresh_token" and nothing else. That doesn't quite fit with how `get_POST_body was written. So for now I'm going to take the "executive decision" to merge this then add the small patch that fixes that error by just only allowing a "refresh_token" field; but you're more than welcome to just propose another change if you like, after.

Thanks for the fix. I will address the dynamic behavior of this method while implementing authentication/credentials (#3) which will most likely need to manage another grant type.

Right, sounds good. I realized after pushing it that I was being a bit braindead, as well as any sophisticated way of doing it, there is a very simply way of doing it: deep copy the request object :) But right, fold it in with another change.

theborakompanioni · 2023-09-08T09:33:08Z

@theborakompanioni please see the follow up commit directly after ( a847df9 ), and let us know how this goes for JAM clients, as I can well imagine this is a non trivial extra bit of coding client side.

Will immediately start after the new Jam version for v0.9.10 is released. 🙏

theborakompanioni · 2023-09-21T14:13:14Z

I am currently implementing this for Jam.
First thing noticeable is that there seems to be a problem with wallets having space ( ) in it, e.g. import test.jmdat.

This is the result of a recovery test (/recover endpoint):

import.jmdat:

{
 [...]
  "scope": "test.jmdat walletrpc import.jmdat"
}

import test.jmdat:

{
 [...]
  "scope": "test.jmdat walletrpc import test.jmdat import.jmdat"
}

Note: Existing wallets are test.jmdat, import.jmdat and import test.jmdat.

roshii · 2023-09-21T17:32:00Z

I am currently implementing this for Jam.
First thing noticeable is that there seems to be a problem with wallets having space ( ) in it, e.g. import test.jmdat.

This is the result of a recovery test (/recover endpoint):

import.jmdat:
{
 [...]
  "scope": "test.jmdat walletrpc import.jmdat"
}
import test.jmdat:
{
 [...]
  "scope": "test.jmdat walletrpc import test.jmdat import.jmdat"
}
Note: Existing wallets are test.jmdat, import.jmdat and import test.jmdat.

Thanks for the info, I'll have a look at it shortly.

theborakompanioni · 2023-09-22T16:16:45Z

fyi @roshii, I don't know if this is on purpose or a mistake. When the access token is not refreshed and a request is made with the expired token, the message in www-authenticate response header contains a stacktrace:

HTTP/1.1 401 Unauthorized
X-Powered-By: Express
transfer-encoding: chunked
connection: close
server: TwistedWeb/22.4.0
date: Wed Sep 20 2023 18:50:21 GMT-0700
www-authenticate: error_description="[Failure instance: Traceback: <class 'jmclient.wallet_rpc.InvalidToken'>: The access token provided is expired. /usr/local/lib/python3.9/dist-packages/twisted/web/server.py:292:render /usr/local/lib/python3.9/dist-packages/klein/_resource.py:204:render /usr/local/lib/python3.9/dist-packages/twisted/internet/defer.py:190:maybeDeferred /usr/local/lib/python3.9/dist-packages/klein/_resource.py:196:_execute --- <exception caught here> --- /usr/local/lib/python3.9/dist-packages/twisted/internet/defer.py:190:maybeDeferred /usr/local/lib/python3.9/dist-packages/klein/_app.py:129:execute_endpoint /usr/local/lib/python3.9/dist-packages/klein/_app.py:241:_f /usr/local/lib/python3.9/dist-packages/klein/_app.py:60:_call /src/jmclient/jmclient/wallet_rpc.py:707:session /src/jmclient/jmclient/wallet_rpc.py:412:check_cookie_if_present /src/jmclient/jmclient/wallet_rpc.py:400:check_cookie ]"
content-type: text/html

Except for the report above (space in wallet name), it seems to work fine. 💪
Will keep reporting additional findings, should anything else come up.

roshii · 2023-09-22T17:31:30Z

fyi @roshii, I don't know if this is on purpose or a mistake. When the access token is not refreshed and a request is made with the expired token, the message in www-authenticate response header contains a stacktrace:

HTTP/1.1 401 Unauthorized
X-Powered-By: Express
transfer-encoding: chunked
connection: close
server: TwistedWeb/22.4.0
date: Wed Sep 20 2023 18:50:21 GMT-0700
www-authenticate: error_description="[Failure instance: Traceback: <class 'jmclient.wallet_rpc.InvalidToken'>: The access token provided is expired. /usr/local/lib/python3.9/dist-packages/twisted/web/server.py:292:render /usr/local/lib/python3.9/dist-packages/klein/_resource.py:204:render /usr/local/lib/python3.9/dist-packages/twisted/internet/defer.py:190:maybeDeferred /usr/local/lib/python3.9/dist-packages/klein/_resource.py:196:_execute --- <exception caught here> --- /usr/local/lib/python3.9/dist-packages/twisted/internet/defer.py:190:maybeDeferred /usr/local/lib/python3.9/dist-packages/klein/_app.py:129:execute_endpoint /usr/local/lib/python3.9/dist-packages/klein/_app.py:241:_f /usr/local/lib/python3.9/dist-packages/klein/_app.py:60:_call /src/jmclient/jmclient/wallet_rpc.py:707:session /src/jmclient/jmclient/wallet_rpc.py:412:check_cookie_if_present /src/jmclient/jmclient/wallet_rpc.py:400:check_cookie ]"
content-type: text/html

Thanks for the info. Headers should contain the following info, with no hint on class name.

www-authenticate: error_description=The access token provided is expired.

I'll take a look to have it done has it should.

Except for the report above (space in wallet name), it seems to work fine. 💪
Will keep reporting additional findings, should anything else come up.

Good to read. Thanks again.

c88429d JWT authority fixes (roshii) Pull request description: - Fix JWT unit tests (`async` was somehow making tests always successful therefore hiding some issues) - Fix `WWW-Authenticate` header construction (#1480 (comment)) - Encode wallet names with base64 in scopes to allow for space delimited names (#1480 (comment), joinmarket-webui/jam#663 (comment)) - Fix syntax errors in OpenAPI RPC documentation (#1559) Top commit has no ACKs. Tree-SHA512: 6625c4c457c4caf3b4979505334c955bec50fcc0b01707e313dc772571c5c8c8b3ca359a18b5e67f1b0d0eb9b2b7c234ae9716d785234e8de0f3bfb76d53d29a

The JWT validation update in PR 1480 contained a small error: by calling the function get_POST_body() twice, the content of the request was unavailable in the second call. This calls only once, but has the downside of requiring a specific set of keys in the json request data. Hence this might be fixed to be more flexible later, see comments in the PR.

roshii changed the title ~~Decode JWT token for validation~~ [WIP] Decode JWT token for validation Apr 25, 2023

roshii changed the title ~~[WIP] Decode JWT token for validation~~ Decode JWT token for validation Apr 28, 2023

roshii marked this pull request as ready for review April 28, 2023 22:10

roshii mentioned this pull request May 2, 2023

RPC API returns 405 #1483

Closed

kristapsk added the RPC-API label May 3, 2023

roshii force-pushed the decode-jwt branch from 3f87135 to e807421 Compare May 8, 2023 13:29

roshii force-pushed the decode-jwt branch from e807421 to 2b29e67 Compare May 14, 2023 16:04

roshii force-pushed the decode-jwt branch from ff00830 to b0421f5 Compare July 14, 2023 09:42

roshii force-pushed the decode-jwt branch from b0421f5 to cc5b46b Compare July 28, 2023 18:33

AdamISZ reviewed Jul 30, 2023

View reviewed changes

jmclient/jmclient/auth.py Outdated Show resolved Hide resolved

AdamISZ reviewed Jul 30, 2023

View reviewed changes

jmclient/jmclient/auth.py Outdated Show resolved Hide resolved

AdamISZ reviewed Jul 30, 2023

View reviewed changes

jmclient/jmclient/auth.py Outdated Show resolved Hide resolved

AdamISZ reviewed Jul 30, 2023

View reviewed changes

jmclient/jmclient/websocketserver.py Outdated Show resolved Hide resolved

roshii force-pushed the decode-jwt branch from f352960 to 5c1e565 Compare August 4, 2023 10:50

AdamISZ reviewed Aug 14, 2023

View reviewed changes

Implement wallet RPC's JWT token authority

b2822dd

roshii force-pushed the decode-jwt branch from 0a4c073 to b2822dd Compare August 29, 2023 19:35

roshii mentioned this pull request Sep 1, 2023

Credentials/auth for client connection #3

Open

AdamISZ reviewed Sep 1, 2023

View reviewed changes

jmclient/jmclient/wallet_rpc.py Show resolved Hide resolved

AdamISZ merged commit 6e6e68b into JoinMarket-Org:master Sep 4, 2023
20 checks passed

roshii deleted the decode-jwt branch September 4, 2023 19:21

theborakompanioni mentioned this pull request Sep 19, 2023

feat: use new authentication mechanism (access/refresh token) joinmarket-webui/jam#663

Closed

roshii mentioned this pull request Sep 29, 2023

JWT follow-up #1562

Merged

Decode JWT token for validation #1480

Decode JWT token for validation #1480

Conversation

roshii commented Apr 24, 2023 • edited

roshii commented Apr 29, 2023 • edited

roshii commented Apr 29, 2023

AdamISZ commented Apr 29, 2023

roshii commented May 8, 2023

roshii commented May 8, 2023

AdamISZ commented May 8, 2023

roshii commented May 9, 2023

AdamISZ commented May 9, 2023

roshii commented May 9, 2023

roshii commented May 14, 2023

roshii commented May 19, 2023

roshii commented May 19, 2023

AdamISZ commented Jul 30, 2023

AdamISZ commented Aug 3, 2023

roshii commented Aug 4, 2023

roshii commented Aug 4, 2023

AdamISZ commented Aug 14, 2023

AdamISZ Aug 14, 2023

Choose a reason for hiding this comment

roshii Aug 22, 2023

Choose a reason for hiding this comment

AdamISZ commented Aug 17, 2023

roshii commented Aug 19, 2023

roshii commented Aug 22, 2023

AdamISZ commented Aug 22, 2023

roshii commented Aug 23, 2023

roshii commented Aug 29, 2023

AdamISZ commented Aug 29, 2023

AdamISZ commented Sep 1, 2023

AdamISZ commented Sep 4, 2023

AdamISZ commented Sep 4, 2023

roshii commented Sep 4, 2023

AdamISZ commented Sep 4, 2023

theborakompanioni commented Sep 8, 2023

theborakompanioni commented Sep 21, 2023 • edited

roshii commented Sep 21, 2023

theborakompanioni commented Sep 22, 2023

roshii commented Sep 22, 2023 • edited

roshii commented Apr 24, 2023 •

edited

roshii commented Apr 29, 2023 •

edited

theborakompanioni commented Sep 21, 2023 •

edited

roshii commented Sep 22, 2023 •

edited