Unable to set secret_key in runtime.exs

geofflane commented 1 year ago

I've assumed that setting secret_key was something that you would want to be secret and so am using the "12 Factor" style for pulling in that value to the config at runtime instead of committing a hard-coded value to the config.exs in the repo. The recent change #702 changed the reading of all of those config values to compile time which breaks the setting of that value at runtime.

It feels like all of those things, including TTL and Issuer you might want to vary by environment for various reasons and should be read at runtime and not at compile time.

Steps to Reproduce

Have a default value for your secret_key in config/config.exs
Override that value in config/runtime.exs

Expected Result

Runtime value is used for secret, TTL, etc as needed.

Actual Result

ERROR! the application :photon has a different value set for key PhotonPerm.Guardian during runtime compared to compile time. Since this application environment entry was marked as compile time, this difference can lead to different behaviour than expected:

Compile time value was set to: [issuer: "PFF.Photon", ttl: {14, :days}, secret_key: "not secret, override in prod.exs"]
Runtime value was set to: [issuer: "PFF.Photon", ttl: {14, :days}, secret_key: "xxxxxxx"]

To fix this error, you might:

Make the runtime value match the compile time one
Recompile your project. If the misconfigured application is a dependency, you may need to run "mix deps.compile photon --force"

Hanspagh commented 1 year ago

Hi @geofflane. Thanks for the bug report. As I read the code, if you omit the secret_key from your config at compile time, Guardian should correctly fetch it from the env at runtime. The compile time envs should only be used at compile time by the macro. With the change to use compile_env elixir now tools like Mix will store the values used during compilation and compare the compilation values with the runtime values whenever your system starts, raising an error in case they differ. This properly why you get an error.

Would mind trying to not set the secret key at compile time and only at runtime.

geofflane commented 1 year ago

# config/config.exs
config :photon, PhotonPerm.Guardian,
  issuer: "PFF.Photon"

with

# config/runtime.exs
config :photon, PhotonPerm.Guardian,
  ttl: {14, :days},
  secret_key: System.fetch_env!("SECRET_KEY_BASE")

Has the same issue:

* Compile time value was set to: [issuer: "PFF.Photon"]
* Runtime value was set to: [issuer: "PFF.Photon", ttl: {14, :days}, secret_key: "xxx"]

idyll commented 1 year ago

This is impacting us as well and I don't think the changes that were made are actually intended...

Just spelunking in the code a bit here:

https://github.com/ueberauth/guardian/blob/master/lib/guardian.ex#L343

      # Provide a way to get at the configuration during compile time
      # for other macros that may want to use them
      @config fn ->
        the_otp_app |> Application.compile_env(__MODULE__, []) |> Keyword.merge(the_opts)
      end
      @config_with_key fn key ->
        @config.() |> Keyword.get(key) |> Guardian.Config.resolve_value()
      end
      @config_with_key_and_default fn key, default ->
        @config.() |> Keyword.get(key, default) |> Guardian.Config.resolve_value()
      end

^^^ I don't see anywhere that this is used.

And the problem is actually this code itself I think. It's not obvious to me what this is being used for but I think it's the issue. @doomspork can you comment on where @config is used?

I think the way to resolve this is to just delete that code. I think that Elixir releases means that it no longer behaves the way it was intended 5 years ago when it was written.

(but I could totally be wrong because it's not clear to me what uses it.)

idyll commented 1 year ago

Digging a bit deeper it looks like permissions uses @config_with_key. But I think it can be refactored so that it uses a function. @config_with_key_and_default isn't used.

If permissions really needs this at compile time maybe we could compile_env for just that one key?

Hanspagh commented 1 year ago

# config/config.exs
config :photon, PhotonPerm.Guardian,
  issuer: "PFF.Photon"

with

# config/runtime.exs
config :photon, PhotonPerm.Guardian,
  ttl: {14, :days},
  secret_key: System.fetch_env!("SECRET_KEY_BASE")

Has the same issue:

* Compile time value was set to: [issuer: "PFF.Photon"]
* Runtime value was set to: [issuer: "PFF.Photon", ttl: {14, :days}, secret_key: "xxx"]

Yea okay, this seems to not be intended. As @idyll writes looks like this compile time env vars are specific to the permission, the only concern I have is if some library users use this as well. I guess we can fix this up and cut a major release just to be sure?

Hanspagh commented 1 year ago

I think we should be able to the compile_only only for the permission key of the environment, and then consider that to be the only compile time var atm.

idyll commented 1 year ago

That's my expectation. That should fix it.

Hanspagh commented 1 year ago

Would any of you @idyll @geofflane be interested in fixing this or maybe @yordis. I have quite a busy schedule lately so I wont have time to look at this atm.

yordis commented 1 year ago

Something told me that the fix wouldn't work. I asked Jose about it, and he said it was safe to merge. My apologies.

This week I am in the middle of deploying a brand new system, so I don't have time, but I am more than happy to help with some code review.

If you want to take the lead, please do. Otherwise, let me see if I find some time for it.

idyll commented 1 year ago

I probably can look in a day. If someone gets to it before me, let me know.

ericdude4 commented 1 year ago

# config/config.exs
config :photon, PhotonPerm.Guardian,
  issuer: "PFF.Photon"

with

# config/runtime.exs
config :photon, PhotonPerm.Guardian,
  ttl: {14, :days},
  secret_key: System.fetch_env!("SECRET_KEY_BASE")

Has the same issue:

* Compile time value was set to: [issuer: "PFF.Photon"]
* Runtime value was set to: [issuer: "PFF.Photon", ttl: {14, :days}, secret_key: "xxx"]

Any update here? Running into the same issue on our application with this exact same configuration

geofflane commented 1 year ago

Took a stab at a PR for this. It's runtime behaviour so it's a bit hard to write tests around. Any help there would be appreciated.

ueberauth / guardian