Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WFCORE-4360] Support expression resolution backed by a CredentialStore. #373

Merged
merged 23 commits into from Mar 10, 2021

Conversation

darranl
Copy link
Contributor

@darranl darranl commented Feb 10, 2021

This pull request supersedes #213

This analysis discusses the resources and utilities that will be provided to support the encryption of expressions in the management model.

https://issues.jboss.org/browse/EAP7-677
https://issues.jboss.org/browse/WFCORE-4360

At this point in time unless there are problems in the late stages if implementation it is not expected that the details will diverge too fat from this analysis.

… requirements and nice to have requirements will be handled.
…nges required for SecretKey handling with the CredentialStore.
… will be added to the server and configured as well as operations and command line updates to support encryption of the values.
… key actions for the credential-store command.
credential store or from separate credential stores - this relates to the "chicken and egg" problem which will be described later.

The initial integration of this enhancement will not support specifying custom `java.security.Provider` instances as the key type and transformation will be
using pre-defined defaults - however the expression resolver will be initialised after the Elytron subsystem has performed initialisation and registration of
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about attributes of the Elytron subsystem, provider-loader, and credential-store that allow expressions? Do I understand it well that those will be processed just with the general resolver but the new credential store related resolver will not be applied to those?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On this point there is going to be a limit to how far we can go.

Within the implementation the act of resolving an expression could trigger some initialisation - i.e. we need a credential store and we need the providers registered. This initialisation could trigger the need for expression resolution.

We must be able to support the scenario where the clear-text password of one credential store is encrypted using the secret key of another credential store so if there is any recursion triggered for expression resolution that must be supported.

If however the recursion leads to an unresolvable cycle that will have to fail and lead to an error. A cycle should be easy to detect i.e. if on first use of "resolverOne" the initialisation triggers something else requiring "resolverOne" we know we have a cycle - on a ThreadLocal we will likely maintain a Set of requested resolvers which would enable us to detect a longer chain forming a cycle.

If recursion is triggered alternative expression resolution using system properties should remain possible.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the explanation. Maybe it's worth mentioning the solution here. In case you consider this just an implementation detail, this conversation can be resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this specific detail is an implementation detail, but this discussion has reminded me that we should list the relative-to paths that we can support. As expression resolution is early we can not use user defined paths but we should support the standard set e.g. jboss.server.config.dir so I should list those.

Presently WildFly Core supports an expression resolver that can delegate to a Vault configuration and if that is not available fall back to use either system
properties or environment variables. This will be updated to make use of the `CapabilityRegistry` and attempt to lookup a capability using a predefined
constant `org.wildfly.controller.expression-resolver` which exposes an expression resolver runtime API. The expression resolver looked up using a capability
will be used after first attempting to resolve the expression as a Vault expression to resolve any expression.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand it correctly that the new ordering will be: Vault, capabilities, system properties, environment variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would need to double check the exact order but 100% this new capaility based resolution is also immediately after Vault.

Also this should not impact any existing recursion that is (or is not) possible with vault.

configuration the result will be usable as returned, caution may however be required if an expression is being prepared on one process or profile to be used
on another in case there are differences in the resolver configuration.

We will not support the decryption of expressions other than the support within the management model to resolve existing expressions.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about :resolve-expression(expression=${ENC:Resolver:ENCRYPTED_DATA}), will it work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually yes, I believe that call should work.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is covered by https://issues.redhat.com/browse/WFWIP-373 now, the conversation can be resolved.


The `credential-store` command will be updated with an additional action `encrypt`, this in turn will make use of two parameters:

* `--alias` - The alias of the `SecretKey` stored within the credential store.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the implementation, an alias is taken as the argument of the action encrypt. The implementation is consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@darranl
Copy link
Contributor Author

darranl commented Feb 18, 2021

@OndrejKotek I have had another pass through, other than the "read-aliases" operation which you may want to discuss further I think I have covered all the points you raised.

@bstansberry bstansberry merged commit af0c9e0 into wildfly:master Mar 10, 2021

A new credential store type will be implemented, this credential store will be dedicated to the storage of `SecretKeyCredential` instances which will be stored
in a properties file. Unlike other credential store types this implementation will not be adding any additional protection to the keys it contains, this
credential store is primarily intended to solve the problem of how we provide the first secret key to the application server process without relying on a hard
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just realized that an external source can be used for the master password, which is not documented upstream, see https://issues.redhat.com/browse/WFLY-14589

For the documentation (analysis) purposes, what is the preferred way how to secure the master password? For example, I am not sure whether secret-key-credential-store could be used for a FIPS compliant configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes using CMD or EXT to obtain a password to unlock a credential store does remain an option so if a user wants they don't have to use the secret-key-credential-store - overall they have a few options now to decide the most appropriate approach for their initial secrets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants