From cce70cf552662ea6fe8a8f36c3d6ddb0ec49f2d1 Mon Sep 17 00:00:00 2001 From: Andrew Dryga Date: Mon, 23 Jan 2023 15:28:25 -0600 Subject: [PATCH] Fix SAML regression (#1371) Regression was introduced in #1350: the path order did not allow auth callback to be ever called. Additionally, acceptance tests for SAML are added and we added a blacklist of SAML config ids to prevent further route collisions. Fixes #1362 --- .github/workflows/test.yml | 12 ++++++ .../configuration/saml_identity_provider.ex | 8 ++++ .../controllers/auth_controller.ex | 11 ++++++ apps/fz_http/lib/fz_http_web/router.ex | 37 ++++++++++--------- .../acceptance/authentication_test.exs | 36 +++++++++++++++++- apps/fz_http/test/support/acceptance_case.ex | 2 +- .../support/acceptance_case/simple_saml.ex | 29 +++++++++++++++ docker-compose.yml | 16 ++++++++ 8 files changed, 131 insertions(+), 20 deletions(-) create mode 100644 apps/fz_http/test/support/acceptance_case/simple_saml.ex diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 28754e0e6..557c01642 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -290,6 +290,18 @@ jobs: - 8200:8200/tcp options: --cap-add=IPC_LOCK + saml-idp: + image: vihangk1/docker-test-saml-idp:latest + env: + SIMPLESAMLPHP_SP_ENTITY_ID: 'urn:firezone.dev:firezone-app' + SIMPLESAMLPHP_SP_ASSERTION_CONSUMER_SERVICE: 'http://localhost:4002/auth/saml/sp/consume/mysamlidp' + SIMPLESAMLPHP_SP_SINGLE_LOGOUT_SERVICE: 'http://localhost:4002/auth/saml/sp/logout/mysamlidp' + SIMPLESAMLPHP_SP_NAME_ID_FORMAT: 'urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress' + SIMPLESAMLPHP_SP_NAME_ID_ATTRIBUTE: 'email' + SIMPLESAMLPHP_IDP_AUTH: 'example-userpass' + ports: + - 8400:8080/tcp + - 8443:8443/tcp steps: - uses: nanasess/setup-chromedriver@v1 - run: | diff --git a/apps/fz_http/lib/fz_http/configurations/configuration/saml_identity_provider.ex b/apps/fz_http/lib/fz_http/configurations/configuration/saml_identity_provider.ex index 6af14b2a0..2f3ca5113 100644 --- a/apps/fz_http/lib/fz_http/configurations/configuration/saml_identity_provider.ex +++ b/apps/fz_http/lib/fz_http/configurations/configuration/saml_identity_provider.ex @@ -5,6 +5,12 @@ defmodule FzHttp.Configurations.Configuration.SAMLIdentityProvider do use FzHttp, :schema import Ecto.Changeset + @reserved_config_ids [ + "identity", + "saml", + "magic_link" + ] + @primary_key false embedded_schema do field :id, :string @@ -40,6 +46,8 @@ defmodule FzHttp.Configurations.Configuration.SAMLIdentityProvider do ]) |> FzHttp.Validator.validate_uri(:base_url) |> validate_metadata() + # Don't allow users to enter reserved config ids + |> validate_exclusion(:id, @reserved_config_ids) end def validate_metadata(changeset) do diff --git a/apps/fz_http/lib/fz_http_web/controllers/auth_controller.ex b/apps/fz_http/lib/fz_http_web/controllers/auth_controller.ex index 0fe134075..249653433 100644 --- a/apps/fz_http/lib/fz_http_web/controllers/auth_controller.ex +++ b/apps/fz_http/lib/fz_http_web/controllers/auth_controller.ex @@ -58,6 +58,17 @@ defmodule FzHttpWeb.AuthController do with {:ok, user} <- UserFromAuth.find_or_create(:saml, idp, %{"email" => assertion.subject.name}) do do_sign_in(conn, user, %{provider: idp}) + else + {:error, %{errors: [email: {"is invalid email address", _metadata}]}} -> + conn + |> put_flash( + :error, + "SAML provider did not return a valid email address in `name` assertion" + ) + |> redirect(to: ~p"/") + + other -> + other end end diff --git a/apps/fz_http/lib/fz_http_web/router.ex b/apps/fz_http/lib/fz_http_web/router.ex index 71c53a71d..519d105e8 100644 --- a/apps/fz_http/lib/fz_http_web/router.ex +++ b/apps/fz_http/lib/fz_http_web/router.ex @@ -52,6 +52,25 @@ defmodule FzHttpWeb.Router do plug FzHttpWeb.Plug.SamlyTargetUrl end + # Local auth routes + scope "/auth", FzHttpWeb do + pipe_through [ + :browser, + :html_auth, + :require_unauthenticated, + :require_local_auth + ] + + get "/reset_password", AuthController, :reset_password + post "/magic_link", AuthController, :magic_link + get "/magic/:user_id/:token", AuthController, :magic_sign_in + + get "/:provider", AuthController, :request + get "/:provider/callback", AuthController, :callback + post "/:provider/callback", AuthController, :callback + end + + # XXX: Those routes conflict with the ones above, we must change them in 1.0 # OIDC auth routes scope "/auth/oidc", FzHttpWeb do pipe_through [ @@ -71,24 +90,6 @@ defmodule FzHttpWeb.Router do forward "/", Samly.Router end - # Local auth routes - scope "/auth", FzHttpWeb do - pipe_through [ - :browser, - :html_auth, - :require_unauthenticated, - :require_local_auth - ] - - get "/reset_password", AuthController, :reset_password - post "/magic_link", AuthController, :magic_link - get "/magic/:user_id/:token", AuthController, :magic_sign_in - - get "/:provider", AuthController, :request - get "/:provider/callback", AuthController, :callback - post "/:provider/callback", AuthController, :callback - end - # Unauthenticated routes scope "/", FzHttpWeb do pipe_through [ diff --git a/apps/fz_http/test/fz_http_web/acceptance/authentication_test.exs b/apps/fz_http/test/fz_http_web/acceptance/authentication_test.exs index 6a67082e6..0ccb2055b 100644 --- a/apps/fz_http/test/fz_http_web/acceptance/authentication_test.exs +++ b/apps/fz_http/test/fz_http_web/acceptance/authentication_test.exs @@ -100,7 +100,38 @@ defmodule FzHttpWeb.Acceptance.AuthenticationTest do end end - describe "using OIDC provider" do + describe "using SAML provider" do + feature "creates a user when auto_create_users is true", %{session: session} do + :ok = SimpleSAML.setup_saml_provider() + + session + |> visit(~p"/") + |> assert_el(Query.text("Sign In", minimum: 1)) + |> click(Query.link("Sign in with test-saml-idp")) + |> assert_el(Query.link("Enter your username and password")) + |> fill_in(Query.fillable_field("username"), with: "user1") + |> fill_in(Query.fillable_field("password"), with: "user1pass") + |> click(Query.button("Login")) + |> assert_el(Query.text("Your Devices")) + end + + feature "does not create new users when auto_create_users is false", %{session: session} do + FzHttp.Configurations.put!(:local_auth_enabled, false) + :ok = SimpleSAML.setup_saml_provider(%{"auto_create_users" => false}) + + session + |> visit(~p"/") + |> assert_el(Query.text("Sign In", minimum: 1)) + |> click(Query.link("Sign in with test-saml-idp")) + |> assert_el(Query.link("Enter your username and password")) + |> fill_in(Query.fillable_field("username"), with: "user1") + |> fill_in(Query.fillable_field("password"), with: "user1pass") + |> click(Query.button("Login")) + |> assert_el(Query.text("Local auth disabled")) + end + end + + describe "using OpenID Connect provider" do feature "creates a user when auto_create_users is true", %{session: session} do oidc_login = "firezone-1" oidc_password = "firezone1234_oidc" @@ -111,6 +142,7 @@ defmodule FzHttpWeb.Acceptance.AuthenticationTest do session |> visit(~p"/") + |> assert_el(Query.text("Sign In", minimum: 1)) |> click(Query.link("OIDC Vault")) |> Vault.userpass_flow(oidc_login, oidc_password) |> assert_el(Query.text("Your Devices")) @@ -133,6 +165,7 @@ defmodule FzHttpWeb.Acceptance.AuthenticationTest do session |> visit(~p"/") + |> assert_el(Query.text("Sign In", minimum: 1)) |> click(Query.link("OIDC Vault")) |> Vault.userpass_flow(oidc_login, oidc_password) |> find(Query.text("Users", count: 2), fn _ -> :ok end) @@ -155,6 +188,7 @@ defmodule FzHttpWeb.Acceptance.AuthenticationTest do session |> visit(~p"/") + |> assert_el(Query.text("Sign In", minimum: 1)) |> click(Query.link("OIDC Vault")) |> Vault.userpass_flow(oidc_login, oidc_password) |> assert_error_flash("Error signing in: user not found and auto_create_users disabled") diff --git a/apps/fz_http/test/support/acceptance_case.ex b/apps/fz_http/test/support/acceptance_case.ex index d31c36d03..e6dc42dc2 100644 --- a/apps/fz_http/test/support/acceptance_case.ex +++ b/apps/fz_http/test/support/acceptance_case.ex @@ -10,7 +10,7 @@ defmodule FzHttpWeb.AcceptanceCase do use FzHttpWeb, :verified_routes import FzHttpWeb.AcceptanceCase alias FzHttp.Repo - alias FzHttpWeb.AcceptanceCase.{Vault, Auth} + alias FzHttpWeb.AcceptanceCase.{Vault, SimpleSAML, Auth} # The default endpoint for testing @endpoint FzHttpWeb.Endpoint diff --git a/apps/fz_http/test/support/acceptance_case/simple_saml.ex b/apps/fz_http/test/support/acceptance_case/simple_saml.ex new file mode 100644 index 000000000..1621f9364 --- /dev/null +++ b/apps/fz_http/test/support/acceptance_case/simple_saml.ex @@ -0,0 +1,29 @@ +defmodule FzHttpWeb.AcceptanceCase.SimpleSAML do + @endpoint "http://localhost:8400" + + def fetch_metadata!(endpoint) do + metadata_url = "#{endpoint}/simplesaml/saml2/idp/metadata.php" + {:ok, 200, _headers, metadata} = :hackney.request(:get, metadata_url, [], "", [:with_body]) + metadata + end + + def setup_saml_provider(attrs_overrides \\ %{}) do + metadata = fetch_metadata!(@endpoint) + + FzHttp.Configurations.put!(:saml_identity_providers, [ + %{ + "id" => "mysamlidp", + "label" => "test-saml-idp", + "auto_create_users" => true, + "sign_requests" => true, + "sign_metadata" => true, + "signed_assertion_in_resp" => true, + "signed_envelopes_in_resp" => true, + "metadata" => metadata + } + |> Map.merge(attrs_overrides) + ]) + + :ok + end +end diff --git a/docker-compose.yml b/docker-compose.yml index 6988e1227..82014be17 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -84,6 +84,22 @@ services: networks: - app + saml-idp: + # This is a container with this PR merged: https://github.com/kristophjunge/docker-test-saml-idp/pull/27 + image: vihangk1/docker-test-saml-idp:latest + environment: + SIMPLESAMLPHP_SP_ENTITY_ID: 'urn:firezone.dev:firezone-app' + SIMPLESAMLPHP_SP_ASSERTION_CONSUMER_SERVICE: 'http://localhost:4002/auth/saml/sp/consume/mysamlidp' + SIMPLESAMLPHP_SP_SINGLE_LOGOUT_SERVICE: 'http://localhost:4002/auth/saml/sp/logout/mysamlidp' + SIMPLESAMLPHP_SP_NAME_ID_FORMAT: 'urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress' + SIMPLESAMLPHP_SP_NAME_ID_ATTRIBUTE: 'email' + SIMPLESAMLPHP_IDP_AUTH: 'example-userpass' + ports: + - 8400:8080/tcp + - 8443:8443/tcp + networks: + - app + # Unfortunately the Linux VM kernel for Docker Desktop is not compiled with # Dynamic Debug enabled, so we're unable to enable WireGuard debug logging. # Since WireGuard is designed to be silent by default, this basically does