diff --git a/CHANGELOG.md b/CHANGELOG.md index 7501fb6..08a393a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # CHANGELOG +### v0.9.1 + ++ Remove the need for supplying certicate and key files if the requests are + not signed (Issue #16). Useful during development when the corresponding + Identity Provider is setup for unsigned requests/responses. Use signing + for production deployments. The defaults expect signed requests/responses. + ### v0.9.0 + Issue: #12. Support for IDP initiated SSO flow. diff --git a/README.md b/README.md index ccf9d49..e941869 100644 --- a/README.md +++ b/README.md @@ -179,8 +179,8 @@ config :samly, Samly.Provider, | **Service Provider Parameters** | | | `id` | _(mandatory)_ | | `identity_id` | _(optional)_ If omitted, the metadata URL will be used | -| `certfile` | _(optional)_ Defaults to "samly.crt" | -| `keyfile` | _(optional)_ Defaults to "samly.pem" | +| `certfile` | _(optional)_ This is needed when SAML requests/responses need to be signed. Make sure to set this in a production deployment. Could be omitted during development if your IDP is setup to not require signing. If that is the case, the following **Identity Provider Parameters** must be explicitly set to false: `sign_requests`, `sign_metadata`, `signed_assertion_in_resp`, `signed_envelopes_in_resp`| +| `keyfile` | _(optional)_ Similar to `certfile` | | `contact_name` | _(optional)_ Technical contact name for the Service Provider | | `contact_email` | _(optional)_ Technical contact email address | | `org_name` | _(optional)_ SAML Service Provider (your app) Organization name | @@ -193,7 +193,7 @@ config :samly, Samly.Provider, | `metadata_file` | _(mandatory)_ Path to the IdP metadata XML file obtained from the Identity Provider. | | `pre_session_create_pipeline` | _(optional)_ Check the customization section. | | `use_redirect_for_req` | _(optional)_ Default is `false`. When this is `false`, `Samly` will POST to the IdP SAML endpoints. | -| `signed_requests`, `signed_metadata` | _(optional)_ Default is `true`. | +| `sign_requests`, `sign_metadata` | _(optional)_ Default is `true`. | | `signed_assertion_in_resp`, `signed_envelopes_in_resp` | _(optional)_ Default is `true`. When `true`, `Samly` expects the requests and responses from IdP to be signed. | | `allow_idp_initiated_flow` | _(optional)_ Default is `false`. IDP initiated SSO is allowed only when this is set to `true`. | | `allowed_target_urls` | _(optional)_ Default is `[]`. `Samly` uses this **only** when `allow_idp_initiated_flow` parameter is set to `true`. Make sure to set this to one or more exact URLs you want to allow (whitelist). The URL to redirect the user after completing the SSO flow is sent from IDP in auth response as `relay_state`. This `relay_state` target URL is matched against this URL list. Set the value to `nil` if you do not want this whitelist capability. | diff --git a/lib/samly/idp_data.ex b/lib/samly/idp_data.ex index 5795568..e305699 100644 --- a/lib/samly/idp_data.ex +++ b/lib/samly/idp_data.ex @@ -140,7 +140,7 @@ defmodule Samly.IdpData do %SpData{} = sp -> idp_data = %IdpData{idp_data | esaml_idp_rec: to_esaml_idp_metadata(idp_data, opts_map)} idp_data = %IdpData{idp_data | esaml_sp_rec: get_esaml_sp(sp, idp_data)} - %IdpData{idp_data | valid?: true} + %IdpData{idp_data | valid?: cert_config_ok?(idp_data, sp)} _ -> Logger.error("[Samly] Unknown/invalid sp_id: #{idp_data.sp_id}") @@ -148,6 +148,20 @@ defmodule Samly.IdpData do end end + @spec cert_config_ok?(%IdpData{}, %SpData{}) :: boolean + defp cert_config_ok?(%IdpData{} = idp_data, %SpData{} = sp_data) do + if (idp_data.sign_metadata || + idp_data.sign_requests || + idp_data.signed_assertion_in_resp || + idp_data.signed_envelopes_in_resp) && + (sp_data.cert == :undefined || sp_data.key == :undefined) do + Logger.error("[Samly] SP cert or key missing - Skipping identity provider: #{idp_data.id}") + false + else + true + end + end + @default_metadata_file "idp_metadata.xml" @spec set_metadata_file(%IdpData{}, map()) :: %IdpData{} diff --git a/lib/samly/sp_data.ex b/lib/samly/sp_data.ex index cde934f..166e623 100644 --- a/lib/samly/sp_data.ex +++ b/lib/samly/sp_data.ex @@ -80,6 +80,9 @@ defmodule Samly.SpData do end @spec load_cert(%SpData{}, map()) :: %SpData{} + defp load_cert(%SpData{certfile: ""} = sp_data, _) do + %SpData{sp_data | cert: :undefined} + end defp load_cert(%SpData{certfile: certfile} = sp_data, %{} = opts_map) do try do cert = :esaml_util.load_certificate(certfile) @@ -92,6 +95,9 @@ defmodule Samly.SpData do end @spec load_key(%SpData{}, map()) :: %SpData{} + defp load_key(%SpData{keyfile: ""} = sp_data, _) do + %SpData{sp_data | key: :undefined} + end defp load_key(%SpData{keyfile: keyfile} = sp_data, %{} = opts_map) do try do key = :esaml_util.load_private_key(keyfile) diff --git a/mix.exs b/mix.exs index 31d4f51..92e5e9f 100644 --- a/mix.exs +++ b/mix.exs @@ -1,7 +1,7 @@ defmodule Samly.Mixfile do use Mix.Project - @version "0.9.0" + @version "0.9.1" @description "SAML SP SSO made easy" @source_url "https://github.com/handnot2/samly" diff --git a/test/samly_idp_data_test.exs b/test/samly_idp_data_test.exs index 73897c1..83d7f54 100644 --- a/test/samly_idp_data_test.exs +++ b/test/samly_idp_data_test.exs @@ -16,6 +16,20 @@ defmodule SamlyIdpDataTest do keyfile: "test/data/test.pem" } + @sp_config3 %{ + id: "sp3", + keyfile: "test/data/test.pem" + } + + @sp_config4 %{ + id: "sp4", + certfile: "test/data/test.crt" + } + + @sp_config5 %{ + id: "sp5" + } + @idp_config1 %{ id: "idp1", sp_id: "sp1", @@ -33,7 +47,15 @@ defmodule SamlyIdpDataTest do setup context do sp_data1 = SpData.load_provider(@sp_config1) sp_data2 = SpData.load_provider(@sp_config2) - [sps: %{sp_data1.id => sp_data1, sp_data2.id => sp_data2}] |> Enum.into(context) + sp_data3 = SpData.load_provider(@sp_config3) + sp_data4 = SpData.load_provider(@sp_config4) + sp_data5 = SpData.load_provider(@sp_config5) + [sps: %{ + sp_data1.id => sp_data1, + sp_data2.id => sp_data2, + sp_data3.id => sp_data3, + sp_data4.id => sp_data4, + sp_data5.id => sp_data5}] |> Enum.into(context) end test "valid-idp-config-1", %{sps: sps} do @@ -179,4 +201,49 @@ defmodule SamlyIdpDataTest do %IdpData{} = idp_data = IdpData.load_provider(idp_config, sps) refute idp_data.valid? end + + test "valid-idp-config-signing-turned-off", %{sps: sps} do + idp_config = + Map.merge(@idp_config1, %{ + sp_id: "sp5", + use_redirect_for_req: true, + sign_requests: false, + sign_metadata: false, + signed_assertion_in_resp: false, + signed_envelopes_in_resp: false + }) + + %IdpData{} = idp_data = IdpData.load_provider(idp_config, sps) + assert idp_data.valid? + end + + test "invalid-idp-config-signing-on-cert-missing", %{sps: sps} do + idp_config = + Map.merge(@idp_config1, %{ + sp_id: "sp3", + use_redirect_for_req: true, + sign_requests: true, + sign_metadata: false, + signed_assertion_in_resp: false, + signed_envelopes_in_resp: false + }) + + %IdpData{} = idp_data = IdpData.load_provider(idp_config, sps) + refute idp_data.valid? + end + + test "invalid-idp-config-signing-on-key-missing", %{sps: sps} do + idp_config = + Map.merge(@idp_config1, %{ + sp_id: "sp4", + use_redirect_for_req: true, + sign_requests: true, + sign_metadata: false, + signed_assertion_in_resp: false, + signed_envelopes_in_resp: false + }) + + %IdpData{} = idp_data = IdpData.load_provider(idp_config, sps) + refute idp_data.valid? + end end diff --git a/test/samly_sp_data_test.exs b/test/samly_sp_data_test.exs index 6cf5e0b..ee950d0 100644 --- a/test/samly_sp_data_test.exs +++ b/test/samly_sp_data_test.exs @@ -14,44 +14,44 @@ defmodule SamlySpDataTest do assert sp_data.valid? end - test "invalid-sp-config-1" do - sp_config = %{@sp_config1 | id: ""} - %SpData{} = sp_data = SpData.load_provider(sp_config) - refute sp_data.valid? - end - - test "invalid-sp-config-2" do + test "cert-unspecified-sp-config" do sp_config = %{@sp_config1 | certfile: ""} - %SpData{} = sp_data = SpData.load_provider(sp_config) - refute sp_data.valid? + %SpData{cert: :undefined} = sp_data = SpData.load_provider(sp_config) + assert sp_data.valid? end - test "invalid-sp-config-3" do + test "key-unspecified-sp-config" do sp_config = %{@sp_config1 | keyfile: ""} + %SpData{key: :undefined} = sp_data = SpData.load_provider(sp_config) + assert sp_data.valid? + end + + test "invalid-sp-config-1" do + sp_config = %{@sp_config1 | id: ""} %SpData{} = sp_data = SpData.load_provider(sp_config) refute sp_data.valid? end - test "invalid-sp-config-4" do + test "invalid-sp-config-2" do sp_config = %{@sp_config1 | certfile: "non-existent.crt"} %SpData{} = sp_data = SpData.load_provider(sp_config) refute sp_data.valid? end - test "invalid-sp-config-5" do + test "invalid-sp-config-3" do sp_config = %{@sp_config1 | keyfile: "non-existent.pem"} %SpData{} = sp_data = SpData.load_provider(sp_config) refute sp_data.valid? end - test "invalid-sp-config-6" do + test "invalid-sp-config-4" do sp_config = %{@sp_config1 | certfile: "test/data/test.pem"} %SpData{} = sp_data = SpData.load_provider(sp_config) refute sp_data.valid? end @tag :skip - test "invalid-sp-config-7" do + test "invalid-sp-config-5" do sp_config = %{@sp_config1 | keyfile: "test/data/test.crt"} %SpData{} = sp_data = SpData.load_provider(sp_config) refute sp_data.valid?