-
Notifications
You must be signed in to change notification settings - Fork 180
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
base: master
Are you sure you want to change the base?
OpenID Connect Login #426
Changes from all commits
4df2c3e
4251e2f
6b10355
b4a5951
9239a47
59e3edc
5f507f5
888555b
1a6339c
a645f24
60aa6d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -299,6 +299,23 @@ realm = "{{ cfg.turn.realm }}" | |
public_tls_ports = "{{ cfg.turn.public_tls_ports }}" | ||
{{/if}} | ||
|
||
{{#if cfg.oidc }} | ||
{{#with cfg.oidc }} | ||
[ret."Elixir.RetWeb.OIDCAuthChannel"] | ||
auth_endpoint = {{ toToml auth_endpoint }} | ||
token_endpoint = {{ toToml token_endpoint }} | ||
scopes = {{ toToml scopes }} | ||
client_id = {{ toToml client_id }} | ||
client_secret = {{ toToml client_secret }} | ||
|
||
[ret."Elixir.Ret.RemoteOIDCToken"] | ||
allowed_algos = {{ toToml allowed_algos }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
verification_secret_jwk_set = """ | ||
{{ verification_secret_jwk_set }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
""" | ||
{{/with}} | ||
{{/if}} | ||
|
||
[web_push_encryption.vapid_details] | ||
subject = "{{ cfg.web_push.subject }}" | ||
public_key = "{{ cfg.web_push.public_key }}" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
defmodule Ret.RemoteOIDCToken do | ||
@moduledoc """ | ||
This represents an OpenID Connect token returned from a remote service. | ||
These tokens are never created locally, only ever provided externally and verified locally. | ||
""" | ||
use Guardian, | ||
otp_app: :ret, | ||
secret_fetcher: Ret.RemoteOIDCTokenSecretsFetcher | ||
|
||
def subject_for_token(_, _), do: {:ok, nil} | ||
def resource_from_claims(_), do: {:ok, nil} | ||
end | ||
|
||
defmodule Ret.RemoteOIDCTokenSecretsFetcher do | ||
@moduledoc """ | ||
This represents the public keys for an OpenID Connect endpoint used to verify tokens. | ||
The public keys will be configured by an admin for a particular setup. These can not be used for signing. | ||
""" | ||
|
||
def fetch_signing_secret(_mod, _opts) do | ||
{:error, :not_implemented} | ||
end | ||
|
||
def fetch_verifying_secret(mod, %{"kid" => kid, "typ" => "JWT"}, _opts) do | ||
# TODO implement read through cache that hits discovery endpoint instead of hardcoding keys in config as per https://openid.net/specs/openid-connect-core-1_0.html#RotateSigKeys | ||
case Application.get_env(:ret, mod)[:verification_secret_jwk_set] | ||
|> Poison.decode!() | ||
|> Map.get("keys") | ||
|> Enum.find(&(Map.get(&1, "kid") == kid)) do | ||
nil -> {:error, :invalid_key_id} | ||
key -> {:ok, key |> JOSE.JWK.from_map()} | ||
end | ||
end | ||
|
||
def fetch_verifying_secret(_mod, _token_headers_, _optss) do | ||
{:error, :invalid_token} | ||
end | ||
end |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,172 @@ | ||||||
defmodule RetWeb.OIDCAuthChannel do | ||||||
@moduledoc "Ret Web Channel for OpenID Connect Authentication" | ||||||
|
||||||
use RetWeb, :channel | ||||||
import Canada, only: [can?: 2] | ||||||
|
||||||
alias Ret.{Statix, Account, OAuthToken, RemoteOIDCToken} | ||||||
|
||||||
intercept(["auth_credentials"]) | ||||||
|
||||||
def join("oidc:" <> _topic_key, _payload, socket) do | ||||||
# Expire channel in 5 minutes | ||||||
Process.send_after(self(), :channel_expired, 60 * 1000 * 5) | ||||||
|
||||||
# Rate limit joins to reduce attack surface | ||||||
:timer.sleep(500) | ||||||
|
||||||
Statix.increment("ret.channels.oidc.joins.ok") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||
{:ok, %{session_id: socket.assigns.session_id}, socket} | ||||||
end | ||||||
|
||||||
defp module_config(key) do | ||||||
Application.get_env(:ret, __MODULE__)[key] | ||||||
end | ||||||
|
||||||
defp get_redirect_uri(), do: RetWeb.Endpoint.url() <> "/verify" | ||||||
netpro2k marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
defp get_authorize_url(state, nonce) do | ||||||
"#{module_config(:auth_endpoint)}?" <> | ||||||
URI.encode_query(%{ | ||||||
response_type: "code", | ||||||
response_mode: "query", | ||||||
client_id: module_config(:client_id), | ||||||
scope: "openid profile", | ||||||
state: state, | ||||||
nonce: nonce, | ||||||
redirect_uri: get_redirect_uri() | ||||||
}) | ||||||
end | ||||||
|
||||||
def handle_in("auth_request", _payload, socket) do | ||||||
if Map.get(socket.assigns, :nonce) do | ||||||
{:reply, {:error, "Already started an auth request on this session"}, socket} | ||||||
else | ||||||
"oidc:" <> topic_key = socket.topic | ||||||
oidc_state = Ret.OAuthToken.token_for_oidc_request(topic_key, socket.assigns.session_id) | ||||||
nonce = SecureRandom.uuid() | ||||||
authorize_url = get_authorize_url(oidc_state, nonce) | ||||||
|
||||||
socket = socket |> assign(:nonce, nonce) | ||||||
|
||||||
{:reply, {:ok, %{authorize_url: authorize_url}}, socket} | ||||||
end | ||||||
end | ||||||
|
||||||
def handle_in("auth_verified", %{"token" => code, "payload" => state}, socket) do | ||||||
Process.send_after(self(), :close_channel, 1000 * 5) | ||||||
|
||||||
# Slow down any brute force attacks | ||||||
:timer.sleep(500) | ||||||
|
||||||
"oidc:" <> expected_topic_key = socket.topic | ||||||
|
||||||
with {:ok, | ||||||
%{ | ||||||
"topic_key" => topic_key, | ||||||
"session_id" => session_id, | ||||||
"aud" => "ret_oidc" | ||||||
}} | ||||||
when topic_key == expected_topic_key <- OAuthToken.decode_and_verify(state), | ||||||
{:ok, | ||||||
%{ | ||||||
"access_token" => _access_token, | ||||||
"id_token" => raw_id_token | ||||||
}} <- fetch_oidc_tokens(code), | ||||||
{:ok, | ||||||
%{ | ||||||
"aud" => _aud, | ||||||
"nonce" => nonce, | ||||||
"sub" => remote_user_id | ||||||
} = id_token} <- RemoteOIDCToken.decode_and_verify(raw_id_token) do | ||||||
# TODO we may want to verify some more fields like issuer and expiration time | ||||||
|
||||||
displayname = | ||||||
id_token | ||||||
|> Map.get( | ||||||
"preferred_username", | ||||||
id_token |> Map.get("name", remote_user_id) | ||||||
) | ||||||
|
||||||
broadcast_credentials_and_payload( | ||||||
remote_user_id, | ||||||
%{displayName: displayname}, | ||||||
%{session_id: session_id, nonce: nonce}, | ||||||
socket | ||||||
) | ||||||
|
||||||
{:reply, :ok, socket} | ||||||
else | ||||||
# intentionally not exposing the nature of the error, can uncomment this to return more details to the client | ||||||
# {:error, error} -> | ||||||
# {:reply, {:error, %{message: error}}, socket} | ||||||
|
||||||
_ -> | ||||||
{:reply, {:error, %{message: "error fetching or verifying token"}}, socket} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add a Statix error metric here? |
||||||
end | ||||||
end | ||||||
|
||||||
def handle_in(_event, _payload, socket) do | ||||||
{:noreply, socket} | ||||||
end | ||||||
|
||||||
def fetch_oidc_tokens(oauth_code) do | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
body = | ||||||
{:form, | ||||||
[ | ||||||
client_id: module_config(:client_id), | ||||||
client_secret: module_config(:client_secret), | ||||||
grant_type: "authorization_code", | ||||||
redirect_uri: get_redirect_uri(), | ||||||
code: oauth_code, | ||||||
scope: "openid profile" | ||||||
]} | ||||||
|
||||||
headers = [{"content-type", "application/x-www-form-urlencoded"}] | ||||||
|
||||||
case Ret.HttpUtils.retry_post_until_success("#{module_config(:token_endpoint)}", body, headers) do | ||||||
%HTTPoison.Response{body: body} -> body |> Poison.decode() | ||||||
_ -> {:error, "Failed to fetch tokens"} | ||||||
end | ||||||
end | ||||||
|
||||||
def handle_info(:close_channel, socket) do | ||||||
GenServer.cast(self(), :close) | ||||||
{:noreply, socket} | ||||||
end | ||||||
|
||||||
def handle_info(:channel_expired, socket) do | ||||||
GenServer.cast(self(), :close) | ||||||
{:noreply, socket} | ||||||
end | ||||||
|
||||||
# Only send creddentials back down to the original socket that started the request | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo
Suggested change
|
||||||
def handle_out( | ||||||
"auth_credentials" = event, | ||||||
%{credentials: credentials, user_info: user_info, verification_info: verification_info}, | ||||||
socket | ||||||
) do | ||||||
Process.send_after(self(), :close_channel, 1000 * 5) | ||||||
|
||||||
if Map.get(socket.assigns, :session_id) == Map.get(verification_info, :session_id) and | ||||||
Map.get(socket.assigns, :nonce) == Map.get(verification_info, :nonce) do | ||||||
push(socket, event, %{credentials: credentials, user_info: user_info}) | ||||||
end | ||||||
|
||||||
{:noreply, socket} | ||||||
end | ||||||
|
||||||
defp broadcast_credentials_and_payload(nil, _user_info, _verification_info, _socket), do: nil | ||||||
|
||||||
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() | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||||||
|
||||||
broadcast!(socket, "auth_credentials", %{ | ||||||
credentials: credentials, | ||||||
user_info: user_info, | ||||||
verification_info: verification_info | ||||||
}) | ||||||
end | ||||||
end |
There was a problem hiding this comment.
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.