Secret is lost when svc process crashes and restarted

[gomoripeti]

When credentials_obfuscation_svc crashes and restarted with a fresh state it will have the initial pending-secret. If no caller calls set_secret again (and how would the caller knew it should) it will stay in this state going forward. It can result in:

• from this point on no data encrypted before the crash can be decrypted
• exposing this bug https://github.com/rabbitmq/credentials-obfuscation/pull/17

What if the secret (and probably other state as well) would be stored in persistent_term. encrypt/decrypt could be executed in the calling process, the gen_server is only kept for serialising state updates?

This would also prevent the secrets being logged at gen_server crash. Would this hurt observability?

Any feedback is appreciated.

[gomoripeti]

alternative 3: use anonymous funs instead of encryption eg as in https://github.com/rabbitmq/rabbitmq-server/pull/3806

[michaelklishin]

Supporting anonymous functions or MFAs to set the secret should be fine as long as we make sure their arguments themselves are never logged.

[gomoripeti]

what I meant is just wrapping a term into an anon fun (instead of encryption) as a way of obfuscation. Then it does not need encryption config and state/secret and no need for a gen_server process. Eg:

encrypt(Term) -> {encrypted, fun() -> Term end}.
decrypt({encrypted, Fun}) -> Fun().

Or is there any use-case/customer that needs actually encryption? (One disadvantage I can think of is if the secret at a past point in time is known from some source eg it is explicitly configured, then encrypted secrets in old logs can still be decrypted for debugging purposes. There is no way to decrypt #Fun<...> just from the logs, without the actual term in the live VM being available)

[michaelklishin]

This option was considered but we decided to go with encryption. You cannot know how a future Erlang logger would potentially log anonymous functions differently.

[gomoripeti]

makes sense, thanks for the historic insight.

then in order to address the original issue, that if the svc process crashes, it loses the secret, would storing the state in persistent_term be acceptable? (now that only new enough versions of OTP are supported which all have persistent_term) (if yes, Im willing to prepare a PR)

[michaelklishin]

@gomoripeti a persistent term with a well-known name sounds like a terrible idea security-wise. Persistent terms can be accessed by any process.

If we generate a persistent term name from a unique value seeded on node boot (or application start), we might be able to make it much harder to discover since there is no way to list persistent terms.

Another thing @MirahImage and I have discussed is using process state and changing the supervision tree. Unfortunately, I don't see a sane way around the same problem we developed this library to solve: supervisor arguments can be logged by Erlang's logger without much control from us. Log message filtering is too fragile to address that.

So I don't have a better idea than:

• Generate a persistent term key "identity" on application start
• Find a way to transfer it to the supervision tree that the svc process belongs to
• Fetch the term there

This may not be meaningfully more secure without wrapping this secret into another one — a technique commonly used in all kinds of major cloud services — but ultimately there will be a value read from memory that's used as key material in the current design.

Maybe at some point this would offer Vault integration, which would be the best option IMO: just offload key management to a service designed for it.

[michaelklishin]

@gomoripeti actually, an anonymous function might be a good enough solution to avoid logging this "unique persistent key identity" (UPKI) as part of supervisor arguments.

With UPKI we can also clear the term when the application is stopped.

[gomoripeti]

thanks Michal, I see this is complicated, so I refrain from major changes of this lib for now

Just to clarify the expectations against this lib

• I see in the README:

  this application cannot protect against heap dumping attacks and only helps avoid sensitive data appearing in log files.

• Also currently there is credentials_obfuscation:secret() to fetch the secret (which is the same as reading a well-known key from persistent_term).

Because of this existing API and the Readme I thought it is not a goal to give protection if someone has shell access to the node.

[michaelklishin]

Fair enough, we can use a persistent term, even though it would be susceptible to another type of attack: bogus plugins that would be able to access the term.

[michaelklishin]

@gomoripeti I asked half a dozen of engineers about this and we have a consensus that using a persistent term with a well-known key is fine. We just have to be careful to also pass the fallback secret on the same way.