From c5ef5d3661b1dbe23d27dc7cc7a85c350c7f0a5f Mon Sep 17 00:00:00 2001 From: Luiz Carlos Date: Tue, 8 Jun 2021 11:22:21 -0300 Subject: [PATCH 1/4] fix: public applications and request content-type --- .../lib/sign_in/commands/resource_owner.ex | 14 +++++++------- .../lib/identities/schemas/client_application.ex | 2 +- .../rest_api/test/controllers/public/auth_test.exs | 7 +++++++ 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/apps/authenticator/lib/sign_in/commands/resource_owner.ex b/apps/authenticator/lib/sign_in/commands/resource_owner.ex index bfae29d..1c66b6c 100644 --- a/apps/authenticator/lib/sign_in/commands/resource_owner.ex +++ b/apps/authenticator/lib/sign_in/commands/resource_owner.ex @@ -32,7 +32,7 @@ defmodule Authenticator.SignIn.Commands.ResourceOwner do Sign in an user identity by ResouceOnwer flow. The application has to be active, using openid-connect protocol and with access_type - confidential in order to use this flow. + public in order to use this flow. When the client application has a public_key saved on database we force the use of client_assertions on input to avoid passing it's secret open on requests. @@ -46,10 +46,10 @@ defmodule Authenticator.SignIn.Commands.ResourceOwner do {:flow_enabled?, true} <- {:flow_enabled?, "resource_owner" in app.grant_flows}, {:app_active?, true} <- {:app_active?, app.status == "active"}, {:secret_matches?, true} <- {:secret_matches?, secret_matches?(app, input)}, - {:confidential?, true} <- {:confidential?, app.access_type == "confidential"}, {:valid_protocol?, true} <- {:valid_protocol?, app.protocol == "openid-connect"}, {:user, {:ok, user}} <- {:user, Port.get_identity(%{username: username})}, {:user_active?, true} <- {:user_active?, user.status == "active"}, + {:public?, true} <- {:public?, app.access_type == "public"}, {:pass_matches?, true} <- {:pass_matches?, VerifyHash.execute(user, input.password)}, {:ok, access_token, claims} <- generate_access_token(user, app, scope), {:ok, refresh_token, _} <- generate_refresh_token(app, claims), @@ -76,11 +76,6 @@ defmodule Authenticator.SignIn.Commands.ResourceOwner do FakeVerifyHash.execute(:argon2) {:error, :unauthenticated} - {:confidential?, false} -> - Logger.info("Client application #{client_id} is not confidential") - FakeVerifyHash.execute(:argon2) - {:error, :unauthenticated} - {:valid_protocol?, false} -> Logger.info("Client application #{client_id} protocol is not openid-connect") FakeVerifyHash.execute(:argon2) @@ -96,6 +91,11 @@ defmodule Authenticator.SignIn.Commands.ResourceOwner do FakeVerifyHash.execute(:argon2) {:error, :unauthenticated} + {:public?, false} -> + Logger.info("Client application #{client_id} is not public") + FakeVerifyHash.execute(:argon2) + {:error, :unauthenticated} + {:pass_matches?, false} -> Logger.info("User #{username} password do not match any credential") generate_attempt(input, false) diff --git a/apps/resource_manager/lib/identities/schemas/client_application.ex b/apps/resource_manager/lib/identities/schemas/client_application.ex index 8c0d1f2..85147eb 100644 --- a/apps/resource_manager/lib/identities/schemas/client_application.ex +++ b/apps/resource_manager/lib/identities/schemas/client_application.ex @@ -44,7 +44,7 @@ defmodule ResourceManager.Identities.Schemas.ClientApplication do field :status, :string, default: "active" field :blocked_until, :naive_datetime field :protocol, :string, default: "openid-connect" - field :access_type, :string, default: "confidential" + field :access_type, :string, default: "public" field :is_admin, :boolean, default: false field :grant_flows, {:array, :string} field :redirect_uri, :string diff --git a/apps/rest_api/test/controllers/public/auth_test.exs b/apps/rest_api/test/controllers/public/auth_test.exs index 63c8562..4b9f2ce 100644 --- a/apps/rest_api/test/controllers/public/auth_test.exs +++ b/apps/rest_api/test/controllers/public/auth_test.exs @@ -4,6 +4,7 @@ defmodule RestAPI.Controllers.Public.AuthTest do alias Authenticator.SignIn.Inputs.{ClientCredentials, RefreshToken, ResourceOwner} alias RestAPI.Ports.AuthenticatorMock + @content_type "application/x-www-form-urlencoded" @token_endpoint "/api/v1/auth/protocol/openid-connect/token" @logout_endpoint "api/v1/auth/protocol/openid-connect/logout" @logout_all_endpoint "api/v1/auth/protocol/openid-connect/logout-all-sessions" @@ -25,6 +26,7 @@ defmodule RestAPI.Controllers.Public.AuthTest do assert %{"access_token" => _, "refresh_token" => _, "token_type" => _, "expires_in" => _} = conn + |> put_req_header("content-type", @content_type) |> post(@token_endpoint, params) |> json_response(200) end @@ -41,6 +43,7 @@ defmodule RestAPI.Controllers.Public.AuthTest do assert %{"access_token" => _, "refresh_token" => _, "token_type" => _, "expires_in" => _} = conn + |> put_req_header("content-type", @content_type) |> post(@token_endpoint, params) |> json_response(200) end @@ -59,6 +62,7 @@ defmodule RestAPI.Controllers.Public.AuthTest do assert %{"access_token" => _, "refresh_token" => _, "token_type" => _, "expires_in" => _} = conn + |> put_req_header("content-type", @content_type) |> post(@token_endpoint, params) |> json_response(200) end @@ -79,6 +83,7 @@ defmodule RestAPI.Controllers.Public.AuthTest do } } = conn + |> put_req_header("content-type", @content_type) |> post(@token_endpoint, %{"grant_type" => "password"}) |> json_response(400) end @@ -90,6 +95,7 @@ defmodule RestAPI.Controllers.Public.AuthTest do assert %{"response" => %{"refresh_token" => ["can't be blank"]}} = conn + |> put_req_header("content-type", @content_type) |> post(@token_endpoint, %{"grant_type" => "refresh_token"}) |> json_response(400) end @@ -108,6 +114,7 @@ defmodule RestAPI.Controllers.Public.AuthTest do } } = conn + |> put_req_header("content-type", @content_type) |> post(@token_endpoint, %{"grant_type" => "client_credentials"}) |> json_response(400) end From 0868c29fbd86ff7280156d1fafdbf56b5a687ebd Mon Sep 17 00:00:00 2001 From: Luiz Carlos Date: Wed, 9 Jun 2021 08:32:35 -0300 Subject: [PATCH 2/4] feat: add confidential flow and more tests --- .../lib/sign_in/commands/resource_owner.ex | 76 +++++++++++++------ .../sign_in/commands/resource_owner_test.exs | 54 ++++++------- apps/authorizer/coveralls.json | 3 +- apps/resource_manager/test/support/factory.ex | 2 + 4 files changed, 82 insertions(+), 53 deletions(-) diff --git a/apps/authenticator/lib/sign_in/commands/resource_owner.ex b/apps/authenticator/lib/sign_in/commands/resource_owner.ex index 1c66b6c..a6c1c4b 100644 --- a/apps/authenticator/lib/sign_in/commands/resource_owner.ex +++ b/apps/authenticator/lib/sign_in/commands/resource_owner.ex @@ -41,20 +41,16 @@ defmodule Authenticator.SignIn.Commands.ResourceOwner do to avoid exposing identity existance and time attacks. """ @impl true - def execute(%Input{username: username, client_id: client_id, scope: scope} = input) do + def execute(%Input{username: username, client_id: client_id} = input) do with {:app, {:ok, app}} <- {:app, Port.get_identity(%{client_id: client_id})}, {:flow_enabled?, true} <- {:flow_enabled?, "resource_owner" in app.grant_flows}, {:app_active?, true} <- {:app_active?, app.status == "active"}, - {:secret_matches?, true} <- {:secret_matches?, secret_matches?(app, input)}, {:valid_protocol?, true} <- {:valid_protocol?, app.protocol == "openid-connect"}, {:user, {:ok, user}} <- {:user, Port.get_identity(%{username: username})}, {:user_active?, true} <- {:user_active?, user.status == "active"}, - {:public?, true} <- {:public?, app.access_type == "public"}, - {:pass_matches?, true} <- {:pass_matches?, VerifyHash.execute(user, input.password)}, - {:ok, access_token, claims} <- generate_access_token(user, app, scope), - {:ok, refresh_token, _} <- generate_refresh_token(app, claims), - {:ok, _session} <- generate_and_save(input, claims) do - {:ok, parse_response(access_token, refresh_token, claims)} + {:public?, true, _identities} <- {:public?, app.access_type == "public", {user, app}} do + Logger.info("Running public authentication flow for client #{client_id}") + run_public_authentication(user, app, input) else {:app, {:error, :not_found}} -> Logger.info("Client application #{client_id} not found") @@ -71,11 +67,6 @@ defmodule Authenticator.SignIn.Commands.ResourceOwner do FakeVerifyHash.execute(:argon2) {:error, :unauthenticated} - {:secret_matches?, false} -> - Logger.info("Client application #{client_id} credential didn't matches") - FakeVerifyHash.execute(:argon2) - {:error, :unauthenticated} - {:valid_protocol?, false} -> Logger.info("Client application #{client_id} protocol is not openid-connect") FakeVerifyHash.execute(:argon2) @@ -91,15 +82,9 @@ defmodule Authenticator.SignIn.Commands.ResourceOwner do FakeVerifyHash.execute(:argon2) {:error, :unauthenticated} - {:public?, false} -> - Logger.info("Client application #{client_id} is not public") - FakeVerifyHash.execute(:argon2) - {:error, :unauthenticated} - - {:pass_matches?, false} -> - Logger.info("User #{username} password do not match any credential") - generate_attempt(input, false) - {:error, :unauthenticated} + {:public?, false, {user, app}} -> + Logger.info("Running confidential authentication flow for client #{client_id}") + run_confidential_authentication(user, app, input) error -> Logger.error("Failed to run command becuase of unknow error", error: inspect(error)) @@ -125,7 +110,44 @@ defmodule Authenticator.SignIn.Commands.ResourceOwner do end end - def execute(_any), do: {:error, :invalid_params} + defp run_public_authentication(user, app, input) do + if VerifyHash.execute(user, input.password) do + user + |> generate_tokens(app, input) + |> parse_response() + else + Logger.info("User #{user.username} password do not match any credential") + generate_attempt(input, false) + {:error, :unauthenticated} + end + end + + defp run_confidential_authentication(user, app, input) do + with {:secret_matches?, true} <- {:secret_matches?, secret_matches?(app, input)}, + {:pass_matches?, true} <- {:pass_matches?, VerifyHash.execute(user, input.password)} do + user + |> generate_tokens(app, input) + |> parse_response() + else + {:secret_matches?, false} -> + Logger.info("Client application #{app.client_id} credential didn't matches") + FakeVerifyHash.execute(:argon2) + {:error, :unauthenticated} + + {:pass_matches?, false} -> + Logger.info("User #{user.username} password do not match any credential") + generate_attempt(input, false) + {:error, :unauthenticated} + end + end + + defp generate_tokens(user, app, input) do + with {:ok, access_token, claims} <- generate_access_token(user, app, input.scope), + {:ok, refresh_token, _} <- generate_refresh_token(app, claims), + {:ok, _session} <- generate_and_save(input, claims) do + {:ok, access_token, refresh_token, claims} + end + end defp secret_matches?(%{client_id: id, public_key: public_key}, %{client_assertion: assertion}) when is_binary(assertion) do @@ -223,12 +245,16 @@ defmodule Authenticator.SignIn.Commands.ResourceOwner do }) end - defp parse_response(access_token, refresh_token, %{"ttl" => ttl, "typ" => typ}) do - %{ + defp parse_response({:ok, access_token, refresh_token, %{"ttl" => ttl, "typ" => typ}}) do + payload = %{ access_token: access_token, refresh_token: refresh_token, expires_in: ttl, token_type: typ } + + {:ok, payload} end + + defp parse_response({:error, _reason} = error), do: error end diff --git a/apps/authenticator/test/authenticator/sign_in/commands/resource_owner_test.exs b/apps/authenticator/test/authenticator/sign_in/commands/resource_owner_test.exs index b016575..4358656 100644 --- a/apps/authenticator/test/authenticator/sign_in/commands/resource_owner_test.exs +++ b/apps/authenticator/test/authenticator/sign_in/commands/resource_owner_test.exs @@ -128,7 +128,7 @@ defmodule Authenticator.SignIn.Commands.ResourceOwnerTest do test "succeeds using client_assertions and generates an access_token" do scopes = RF.insert_list!(:scope, 3) user = RF.insert!(:user) - app = RF.insert!(:client_application) + app = RF.insert!(:client_application, access_type: "confidential") public_key = RF.insert!(:public_key, client_application: app, value: get_public_key()) hash = RF.gen_hashed_password("MyPassw@rd234") password = RF.insert!(:password, user: user, password_hash: hash) @@ -176,7 +176,13 @@ defmodule Authenticator.SignIn.Commands.ResourceOwnerTest do test "succeeds using client_assertions and generates a refresh_token" do scopes = RF.insert_list!(:scope, 3) user = RF.insert!(:user) - app = RF.insert!(:client_application, grant_flows: ["resource_owner", "refresh_token"]) + + app = + RF.insert!(:client_application, + access_type: "confidential", + grant_flows: ["resource_owner", "refresh_token"] + ) + public_key = RF.insert!(:public_key, client_application: app, value: get_public_key()) hash = RF.gen_hashed_password("MyPassw@rd234") password = RF.insert!(:password, user: user, password_hash: hash) @@ -222,10 +228,20 @@ defmodule Authenticator.SignIn.Commands.ResourceOwnerTest do end test "fails if params are invalid" do - assert {:error, :invalid_params} == Command.execute(%{}) - assert {:error, changeset} = Command.execute(%{grant_type: "password"}) + assert %{ + client_assertion_type: ["can't be blank"], + client_assertion: ["can't be blank"], + username: ["can't be blank"], + password: ["can't be blank"], + client_id: ["can't be blank"], + ip_address: ["can't be blank"], + scope: ["can't be blank"] + } = errors_on(changeset) + + assert {:error, changeset} = Command.execute(%{"grant_type" => "password"}) + assert %{ client_assertion_type: ["can't be blank"], client_assertion: ["can't be blank"], @@ -290,37 +306,21 @@ defmodule Authenticator.SignIn.Commands.ResourceOwnerTest do end test "fails if client application secret do not match credential" do - input = %{ - username: "my-username", - password: "MyPassw@rd234", - grant_type: "password", - scope: "admin:read", - ip_address: "45.232.192.12", - client_id: Ecto.UUID.generate(), - client_secret: Ecto.UUID.generate() - } - - expect(ResourceManagerMock, :get_identity, fn _ -> - {:ok, RF.insert!(:client_application, secret: "another-secret")} - end) - - assert {:error, :unauthenticated} == Command.execute(input) - end + user = RF.insert!(:user) + app = RF.insert!(:client_application, access_type: "confidential", secret: "another-secret") - test "fails if client application is not confidential" do input = %{ - username: "my-username", + username: user.username, password: "MyPassw@rd234", grant_type: "password", scope: "admin:read", ip_address: "45.232.192.12", client_id: Ecto.UUID.generate(), - client_secret: "my-secret" + client_secret: Ecto.UUID.generate() } - expect(ResourceManagerMock, :get_identity, fn _ -> - {:ok, RF.insert!(:client_application, access_type: "public")} - end) + expect(ResourceManagerMock, :get_identity, fn _ -> {:ok, app} end) + expect(ResourceManagerMock, :get_identity, fn _ -> {:ok, user} end) assert {:error, :unauthenticated} == Command.execute(input) end @@ -410,7 +410,7 @@ defmodule Authenticator.SignIn.Commands.ResourceOwnerTest do end) expect(ResourceManagerMock, :get_identity, fn %{username: _} -> - user = RF.insert!(:user, status: "blocked") + user = RF.insert!(:user) hash = RF.gen_hashed_password("AnotherPassword") password = RF.insert!(:password, user: user, password_hash: hash) {:ok, %{user | password: password}} diff --git a/apps/authorizer/coveralls.json b/apps/authorizer/coveralls.json index ce7927a..173a9c6 100644 --- a/apps/authorizer/coveralls.json +++ b/apps/authorizer/coveralls.json @@ -7,6 +7,7 @@ "skip_files": [ "test/support/data_case.ex", "test/support/factory.ex", - "lib/ports/resource_manager.ex" + "lib/ports/resource_manager.ex", + "lib/authorizer.ex" ] } \ No newline at end of file diff --git a/apps/resource_manager/test/support/factory.ex b/apps/resource_manager/test/support/factory.ex index d13ab9b..771f4fb 100644 --- a/apps/resource_manager/test/support/factory.ex +++ b/apps/resource_manager/test/support/factory.ex @@ -25,6 +25,8 @@ defmodule ResourceManager.Factory do description: "It's a test application", grant_flows: ["resource_owner"], status: "active", + protocol: "openid-connect", + access_type: "public", is_admin: false, secret: gen_hashed_password(Ecto.UUID.generate(), :bcrypt) } From 784acaa22359e003097045ef556fb2c16c38d69e Mon Sep 17 00:00:00 2001 From: Luiz Carlos Date: Wed, 9 Jun 2021 08:42:16 -0300 Subject: [PATCH 3/4] chore: update moduledoc --- apps/authenticator/lib/sign_in/commands/resource_owner.ex | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/apps/authenticator/lib/sign_in/commands/resource_owner.ex b/apps/authenticator/lib/sign_in/commands/resource_owner.ex index a6c1c4b..a9594e1 100644 --- a/apps/authenticator/lib/sign_in/commands/resource_owner.ex +++ b/apps/authenticator/lib/sign_in/commands/resource_owner.ex @@ -5,8 +5,9 @@ defmodule Authenticator.SignIn.Commands.ResourceOwner do With the resource owner password credentials grant type, the user provides their username and password directly and we uses it to authenticates then. - The Client application should pass their secret (or client assertion) in order to be authorized - to exchange the credentials for an access_token. + When dealing with public applications we cannot ensure that the secret is safe so + we diden't request it unlike confidential applications where we can request a secret + or an client_assertion. When a public key is registered for the client application this flow will require that an assertion is passed instead of the raw secret to avoid sending it on requests. From 33d14f94cfb31a52978b4d332f6dad6d66cb4dc8 Mon Sep 17 00:00:00 2001 From: Luiz Carlos Date: Wed, 9 Jun 2021 08:45:02 -0300 Subject: [PATCH 4/4] chore: update function docs --- apps/authenticator/lib/sign_in/commands/resource_owner.ex | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/apps/authenticator/lib/sign_in/commands/resource_owner.ex b/apps/authenticator/lib/sign_in/commands/resource_owner.ex index a9594e1..e2d6c3e 100644 --- a/apps/authenticator/lib/sign_in/commands/resource_owner.ex +++ b/apps/authenticator/lib/sign_in/commands/resource_owner.ex @@ -32,8 +32,7 @@ defmodule Authenticator.SignIn.Commands.ResourceOwner do @doc """ Sign in an user identity by ResouceOnwer flow. - The application has to be active, using openid-connect protocol and with access_type - public in order to use this flow. + The application has to be active, using openid-connect in order to use this flow. When the client application has a public_key saved on database we force the use of client_assertions on input to avoid passing it's secret open on requests.