[Generic] support providers that require 'nonce' and return an inlined 'id_token'

[jimdigriz]

N.B. WiP, tests not yet passing, looking for feedback before fixing them, Works For Me(tm)

Portier only validates an identity and does not provide an access_token suitable to send access to resource providers, as such the access_token is literally set to UNUSED.

Portier also requires that nonce is provided so that it is verified in by the authenticator later.

An example response to calling token_url:

{'access_token': 'UNUSED', 'id_token': 'eyJhbGciOiJSUzI1NiIsImtpZCI6ImJfMC1QMTFDZnpISl9OZGtTZzBHdWhlRFptSTNaNnE4Q1JHanJDS000WEkifQ.eyJhdWQiOiJodHRwOi8vbG9jYWxob3N0OjgwODAiLCJlbWFpbCI6ImFsZXhAY29yZW1lbS5jb20iLCJlbWFpbF9vcmlnaW5hbCI6ImFsZXhAY29yZW1lbS5jb20iLCJlbWFpbF92ZXJpZmllZCI6dHJ1ZSwiZXhwIjoxNjY1NTk1MTA4LCJpYXQiOjE2NjU1OTQ1MDgsImlzcyI6Imh0dHBzOi8vYnJva2VyLnBvcnRpZXIuaW8iLCJub25jZSI6IjY5ZjU2YzY3Njk5YmYwMTM2NmZiMDY0MjFlMjY5ZDJkIiwic3ViIjoiYWxleEBjb3JlbWVtLmNvbSJ9.oQtdZaTc_wNT2D6DJjExuIY1VGW_zKN1zzMqj4Eau-PAzItVcVTkss7cbObNq1D0_cPqkHVKcrxjM1G1_mYJMM02MnJ71W-yIamCX5BGL8rjHB5AbKffK9L23AitoQf4x5hjW9FJUzAdgOT_3mDDfv7Cn5N2TKhw794QNe3TXoD2OYvUxtaMbhnXSzuUGh30p78nghazcVp-BS--q9IQmNBEcjMFrN3TFX1Wdo6IFCBN1UZmsfQfoHGbQPjyCKR8yRwGjQUjHrH5Q5hsKpMQQVFHYHwxi93rBX9vvCYo00nUw-l_D5nd2AOGi3bZitRg96wDwOcgVbZHNPbLRGSXow', 'token_type': 'bearer'}

Configuration used to make this work for me:

from oauthenticator.generic import GenericOAuthenticator
c.JupyterHub.authenticator_class = GenericOAuthenticator
c.GenericOAuthenticator.client_id = 'http://localhost:8000'
c.GenericOAuthenticator.scope = [ 'openid', 'email' ]
c.GenericOAuthenticator.oauth_callback_url = 'http://localhost:8000/hub/oauth_callback'
c.GenericOAuthenticator.authorize_url = 'https://broker.portier.io/auth'
c.GenericOAuthenticator.token_url = 'https://broker.portier.io/token'
c.GenericOAuthenticator.username_key = 'email'

[manics]

Is nonce an OpenID Connect feature, or is it generally supported by other OAuth2 providers too?

[jimdigriz]

Is nonce an OpenID Connect feature, or is it generally supported by other OAuth2 providers too?

The spec states REQUIRED, but Real Life(tm) of course can be different.

Happy to change the logic to something more backwards compatible for existing deployments. For example default to (nonce=None) where we always send nonce but only verify it if we see it in the id_token response. Where nonce=True we send and enforce its presence and nonce=False is where we do not send it at all. Maybe you prefer to be 100% backwards compatible and we default to False?

[manics]

The OpenID spec you've linked to says it's required, but this is a Generic OAuth2 authenticator, not an OpenID authenticator, so what's important is how non-OpenID OAuth 2 providers will handle a new nonce parameter.

[jimdigriz]

The OpenID spec you've linked to says it's required, but this is a Generic OAuth2 authenticator, not an OpenID authenticator, so what's important is how non-OpenID OAuth 2 providers will handle a new nonce parameter.

Okay, I do not think it is practical to review all the OAuth2 providers in the universe, so what do you want to do?

Maybe we could consider some of the options I put on the table around backwards compatibility?

[jimdigriz]

Maybe we could consider some of the options I put on the table around backwards compatibility?

Updated so the nonce is always sent and only validated when present in the response.

[consideRatio]

Even though I'm a maintainer of this package I'm not an expert in OAuth2 and OpenID Connect protocol etc. Due to that, I'm not confident from the PR description or code changes if this feature is in scope for the GenericOAuthenticator.

Is supporting this change to support something specified in OAuth2 or OpenID protocols, or is it a custom behavior of the identity provider Portier? If its not part of the OAuth2 or OpenID specs, I'd like to see a thorough motivation that this is a well established practice or similar, because otherwise I find this not to be generic enough or sustainable to maintain.

Reviewing the changes briefly, I saw verify_signature be set to False. If that is a well motivated decision and acceptable from a rigorous security standpoint, it has to be clearly motivated in the code as it seems like a big red flag otherwise.

[jimdigriz]

Even though I'm a maintainer of this package I'm not an expert in OAuth2 and OpenID Connect protocol etc. Due to that, I'm not confident from the PR description or code changes if this feature is in scope for the GenericOAuthenticator.

My recommendation is for you to put your energy into drawing expertise from the community of users and developers working with your project who do have that knowledge. Find people you trust and lean on them. You are only as good as the people around you.

Pulling down the shutters on contributions on the grounds of unfamiliarity is not healthy for a project.

You are, whether desired or not, the guardian of the defacto major method of authenticating against JupyterHub. Blocking contributions here at your project has a direct impact on the deployment options available to the users of JupyterHub.

Personally I submitted my OpenID change as I had written it and thought someone may find value in it. If it makes it into your project great, if it does not, no bother I can continue to carry the fork I am already maintaining.

Others though may appreciate that their user base can now log in with no more than an email address and not have to find/pay/give-all-their-data-to-{google,microsoft,...} to just be able to click a 'login' button.

Is supporting this change to support something specified in OAuth2 or OpenID protocols, or is it a custom behavior of the identity provider Portier? If its not part of the OAuth2 or OpenID specs,

This is part of the OpenID specs.

https://openid.net/specs/openid-connect-core-1_0.html#CodeFlowSteps

None of this is bespoke to Portier, if anything Portier just in my opinion overly strict about implementing to the letter the standard docs; so they reject any request that does not have a nonce. This is understandable as the id_token is returned directly and there needs to be some validation to make sure there is no naughty business (MitM/bad actor) going on.

I'd like to see a thorough motivation that this is a well established practice or similar, because otherwise I find this not to be generic enough or sustainable to maintain.

The nonce functionality is generic but more importantly it is also opportunistic. This means we send the nonce and only check it if the provider returns it as not all will. If it is not present in the response from the provider, nothing changes and the code continues as if we never sent the nonce through to completion.

This is implemented as described in the OpenID spec.

Reviewing the changes briefly, I saw verify_signature be set to False. If that is a well motivated decision and acceptable from a rigorous security standpoint, it has to be clearly motivated in the code as it seems like a big red flag otherwise.

The problem is your project does not support:

• discovery (/.well-known/openid-configuration)
• jwks_uri

This means there are no keys for it to validate against; there is nothing to pass as key to jwt.decode.

At a glance, your Azure provider also sets verify_signature to False for exactly this reason.

I could add the support for this to your project, as well as the unit tests needed for this, but my spidery senses are telling me that even this contribution for non-technical reasons is dead on arrival...

[minrk]

@jimdigriz thanks for your contribution. I'll ask for your patience with maintainers, who have many responsibilities, many of whom maintain this and other repos on a volunteer basis. "Radio silence" mostly means not having time to devote to every repo, and we're doing our best. Friendly pings requesting feedback if you've been waiting are always okay; nobody's pulling down any shutters.

I think there's some confusion in particular around what the base OAuthenticator class is, as OIDC specs have been referenced several times. As @manics pointed out, it is not an OIDC authenticator, so adding OIDC-specific features (jwt, nonce, id_token, etc.) is not really appropriate for that class - it's mostly a base class for further implementation in subclasses. However, what would be in-scope for the oauthenticator package would be a new OpenIDAuthenticator subclass that implements the things OIDC adds to OAuth - nonce, id_token, jwks discovery and validation, etc. as there are a lot of OIDC providers out there.

Commit d547ac7 does appear fix #564, if you'd like to extract that as a separate PR, or we can take care of that if you're not inclined.

[jimdigriz]

I'll ask for your patience with maintainers, who have many responsibilities, many of whom maintain this and other repos on a volunteer basis

The problem is those PRs come from volunteers too and from their end they get to watch the commit history continue to make forward progress whilst everyones PRs are neglected and left by the wayside.

This leaves an awful impression on how the maintainers value contributions.

However, what would be in-scope for the oauthenticator package would be a new OpenIDAuthenticator subclass that implements the things OIDC adds to OAuth - nonce, id_token, jwks discovery and validation, etc. as there are a lot of OIDC providers out there.

Directly from https://github.com/jupyterhub/oauthenticator/blob/main/CONTRIBUTING.md:

"Note: OAuthenticator is not accepting pull requests adding new OAuth providers. See the documentation for how to use GenericOAuthenticator with your provider or to write your own OAuthenticator class for your provider."

This has a direct impact on how contributors build out their PRs to avoid wasting time. This is why I applied the changes to GenericOAuthenticator.

Commit d547ac7 does appear fix #564, if you'd like to extract that as a separate PR, or we can take care of that if you're not inclined.

I'll leave this with the maintainers.