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

SEP-24 passing the JWT into the interactive url #457

Closed
istrau2 opened this issue Nov 14, 2019 · 10 comments · Fixed by #459
Closed

SEP-24 passing the JWT into the interactive url #457

istrau2 opened this issue Nov 14, 2019 · 10 comments · Fixed by #459

Comments

@istrau2
Copy link
Contributor

@istrau2 istrau2 commented Nov 14, 2019

This issue relates to the following part of SEP-24:

Since the user has authenticated with SEP-0010, the wallet should pass it into the interactive URL... If the JWT is valid: The anchor must honor the user's authenticated session. The anchor should respond with a 302 redirect response that sets a cookie authenticating the user and redirects the user to a different URL that does not contain the jwt parameter. This is to avoid the JWT appearing in the user's browser history. After the redirect, the authenticated interactive flow must continue as usual.

The way I read this, the SEP is telling wallets that they should append the JWT as a &jwt=<JWT> query parameter on the interactive url that is returned by the anchor. Then, the anchor should parse the JWT and redirect 302 to prevent the JWT being included in the browser history.

I understand the motivation here, however I disagree with the approach:

  • If the anchor wants the JWT included in the url, it should already have it in there (remember, the JWT was already passed to the anchor in the /deposit or /withdraw endpoint in which the anchor is forming the interactive url). The wallet shouldn't need to append the JWT to the url.

  • Ultimately, it is the anchor's responsibility to implement security around this workflow. The anchor is forming the redirect url as well as producing the page that is redirected to. I think this SEP can suggest a way for anchors to deal with the security of sending the JWT (it was helpful to me), but probably not mandate a certain approach. I can think of many ways of dealing with the issue of the JWT appearing in browser history including using a 302 redirect on the regular sep10 JWT as suggested, producing a special one-time use (or very short-lived) JWT for this redirect (so that it is harmless if it gets recorded in history) as well as having the redirected page simply replace the browser history. Perhaps there is an anchor who doesn't even need a JWT at all in the redirect (I believe there are anchors that don't use SEP10 at all currently).

Instead of mandating that the wallet pass the JWT in the redirect, I think it is better to suggest to anchors a proper way of going about security but ultimately, leaving the implementation up to the anchor.

@andywer

This comment has been minimized.

Copy link
Contributor

@andywer andywer commented Nov 15, 2019

Great point!
Wrap-up: Passing the JWT as a query parameter smells like bad practice as the URL (incl. query params) is generally considered public data, whereas you want to keep the JWT secret.

I am just not sure if leaving it completely to the anchor to come up with a solution is a good idea... Most anchors are small start-ups right now and not all of them might be so deep into the topic to realize these security concerns. Let this be judged by @msfeldstein 😉

@msfeldstein

This comment has been minimized.

Copy link
Member

@msfeldstein msfeldstein commented Nov 15, 2019

Yeah i agree this smells incorrect.

Regarding the first point of the anchor putting it there if they want it, i definitely agree, and that alone might be enough reason to remove the order for the wallet to put the jwt in there.

I do want to have a good story for security here, and im not sure there's a way to do it without collaboration from the wallet it in some way. One option could be the anchor includes a one-time-use token in the URL it returns that then stores everything in a session after that. The token might as well leak but it is useless after that. Originally i was ok with this JWT approach because the jwt's are generally short-lived (though we should clarify that explicitly).

Thoughts on proper solutions to this? I definitely don't think just offloading responsibility with no guidance is the right approch

@istrau2

This comment has been minimized.

Copy link
Contributor Author

@istrau2 istrau2 commented Nov 15, 2019

@msfeldstein

A couple points:

  • I agree that guidance around security is great in these SEPs. I hadn't thought of the risk of JWT leaking through the browser's history before I read SEP24 and it definitely got me to thinking about that particular risk.
  • I am not sure why we need the wallet's participation as far as securing the JWT in this particular workflow. Of course, the wallet has access to the JWT and should store it safely etc. but as far as choosing whether or not it should be included in the redirect url, I think the wallet shouldn't force to the anchor to do it one way or another. The anchor already has that choice when it forms the url as discussed.

In terms of possible approaches for JWT protection in the browser history:

Don't pass the JWT
I suppose there are situations where the anchor might not want to pass the JWT in the redirect url at all. Perhaps the anchor is very security conscious and wants to require the user login again on redirect. Or perhaps the anchor simply doesn't care if users can see each other deposit destinations (assuming there is no KYC).

302 Redirect
The solution originally presented in the SEP seems like it would work. To recap: The JWT is included in the url (by the anchor, not by the wallet) and upon redirect, the anchor, before returning the html response, checks the JWT and issues a 302 to another url that does not have the JWT in it. Note, I could not find any literature or standards stating that browsers not store the original url in history on 3xx responses (I am just assuming this to be true). There are certainly cases where the original url will be stored on 3xx responses, like in a browser's dev tools (when the "Preserve log" is selected) or in something like fiddler.

Use a harmless JWT
There are many approaches here. The idea is that the anchor, when it is forming the redirect url, has the JWT passed by the user. It can now include a separate JWT to include in the url. Perhaps, this new JWT is a one-time use or very short lived JWT that the user can then exchange for another longer lived JWT upon redirect. Again, there may be some anchors where all they are really doing is showing the deposit destination, in this case, maybe a JWT that lasts a hour is not that big of a risk?

Replace the browser history
Many browsers support replace state client side. This could be used by the resulting page to remove the JWT from the history.

I am sure there are other approaches that I haven't thought of. Anchors might want to try combinations of these various approaches as well.

@msfeldstein

This comment has been minimized.

Copy link
Member

@msfeldstein msfeldstein commented Nov 15, 2019

I am not sure why we need the wallet's participation as far as securing the JWT in this particular workflow.

You're right, we don't.

Use a harmless JWT

I think this is the right approach. The other approaches involve cleaning something up which seems dangerous and dependent on external implementations

@istrau2

This comment has been minimized.

Copy link
Contributor Author

@istrau2 istrau2 commented Nov 15, 2019

I think this is the right approach. The other approaches involve cleaning something up which seems dangerous and dependent on external implementations

fair enough =)

@msfeldstein

This comment has been minimized.

Copy link
Member

@msfeldstein msfeldstein commented Nov 15, 2019

So it seems like solution is

  • Add Instruction to anchors to use a one-time / short-lived token in the interactive URL returned to the wallet, and then store auth information in sessions/cookies upon first visit
  • Remove instruction for wallets to add the jwt (or anything) to the interactive URL

If anyone wants to take a run at a PR I think this sounds right, otherwise i can get to it some time next week

@leighmcculloch

This comment has been minimized.

Copy link
Member

@leighmcculloch leighmcculloch commented Nov 15, 2019

I would go as far as not telling the anchor how to store auth information in sessions/cookies upon first visit. It's out of scope.

@istrau2

This comment has been minimized.

Copy link
Contributor Author

@istrau2 istrau2 commented Nov 15, 2019

I would go as far as not telling the anchor how to store auth information in sessions/cookies upon first visit. It's out of scope.

Agreed. Another approach the anchor can take is to simply exchange the one-time token for another JWT upon redirect (and not deal with cookies or sessions at all).

@msfeldstein

This comment has been minimized.

Copy link
Member

@msfeldstein msfeldstein commented Nov 18, 2019

exchange the one-time token for another JWT upon redirect (and not deal with cookies or sessions at all).

Wouldn't this suffer the same problem of having a JWT in the URL? Whats the purpose of the one-time token in that case?

@istrau2

This comment has been minimized.

Copy link
Contributor Author

@istrau2 istrau2 commented Nov 19, 2019

@msfeldstein I just mean that the one-time use jwt can only be used to exchange for another JWT once. The other jwt wouldn't be in any urls at all.

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

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.