From 55e5277959573dfdedb99bdb3284040c6afcf538 Mon Sep 17 00:00:00 2001 From: Dominik Stanaszek Date: Tue, 3 Sep 2019 13:50:37 +0200 Subject: [PATCH 1/7] Allow redirect binding on logout --- lib/samly/auth_handler.ex | 19 +++++++++++-------- lib/samly/helper.ex | 36 +++++++++++++++++++++++++++++++++--- lib/samly/idp_data.ex | 1 + lib/samly/router_util.ex | 7 ++++--- 4 files changed, 49 insertions(+), 14 deletions(-) diff --git a/lib/samly/auth_handler.ex b/lib/samly/auth_handler.ex index 57a0f8f..cc3f493 100644 --- a/lib/samly/auth_handler.ex +++ b/lib/samly/auth_handler.ex @@ -5,7 +5,7 @@ defmodule Samly.AuthHandler do import Plug.Conn alias Samly.{Assertion, IdpData, Helper, State, Subject} - import Samly.RouterUtil, only: [ensure_sp_uris_set: 2, send_saml_request: 5, redirect: 3] + import Samly.RouterUtil, only: [ensure_sp_uris_set: 2, send_saml_request: 5, send_saml_request: 6, redirect: 3] @sso_init_resp_template """ delete_session("samly_assertion_key") |> send_saml_request( idp_signout_url, - idp.use_redirect_for_req, + idp.use_redirect_for_logout_req, req_xml_frag, - relay_state + relay_state, + sp: sp, slo_post: slo_post, slo_redirect: slo_redirect ) _ -> diff --git a/lib/samly/helper.ex b/lib/samly/helper.ex index aa1bf7e..661a07d 100644 --- a/lib/samly/helper.ex +++ b/lib/samly/helper.ex @@ -55,10 +55,40 @@ defmodule Samly.Helper do {idp_signin_url, xml_frag} end - def gen_idp_signout_req(sp, idp_metadata, subject_rec, session_index) do + def gen_idp_signout_req(sp, idp_metadata, subject_rec, session_index, opts) do idp_signout_url = Esaml.esaml_idp_metadata(idp_metadata, :logout_location) - xml_frag = :esaml_sp.generate_logout_request(idp_signout_url, session_index, subject_rec, sp) - {idp_signout_url, xml_frag} + + case get_binding_type(opts) do + {:ok, binding_type} -> + xml_frag = :esaml_sp.generate_logout_request(idp_signout_url, + session_index, + subject_rec, + sp, + binding_type: binding_type) + {:ok, {idp_signout_url, xml_frag}} + error -> + error + end + end + + defp get_binding_type(opts) do + # TODO refactor + use_redirect? = Keyword.get(opts, :use_redirect?) + redirect = Keyword.get(opts, :slo_redirect) + post = Keyword.get(opts, :slo_post) + if use_redirect? do + if redirect do + {:ok, :redirect} + else + {:error, :no_binding} + end + else + if post do + {:ok, :post} + else + {:error, :no_binding} + end + end end def gen_idp_signout_resp(sp, idp_metadata, signout_status) do diff --git a/lib/samly/idp_data.ex b/lib/samly/idp_data.ex index f641ba5..bd56196 100644 --- a/lib/samly/idp_data.ex +++ b/lib/samly/idp_data.ex @@ -119,6 +119,7 @@ defmodule Samly.IdpData do |> set_pipeline(opts_map) |> set_allowed_target_urls(opts_map) |> set_boolean_attr(opts_map, :use_redirect_for_req) + |> set_boolean_attr(opts_map, :use_redirect_for_logout_req) |> set_boolean_attr(opts_map, :sign_requests) |> set_boolean_attr(opts_map, :sign_metadata) |> set_boolean_attr(opts_map, :signed_assertion_in_resp) diff --git a/lib/samly/router_util.ex b/lib/samly/router_util.ex index 66407db..706ef51 100644 --- a/lib/samly/router_util.ex +++ b/lib/samly/router_util.ex @@ -82,15 +82,16 @@ defmodule Samly.RouterUtil do end end - def send_saml_request(conn, idp_url, use_redirect?, signed_xml_payload, relay_state) do + def send_saml_request(conn, idp_url, use_redirect?, xml_payload, relay_state, opts \\ []) do if use_redirect? do + redirect_opts = [key: opts |> Keyword.get(:sp) |> Esaml.esaml_sp(:key)] url = - :esaml_binding.encode_http_redirect(idp_url, signed_xml_payload, :undefined, relay_state) + :esaml_binding.encode_http_redirect(idp_url, xml_payload, :undefined, relay_state, redirect_opts) conn |> redirect(302, url) else nonce = conn.private[:samly_nonce] - resp_body = :esaml_binding.encode_http_post(idp_url, signed_xml_payload, relay_state, nonce) + resp_body = :esaml_binding.encode_http_post(idp_url, xml_payload, relay_state, nonce) conn |> Conn.put_resp_header("content-type", "text/html") From 084bb1aeb1e944dc7561463fdf525ac99ff2f26e Mon Sep 17 00:00:00 2001 From: Dominik Stanaszek Date: Tue, 3 Sep 2019 13:50:45 +0200 Subject: [PATCH 2/7] Format code --- lib/samly.ex | 1 - lib/samly/auth_handler.ex | 29 ++++++++++++++++-------- lib/samly/helper.ex | 16 +++++++++----- lib/samly/idp_data.ex | 43 ++++++++++++++++++------------------ lib/samly/router_util.ex | 9 +++++++- lib/samly/sp_handler.ex | 1 + test/samly_idp_data_test.exs | 1 + 7 files changed, 63 insertions(+), 37 deletions(-) diff --git a/lib/samly.ex b/lib/samly.ex index 12a5920..601e6e8 100644 --- a/lib/samly.ex +++ b/lib/samly.ex @@ -64,5 +64,4 @@ defmodule Samly do conn |> Conn.delete_session("samly_assertion_key") end - end diff --git a/lib/samly/auth_handler.ex b/lib/samly/auth_handler.ex index cc3f493..00a1f20 100644 --- a/lib/samly/auth_handler.ex +++ b/lib/samly/auth_handler.ex @@ -5,7 +5,8 @@ defmodule Samly.AuthHandler do import Plug.Conn alias Samly.{Assertion, IdpData, Helper, State, Subject} - import Samly.RouterUtil, only: [ensure_sp_uris_set: 2, send_saml_request: 5, send_saml_request: 6, redirect: 3] + import Samly.RouterUtil, + only: [ensure_sp_uris_set: 2, send_saml_request: 5, send_saml_request: 6, redirect: 3] @sso_init_resp_template """ send_resp(500, "request_failed") end - def send_signout_req(conn) do - %IdpData{id: idp_id, slo_post_url: slo_post, slo_redirect_url: slo_redirect} = idp = conn.private[:samly_idp] + %IdpData{id: idp_id, slo_post_url: slo_post, slo_redirect_url: slo_redirect} = + idp = conn.private[:samly_idp] + %IdpData{esaml_idp_rec: idp_rec, esaml_sp_rec: sp_rec} = idp sp = ensure_sp_uris_set(sp_rec, conn) @@ -108,7 +114,10 @@ defmodule Samly.AuthHandler do {:ok, {idp_signout_url, req_xml_frag}} = Helper.gen_idp_signout_req(sp, idp_rec, subject_rec, session_index, - slo_post: slo_post, slo_redirect: slo_redirect, use_redirect?: idp.use_redirect_for_logout_req) + slo_post: slo_post, + slo_redirect: slo_redirect, + use_redirect?: idp.use_redirect_for_logout_req + ) conn = State.delete_assertion(conn, assertion_key) relay_state = State.gen_id() @@ -123,7 +132,9 @@ defmodule Samly.AuthHandler do idp.use_redirect_for_logout_req, req_xml_frag, relay_state, - sp: sp, slo_post: slo_post, slo_redirect: slo_redirect + sp: sp, + slo_post: slo_post, + slo_redirect: slo_redirect ) _ -> @@ -135,10 +146,10 @@ defmodule Samly.AuthHandler do # Logger.error("#{inspect error}") # conn |> send_resp(500, "request_failed") end - + defp strip_subdomains(host, n_of_subdomains) do host |> String.split(".", parts: n_of_subdomains + 1) - |> List.last + |> List.last() end end diff --git a/lib/samly/helper.ex b/lib/samly/helper.ex index 661a07d..d2f5003 100644 --- a/lib/samly/helper.ex +++ b/lib/samly/helper.ex @@ -60,12 +60,17 @@ defmodule Samly.Helper do case get_binding_type(opts) do {:ok, binding_type} -> - xml_frag = :esaml_sp.generate_logout_request(idp_signout_url, - session_index, - subject_rec, - sp, - binding_type: binding_type) + xml_frag = + :esaml_sp.generate_logout_request( + idp_signout_url, + session_index, + subject_rec, + sp, + binding_type: binding_type + ) + {:ok, {idp_signout_url, xml_frag}} + error -> error end @@ -76,6 +81,7 @@ defmodule Samly.Helper do use_redirect? = Keyword.get(opts, :use_redirect?) redirect = Keyword.get(opts, :slo_redirect) post = Keyword.get(opts, :slo_post) + if use_redirect? do if redirect do {:ok, :redirect} diff --git a/lib/samly/idp_data.ex b/lib/samly/idp_data.ex index bd56196..cef78cc 100644 --- a/lib/samly/idp_data.ex +++ b/lib/samly/idp_data.ex @@ -77,13 +77,13 @@ defmodule Samly.IdpData do @signing_keys_selector ~x"//#{@entdesc}/#{@idpdesc}/#{@keydesc}[@use != 'encryption']"l @enc_keys_selector ~x"//#{@entdesc}/#{@idpdesc}/#{@keydesc}[@use = 'encryption']"l - # These functions work on EntityDescriptor element @sso_redirect_url_selector ~x"/#{@entdesc}/#{@idpdesc}/#{@ssos}[@Binding = '#{@redirect}']/@Location"s @sso_post_url_selector ~x"/#{@entdesc}/#{@idpdesc}/#{@ssos}[@Binding = '#{@post}']/@Location"s @slo_redirect_url_selector ~x"/#{@entdesc}/#{@idpdesc}/#{@slos}[@Binding = '#{@redirect}']/@Location"s @slo_post_url_selector ~x"/#{@entdesc}/#{@idpdesc}/#{@slos}[@Binding = '#{@post}']/@Location"s - @nameid_format_selector ~x"/#{@entdesc}/#{@idpdesc}/#{@nameid}/text()[1]"s # TODO How to deal with multiple nameid formats? + # TODO How to deal with multiple nameid formats? + @nameid_format_selector ~x"/#{@entdesc}/#{@idpdesc}/#{@nameid}/text()[1]"s @signing_keys_in_idp_selector ~x"./#{@idpdesc}/#{@keydesc}[@use != 'encryption']"l @cert_selector ~x"./ds:KeyInfo/ds:X509Data/ds:X509Certificate/text()"s @@ -277,34 +277,35 @@ defmodule Samly.IdpData do entity_md_xml = get_entity_descriptor(md_xml, entityID) - - case entity_md_xml do nil -> Logger.warn("[Samly] Entity #{inspect(entityID)} not found") {:ok, idp_data} + {:error, :entity_not_found} = err -> Logger.warn("[Samly] Entity not found due to configuration error") {:ok, idp_data} + {:error, reason} -> Logger.warn("[Samly] Parsing error due to: #{inspect(reason)}") {:ok, idp_data} + _ -> signing_certs = get_signing_certs_in_idp(entity_md_xml) {:ok, - %IdpData{ - idp_data - | entity_id: entityID, - signed_requests: get_req_signed(md_xml), - certs: signing_certs, - fingerprints: idp_cert_fingerprints(signing_certs), - sso_redirect_url: get_sso_redirect_url(entity_md_xml), - sso_post_url: get_sso_post_url(entity_md_xml), - slo_redirect_url: get_slo_redirect_url(entity_md_xml), - slo_post_url: get_slo_post_url(entity_md_xml), - nameid_format: get_nameid_format(entity_md_xml) - }} + %IdpData{ + idp_data + | entity_id: entityID, + signed_requests: get_req_signed(md_xml), + certs: signing_certs, + fingerprints: idp_cert_fingerprints(signing_certs), + sso_redirect_url: get_sso_redirect_url(entity_md_xml), + sso_post_url: get_sso_post_url(entity_md_xml), + slo_redirect_url: get_slo_redirect_url(entity_md_xml), + slo_post_url: get_slo_post_url(entity_md_xml), + nameid_format: get_nameid_format(entity_md_xml) + }} end end @@ -389,7 +390,7 @@ defmodule Samly.IdpData do ) end - @spec get_entity_id(:xmlElement) :: binary() + @spec get_entity_id(:xmlElement) :: binary() def get_entity_id(md_elem) do md_elem |> xpath(@entity_id_selector |> add_ns()) |> hd() |> String.trim() end @@ -405,8 +406,8 @@ defmodule Samly.IdpData do @spec get_req_signed(:xmlElement) :: binary() def get_req_signed(md_elem), do: get_data(md_elem, @req_signed_selector) - #@spec get_signing_certs(:xmlElement) :: certs() - #def get_signing_certs(md_elem), do: get_certs(md_elem, signing_keys_selector()) + # @spec get_signing_certs(:xmlElement) :: certs() + # def get_signing_certs(md_elem), do: get_certs(md_elem, signing_keys_selector()) def get_signing_certs_in_idp(md_elem), do: get_certs(md_elem, @signing_keys_in_idp_selector) @@ -460,11 +461,11 @@ defmodule Samly.IdpData do @spec get_entity_descriptor(:xmlElement, entityID :: binary()) :: :xmlElement | nil defp get_entity_descriptor(md_xml, entityID) do selector = entity_by_id_selector(entityID) |> add_ns() + try do SweetXml.xpath(md_xml, selector) rescue _ -> {:error, :entity_not_found} end end - -end +end diff --git a/lib/samly/router_util.ex b/lib/samly/router_util.ex index 706ef51..0a68207 100644 --- a/lib/samly/router_util.ex +++ b/lib/samly/router_util.ex @@ -85,8 +85,15 @@ defmodule Samly.RouterUtil do def send_saml_request(conn, idp_url, use_redirect?, xml_payload, relay_state, opts \\ []) do if use_redirect? do redirect_opts = [key: opts |> Keyword.get(:sp) |> Esaml.esaml_sp(:key)] + url = - :esaml_binding.encode_http_redirect(idp_url, xml_payload, :undefined, relay_state, redirect_opts) + :esaml_binding.encode_http_redirect( + idp_url, + xml_payload, + :undefined, + relay_state, + redirect_opts + ) conn |> redirect(302, url) else diff --git a/lib/samly/sp_handler.ex b/lib/samly/sp_handler.ex index a7661d1..bedd739 100644 --- a/lib/samly/sp_handler.ex +++ b/lib/samly/sp_handler.ex @@ -68,6 +68,7 @@ defmodule Samly.SPHandler do defp maybe_redirect?(%{host: host} = conn, target_url) do %URI{host: target_host} = URI.parse(target_url) + if target_host != host do current_uri = URI.parse(Phoenix.Router.Helpers.url(Samly.Router, conn) <> conn.request_path) %URI{host: target_host} = URI.parse(target_url) diff --git a/test/samly_idp_data_test.exs b/test/samly_idp_data_test.exs index e2133fa..ccb2530 100644 --- a/test/samly_idp_data_test.exs +++ b/test/samly_idp_data_test.exs @@ -292,6 +292,7 @@ defmodule SamlyIdpDataTest do test "Federation metadata can be provided for an idp along with its entityID", %{sps: sps} do idp_data = IdpData.load_provider(@federation_idp_config1, sps) + %IdpData{ entity_id: entity_id, sso_post_url: sso_post_url, From ca652c930b3d8d57e9371371ba1acb0efd66f587 Mon Sep 17 00:00:00 2001 From: Dominik Stanaszek Date: Tue, 3 Sep 2019 13:54:04 +0200 Subject: [PATCH 3/7] Update esaml dep --- mix.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mix.exs b/mix.exs index 2897126..f4ec9c1 100644 --- a/mix.exs +++ b/mix.exs @@ -30,7 +30,7 @@ defmodule Samly.Mixfile do defp deps() do [ {:plug, "~> 1.6"}, - {:esaml, "~> 4.2"}, + {:esaml, git: "https://github.com/kanes115/esaml", ref: "logout_binding"}, {:sweet_xml, "~> 0.6.6"}, {:ex_doc, "~> 0.19.0", only: :dev, runtime: false}, {:inch_ex, "~> 1.0", only: [:dev, :test]} From 4d29051bb1be6d5a5663c51829adcf05dc763480 Mon Sep 17 00:00:00 2001 From: Dominik Stanaszek Date: Tue, 3 Sep 2019 15:25:41 +0200 Subject: [PATCH 4/7] Add a pre logout pipeline --- lib/samly/auth_handler.ex | 5 +++++ lib/samly/idp_data.ex | 11 +++++++---- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/samly/auth_handler.ex b/lib/samly/auth_handler.ex index 00a1f20..204e605 100644 --- a/lib/samly/auth_handler.ex +++ b/lib/samly/auth_handler.ex @@ -102,6 +102,7 @@ defmodule Samly.AuthHandler do idp = conn.private[:samly_idp] %IdpData{esaml_idp_rec: idp_rec, esaml_sp_rec: sp_rec} = idp + %IdpData{pre_logout_pipeline: pipeline} = idp sp = ensure_sp_uris_set(sp_rec, conn) target_url = conn.private[:samly_target_url] || "/" @@ -119,6 +120,7 @@ defmodule Samly.AuthHandler do use_redirect?: idp.use_redirect_for_logout_req ) + conn = pipethrough(conn, pipeline) conn = State.delete_assertion(conn, assertion_key) relay_state = State.gen_id() @@ -147,6 +149,9 @@ defmodule Samly.AuthHandler do # conn |> send_resp(500, "request_failed") end + defp pipethrough(conn, nil), do: conn + defp pipethrough(conn, pipeline), do: pipeline.call(conn) + defp strip_subdomains(host, n_of_subdomains) do host |> String.split(".", parts: n_of_subdomains + 1) diff --git a/lib/samly/idp_data.ex b/lib/samly/idp_data.ex index cef78cc..680ee31 100644 --- a/lib/samly/idp_data.ex +++ b/lib/samly/idp_data.ex @@ -15,6 +15,7 @@ defmodule Samly.IdpData do base_url: nil, metadata_file: nil, pre_session_create_pipeline: nil, + pre_logout_pipeline: nil, use_redirect_for_req: false, sign_requests: true, sign_metadata: true, @@ -41,6 +42,7 @@ defmodule Samly.IdpData do base_url: nil | binary(), metadata_file: nil | binary(), pre_session_create_pipeline: nil | module(), + pre_logout_pipeline: nil | module(), use_redirect_for_req: boolean(), sign_requests: boolean(), sign_metadata: boolean(), @@ -116,7 +118,7 @@ defmodule Samly.IdpData do when is_binary(id) and is_binary(sp_id) do %IdpData{idp_data | id: id, sp_id: sp_id, base_url: Map.get(opts_map, :base_url)} |> set_metadata_file(opts_map) - |> set_pipeline(opts_map) + |> set_pipelines(opts_map) |> set_allowed_target_urls(opts_map) |> set_boolean_attr(opts_map, :use_redirect_for_req) |> set_boolean_attr(opts_map, :use_redirect_for_logout_req) @@ -194,10 +196,11 @@ defmodule Samly.IdpData do %IdpData{idp_data | metadata_file: Map.get(opts_map, :metadata_file, @default_metadata_file)} end - @spec set_pipeline(%IdpData{}, map()) :: %IdpData{} - defp set_pipeline(%IdpData{} = idp_data, %{} = opts_map) do + @spec set_pipelines(%IdpData{}, map()) :: %IdpData{} + defp set_pipelines(%IdpData{} = idp_data, %{} = opts_map) do pipeline = Map.get(opts_map, :pre_session_create_pipeline) - %IdpData{idp_data | pre_session_create_pipeline: pipeline} + logout_pipeline = Map.get(opts_map, :pre_logout_pipeline) + %IdpData{idp_data | pre_session_create_pipeline: pipeline, pre_logout_pipeline: logout_pipeline} end defp set_allowed_target_urls(%IdpData{} = idp_data, %{} = opts_map) do From 618058eeca69a2b32797271add319b2f8f152b42 Mon Sep 17 00:00:00 2001 From: Dominik Stanaszek Date: Wed, 4 Sep 2019 15:09:18 +0200 Subject: [PATCH 5/7] Redirect to correct host after logout response --- lib/samly/sp_handler.ex | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/samly/sp_handler.ex b/lib/samly/sp_handler.ex index bedd739..5500ec2 100644 --- a/lib/samly/sp_handler.ex +++ b/lib/samly/sp_handler.ex @@ -138,12 +138,15 @@ defmodule Samly.SPHandler do %IdpData{id: idp_id} = idp = conn.private[:samly_idp] %IdpData{esaml_idp_rec: _idp_rec, esaml_sp_rec: sp_rec} = idp sp = ensure_sp_uris_set(sp_rec, conn) + sp = Esaml.esaml_sp(sp, trusted_fingerprints: :any) # TODO saml_encoding = conn.body_params["SAMLEncoding"] saml_response = conn.body_params["SAMLResponse"] relay_state = conn.body_params["RelayState"] |> safe_decode_www_form() - with {:ok, _payload} <- Helper.decode_idp_signout_resp(sp, saml_encoding, saml_response), + with target_url <- auth_target_url(conn, nil, relay_state), + {:redirect?, :no_redirection} <- {:redirect?, maybe_redirect?(conn, target_url)}, + {:ok, _payload} <- Helper.decode_idp_signout_resp(sp, saml_encoding, saml_response), ^relay_state when relay_state != nil <- get_session(conn, "relay_state"), ^idp_id <- get_session(conn, "idp_id"), target_url when target_url != nil <- conn.cookies["target_url"] do @@ -151,6 +154,7 @@ defmodule Samly.SPHandler do |> configure_session(drop: true) |> redirect(302, target_url) else + {:redirect?, conn} -> conn error -> conn |> send_resp(403, "invalid_request #{inspect(error)}") end From be51c54a82d48e37e2ed2f57b0f9640afc82f576 Mon Sep 17 00:00:00 2001 From: Dominik Stanaszek Date: Fri, 6 Sep 2019 11:31:01 +0200 Subject: [PATCH 6/7] Tidy up target url on signout --- lib/samly/auth_handler.ex | 6 ++++-- lib/samly/sp_handler.ex | 24 ++++++++++++++++++------ 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/lib/samly/auth_handler.ex b/lib/samly/auth_handler.ex index 204e605..0d2b768 100644 --- a/lib/samly/auth_handler.ex +++ b/lib/samly/auth_handler.ex @@ -42,6 +42,7 @@ defmodule Samly.AuthHandler do import Plug.CSRFProtection, only: [get_csrf_token: 0] target_url = conn.private[:samly_target_url] || "/" + target_url = conn.params["target_url"] opts = [ nonce: conn.private[:samly_nonce], @@ -105,7 +106,8 @@ defmodule Samly.AuthHandler do %IdpData{pre_logout_pipeline: pipeline} = idp sp = ensure_sp_uris_set(sp_rec, conn) - target_url = conn.private[:samly_target_url] || "/" + target_url = conn.params["target_url"] |> URI.decode_www_form() + assertion_key = get_session(conn, "samly_assertion_key") case State.get_assertion(conn, assertion_key) do @@ -150,7 +152,7 @@ defmodule Samly.AuthHandler do end defp pipethrough(conn, nil), do: conn - defp pipethrough(conn, pipeline), do: pipeline.call(conn) + defp pipethrough(conn, pipeline), do: pipeline.call(conn, []) defp strip_subdomains(host, n_of_subdomains) do host diff --git a/lib/samly/sp_handler.ex b/lib/samly/sp_handler.ex index 5500ec2..c22d4f1 100644 --- a/lib/samly/sp_handler.ex +++ b/lib/samly/sp_handler.ex @@ -143,19 +143,22 @@ defmodule Samly.SPHandler do saml_encoding = conn.body_params["SAMLEncoding"] saml_response = conn.body_params["SAMLResponse"] relay_state = conn.body_params["RelayState"] |> safe_decode_www_form() + + redirection_url = URI.decode_www_form(conn.cookies["target_url"]) - with target_url <- auth_target_url(conn, nil, relay_state), - {:redirect?, :no_redirection} <- {:redirect?, maybe_redirect?(conn, target_url)}, - {:ok, _payload} <- Helper.decode_idp_signout_resp(sp, saml_encoding, saml_response), + with {:redirect?, :no_redirection} <- {:redirect?, maybe_redirect?(conn, redirection_url)}, + :ok <- assert_correct_response(sp, saml_encoding, saml_response), ^relay_state when relay_state != nil <- get_session(conn, "relay_state"), ^idp_id <- get_session(conn, "idp_id"), - target_url when target_url != nil <- conn.cookies["target_url"] do + target_url when target_url != nil <- get_session(conn, "target_url") do conn |> configure_session(drop: true) - |> redirect(302, target_url) + |> redirect(303, target_url) else {:redirect?, conn} -> conn - error -> conn |> send_resp(403, "invalid_request #{inspect(error)}") + error -> + conn + |> send_resp(403, "invalid_request #{inspect(error)}") end # rescue @@ -164,6 +167,15 @@ defmodule Samly.SPHandler do # conn |> send_resp(500, "request_failed") end + def assert_correct_response(sp, saml_encoding, saml_response) do + case Helper.decode_idp_signout_resp(sp, saml_encoding, saml_response) do + {:ok, _} -> :ok + # Below is the case where the user is already logged out. + {:error, :Requester} -> :ok + e -> e + end + end + # non-ui logout request from IDP def handle_logout_request(conn) do %IdpData{id: idp_id} = idp = conn.private[:samly_idp] From 7bcc21bfc5fe7783b7a0716836de457ef5c420a6 Mon Sep 17 00:00:00 2001 From: Dominik Stanaszek Date: Fri, 6 Sep 2019 13:19:40 +0200 Subject: [PATCH 7/7] Add redirection after sso to signout --- lib/samly/idp_data.ex | 7 ++++++- lib/samly/sp_handler.ex | 17 +++++++++++++---- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/lib/samly/idp_data.ex b/lib/samly/idp_data.ex index 680ee31..99c9f08 100644 --- a/lib/samly/idp_data.ex +++ b/lib/samly/idp_data.ex @@ -200,7 +200,12 @@ defmodule Samly.IdpData do defp set_pipelines(%IdpData{} = idp_data, %{} = opts_map) do pipeline = Map.get(opts_map, :pre_session_create_pipeline) logout_pipeline = Map.get(opts_map, :pre_logout_pipeline) - %IdpData{idp_data | pre_session_create_pipeline: pipeline, pre_logout_pipeline: logout_pipeline} + + %IdpData{ + idp_data + | pre_session_create_pipeline: pipeline, + pre_logout_pipeline: logout_pipeline + } end defp set_allowed_target_urls(%IdpData{} = idp_data, %{} = opts_map) do diff --git a/lib/samly/sp_handler.ex b/lib/samly/sp_handler.ex index c22d4f1..6e80702 100644 --- a/lib/samly/sp_handler.ex +++ b/lib/samly/sp_handler.ex @@ -138,12 +138,13 @@ defmodule Samly.SPHandler do %IdpData{id: idp_id} = idp = conn.private[:samly_idp] %IdpData{esaml_idp_rec: _idp_rec, esaml_sp_rec: sp_rec} = idp sp = ensure_sp_uris_set(sp_rec, conn) - sp = Esaml.esaml_sp(sp, trusted_fingerprints: :any) # TODO + # TODO + sp = Esaml.esaml_sp(sp, trusted_fingerprints: :any) saml_encoding = conn.body_params["SAMLEncoding"] saml_response = conn.body_params["SAMLResponse"] relay_state = conn.body_params["RelayState"] |> safe_decode_www_form() - + redirection_url = URI.decode_www_form(conn.cookies["target_url"]) with {:redirect?, :no_redirection} <- {:redirect?, maybe_redirect?(conn, redirection_url)}, @@ -155,7 +156,9 @@ defmodule Samly.SPHandler do |> configure_session(drop: true) |> redirect(303, target_url) else - {:redirect?, conn} -> conn + {:redirect?, conn} -> + conn + error -> conn |> send_resp(403, "invalid_request #{inspect(error)}") @@ -186,7 +189,10 @@ defmodule Samly.SPHandler do saml_request = conn.body_params["SAMLRequest"] relay_state = conn.body_params["RelayState"] |> safe_decode_www_form() - with {:ok, payload} <- Helper.decode_idp_signout_req(sp, saml_encoding, saml_request) do + redirection_url = URI.decode_www_form(conn.cookies["target_url"]) + + with {:redirect?, :no_redirection} <- {:redirect?, maybe_redirect?(conn, redirection_url)}, + {:ok, payload} <- Helper.decode_idp_signout_req(sp, saml_encoding, saml_request) do Esaml.esaml_logoutreq(name: nameid, issuer: _issuer) = payload assertion_key = {idp_id, nameid} @@ -206,6 +212,9 @@ defmodule Samly.SPHandler do |> configure_session(drop: true) |> send_saml_request(idp_signout_url, idp.use_redirect_for_req, resp_xml_frag, relay_state) else + {:redirect?, conn} -> + conn + error -> Logger.error("#{inspect(error)}") {idp_signout_url, resp_xml_frag} = Helper.gen_idp_signout_resp(sp, idp_rec, :denied)