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

adds GatewayClient.auth_scheme configurable #529

Conversation

telamonian
Copy link
Contributor

@telamonian telamonian commented May 28, 2021

This lets you configure the "<type>" string that gets added (along with a space) to the front of the auth token in the header of all HTTP requests made to a gateway.

Technically, the current default ("token") is invalid with respect to the authorization header spec, but I kept it for backwards compatibility: https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication#authentication_schemes

@welcome
Copy link

welcome bot commented May 28, 2021

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@telamonian telamonian force-pushed the make-auth-token-prefix-configurable branch from acf1663 to e135fd6 Compare May 28, 2021 22:46
@codecov-commenter
Copy link

codecov-commenter commented May 28, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@40c477b). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #529   +/-   ##
=========================================
  Coverage          ?   76.30%           
=========================================
  Files             ?      110           
  Lines             ?     9814           
  Branches          ?     1067           
=========================================
  Hits              ?     7489           
  Misses            ?     1944           
  Partials          ?      381           
Impacted Files Coverage Δ
jupyter_server/gateway/gateway_client.py 75.27% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40c477b...e135fd6. Read the comment docs.

@kevin-bates
Copy link
Member

Hi @telamonian - thank you for this pull request - it looks good to me.

For my own education (as I know next to zero about this), I'm curious how the current Authorization: token {token} header works at all since I'm not finding this "to spec" as you mentioned? Its been this way since NB2KG was introduced in 2016. Is there tolerance to a scheme of token that may have previously existed? (Btw, I believe it's the correct decision to leave as-is for now - thanks.)

Based on the link you sent (thank you!) and some other poking around I've done the last couple of hours, it seems like Bearer is probably what we ultimately want since that appears to be the authorization type that most closely resembles what we have. Is that correct?

The closest thing I could find relative to the token {token} syntax is rfc7235 which was introduced in 2014, so the timeframe "fits". Any insights on this would be greatly appreciated.

I'm approving this but will give others the weekend to take a look as well. Thanks again!

@telamonian
Copy link
Contributor Author

telamonian commented May 31, 2021

For the the token {token} syntax, I think the reason it works is that, unlike HTTP requests that originate or terminate in a browser, very little (none?) validation takes place w.r.t. the request headers for tornado <-> tornado comms. tornado leaves it up to us as to how to implement/interpret any particular value of the Authorization header in a handler, the prepare method, etc. The real reason why I need this change is precisely because I'm trying to plug into an existing in-house implementation of tornado auth/z that explicitly assumes the use of the Bearer {token} format.

@telamonian
Copy link
Contributor Author

telamonian commented May 31, 2021

As for the default in the future, Bearer makes sense to me, since that's literally what I'm currently hacking into my existing deployment. From rfc6750:

Bearer Token
A security token with the property that any party in possession of
the token (a "bearer") can use the token in any way that any other
party in possession of it can. Using a bearer token does not
require a bearer to prove possession of cryptographic key material

(proof-of-possession).

The italics above are mine. The point is, this is a matter of two python backend servers (juptyer_server and gateway) communicating with each other without direct user interaction, with no chance in this exchange for a user to provide credentials, keys, whatever. Presumably then, those credentials must therefore have been supplied earlier in exchange for the token. This fits perfectly with the currently implemented flow, which really only allows for a static token to be supplied once at jupyter_server startup via the GatewayClient.auth_token configurable.

@telamonian
Copy link
Contributor Author

The Bearer scheme is also something that can be documented as part of an openapi 3.0 spec: https://swagger.io/docs/specification/authentication/bearer-authentication/

@kevin-bates
Copy link
Member

Excellent. Thank you for the great explanation Max. I'm super happy it matches my shallow understanding!
I'll merge this tomorrow morning when I get in.

@Zsailer Zsailer merged commit f29bb11 into jupyter-server:master Jun 1, 2021
@welcome
Copy link

welcome bot commented Jun 1, 2021

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@blink1073 blink1073 added this to the 1.9 milestone Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants