Implement PKCE

CHANGELOG.md

@@ -36,6 +36,28 @@
 
 This is a full rewrite of the library, and are several breaking changes. You're encouraged to test your app well if you upgrade from 0.4.
 
+### Upgrading from 0.5
+
+#### 1. DB
+
+Add the string fields `code_challenge` and `code_challenge_method` to the table `<scope>_access_grants`.
+Example migration file:
+
+```elixir
+# file: accounts/priv/repo/migrations/20210821193238_update_oauth_tables.exs
+defmodule Accounts.Repo.Migrations.UpdateOauthTables do
+  use Ecto.Migration
+
+  def change do
+    alter table(:oauth_access_grants) do
+      add :code_challenge, :string
+      add :code_challenge_method, :string
+    end
+  end
+end
+
+```
+
 ### Upgrading from 0.4
 
 #### 1. Schema modules

README.md

@@ -91,7 +91,22 @@ Revocation will return `{:ok, %{}}` status even if the token is invalid.
 
 ### Authorization code flow in a Single Page Application
 
-ExOauth2Provider doesn't support **implicit** grant flow. Instead you should set up an application with no client secret, and use the **Authorize code** grant flow. `client_secret` isn't required unless it has been set for the application.
+ExOauth2Provider doesn't support **implicit** grant flow. Instead you should set up an application with no client secret, and use the **Authorize code** grant flow. `client_secret` isn't required unless it has been set for the application. 
+It's **strongly** encouraged to enable PKCE for these applications.
+
+
+#### PKCE
+
+Enable PKCE in configuration `config/config.ex`:
+
+```elixir
+config :my_app, ExOauth2Provider,
+  # ...
+  # this will enable PKCE for *all* applications
+  use_pkce: true
+```
+
+When making an authorization code flow you are now required to provide a `code_challenge` and `code_challenge_method` query fields for the authorization request and `code_verifier` field for the access token request, as per [RFC-7637](https://datatracker.ietf.org/doc/html/rfc7636).
 
 ### Other supported token grants
 

lib/ex_oauth2_provider/access_grants/access_grant.ex

@@ -27,7 +27,9 @@ defmodule ExOauth2Provider.AccessGrants.AccessGrant do
       {:expires_in, :integer, null: false},
       {:redirect_uri, :string, null: false},
       {:revoked_at, :utc_datetime},
-      {:scopes, :string}
+      {:scopes, :string},
+      {:code_challenge, :string},
+      {:code_challenge_method, :string}
     ]
   end
 
@@ -66,7 +68,7 @@ defmodule ExOauth2Provider.AccessGrants.AccessGrant do
   @spec changeset(Ecto.Schema.t(), map(), keyword()) :: Changeset.t()
   def changeset(grant, params, config) do
     grant
-    |> Changeset.cast(params, [:redirect_uri, :expires_in, :scopes])
+    |> Changeset.cast(params, [:redirect_uri, :expires_in, :scopes, :code_challenge, :code_challenge_method])
     |> Changeset.assoc_constraint(:application)
     |> Changeset.assoc_constraint(:resource_owner)
     |> put_token()

lib/ex_oauth2_provider/config.ex

@@ -100,6 +100,11 @@ defmodule ExOauth2Provider.Config do
   def use_refresh_token?(config),
     do: get(config, :use_refresh_token, false)
 
+  # Use PKCE for 'authorization code' grant type (https://datatracker.ietf.org/doc/html/rfc7636)
+  @spec use_pkce?(keyword()) :: boolean()
+  def use_pkce?(config),
+    do: get(config, :use_pkce, false)
+
   # Password auth method to use. Disabled by default. When set, it'll enable
   # password auth strategy. Set config as:
   # `password_auth: {MyModule, :my_auth_method}`

lib/ex_oauth2_provider/oauth2/authorization/strategy/code.ex

@@ -63,7 +63,8 @@ defmodule ExOauth2Provider.Authorization.Code do
     Authorization.Utils.Response,
     RedirectURI,
     Scopes,
-    Utils.Error}
+    Utils.Error,
+    Utils.Validation}
   alias Ecto.Schema
 
   @doc """
@@ -140,9 +141,15 @@ defmodule ExOauth2Provider.Authorization.Code do
 
   defp issue_grant({:error, %{error: _error} = params}, _config), do: {:error, params}
   defp issue_grant({:ok, %{resource_owner: resource_owner, client: application, request: request} = params}, config) do
-    grant_params =
-      request
-      |> Map.take(["redirect_uri", "scope"])
+    filtered_request = if Config.use_pkce?(config) do
+      Map.merge(%{"code_challenge_method" => "plain"}, request)
+      |> Map.take(["redirect_uri", "scope", "code_challenge", "code_challenge_method"])
+      |> Map.update!("code_challenge", fn v -> String.replace(v, "=", "") end)
+    else
+      Map.take(request, ["redirect_uri", "scope"])
+    end
+
+    grant_params = filtered_request
       |> Map.new(fn {k, v} ->
         case k do
           "scope" -> {:scopes, v}
@@ -188,6 +195,7 @@ defmodule ExOauth2Provider.Authorization.Code do
     |> validate_resource_owner()
     |> validate_redirect_uri(config)
     |> validate_scopes(config)
+    |> validate_pkce(Config.use_pkce?(config))
   end
 
   defp validate_resource_owner({:ok, %{resource_owner: resource_owner} = params}) do
@@ -225,4 +233,29 @@ defmodule ExOauth2Provider.Authorization.Code do
     end
   end
   defp validate_redirect_uri({:ok, params}, _config), do: Error.add_error({:ok, params}, Error.invalid_request())
+
+  defp validate_pkce({:error, params}, _use_pkce?), do: {:error, params}
+  defp validate_pkce({:ok, params}, false), do: {:ok, params}
+  defp validate_pkce({:ok, %{request: %{"code_challenge" => code_challenge} = request} = params}, true) do
+    code_challenge_method = Map.get(request, "code_challenge_method", "plain")
+
+    if valid_code_challenge_format?(code_challenge, code_challenge_method) do
+      {:ok, params}
+    else
+      Error.add_error({:ok, params}, Error.invalid_request())
+    end
+  end
+  defp validate_pkce({:ok, params}, true), do: Error.add_error({:ok, params}, Error.invalid_request()) # missing code_challenge
+
+  @sha256_byte_size 256/8
+
+  defp valid_code_challenge_format?(nil, _code_challenge_method), do: false
+  defp valid_code_challenge_format?(code_challenge, "plain"), do: Validation.valid_code_verifier_format?(code_challenge)
+  defp valid_code_challenge_format?(code_challenge, "S256") do
+    case Base.url_decode64(code_challenge, padding: false) do # padding '=' deliberately accepted
+      {:ok, bin} -> byte_size(bin) == @sha256_byte_size
+      :error -> false
+    end
+  end
+  defp valid_code_challenge_format?(_code_challenge, _code_challenge_method), do: false
 end

lib/ex_oauth2_provider/oauth2/token/strategy/authorization_code.ex

@@ -8,7 +8,8 @@ defmodule ExOauth2Provider.Token.AuthorizationCode do
     Config,
     Token.Utils,
     Token.Utils.Response,
-    Utils.Error}
+    Utils.Error,
+    Utils.Validation}
 
   @doc """
   Will grant access token by client credentials.
@@ -32,6 +33,7 @@ defmodule ExOauth2Provider.Token.AuthorizationCode do
     |> Utils.load_client(config)
     |> load_active_access_grant(config)
     |> validate_redirect_uri()
+    |> validate_pkce()
     |> issue_access_token_by_grant(config)
     |> Response.response(config)
   end
@@ -89,4 +91,22 @@ defmodule ExOauth2Provider.Token.AuthorizationCode do
     end
   end
   defp validate_redirect_uri({:ok, params}), do: Error.add_error({:ok, params}, Error.invalid_grant())
+
+  defp validate_pkce({:error, params}), do: {:error, params}
+  defp validate_pkce({:ok, %{access_grant: %{code_challenge_method: nil}} = params}), do: {:ok, params} # pkce not enabled for this grant
+  defp validate_pkce({:ok, %{request: %{"code_verifier" => actual_code}, access_grant: %{code_challenge: expected_code, code_challenge_method: challenge_method}} = params}) do
+    if Validation.valid_code_verifier_format?(actual_code) && valid_pkce?(actual_code, expected_code, challenge_method) do
+      {:ok, params}
+    else
+      Error.add_error({:ok, params}, Error.invalid_grant())
+    end
+  end
+  defp validate_pkce({:ok, params}), do: Error.add_error({:ok, params}, Error.invalid_request())
+
+  defp valid_pkce?(actual_code, expected_code, "plain"), do: Plug.Crypto.secure_compare(actual_code, expected_code)
+  defp valid_pkce?(actual_code, expected_code, "S256") do
+    :crypto.hash(:sha256, actual_code)
+    |> Base.url_encode64(padding: false)
+    |> Plug.Crypto.secure_compare(expected_code)
+  end
 end

lib/ex_oauth2_provider/oauth2/utils/validation.ex

@@ -0,0 +1,10 @@
+defmodule ExOauth2Provider.Utils.Validation do
+  @moduledoc false
+
+  # see https://datatracker.ietf.org/doc/html/rfc7636#section-4.1
+  @code_verifier_regex ~r/^[[:alnum:]._~-]{43,128}$/
+
+  @spec valid_code_verifier_format?(String.t()) :: boolean
+  def valid_code_verifier_format?(nil), do: false
+  def valid_code_verifier_format?(code_verifier), do: String.match?(code_verifier, @code_verifier_regex)
+end

test/ex_oauth2_provider/oauth2/authorization/strategy/code_pkce_test.exs

@@ -0,0 +1,135 @@
+defmodule ExOauth2Provider.Authorization.CodePkceTest do
+  use ExOauth2Provider.TestCase
+
+  alias ExOauth2Provider.Authorization
+  alias ExOauth2Provider.Test.Fixtures
+  alias Dummy.{OauthAccessGrants.OauthAccessGrant, Repo}
+
+  @code_challenge "1234567890abcdefrety1234567890abcdefrety.~-_"
+  @s256_code_challenge :crypto.hash(:sha256, @code_challenge) |> Base.url_encode64(padding: false)
+  @client_id "Jf5rM8hQBc"
+  @missing_code_challenge_request %{
+    "client_id" => @client_id,
+    "response_type" => "code",
+    "scope" => "app:read app:write"
+  }
+  @valid_plain_request Map.merge(@missing_code_challenge_request, %{
+                         "code_challenge" => @code_challenge
+                       })
+  @valid_explicit_plain_request Map.merge(@missing_code_challenge_request, %{
+                                  "code_challenge" => @code_challenge,
+                                  "code_challenge_method" => "plain"
+                                })
+  @valid_s256_request Map.merge(@missing_code_challenge_request, %{
+                        "code_challenge" => @s256_code_challenge,
+                        "code_challenge_method" => "S256"
+                      })
+  @invalid_request %{
+    error: :invalid_request,
+    error_description:
+      "The request is missing a required parameter, includes an unsupported parameter value, or is otherwise malformed."
+  }
+
+  setup_all %{} do
+    new_conf = Application.get_env(:ex_oauth2_provider, ExOauth2Provider) ++ [use_pkce: true]
+    Application.put_env(:ex_oauth2_provider, ExOauth2Provider, new_conf)
+
+    :ok
+  end
+
+  setup do
+    resource_owner = Fixtures.resource_owner()
+    application = Fixtures.application(uid: @client_id, scopes: "app:read app:write")
+    {:ok, %{resource_owner: resource_owner, application: application}}
+  end
+
+  test "#preauthorize/3 missing code_challenge", %{resource_owner: resource_owner} do
+    assert Authorization.preauthorize(resource_owner, @missing_code_challenge_request,
+             otp_app: :ex_oauth2_provider
+           ) == {:error, @invalid_request, :bad_request}
+  end
+
+  test "#authorize/3 missing code_challenge", %{resource_owner: resource_owner} do
+    assert Authorization.authorize(resource_owner, @missing_code_challenge_request,
+             otp_app: :ex_oauth2_provider
+           ) == {:error, @invalid_request, :bad_request}
+  end
+
+  test "#authorize/3 invalid code_challenge_method", %{resource_owner: resource_owner} do
+    request = Map.merge(@valid_plain_request, %{"code_challenge_method" => "invalid"})
+
+    assert Authorization.authorize(resource_owner, request, otp_app: :ex_oauth2_provider) ==
+             {:error, @invalid_request, :bad_request}
+  end
+
+  test "#authorize/3 invalid plain code_challenge", %{resource_owner: resource_owner} do
+    request =
+      Map.merge(@valid_plain_request, %{
+        "code_challenge" => @code_challenge <> "<<bad_character>>"
+      })
+
+    assert Authorization.authorize(resource_owner, request, otp_app: :ex_oauth2_provider) ==
+             {:error, @invalid_request, :bad_request}
+  end
+
+  test "#authorize/3 invalid s256 code_challenge", %{resource_owner: resource_owner} do
+    request = Map.merge(@valid_s256_request, %{"code_challenge" => @s256_code_challenge <> "baddecode"})
+
+    assert Authorization.authorize(resource_owner, request, otp_app: :ex_oauth2_provider) ==
+             {:error, @invalid_request, :bad_request}
+  end
+
+  test "#authorize/3 implicit plain code_challenge_method generates grant", %{
+    resource_owner: resource_owner
+  } do
+    assert {:native_redirect, %{code: code}} =
+             Authorization.authorize(resource_owner, @valid_plain_request,
+               otp_app: :ex_oauth2_provider
+             )
+
+    owner_id = resource_owner.id
+
+    assert %{
+             resource_owner_id: ^owner_id,
+             code_challenge: @code_challenge,
+             code_challenge_method: "plain",
+             scopes: "app:read app:write"
+           } = Repo.get_by(OauthAccessGrant, token: code)
+  end
+
+  test "#authorize/3 explicit plain code_challenge_method generates grant", %{
+    resource_owner: resource_owner
+  } do
+    assert {:native_redirect, %{code: code}} =
+             Authorization.authorize(resource_owner, @valid_explicit_plain_request,
+               otp_app: :ex_oauth2_provider
+             )
+
+    owner_id = resource_owner.id
+
+    assert %{
+             resource_owner_id: ^owner_id,
+             code_challenge: @code_challenge,
+             code_challenge_method: "plain",
+             scopes: "app:read app:write"
+           } = Repo.get_by(OauthAccessGrant, token: code)
+  end
+
+  test "#authorize/3 S256 code_challenge_method generates grant", %{
+    resource_owner: resource_owner
+  } do
+    assert {:native_redirect, %{code: code}} =
+             Authorization.authorize(resource_owner, @valid_s256_request,
+               otp_app: :ex_oauth2_provider
+             )
+
+    owner_id = resource_owner.id
+
+    assert %{
+             resource_owner_id: ^owner_id,
+             code_challenge: @s256_code_challenge,
+             code_challenge_method: "S256",
+             scopes: "app:read app:write"
+           } = Repo.get_by(OauthAccessGrant, token: code)
+  end
+end

test/ex_oauth2_provider/oauth2/authorization/strategy/code_test.exs

@@ -154,6 +154,13 @@ defmodule ExOauth2Provider.Authorization.CodeTest do
     assert Authorization.authorize(resource_owner, request, otp_app: :ex_oauth2_provider) == {:error, @invalid_redirect_uri, :unprocessable_entity}
   end
 
+  test "#authorize/3 generates grant without persisting code_challenge, code_challenge_method on disabled pkce", %{resource_owner: resource_owner} do
+    request = Map.merge(@valid_request, %{"code_challenge" => "1234567890abcdefrety1234567890abcdefrety1234", "code_challenge_method" => "plain"})
+
+    assert {:native_redirect, %{code: code}} = Authorization.authorize(resource_owner, request, otp_app: :ex_oauth2_provider)
+    assert %{code_challenge: nil, code_challenge_method: nil} = Repo.get_by(OauthAccessGrant, token: code)
+  end
+
   test "#authorize/3 generates grant", %{resource_owner: resource_owner} do
     assert {:native_redirect, %{code: code}} = Authorization.authorize(resource_owner, @valid_request, otp_app: :ex_oauth2_provider)
     access_grant = Repo.get_by(OauthAccessGrant, token: code)