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
17 changes: 17 additions & 0 deletions habitat/config/config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
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.

client_id = {{ toToml client_id }}
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?

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.

"""
{{/with}}
{{/if}}

[web_push_encryption.vapid_details]
subject = "{{ cfg.web_push.subject }}"
public_key = "{{ cfg.web_push.public_key }}"
Expand Down
14 changes: 14 additions & 0 deletions lib/ret/oauth_token.ex
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,20 @@ defmodule Ret.OAuthToken do

token
end

def token_for_oidc_request(topic_key, session_id) do
{:ok, token, _claims} =
Ret.OAuthToken.encode_and_sign(
# OAuthTokens do not have a resource associated with them
nil,
%{topic_key: topic_key, session_id: session_id, aud: :ret_oidc},
allowed_algos: ["HS512"],
ttl: {10, :minutes},
allowed_drift: 60 * 1000
)

token
end
end

defmodule Ret.OAuthTokenSecretFetcher do
Expand Down
38 changes: 38 additions & 0 deletions lib/ret/remote_oidc_token.ex
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
172 changes: 172 additions & 0 deletions lib/ret_web/channels/oidc_auth_channel.ex
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")
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.

{: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}
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?

end
end

def handle_in(_event, _payload, socket) do
{: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?

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
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

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()
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.


broadcast!(socket, "auth_credentials", %{
credentials: credentials,
user_info: user_info,
verification_info: verification_info
})
end
end
1 change: 1 addition & 0 deletions lib/ret_web/channels/session_socket.ex
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ defmodule RetWeb.SessionSocket do
channel("hub:*", RetWeb.HubChannel)
channel("link:*", RetWeb.LinkChannel)
channel("auth:*", RetWeb.AuthChannel)
channel("oidc:*", RetWeb.OIDCAuthChannel)

def id(socket) do
"session:#{socket.assigns.session_id}"
Expand Down