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

[SEP24] Don't recommend adding jwt to interactive url #459

Merged
merged 2 commits into from
Nov 19, 2019

Conversation

msfeldstein
Copy link
Contributor

Fixes #457

Having wallets add JWTs to interactive URLs seems like a fishy practice. If the anchor expects a JWT in the interactive url it can put it there itself. We should leave the specific security of interactive webapps to the anchors discretion. Instead we add a small hint as to how to keep continuity from authenticated API calls to fresh interactive flow requests.

Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

I think this is good, it reduces the auth component of the SEP to just what's relevant. I love how the concepts of using a token or JWT are still mentioned as a suggestion though.

@msfeldstein msfeldstein merged commit f4f1f61 into stellar:master Nov 19, 2019
@yuriescl
Copy link
Contributor

yuriescl commented Nov 19, 2019

I agree, the Anchor should deal with the security of the URL.
The JWT generated in SEP10 is supposed to be used between the Wallet and the Anchor, the user shouldn't have access to it since the JWT is signed by who owns the account private key. It should be up to the Wallet to decide granting the user access to that private key.
My suggestion is, as an Anchor, when the first POST /transactions/deposit/interactive is called by the Wallet, an internal unique ID (deposit_id) for that deposit is generated and saved in database. Then, a new JWT token (one_time_jwt) is generated, with expiration time of 10 minutes and only allowed to be used once, and its jti field contains that deposit_id, so it allows retrieving the deposit info:

from datetime import timezone, timedelta
from datetime import datetime as dt

deposit_id = new_deposit_uuid() # unique ID for the deposit
current_time = dt.now(timezone.utc).timestamp()
jwt_dict = {
    "iat": current_time,
    "exp": current_time + timedelta(minutes=10).total_seconds(),
    "jti": deposit_id,
}
one_time_jwt = jwt.encode(jwt_dict, settings.SERVER_JWT_SECRET, algorithm='RS256').decode("utf-8")

return JsonResponse({
  "type": "interactive_customer_info_needed",
  "url" : "https://website.anchor.example.com/deposit-form?token=" + one_time_jwt,
  "id": deposit_id
})

This allows confirming the token authenticity and retrieving all the deposit info based on the deposit_id in the jti field:

jwt_dict = jwt.decode(token, settings.SERVER_JWT_PUBKEY, algorithm='RS256')
current_time = dt.now(timezone.utc).timestamp()
if current_time < jwt_dict["iat"] or current_time > jwt_dict["exp"]:
    raise Exception("invalid token")

# ... check if token was already used ...

deposit_id = jwt_dict["jti"]
# handle deposit

@JakeUrban
Copy link
Contributor

I like the approach @yuriescl outlined.

However I want to point out that since the new JWT has an expiration, we need to provide wallets the ability to retrieve a new JWT if their original one expires before it is able to start the interactive flow.

I'll file a new PR for SEP-24 that adds a GET /transactions/<transaction>/interactive endpoint, which generates a fresh one-time JWT for the wallet to use in the event the one-time JWT the wallet received from POST /transactions/<transaction>/interactive expires.

@yuriescl
Copy link
Contributor

yuriescl commented Dec 6, 2019

@JakeUrban That new JWT I suggested would be used in the interactive URL:

{
  "type": "interactive_customer_info_needed",
  "url" : "https://api.example.com/kycflow?token=<new_jwt>",
  "id": <transaction_id>
}

That URL will be accessed by the user. It can be used any number of times (never mind the "one-time token" idea). But in an usual scenario, it will be only used once by the popup window. Once the user finishes the KYC interactive flow, new_jwt will be invalidated.
The Wallet doesn't need to know new_jwt because it can use the existing SEP10 JWT for querying transactions by their ids.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SEP-24 passing the JWT into the interactive url
4 participants