-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
OpenID Connect support #5757
Open
rawnsley
wants to merge
38
commits into
Hubs-Foundation:master
Choose a base branch
from
LearnHub:avn-oidc-spike
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
OpenID Connect support #5757
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ssary while feature is behind 'show_internal_configs' anyway
Simplified original displayName passthrough
…ver view of account
Closed
# Conflicts: # src/react-components/auth/AuthContext.js # src/react-components/auth/VerifyModalContainer.js
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR and it's reticulum companion are an attempt to complete the OpenID Connect (OIDC) integration started in 2020.
Given the complexity of the changes, I feel we need an endorsement from the Hubs product management team that the feature has value on top of the usual engineering discussion of the code in the pull request.
I would like to invite any other Hubs Cloud users that would find OIDC support useful to join the discussion - the stronger the case we can make, the more likely it is to get integrated.
Value proposition
OIDC on top of OAuth2 is the de-facto standard for SSO in almost all popular web-based authentication and authorization systems. While the Hubs password-free system is elegant, there are environments where it is unacceptable or unwieldy. For instance, corporate entities and educational institutions often mandate the use of their specific identity provider. Also, for some users, email is not a regular method of communication and access may actually be more complicated than with SSO.
We expect authentication and identity within Hubs to evolve. One day we might want to support multiple SSO providers or even adopt a decentralized model like Mastadon. Whatever direction it takes, a robust OIDC implementation will always be required for certain Hubs Cloud owners and this work may be useful to support future identity systems.
Usage
Hubs Cloud owners can enable OIDC as the primary authentication system from the admin page: Setup > App Settings > Auth
Configuration of the OIDC parameters is done here: Setup > App Settings > Advanced (to be confirmed)
They must provide the
openid_configuration
URI of the configuration endpoint.They must provide a
client_id
andclient_secret
that securely identifies them as an authorized relaying party.They may customize the
scopes
andpermitted_claims
fields.They may add
additional_authorization_parameters
, such as the ones supported by Google SSO.From the users point of view, the email login page is replaced with a single customizable Sign In button that opens the appropriate page in a new tab. Once sign-in is complete the user is directed back to the main Hubs page in the same way as with email authentication, but in this case the verification page is closed automatically.
Development notes
These PRs represent a working version of OIDC support with all of the obvious issues addressed. There will be things I have overlooked and I know there will be code suggestions as I'm new to Elixir. All comments welcome!
Original source and the merge
These are the original spikes:
They were used as the starting branch point and the current master branches were merged in. The following notable changes were accounted for on the client side:
On the reticulum side some small changes were required to accommodate differences in function arguments.
Implementation notes
If available, the OIDC UserInfo endpoint is now queried during sign in and the result passed through to the client. As in the original spike, the OIDC
sub
field remains the primary identifier so the client and reticulum can share a common convention.Passing through user info from either the UserInfo endpoint or the id_token allows custom clients to use the fields for other things by adding them to the
scopes
request parameter of their OIDC configuration, which otherwise defaults to the common set of "openid profile email". If there is a required claim outside of the standard claims set, it must be added to thepermitted_claims
parameter list.The convention of referring to the
sub
as email in the client code has been preserved to limit the footprint of the PR even though it will not always be an email.It was felt that passing through a field called displayName from reticulum was a bit confusing as it wasn't the same as any of the Display Name fields in the client and was a hard-coded assumption in reticulum, which is the hardest codebase to change. Instead, all suitable user data is passed through from reticulum as a map in an oidc field that is then mapped to extras on the client side in the credentials store. Adding a single object field makes it clearer that this data is outside of the normal scope of Hubs credentials and it is easy to null it out when resetting the user info.
In the client admin pages, the
show_oidc_configs
development URL parameter was removed from the spike, but the OIDC settings remain behind the generalshow_internal_configs
flag for now. Some sort of visibility control seems sensible here to avoid tripping up existing users.Style wise, I would normally add more comments, but the Hubs codebase is minimalist so I have respected that. Happy to add comments where things are unclear
Accounts and admins
New email account creation is disabled by default in OIDC mode, but users can still sign in with existing email accounts. This allows for the original admin to sign in with their email address even after switching to OIDC mode. To see the sign-in box they need to use a URL parameter
https://{hubs-domain}/signin?show_email_signin
The OIDC
sub
is used as the main identifier within reticulum and in the client. To preserve anonimity, the sub is hashed in the same way as emails are hashed, but to avoid potential clashes with emails or subs from other OIDC providers, the OIDC configuration URI is appended with the sub as follows:https://oidctest.wsweet.org/.well-known/openid-configuration#dwho
One implication of this is that when searching in the Accounts admin page this full string must be used. The OIDC URI could be added automatically, but this might get confusing if searching for the root admin account or other email-based accounts. It might also be confusing if the OIDC URI changes for some reason and you wish to search for a legacy account.
Technically I see no reason why we couldn't support email and OIDC simultaneously (even multiple OIDC providers), but there are some UI challenges and it is unlikely to be a common requirement for OIDC Hubs Cloud owners.
There are valid arguments that including the OIDC string as a prefix to the sub is overkill from a security point of view and I'm happy to have that discussion.
Testing
Testing has mostly been done using the new hubs-compose Docker system.
Testing an actual Hubs Cloud setup is harder for me to do, but is the next stage of my testing process once I figure out how habitat works.
Example account details have been added to
dev.exs
to allow testing against a mock OIDC provider. FYI you can logout from the service from here: https://oidctest.wsweet.org/oauth2/logoutAnother useful tip for testing: if the OIDC verification page takes too long to launch because reticulum is slow (when it is compiling for instance) then Chrome blocks the tab from opening because it no longer looks like a user initiated event.
Limitations
Key rotation
This implementation does not support OIDC key rotation or anything else where the OIDC configuration changes dynamically. If those files change, the reticulum service must be restarted. No reason this could not be supported in the future, but it complicates the code for what is probably an edge case.
OIDC-like systems
There are some authentication systems that are similar enough to OIDC to be easy to support even though they do not strictly follow the OIDC conventions. While the current implementation relies on getting most settings from the configuration endpoint, override parameters for those fields would allow more fine-grained control over these settings for increased flexibility.
Access token passthrough
The access token from the OIDC provider is not stored in reticulum or passed through to the client, which means that it can't be used to access authenticated services. I would not recommend storing it in reticulum, but I can see how passing it through to the client would be useful for custom clients. I can't see any obvious security problems in doing this, but I haven't thought it through completely.
Related
GitHub items
Discord activity