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

Throw on missing cipher configuration? #117

Open
cybrox opened this issue Oct 27, 2020 · 2 comments
Open

Throw on missing cipher configuration? #117

cybrox opened this issue Oct 27, 2020 · 2 comments

Comments

@cybrox
Copy link

cybrox commented Oct 27, 2020

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 🤦

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

@paulstatezny
Copy link

paulstatezny commented Apr 12, 2021

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
Copy link
Owner

@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:

cloak/lib/cloak/vault.ex

Lines 295 to 302 in ead16b1

def encrypt(config, plaintext) do
with [{_label, {module, opts}} | _ciphers] <- config[:ciphers] do
module.encrypt(plaintext, opts)
else
_ ->
{:error, Cloak.InvalidConfig.exception("could not encrypt due to missing configuration")}
end
end

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.

@danielberkompas danielberkompas transferred this issue from danielberkompas/cloak_ecto Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants