Hide secrets in Configuration

[radcortez]

Original issue from Quarkus:
quarkusio/quarkus#7442

But I think we should support this at the SmallRye Config level.

[radcortez]

As a first step, I think we could support a isSecret method in our ConfigValue object. This will allow to implement something (most likely an interceptor), that marks configs as a secrets, using a configured list of configuration names.

Ideally, we could also use or combine the target type (from security package) and automatically mark the config as a secret. This will require some changes to be able to pass in the type in the interceptor chain.

If we do add a isSecret method in our ConfigValue, what should getValue return? Do we return a placeholder if is is a secret? Do we continue to return the value as usual? Do we provide a new method to retrieve the plain value? The issue is that other interceptors in the chain may require the plain value and not a placeholder, for instance the ExpressionConfigSourceInterceptor.

Also, by the end of the chain, the plain value is needed to pass to the Converter. In this case, it is easier to manage, since this code is controlled by the implementation.

[radcortez]

A few other ideias to consider:

• Add a new SecretConfigValue. This will allow you retrieve the plain String from the source, while ConfigValue always masks the secret. Most likely, the plain String will still need to be part of the ConfigValue instance.
• Add interface SecretConfigValueAware? This could mark interceptors that are safe to read the plain String and unwrap the value? Or simple rewrite the ConfigValue to make getValue return the plain String and then hide it again when it needs to enter in a non aware secret interceptor?

[dmlloyd]

An interceptor handling secrets would probably run earlier, not later, so it could short-circuit the interception process. It could even throw an exception.

A placeholder makes sense in concept but converters for secret types would have to be aware of the possibility. And it could get difficult if the secret was a clear password of type String or something string-like because any placeholder value would be indistinguishable from a clear password. Supporting clear passwords in configuration in any way is probably not a good security practice - at the least, passwords probably should be masked or encoded somehow.

Interceptors can always make their own config values, so information could easily be lost if we try to subtype ConfigValue in any way.

I don't have any answers, only more problems. :-)

[radcortez]

I don't have any answers, only more problems. :-)

No worries, this was just to brainstorm some ideas :)

An interceptor handling secrets would probably run earlier, not later, so it could short-circuit the interception process. It could even throw an exception.

Yes, and what about expansion? Should expansion be done first or after? If you do it before, you are reading and exposing the secret to the next chain, that may be the secret interceptor or not to hide it. If you do it after, you require the plain String for expand. Unless, we also mark the ConfigValue has requiresExpansion and hide the value right after expansion.

A placeholder makes sense in concept but converters for secret types would have to be aware of the possibility. And it could get difficult if the secret was a clear password of type String or something string-like because any placeholder value would be indistinguishable from a clear password. Supporting clear passwords in configuration in any way is probably not a good security practice - at the least, passwords probably should be masked or encoded somehow.

Yes. I agree. I'm not sure if there is an easy solution about this. While we could find information from the converted or injected type, no one can stop the user to use a simple String to do it, so I think we need to accept that this is going to happen.

Interceptors can always make their own config values, so information could easily be lost if we try to subtype ConfigValue in any way.

We control how ConfigValue is created, so we could keep some record of what we need. On the other hand, I wouldn't be too worried about this. If later in the chain, interceptors provide their own value, this is acceptable as long as we don't leak the secret. Our provided chain must be coherent. If the user does something else, we cannot really control it.

[radcortez]

Trying to capture more detail:

What do we want to prevent:

• Printing / Showing the secrets in logs
• Keep a memory representation of the plain text secret?

We can assume:

• We cannot prevent users to store plain text password in configs
• We cannot rely on users configuring all the keys that store passwords for us to protect
• We cannot prevent users to use a String to retrieve the value (instead of a specific security type to deal with secrets)
• Converters require access to the plain config value
• If we mask the secret we cannot prevent the developer to unmask it manually

What can we do:

• We can mask and salt the secret once we read it
• Provide an API to mask / unmask secret
• We don't want to replace other OOTB solutions like Vault (I think)
• We can filter secrets from being iterated

Questions:

• Do we want to lock the secret access to the entire interceptor chain?
• How do we handle ConfigValue in getConfigValue if you are calling programatically?
• Do we provide a separate API to unmask the secret?
• Should we expand the secret before or after? Or maybe expand it directly in a SecretInterceptor and skip the regular ExpandInterceptor?

[dmlloyd]

That's a really good & clear summary.

Given these assumptions, it seems to me that that the best possible course is as follows:

• Require something to register the configuration keys (or a key-matching predicate) to identify values that are secret (because if we don't know what values are secret, there's nothing else we can do)
• Add an interceptor near the end of the chain that throws a SecurityException if a secret key is accessed, unless the "opt in" flag is set
• Define the "opt in" flag as a hidden thread local boolean which is set using the run(body) pattern we use in other places, which prevents the flag from "leaking"

This solves the problem of how converters would deal with masked values (answer: they do not need to), how to filter secrets from iteration (answer: the user can either explicitly opt in to view secret values, or else catch the exception and note that the value can't be accessed), how to handle programmatic access (it'll give a runtime exception if the keys are locked, just the same as if validation or conversion would fail), and how to handle expansion of secrets (by putting the interceptor at the end, keys which reference secret keys are transitively protected).

Injection of secret values could either be done with secrets unlocked, or else we could possibly introduce some opt-in annotation (though I think that's probably not necessary, since injecting a secret value seems like an explicit opt-in to me and is unlikely to be part of a general iteration/logging kind of case that this mechanism is designed to protect).

I honestly can't think of any other viable solution. I think the usage of the runtime SecurityException is definitely in line with the overall MP Config API philosophy.

[radcortez]

Thanks!

Ok, let me work on a prototype and see how it looks like.