danielberkompas / cloak

Elixir encryption library designed for Ecto
MIT License
564 stars 55 forks source link

Throw on missing cipher configuration? #117

Open cybrox opened 3 years ago

cybrox commented 3 years ago

First of all, thank your for maintaining cloak_ecto!

I was wondering, in terms of usability, wouldn't it make sense for the library to throw an error when no cipher is configured?
As far as I can see, the library is unusable without a suitable configuration.

My reasoning for this is, I just spent an hour attempting to figure out the following error:

** (Ecto.ChangeError) value `"test"` for `MyModel.encrypted_api_token` in `update` does not match type MyApp.Encrypted.Binary
    (ecto 3.5.3) lib/ecto/repo/schema.ex:889: Ecto.Repo.Schema.dump_field!/6
    (ecto 3.5.3) lib/ecto/repo/schema.ex:898: anonymous fn/6 in Ecto.Repo.Schema.dump_fields!/5
    (stdlib 3.12) maps.erl:232: :maps.fold_1/3
    (ecto 3.5.3) lib/ecto/repo/schema.ex:896: Ecto.Repo.Schema.dump_fields!/5
    (ecto 3.5.3) lib/ecto/repo/schema.ex:829: Ecto.Repo.Schema.dump_changes!/6
    (ecto 3.5.3) lib/ecto/repo/schema.ex:334: anonymous fn/15 in Ecto.Repo.Schema.do_update/4
    (ecto 3.5.3) lib/ecto/repo/schema.ex:177: Ecto.Repo.Schema.update!/4
    (elixir 1.11.1) lib/enum.ex:1399: Enum."-map/2-lists^map/1-0-"/2

However, I searched in all the wrong places. I double checked all my database types, checked if my ecto adapter (myxql) supported the correct data type, updated ecto, etc. etc... After a lot of digging, I ended up adding an inspect to lib/cloak_ecto/type.ex in dump/1's error clause and got this helpful message %Cloak.InvalidConfig{message: "could not encrypt due to missing configuration"}}

As it turns out, I had put the following in my vault module:

  @impl GenServer
  def init(config) do
    config =
      Keyword.put(config, :cyphers,
        default: {Cloak.Ciphers.AES.GCM, tag: "AES.GCM.V1", key: decode_env!("CLOAK_KEY")}
      )

    {:ok, config}
  end

After changing :cyphers to :ciphers, as it is spelled in the docs, everything worked fine :facepalm:

So I was wondering, wouldn't it make sense to throw an error when no cipher configuration is provided at all?

paulstatezny commented 3 years ago

Also a thanks from me for cloak_ecto! And ➕ 1 on this. I ran into a similar error and was thrown off until I eventually found this GitHub issue. Thanks for considering!

danielberkompas commented 2 years ago

@cybrox @paulstatezny This is actually an issue for the cloak package, because that's where this error would need to be implemented. It does already return an error when you call encrypt with invalid configuration:

https://github.com/danielberkompas/cloak/blob/ead16b16e34ad8393b1f64b54cbd796da4af6975/lib/cloak/vault.ex#L295-L302

But Cloak.Ecto swallows it in the dump function:

https://github.com/danielberkompas/cloak_ecto/blob/2d7ceb1ea9cbe94bdb72a13a6a01a57d753c7377/lib/cloak_ecto/type.ex#L34-L43

The only way I can think of to fix this is to make the vault raise an error or log a warning when it boots up, if no valid configuration is given.