Hide kty in %JOSE.JWK{} when it is inspected

[spencerdcarlson]

This idea spawned from this discussion

Here is what the output would look like for an elixir client using Joken

iex(1)> signer = Joken.Signer.create("HS256", "s3cret")
%Joken.Signer{
  jwk: #JOSE.JWK<keys: :undefined, fields: %{}, ...>,
  jws: %JOSE.JWS{
    alg: {:jose_jws_alg_hmac, :HS256},
    b64: :undefined,
    fields: %{"typ" => "JWT"}
  },
  alg: "HS256"
}

I am not sure if the keys or fields defined in the jose_jwk record would ever hold private information, but I am hoping that this can start a discussion.

[victorolinasc]

I think that kty is the most safe claim to be printed. For each key type there are different fields considered private. For example, take a public and a private RS key example:

k = JOSE.JWK.generate_key  {:rsa, 1024}
JOSE.JWK.to_map k                      
{%{kty: :jose_jwk_kty_rsa},
 %{
   "d" => "hmNQPURAxO3rO7jLPHDIcbsGGvG1RiobZgqXV049BUdBcnBGUuNuEMF-QZXsrXA-4vcFR44rxehQQhnU_ZYWcTFTlOWRjN4H-6ufoAmZ38BePf3n8hKRT-RlBrlF7F1QOs5cV__AGp7gd9Sbd8ny_5Pw2HQZxPZAuZFOmXuNpzk",
   "dp" => "aR9tQn0EiXCZitSKoqgPfYcIPlREeO4MiQX8zGikVzWNNIp6f1kOrtrxafjJfkoxLlm98G71GxRo6ifcb5HgrQ",
   "dq" => "JtCP6D1eK7FnwogktcNhUchyG-AoQhNJ2_8_wBVV-VPuMlrCFDf-MkMy29kTUFSUBK6lmVQfVoDxWTbIdadRzQ",
   "e" => "AQAB",
   "kty" => "RSA",
   "n" => "mb8rEbiorcezpJ7RyKqXFhikBE6irPzbPn0Ey24-jaNWIujgfr7REGvSIJQ3Vv2unjvbeVLBmfClSbk8UqcAmOI7Jrlevl9b7Cmb1dbRQ3jRMnfwmbrfvF3JL1zXCaxSnHxouiRJPG-FMfbsH2wJ_waHZAA50dDKmXA0gx7jm58",
   "p" => "y7JcFZLoW5gN5Hbn0YQoET8IgLUxlhU0U0dMioY4SEDTCmXAONMxCBS1_zL_AlBRkWxgLGwBtfx86JZXOKYoSw",
   "q" => "wTlvxNUrZ05B60TrZ8hKTe7Y0tVR2cLmpgwXFpjrp0mrdN3Xulj9nPjExRkqZiogL_ctXanekN_ajQ9p945tfQ",
   "qi" => "dKtJWggGCiNMoIekigbVwrT3ovOqsizJ-ju2sgAtXoKjEvZcOGCrWM0YOB15gEknW_McbWZfpw8GQ0A6L2iTAg"
 }}
JOSE.JWK.to_public_map k
{%{kty: :jose_jwk_kty_rsa},
 %{
   "e" => "AQAB",
   "kty" => "RSA",
   "n" => "mb8rEbiorcezpJ7RyKqXFhikBE6irPzbPn0Ey24-jaNWIujgfr7REGvSIJQ3Vv2unjvbeVLBmfClSbk8UqcAmOI7Jrlevl9b7Cmb1dbRQ3jRMnfwmbrfvF3JL1zXCaxSnHxouiRJPG-FMfbsH2wJ_waHZAA50dDKmXA0gx7jm58"
 }}

For this key type, the claims e, kty and n are public fields. All others shouldn't be printed for this example. There are different fields for EC keys and so on. I think we should not merge this as is.

[spencerdcarlson]

@victorolinasc you bring up a good point of only redacting the parts that should truly be private.

I see a few options to go forward:

1. A new callback inspect/2 is added to jose_jwk and every key type dictates what should be displayed and what should be redacted. In this case JOSE.JWK's Inspect defimpl would simply delegate to :jose_jwk.inspect/1 which would delegate to each key type (oct, okp_ed448ph, okp_ed25519ph, okp_x25519, okp_x448, okp_ed25519, okp_ed448, ec, rsa)
2. In a custom defimpl for JOSE.JWK we handle every type of private key (see example code below or sdc/experimental/jwk-inspect).
3. We redact the entire kty value using @derive {Inspect, except: [:kty]} like this PR does.
4. Create Elixir Structs for all of the erlang record types and then use @derive to hide the private parts for each attribute

My vote would be for the third option given that is the easiest to implement and maintain. To be clear, this wouldn't have any impact on the internals of JOSE.JWK and if a user wanted to inspect or view the private parts of a key they can always invoke JOSE.JWK.to_map/1, Map.from_struct/1 directly, or pull out the kty:

jwk = %JOSE.JWK{kty: kty} = JOSE.JWK.generate_key({:rsa, 1024}) 

jwk
|> JOSE.JWK.to_map()
|> IO.inspect()

jwk
|> Map.from_struct()
|> IO.inspect()
 
IO.inspect(kty)

Overall hiding the internals seems like a high value effort since it is common to pass around a %JOSE.JWK{} through Plugs, GenServers, etc which often Log them out.

defimpl Inspect, for: JOSE.JWK do
  @excluded [:__struct__, :__exception__]

  def inspect(%JOSE.JWK{} = struct, opts) do
    struct = reject(struct)

    if gte_elixir_14_safe?() do
      # This might be overkill since we know what the fields in the struct are
      # [
      #  %{field: :keys, required: false},
      #  %{field: :kty, required: false},
      #  %{field: :fields, required: false}
      # ]
      # doing it this way does make it dynamic so that if the struct ever changes
      # then we don't need to modify multiple places.
      infos =
        for %{field: field} = info <- JOSE.JWK.__info__(:struct),
            field not in @excluded,
            do: info

      struct
      |> Inspect.Map.inspect("JOSE.JWK", infos, opts)
    else
      struct
      |> Map.drop(@excluded)
      |> Inspect.Map.inspect("JOSE.JWK", opts)
    end
  end

  def gte_elixir_14_safe?, do: Code.ensure_loaded?(Inspect.Map) and function_exported?(Inspect.Map, :inspect, 4)

  # Handle every type of key by redacting the private parts but keeping the public parts
  defp reject(%JOSE.JWK{kty: {:jose_jwk_kty_rsa, {:RSAPrivateKey, version, n, e, _, _, _, _, _, _, _}}} = struct) do
    # redacting private parts by replacing them with descriptive atoms
    %JOSE.JWK{
      struct
      | kty:
          {:jose_jwk_kty_rsa,
           {:RSAPrivateKey, version, n, e, :private_exponent, :prime_1, :prime_2, :exponent_1, :exponent_2, :coefficient,
            :other_prime_infos}}
    }
  end

  # Fallback to just redact the entire kty value
  defp reject(%JOSE.JWK{kty: kty} = struct), do: %JOSE.JWK{struct | kty: :redacted}
end

Looking forward to any thoughts and guidance.

[spencerdcarlson]

For this key type, the claims e, kty and n are public fields. All others shouldn't be printed for this example.

I'm not an expert, but just taking a look at the RSA Wikipedia Operation section

The public key is represented by the integers n and e, and the private key by the integer d (although n is also used during the decryption process, so it might be considered to be a part of the private key too)

Which would lead me to consider redacting n in a general Inspect of a %JOSE.JWK{} that is of type RSA.

[victorolinasc]

I like the approach of writing a custom inspect (and probably a custom to_string too). Although a higher effort, we have all the infrastructure in the library. It knows the types of keys we have and want here.

So, I think your approach with option 2 is sound and the best way forward here. We can support all the keys we can generate with JOSE.JWK.generate_key and just redact the private fields. One simple way to detect is to convert it to public key and "diff" them. This would, of course, be a little more expensive, but we can start this way and then optimize it case y case afterwards.

To my knowledge, n is considered public in RSA for JWK standard. This is what a public RSA struct here in JOSE has at least.

Wdyt?