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

OpenID Connect Login #426

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

OpenID Connect Login #426

wants to merge 11 commits into from

Conversation

netpro2k
Copy link
Contributor

@netpro2k netpro2k commented Nov 2, 2020

This implements the basic pieces needed for OpenID Connect login. With this and some client changes we should be able to roll out enough to at least test this as a login method on Hubs Cloud. Before rolling it out more widely we would likely need to figure out what we want to do around edge cases, like switching login providers when accounts exist, how to setup the initial admin account when using a third party provider. (this later part could also simplify the setup process for Hubs Cloud since it would get rid of the email sending requirement, but also probably means a CloudFormation template update).

Some of the layers of security here feel a bit redundant, but maybe that's ok. The session_id on a socket is already secured by a token, which guarantees the only socket getting a token back is the one that started the request, so encoding the state as a JWT is probably overkill, but we already had the mechanism in place, and it does provide an added layer including additional replay protection due to the short lived timestamp on the token. The nonce is part of the openid spec and confirms that the id token you get back is in fact for the same request that started it (since the id server will only accept this nonce once).

Currently we rely on the user manually keeping the public signing keys up to date on their instance. We eventually would want to read-through cache this.

Accompanying client PR: Hubs-Foundation/hubs#3331

If you like videos, and already understand OAuth pretty well, the last 10 minutes of this talk is a good quick primer on OpenID Connect (the rest of the talk is also a decent into to OAuth) https://youtu.be/996OiexHze0?t=2820

@netpro2k
Copy link
Contributor Author

netpro2k commented Nov 2, 2020

bit weird that remote_username is sent as "email"

Currently this was just to make the client code simpler since it matches the response for the standard login method (the verify message also ends up being the same here as well, so the only thing that changes is which auth channel to connect to. This is probably not an important property to keep, but just made prototyping it easier)

Copy link
Contributor

@brianpeiris brianpeiris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good so far. Would definitely like to do another review when you've got the full flow working.

lib/ret/remote_oidc_token.ex Outdated Show resolved Hide resolved
lib/ret_web/channels/oidc_auth_channel.ex Show resolved Hide resolved
lib/ret_web/channels/oidc_auth_channel.ex Outdated Show resolved Hide resolved
lib/ret_web/channels/oidc_auth_channel.ex Outdated Show resolved Hide resolved
lib/ret_web/channels/oidc_auth_channel.ex Outdated Show resolved Hide resolved
@netpro2k netpro2k marked this pull request as ready for review December 2, 2020 01:15
Copy link
Contributor

@brianpeiris brianpeiris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few comments. Looks good, mostly.

[ret."Elixir.RetWeb.OIDCAuthChannel"]
auth_endpoint = {{ toToml auth_endpoint }}
token_endpoint = {{ toToml token_endpoint }}
scopes = {{ toToml scopes }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the scopes config isn't actually used. Which seems right, I don't think it makes sense for the scopes to be configurable.

[ret."Elixir.Ret.RemoteOIDCToken"]
allowed_algos = {{ toToml allowed_algos }}
verification_secret_jwk_set = """
{{ verification_secret_jwk_set }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to find a place to document what format this "JWK set" should be in.

client_secret = {{ toToml client_secret }}

[ret."Elixir.Ret.RemoteOIDCToken"]
allowed_algos = {{ toToml allowed_algos }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allowed_algos is also not used?

# Rate limit joins to reduce attack surface
:timer.sleep(500)

Statix.increment("ret.channels.oidc.joins.ok")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the intention with this metric? Doesn't seem particularly useful on its own.

# {:reply, {:error, %{message: error}}, socket}

_ ->
{:reply, {:error, %{message: "error fetching or verifying token"}}, socket}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a Statix error metric here?

{:noreply, socket}
end

def fetch_oidc_tokens(oauth_code) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defp?

{:noreply, socket}
end

# Only send creddentials back down to the original socket that started the request
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

Suggested change
# Only send creddentials back down to the original socket that started the request
# Only send credentials back down to the original socket that started the request

defp broadcast_credentials_and_payload(identifier_hash, user_info, verification_info, socket) do
account_creation_enabled = can?(nil, create_account(nil))
account = identifier_hash |> Account.account_for_login_identifier_hash(account_creation_enabled)
credentials = account |> Account.credentials_for_account()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took me a while to think out the implications of account creation being disabled. I guess the expectation is that users are still able to access the site/room, but are not actually "logged in" if site-wide account creation is disabled?
Might want to explain that out here in a comment.

@tolen23
Copy link

tolen23 commented May 31, 2021

May I ask if you can give a rough timeline for this ticket to be finished? It seems that there is not much left to do. Adding OpenID Connect support would be a great enhancement in my opinion.

@tolen23
Copy link

tolen23 commented May 31, 2022

One year has passed :-)

Are there any updates on this?

@camelgod
Copy link

Is there anything the community can do to speed this up? Let us know!!

@rawnsley
Copy link

This is the single most important feature request for us as well. I would have rolled up my sleeves and implemented it myself by now if it wasn't for the fact that Elixir is so opaque and reticulum server deployments aren't easy to patch in Hubs Cloud. It really needs somebody from the core Hubs team to take pity on us :-)

Cross linking to the original issue.

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

Successfully merging this pull request may close these issues.

5 participants