Fixed #16

Author: handnot2

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.

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

lib/samly/idp_data.ex

@@ -140,14 +140,28 @@ 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}")
         idp_data
     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{}

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)

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"
 

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

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?