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

Nonce in login.gov should be per-session #97

Closed
timothy-spencer opened this issue Mar 11, 2019 · 4 comments
Closed

Nonce in login.gov should be per-session #97

timothy-spencer opened this issue Mar 11, 2019 · 4 comments
Labels

Comments

@timothy-spencer
Copy link
Contributor

Expected Behavior

The login.gov provider in #55 creates a nonce that is used for the lifetime of the proxy, rather than doing it per-session, which is the proper behavior.

Current Behavior

The login.gov provider creates the nonce during the LoginGovProvider initialization and then uses it for the life of the proxy.

Possible Solution

The code will either need to be restructured significantly so that session state is created during the authorization redirect, or perhaps some additional session state storage system could be plugged in to store the nonce that could be looked up during token redemption.

Actually, https://openid.net/specs/openid-connect-core-1_0.html#NonceNotes suggests that we might be able to find some sort of session cookie that is common to both queries and hash that for the nonce. Not sure if we have already created such a thing yet or not, though. Need to get more familiar with the golang proxy stuff.

Steps to Reproduce (for bugs)

If you alter the code to print out the nonce anywhere it is used, you will see it is the same every time.

Context

The nonce can be used to mitigate token replay attacks: https://openid.net/specs/openid-connect-core-1_0.html#NonceNotes

Your Environment

This works during any login.gov authorization/authentication using the LoginGovProvider.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2020

This issue has been inactive for 60 days. If the issue is still relevant please comment to re-activate the issue. If no action is taken within 7 days, the issue will be marked closed.

@github-actions github-actions bot added the Stale label Mar 6, 2020
@JoelSpeed
Copy link
Member

I think this is still relevant, though I'm not sure how many people are using the provider. @timothy-spencer do you have any time to implement a fix for this?

@JoelSpeed JoelSpeed removed the Stale label Mar 6, 2020
@timothy-spencer
Copy link
Contributor Author

There might be a couple of folks using it, but truthfully, it's probably mostly me. :-)

I am currently engaged in other work, but may have some time coming up in a month or so to dig into this. I remember when I created this issue that it looked like it would require a lot of restructuring to do, so I think i was hoping for somebody with a bit more authority to change the architecture to build or point out a more general system for getting a per-session nonce.

It's definitely an improvement that would make things more secure, so it is still relevant. Anyways, I'll see if I can get some time to look into this soon. Thanks for the ping!

@github-actions
Copy link
Contributor

github-actions bot commented May 6, 2020

This issue has been inactive for 60 days. If the issue is still relevant please comment to re-activate the issue. If no action is taken within 7 days, the issue will be marked closed.

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

No branches or pull requests

2 participants