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

allow access_tokens to bypass ban_networks check #1287

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

minrk
Copy link
Member

@minrk minrk commented Apr 8, 2021

ban_networks is disabled for authenticated requests

This does mean that ban_networks does not apply to deployments with authentication enabled. Not sure if we want a separate switch for that.

config dict is sha256 hashes of tokens, assuming tokens themselves are generated with reasonable entropy

for jupyterhub/mybinder.org-deploy#1828 (comment)

Needs tests, but works locally

ban_networks is disabled for authenticated requests

config dict is sha256 hashes of tokens, assuming tokens themselves are generated with reasonable entropy
@minrk minrk requested a review from betatim April 8, 2021 10:07
@betatim
Copy link
Member

betatim commented Apr 10, 2021

config dict is sha256 hashes of tokens, assuming tokens themselves are generated with reasonable entropy

Why the two step procedure of generating a random token and then comparing it to the sha256 of it? Before reading the code I was expecting that we'd have a list of tokens that, maybe, map to human readable usernames.

Another idea I had was to use something like https://github.com/ecordell/pymacaroons (code behind the PyPI API tokens) to create tokens which are/can be restricted to only by pass the IP check for certain repos or some such. Potentially overkill for what we want.


Any idea why codecov has started commenting on the diff? I find it a bit annoying because it breaks the flow of reading the code :-/ WDYT?

@minrk
Copy link
Member Author

minrk commented Apr 12, 2021

Why the two step procedure of generating a random token and then comparing it to the sha256 of it?

To avoid storing the access tokens needed for authentication in plaintext, both in our deployment files and in memory. I think it's pretty standard practice to store only the hashed versions of passwords and tokens (plus salts and extra rounds for passwords) instead of the access tokens themselves.

Another idea I had was to use something like https://github.com/ecordell/pymacaroons

Yeah, I thought of using jwt for the same thing. Then we'd have a key in mybinder.org-deploy and issue tokens which can validate fields like audience, expiry, etc. I'm not sure what's the best way to go. That seems like possible overkill unless we want to expand granular token-based access to BinderHub.

I've also been thinking about how to really use these. For API access, it's fine (for our CI purposes, for instance), but for testing through a VPN that's forced to be in a blocked origin as discussed in the issue, you have to put the token in the URL. Since the desired use case is "a link to click" it's easy to copy/paste that link somewhere else and for the token to leak. That's not a huge deal for one-time-use tokens, but we are talking about long-lived tokens where leaking is a bigger deal.

A separate authentication link that puts the token in a cookie would be better for that case, but more work because we'd need a login page, persistent cookie secret, etc.

@betatim
Copy link
Member

betatim commented Apr 12, 2021

Ah! I somehow ended up thinking that the key in the dict was the hashed value and that the value was the not-hashed value. So you'd have the secret in there. Re-reading the code and your answer made me realise that the value is a label for the token that has nothing to do with the key.

I was thinking for humans using the token to get around network bans we could maybe set a cookie if a URL with a token in it is visited by the user. Subsequent requests could check the cookie first and use that. So people would only once have to visit binderhub.example.com?token=... to set it and after that they don't have to worry about it any more.

@consideRatio
Copy link
Member

This may be related to #1415.

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

Successfully merging this pull request may close these issues.

3 participants