Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

if a "kid" is included in the jwk, pass it to the jws as a hint #654

Merged
merged 3 commits into from Jun 4, 2020

Conversation

mpinkston
Copy link
Contributor

after implementing my own secret fetcher, I realized that the key id passed back in the JWK was not getting added to the signature.
As the secret frequently rotates, and the new key id is generated in the secret fetcher, there isn't a convenient place to pass the key id along in the :headers option.

This PR is to detect if a "kid" has been provided in the JWK and pass it along to the signature for easier lookups when there might be a list of more than one possible public key against which to verify.

Please let me know if this looks like a reasonable approach.

@mpinkston mpinkston requested a review from a team as a code owner May 29, 2020 10:00
Copy link
Contributor

@Hanspagh Hanspagh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @mpinkston. Looks good to me.
I guess this would require a custom secret fetcher to fetch the secret corresponding to a certain kid. Do you by any chance have an example of this, so we could add it to the readme?

@mpinkston
Copy link
Contributor Author

mpinkston commented Jun 1, 2020

sure, I'll add an example implementation in the morning (working JST)

@mpinkston
Copy link
Contributor Author

mpinkston commented Jun 2, 2020

@Hanspagh I wasn't sure how you wanted this organized in the docs, but this might provide something of a start.

Key Rotation

Guardian provides a Guardian.Token.Jwt.SecretFetcher behaviour that allows custom keys to be used for signing and
verifying requests.
This makes it possible to rotate private keys while maintaining a list of valid public keys that can be used both for
validating signatures as well as serving public keys to external services.

Below is a simple example of how this can be implemented using a GenServer.

defmodule MyApp.Guardian.KeyServer do
  @moduledoc ~S"""
  A simple GenServer implementation of a custom `Guardian.Token.Jwt.SecretFetcher`
  This is appropriate for development but should not be used in production
  due to questionable private key storage, lack of multi-node support,
  node restart durability, and public key garbage collection.
  """

  use GenServer

  @behaviour Guardian.Token.Jwt.SecretFetcher

  @impl Guardian.Token.Jwt.SecretFetcher
  # This will always return a valid key as a new one will be generated
  # if it does not already exist.
  def fetch_signing_secret(_mod, _opts),
    do: {:ok, GenServer.call(__MODULE__, :fetch_private_key)}

  @impl Guardian.Token.Jwt.SecretFetcher
  # This assumes that the adapter properly assigned a key id (kid)
  # to the signing key. Make sure it's there! with something like
  # JOSE.JWK.merge(jwk, %{"kid" => JOSE.JWK.thumbprint(jwk)})
  # see https://tools.ietf.org/html/rfc7515#section-4.1.4
  # for details
  def fetch_verifying_secret(_mod, %{"kid" => kid}, _opts) do
    case GenServer.call(__MODULE__, {:fetch_public_key, kid}) do
      {:ok, public_key} -> {:ok, public_key}
      :error -> {:error, :secret_not_found}
    end
  end

  def fetch_verifying_secret(_, _, _), do: {:error, :secret_not_found}

  # This is not a defined callback for the SecretFetcher, but could be useful
  # for providing an endpoint that external services could use to verify tokens
  # for themselves.
  def fetch_verifying_secrets,
    do: GenServer.call(__MODULE__, :fetch_public_keys)

  # Expire the private key so that a new one will be generated on the next
  # signing request. The public key associated with the old private key should
  # be stored at the very least as long as the largest possible "exp"
  # (https://tools.ietf.org/html/rfc7519#section-4.1.4) value for any token
  # signed by the old private key before this method was called.
  def expire_private_key,
    do: GenServer.cast(__MODULE__, :expire_private_key)

  # Generate a new keypair along with the key ID (kid)
  @spec generate_keypair() :: {:ok, JOSE.JWK.t(), JOSE.JWK.t(), String.t()}
  def generate_keypair() do
    # Choose an appropriate signing algorithm for your security needs.
    private_key = JOSE.JWK.generate_key({:okp, :Ed25519})

    # Generate a kid by using the key's thumbprint
    # https://tools.ietf.org/html/draft-ietf-jose-jwk-thumbprint-08#section-1
    kid = JOSE.JWK.thumbprint(private_key)

    # Update the private key to contain the "kid"
    private_key = JOSE.JWK.merge(private_key, %{"kid" => kid})

    # Create a public key based on the private key. It will carry the same "kid"
    public_key = JOSE.JWK.to_public(private_key)

    {:ok, private_key, public_key, kid}
  end

  def start_link(_opts) do
    GenServer.start_link(__MODULE__, :ok, name: __MODULE__)
  end

  def init(_opts) do
    {:ok, %{private_key: nil, public_keys: %{}}}
  end

  # Callbacks

  def handle_cast(:expire_private_key, state),
    do: {:noreply, %{state | private_key: nil}}

  # Generate a new signing key if one does not already exist
  def handle_call(:fetch_private_key, _from, %{private_key: nil, public_keys: key_list}) do
    {:ok, private_key, public_key, kid} = generate_keypair()

    {:reply, private_key,
     %{
       private_key: private_key,
       public_keys: Map.put(key_list, kid, public_key)
     }}
  end

  def handle_call(:fetch_private_key, _from, %{private_key: private_key} = state),
    do: {:reply, private_key, state}

  def handle_call({:fetch_public_key, kid}, _from, %{public_keys: public_keys} = state),
    do: {:reply, Map.fetch(public_keys, kid), state}

  def handle_call(:fetch_public_keys, _from, %{public_keys: public_keys} = state),
    do: {:reply, Map.values(public_keys), state}
end

Update Guardian's configuration to use the custom KeyServer.

## config/config.exs 

config :my_app, MyApp.Guardian,
  issuer: "myapp",
  allowed_algos: ["Ed25519"],
  secret_fetcher: MyApp.Guardian.KeyServer

Start the KeyServer in the supervision tree so it can serve requests.

## lib/my_app/application.ex

def start(_type, _args) do
  # List all child processes to be supervised
  children =
  [
    MyAppWeb.Endpoint,
    MyApp.Guardian.KeyServer
  ]

  # See https://hexdocs.pm/elixir/Supervisor.html
  # for other strategies and supported options
  opts = [strategy: :one_for_one, name: MyApp.Supervisor]
  Supervisor.start_link(children, opts)
end

@Hanspagh
Copy link
Contributor

Hanspagh commented Jun 2, 2020

Really nice :)
What I have done before is to make an example repo, and then add a snippet to the readme, to not make everything too bloated. What we could also do is add it to the guides section. Do you have a preference?

@mpinkston
Copy link
Contributor Author

mpinkston commented Jun 3, 2020

That's a very good question. You're right that it shouldn't get too bloated. I think good documentation describes a feature without prescribing an implementation.
In this case, it's a pretty simple feature, but it can open a rabbit-hole on how exactly it should be used.
How about some middle-ground? I've updated the PR with a snippet, but rather than a whole repo, I've added a link to a gist (which, of course, can be moved to a more appropriate link if needed).

@Hanspagh
Copy link
Contributor

Hanspagh commented Jun 3, 2020

Perfect 👌. I will add the key rotation to the advanced section secrets of the readme

@Hanspagh Hanspagh self-requested a review June 4, 2020 07:22
@Hanspagh Hanspagh merged commit 13815be into ueberauth:master Jun 4, 2020
@Hanspagh
Copy link
Contributor

Hanspagh commented Jun 4, 2020

Thanks 👍

@Hanspagh Hanspagh mentioned this pull request Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants