Fixes #269. Initial work to support secrets.

[radcortez]

This is the initial work to implement #269 .

A new class SecretKeys can be configured with the key names to be considered secrets and access to these keys is protected unless it is explicitly asked for.

There are still some open questions:

• How do we protect ConfigSource#getPropertyNames? We could wrap it in our own source, but then it will become a different type for the user. Is this acceptable?
• We may need to expose the SecretKeys in the interceptor chain, so we can control access from the interceptors as well. This requires to add a getter method in ConfigSourceInterceptorContext.
• Users would need to cast Config to SmallRyeConfig to use this feature. Unless we give them some other direct API.

[radcortez]

@dmlloyd please, let me know if you think this is going into the right direction :)

[radcortez]

@dmlloyd This should be more in line with your review.

I think there are still 2 thing we need to figure out:

• How to protect ConfigSource#getPropertyNames?
• How do interceptors "exclude" or run in a protected context (in the case for a LoggingInterceptor) when the lock is open before the chain.

[dmlloyd]

Using the exception model, we don't need to filter the property names in any way: the key is not secret, and anyway property expressions could reference the key so the existence of the key can't really be hidden.

To "re-lock" we could have opposite methods which run a function of some sort with the lock flag re-set.

[radcortez]

Yes, it would be handled by the exception model.

In my mind, to be extra safe, I thought that we could configure all the secret keys plus any expansion variables that may contain sensitive information and exclude them in the property names.

[dmlloyd]

We don't need to go to that extent. If we rely solely on an exception model, and a user accesses a property with a variable in it which expands to a secret, we get behavior like this:

	Locked	Unlocked
Expanding	SecurityException	OK (secret is in expansion)
Not expanding	OK (no secret in expansion)	OK (no secret in expansion)

So we don't need to do any complex analysis (which is always dangerous when talking about security anyways) for this case. There doesn't seem to be any need to exclude the names of secret properties from this perspective: we have one unified protection mechanism, so that's only one thing that can potentially fail.

[radcortez]

Yes, my idea was not to do any complex analysis. It would on the user to add those names into the secret list and we could just exclude them from everywhere (include the list properties). Nevermind, lets not touch property names.

[dmlloyd]

Furthermore I'd imagine a logger would capture exceptions anyway, to be able to handle a variety of situations. Consider in this example that property2 is a secret, and property3 cannot be accessed (due to a transient problem in the config source perhaps). As a user I'd expect something like this:

property1 = "foo"
property2 = <secret>
property3 = <cannot access>
property4 = "bar ${property2}"

Catching exceptions for the exceptional cases really makes this easy. The logger doesn't really need any special knowledge, it just needs to catch RuntimeException. It doesn't even have to discriminate TBH; that's purely an extra-value feature.

[radcortez]

Yes, that is ok. Unless I'm missing something, the case that I wanted to prevent is when you have the lock open and you have the logger in the middle, but you don't want to log the access, right?

[dmlloyd]

If the question is "do we log accesses to secret keys", I think the answer is probably "why not?": if you're logging config accesses then you probably want that information as well. But whether or not it is actually logged, you still have to figure out whether it's secret, so you'd eventually need something like:

    try {
        ConfigValue maybeLocked = doLocked(() -> context.proceed(name));
        log(name, maybeLocked.getValue());
    } catch (SecurityException e) {
        log(name, "<locked>");
    } catch (RuntimeException e) {
        log(name, "<access error>");
    }
    return context.proceed(name);

[radcortez]

Yes, I think we can log secret config access, but omitting the value of course.

That is why we need a doLocked that we can do in the middle of a unlocked context, because we cannot trust the lock value at that point.

[radcortez]

I think it should be better now :)

[dmlloyd]

Other than the above comments, it's looking good.

[radcortez]

Thanks, let me fix that.