`:before_env_set` hook should be more flexible

[halostatue]

Two suggestions, but they sort of conflict with one another, so it might be better to have two different keys/hooks (only one should be configured):

1. Allow mfa(): before_env_set: {MySecretManager, :resolve_secrets, []} which would be called as apply(mod, fun, [pair | args]).
2. Allow a /2 function, which is not used by Map.new/2 directly, but is used with Enum.reduce/3. I'd love to specify this with mfa() as well, but there's some oddities involved that would need to be resolved.

Both of these are centred around the same issue, using an Elixir implementation of telia-oss/aws-env.

Using an MFA (entry 1), one could write a function that would parse values with sm:// or ssm:// or whatever and make an ExAws call to resolve the value. This would be inefficient, as ssm://foo/bar/baz showing up more than once would (of necessity) retrieve the same value. This could be mitigated with the process dictionary, but one would have no way of clearing that.

A different approach might be to do something like:

before_env_set: [collect: {MySecretManager, :collect, [%{}]}]

# elsewhere
defmodule MySecretManager do
  def collect(env, cache) do
    {vars, _cache} = Enum.reduce(env, {%{}, cache}, &resolve/2)
    vars
  end

  defp resolve({key, "sm://" <> lookup}, {acc, %{lookup => value} = cache}) do
    {Map.put(acc, key, value), cache}
  end

  defp resolve({key, "sm://" <> lookup}, {acc, cache}) 
    value = resolve_sm(lookup)
    {Map.put(acc, key, value), Map.put(cache, lookup, value)}
end

 defp resolve({key, value}, {acc, cache}), do: {Map.put(acc, key, value), cache}

I'm sure there's a better way to write this, but having an efficient way of resolving this without performing duplicate lookups would be very good. (There's more to what we have than what I've shown, but it's not really that different.)

[lud]

Hello,

This is a good idea!

We could have a :before_env_set_all hook that is given a map and must return an enumerable of pairs whose keys and values implement String.Chars.

(only one should be configured):

I'm not sure we should add that restriction, calling all defined hooks is simpler to implement and offers more flexibility.

Both hooks can coexist, I'm not sure which one should be called first though. I guess before_env_set could be implemented by using before_env_set_all, so if you define before_env_set_all then the other one is not called. Not sure if a good idea. Both could be called, but in which order?

ExUnit calls setup_all before setup, Benchee calls before_scenario before before_each, so I guess we can follow the pattern and call before_env_set_all before before_env_set.

BUT looking at the current implementation, Nvir handles regular variables and overwrites in a two-step way, it would be much simpler to add this change if before_env_set_all is called last.

What do you think?

[halostatue]

before_env_set_all sounds good, and yes, it should be last.

Documentation-wise, it should be encouraged to use one or the other, but I agree that there's no reason to prevent both with this definition.

I do think that having them be /1 functions or mfa() is the right call.

[lud]

Ah, in the meantime I was implementing it:

https://github.com/lud/nvir/compare/new-hooks

[lud]

Please test the branch and if it's fine I'll merge and release :)

[halostatue]

I don't have a project to test it in right now, but the concept and basic implementation look good.

[lud]

Alright I'll put that on hold for now :)

[halostatue]

I was able to use Kiro (Claude whatever) to develop a quick test app that mostly matches what we're doing, but it's a lot more verbose for verification. I've reviewed the code and it reasonably simulates what we're doing at work, and an adaptive version of what we'd use if we were to use nvir instead of dotenvy.

Here's the test project: nvir_test_app.tgz

And as a unit test: pseudo-secret-manager.patch

[lud]

=== Loading .env with secret resolution ===

  [FETCH] API_DEBUG_KEY -> secret:///api/keys#debug = pk_test_xyz789
  [CACHE HIT] API_KEY -> secret:///api/keys#prod = pk_live_abc123
  [FETCH] CACHE_TEST -> secret:///cache/test#value = cached_value_123
  [CACHE HIT] CACHE_TEST_2 -> secret:///cache/test#value = cached_value_123
  [FETCH] DATABASE_PASS -> secret:///db/prod/credentials#password = super_secret_pass
  [CACHE HIT] DATABASE_URL -> secret:///db/prod/credentials#url = postgres://prod.example.com:5432/mydb
  [CACHE HIT] DATABASE_USER -> secret:///db/prod/credentials#username = prod_user
  [PLAIN] PLAIN_VALUE = just_a_string

=== Secret Resolution Stats ===
Total fetches: 3
Cache hits: 4
Unique secrets: 3
===============================



=== Verifying resolved values ===
DATABASE_URL: postgres://prod.example.com:5432/mydb
DATABASE_USER: prod_user
DATABASE_PASS: super_secret_pass
API_KEY: pk_live_abc123
API_DEBUG_KEY: pk_test_xyz789
PLAIN_VALUE: just_a_string
CACHE_TEST: cached_value_123
CACHE_TEST_2: cached_value_123
===================================

Thank you, it seems to work in the way you intended. But are you trying to migrate from Dotenvy or something like that? In general in my packages I add new features based on someone's need only, to avoid feature creep.

I'va added your test to my branch

[halostatue]

Yes, I'm trying to make it possible to migrate from Dotenvy, because I think that Nvir's design choices are better.

I'm not entirely sure that I'm going to succeed with the push (our Dotenvy integration works), but to even consider it we need an "all" variant of the hook because each secret request is added to the usage count for that particular AWS API. (We could have implemented a naïve version with the before_set_env hook, but that would increase the number of API calls made. To mitigate that, we could have used Process.get and Process.put to make it work, but we would need to clean that up, leaking that particular detail in config.)

[lud]

Hey thank you :)

IIRC Dotenvy writes all variables to the process dictionary anyway. Add dbg(Process.get(:dotenvy_vars)) in your runtime.exs after the Dotenvy call. On some projects we have dotenvy and we have side_effect: &System.put_env/1 so in that case it's not in the pdict.

But I guess you already figured that out!

I'd love to have more users of my lib! But if I remember correctly the process that is running runtime.exs is short lived, so you have automatic cleanup anyway. That being said, it's true that malicious code could run that file, but if a malicious agent can run Elixir thats not your biggest problem 😅 .

Of course I don't know all your details. What possible threat do you imagine with the pdict?

[halostatue]

I'll need to look up the startup process—I think I always assumed that the runtime.exs processing was the equivalent of PID 0.

If it isn't, then we can skip this second hook, but I think that documentation on this sort of approach would be good to add.

[lud]

I think I'm going to merge it anyway, I just need some more time to review as I did that quickly.

If I print dbg(self()) in runtime.exs I get pid 95, so there are a lot of processes involved in boot I suppose.