[WFLY-14984] Add support for encrypted expressions in the wildfly-config.xml #545
Conversation
PrarthonaPaul
force-pushed
the
WFLY-14984
branch
2 times, most recently
from
30b7c1f to
9e83c80
Compare
|
|
||
| Attribute values for client configuration can include sensitive information. And while credential references can be used for some of them, others still need to be defined using plaintext. As a result, those attributes are still exposed and make the application vulnerable. Additionally, the password for the credential store needs to be defined as clear-text, which is not ideal. | ||
|
|
||
| This feature adds support for encrypted expressions for attributes in `wildfly-config.xml` file, which allows users to avoid specifying sensitive information using plaintext. Instead, encrypted expressions can be used to specify these information. A configuration section named `encrypted-expression` would be added, which would reference a `secret-key-credential-store` to extract encrypted expressions. It would also have a resolver, which would be backed by the secret-key-credential-store. The `secret-key-credential-store` can be created either using the `elytron-tool` or using the Elytron subsystem on the WildFly server. |
There was a problem hiding this comment.
This is probably just a copy/paste thing but since wildfly-config.xml is something that's used on the client side, we shouldn't be mentioning the Elytron subsystem here.
There was a problem hiding this comment.
I was thinking in case the user is doing some server configuration as well like this blog does: https://wildfly-security.github.io/wildfly-elytron/blog/ejb-over-tls/
In these cases, we can also use the management cli to create the credential store as opposed to using the elytron-tool.
WDYT? Could this be a possible use case?
I can make it more general by saying management cli instead of elytron subsystem.
There was a problem hiding this comment.
In the linked blog post, it looks like the credential store is created using elytron-tool.sh. I don't see subsystem commands being used to create it.
The management CLI is used for server config so I think mentioning it just makes things a bit confusing since this RFE is about the client side.
There was a problem hiding this comment.
Changed the wording to only mention client configuration.
| <protection-parameter-credentials> | ||
| <clear-password password="password" /> | ||
| </protection-parameter-credentials> | ||
| </credential-store> |
There was a problem hiding this comment.
The indentation in this block looks a bit off.
|
|
||
| * The `encrypted-expression` configuration can be specified as follows: | ||
| ``` | ||
| <encrypted-expression> |
There was a problem hiding this comment.
It would be good to add more details here about the specific elements, attributes, etc. that are being introduced. Then the block below could be an example.
|
|
||
| ** While parsing the xml file, the section for encrypted expression and its resolver would need to be parsed before any other sections that reference it. | ||
|
|
||
| * Encrypted expressions can be added to other subsystems in addition to elytron. As a result, the parsing rules and sequences should be configured to be compatible with those subsystems as well. |
There was a problem hiding this comment.
Since this is client side, the terminology here needs to be updated. It's currently referencing subsystems.
|
|
||
| // === Related Issues | ||
|
|
||
| // * https://issues.redhat.com/browse/WFCORE[WFCORE-XXXX] |
There was a problem hiding this comment.
Do we need an ELY issue for this?
There was a problem hiding this comment.
Yes. We can reference your newly created issue here.
PrarthonaPaul
force-pushed
the
WFLY-14984
branch
from
9e83c80 to
2711841
Compare
|
|
||
| Attribute values for client configuration can include sensitive information. And while credential references can be used for some of them, others still need to be defined using plaintext. As a result, those attributes are still exposed and make the application vulnerable. Additionally, the password for the credential store needs to be defined as clear-text, which is not ideal. | ||
|
|
||
| This feature adds support for encrypted expressions for attributes in `wildfly-config.xml` file, which allows users to avoid specifying sensitive information using plaintext. Instead, encrypted expressions can be used to specify these information. A configuration section named `encrypted-expression` would be added, which would reference a `secret-key-credential-store` to extract encrypted expressions. It would also have a resolver, which would be backed by the secret-key-credential-store. The `secret-key-credential-store` can be created either using the `elytron-tool`. |
There was a problem hiding this comment.
"to extract encrypted expressions" should be reworded. The credential store contains the secret key that is used to encrypt/decrypt encrypted expressions.
There was a problem hiding this comment.
"The secret-key-credential-store can be created either using the elytron-tool."
This sentence is incomplete I think.
There was a problem hiding this comment.
"It would also have a resolver, which would be backed by the secret-key-credential-store"
Will the resolver be able to reference a credential-store as well in addition to being able to reference a secret-key-credential-store?
There was a problem hiding this comment.
Resolvers are backed by aliases from a CredentialStore object. Secret-key-credential-store is an instance of that class with type PropertiesCredentialStore. So, yes, we can use either type.
I will add a note about that. Thanks!
|
|
||
| * The ability to specify attribute values as encrypted expressions should be added to wildfly-config.xml. | ||
|
|
||
| * The parsing needs to be able to run separately from the application server, as the server may not be running while the client configuration is being set up. |
There was a problem hiding this comment.
I think this could be reword to something like:
Parsing for client configuration is separate from parsing server configuration.
|
|
||
| * Configurations may not depend on components created for server configuration, such as `resolvers` or `elytron/expression`. The resolver for the encrypted expression should be configured through the client configuration. | ||
|
|
||
| * The `encrypted-expression` would be a new configuration block added to the client config under the new schema. The new attributes added include `resolvers`, with attributes called `resolver` under it. Additionally, <credential-stores> attribute would be added to the schema with the <credential-store> attributes added to it. The structures of this attribute would be the same as the one one under the `authentication-client` schema. |
There was a problem hiding this comment.
Just a small thing, resolvers and resolver would be elements (as opposed to attributes).
Same with credential-stores and credential-store.
There was a problem hiding this comment.
Will there be a separate secret-key-credential-store element? (Just noticed that's mentioned in the intro but not in this section)
There was a problem hiding this comment.
Nope, we can just use the credential-store element and specify the type as keystorecredentialstore (which is the default config) or PropertiesCredentialStore.
There was a problem hiding this comment.
Ok thanks. Would be good to update the intro since it currently references a secret-key-credential-store element.
There was a problem hiding this comment.
fixed!
There are still some mentions of a secret-key-credential, but when mentioning the element itself, I am referring to it as a credential-store.
Please let me know if I should change it all to be credential-store
| ** The configuration for this block can be specified as follows: | ||
| ``` | ||
| <encrypted-expression> | ||
| <credential-store name="credential-store-name" type="PKCS12" provider="provider-name"/> |
There was a problem hiding this comment.
Should there be a credential-stores element around this?
| </protection-parameter-credentials> | ||
| </credential-store> | ||
| <resolvers> | ||
| <resolver name= “resolver-name” credential-store= “credential-store-name key=“key”/> |
There was a problem hiding this comment.
credential-store-name is missing the closing quote here and below.
| <resolver name= “resolver-name” credential-store= “credential-store-name key=“key”/> | ||
| <resolver name= “resolver-name-2” credential-store= “credential-store-name key=“example”/> | ||
| </resolvers> | ||
| <default-resolver name=resolver-name> |
There was a problem hiding this comment.
Does default-resolver need to be its own element? Can it be an attribute defined on the expression-resolver like in the server side config? e.g., see the xml config here:
https://wildfly-security.github.io/wildfly-elytron/blog/wildfly-encrypted-expressions/
There was a problem hiding this comment.
if it is not added as a separate element, would we add it as a property for the encrypted expression element?
so like
<encrypted-expression default-resolver="resolver-name">
INSERT CONFIGURATION
</encrypted expression>
If we do go with this, I am not sure if we can ensure that resolver-name is one of the resolvers specified under the elements. I think the parser currently checks to make sure it is.
There was a problem hiding this comment.
Just looking at the example from the blog post:
<expression-resolver default-resolver="main-resolver">
<resolver name="initial-resolver" credential-store="initial" secret-key="key"/>
<resolver name="main-resolver" credential-store="main" secret-key="main-key"/>
</expression-resolver>
Can we make use of this format?
There was a problem hiding this comment.
Yes, I will update the schema and parser to use this.
Also, would it be better to make the default-resolver a required attribute? or make it optional, but if the user does not specify one, then the first resolver becomes the default resolver?
There was a problem hiding this comment.
How is this done on the server side?
There was a problem hiding this comment.
The server does neither.
When adding an expression resource using the following command:
/subsystem=elytron/expression=encryption:add(resolvers=[{name=initial-resolver, credential-store=initial, secret-key=key}])
defining a value for the default-resolver attribute is optional on the server side.
Now if someone tries to create an expression without defining a resolver, then the output is as follows:
/subsystem=elytron/expression=encryption:create-expression(clear-text=password)
{
"outcome" => "failed",
"failure-description" => "WFLYELY01200: The name of the resolver to use was
not specified and no default-resolver has been defined.",
"rolled-back" => true
}
On the client, the create-expression part is done using the --encrypt option on the wildfly-elytron tool, which would give you an output like this:
Clear text encrypted to token 'RUxZAUMQQ1PVWZuimKZCpt0s6MONrl7cbqLGPhyBBmm83fXdfZA=' using alias 'key'.
It would be up to the user to convert it to something like {ENC::resolver_name:RUxZAUMQQ1PVWZuimKZCpt0s6MONrl7cbqLGPhyBBmm83fXdfZA=} (where the expression resolver called resolver-name would be used to decrypt)
or they can convert it to {ENC::RUxZAUMQQ1PVWZuimKZCpt0s6MONrl7cbqLGPhyBBmm83fXdfZA=} (where the default expression resolver would be used to decrypt)
There is code in place already in the resolver class to handle cases where the user goes with the second configuration, but there is no default-expression-resolver defined. So, if we do want to do something similar to what the server does, I think it may be possible to throw an exception when trying to resolve the expression. However, since we are using the wildfly-elytron tool to encrypt, we cannot throw the same exception when creating the expression, which is what the server does.
There was a problem hiding this comment.
Ok given that, I think it would be good to have the default-resolver attribute be optional here too.
An encrypted expression provided in wildfly-config should either specify the resolver name or rely on the default being used.
If relying on the default and the default-resolver hasn't been defined, then that's invalid configuration and an exception should occur when attempting to resolve it.
| <clear-password password="password" /> | ||
| </protection-parameter-credentials> | ||
| </credential-store> | ||
| <resolvers> |
There was a problem hiding this comment.
In the server config, I think we use , right (would be good to double check what's used there)? Any reason to use "resolvers" here instead of "expression-resolvers"?
There was a problem hiding this comment.
Updated to and . I will update the code to reflect this change as well.
| ``` | ||
| Note that the initial password for the secret-key-credential-store used for the resolver would need to be specified in clear-text. | ||
|
|
||
| Once parsing is done, the resolver would need to be created programmatically (if not created by the server already) and be connected with the secret-key-credential-store. |
There was a problem hiding this comment.
Can you explain this some more? The client side config is unrelated to the server side config so not quite following the reference to the server side here.
There was a problem hiding this comment.
"(if not created by the server already)" should not be there since this is entirely on the client side.
I have fixed it.
| </credential-store> | ||
| ``` | ||
|
|
||
| * The encrypted expression would be created using the `--generate-secret-key` command and can be read using the `--export-secret-key` command. |
There was a problem hiding this comment.
Might be good to get some examples added here.
There was a problem hiding this comment.
Added example commands for both generating secret key and encrypting with it.
Thanks @fjuma I have addresses the comments on this PR!
PrarthonaPaul
force-pushed
the
WFLY-14984
branch
2 times, most recently
from
4211b6f to
8abe135
Compare
PrarthonaPaul
force-pushed
the
WFLY-14984
branch
from
8abe135 to
6f1bf8a
Compare
PrarthonaPaul
force-pushed
the
WFLY-14984
branch
from
6f1bf8a to
0341c13
Compare
| </credential-store> | ||
| </credential-stores> | ||
| <expression-resolvers default-resolver="expression-resolver-name"> | ||
| <expression-resolver name= “expression-resolver-name” credential-store= “credential-store-name" alias=“key”/> |
There was a problem hiding this comment.
Just looking at the server configuration and noticed that we use "secret-key" instead of "alias" there. Any reason to use "alias" here? Just wondering if we should use "secret-key" instead just to make it more similar to the configuration on the server side.
There was a problem hiding this comment.
No particular reason. I assumed alias is more unambiguous than secret-key. But it should be easy to change it to secret-key if we decide to go with that.
| Clear text encrypted to token 'RUxZAUMQvGzk6Vaadp2cahhZ6rlPhHOZcWyjXALlAthrENvRTvQ=' using alias 'key'. | ||
| ``` | ||
|
|
||
| * expression-resolvers should be backed by an entry from a credential-store of type KeyStoreCredentialStore. However, when using a KeyStoreCredentialStore type credential store, the protection parameter, which is the password, needs to be specified in clear-text inside the <encrypted-expressions> tag. |
There was a problem hiding this comment.
expression-resolvers can reference any type of credential-store right? Here, KeyStoreCredentialStore is mentioned specifically.
There was a problem hiding this comment.
Changed it to "However, when using other types of credential stores that are protected by a protection parameter, such as a password, that needs to be specified in clear-text inside the tag. "
|
|
||
| * expression-resolvers should be backed by an entry from a credential-store of type KeyStoreCredentialStore. However, when using a KeyStoreCredentialStore type credential store, the protection parameter, which is the password, needs to be specified in clear-text inside the <encrypted-expressions> tag. | ||
|
|
||
| * Attribute value resolvers inside the wildfly-client-config project should be updated with the ability to parse encrypted expressions and resolve them to extract the clear-text sensitive information. In order to avoid circular dependencies, service loader would be used by the wildfly-client-config project to use an EncryptedExpressionResolver class to decrypt an encrypted expression. |
There was a problem hiding this comment.
Might be good to provide some more details on the Service Loader approach if possible.
|
|
||
| === Nice-to-Have Requirements | ||
|
|
||
| * The parsing of the encrypted expression should be added at the top of the xml file and must be parsed before any other class that uses encrypted expressions. |
There was a problem hiding this comment.
Removed this point since it is no longer relevant.
|
|
||
| ** The configuration for this block can be specified as follows: | ||
| ``` | ||
| <encrypted-expressions> |
There was a problem hiding this comment.
Just noticed encrypted-expressions is used here but encrypted-expression is used in the blog post wildfly-security/wildfly-elytron#2088.
There was a problem hiding this comment.
fixed it on the blog!
Thanks, @fjuma for reviewing the proposal!
PrarthonaPaul
force-pushed
the
WFLY-14984
branch
2 times, most recently
from
e1669ea to
254280b
Compare
|
|
||
| Attribute values for client configuration can include sensitive information. And while credential references can be used for some of them, others still need to be defined using plaintext. As a result, those elements are still exposed and make the application vulnerable. Additionally, the password for the credential store needs to be defined as clear-text, which is not ideal. | ||
|
|
||
| This feature adds support for encrypted expressions for elements in `wildfly-config.xml` file, which allows users to avoid specifying sensitive information using plaintext. Instead, encrypted expressions can be used to specify these information. A configuration section named `encrypted-expressions` would be added, which would reference a `secret-key-credential-store` to convert plaintext to encrypted expressions and vice versa. The secret-key-credential-store would be indicated by the `credential-store` tag with the `type` being `PropertiesCredentialStore`. It would also have a expression-resolver, which would be backed by one of the secret key aliases from the the secret-key-credential-store. The `credential-store` can be created using the wildfly-elytron tool using the `credential-store` command and the `--create` option. |
There was a problem hiding this comment.
Change, "these information." to "this information."
It might be helpful to future readers to reference this doc about wildfly-elytron tool,
https://github.com/wildfly/wildfly/blob/main/docs/src/main/asciidoc/_elytron/Credential_Store.adoc
| Clear text encrypted to token 'RUxZAUMQvGzk6Vaadp2cahhZ6rlPhHOZcWyjXALlAthrENvRTvQ=' using alias 'key'. | ||
| ``` | ||
|
|
||
| * expression-resolvers should be backed by an entry from a credential-store of type KeyStoreCredentialStore. However, when using other types of credential stores that are protected by a protection parameter, such as a password, that needs to be specified in clear-text inside the <encrypted-expressions> tag. |
There was a problem hiding this comment.
Change, "that needs to be specified" to "it needs to be specified".
PrarthonaPaul
force-pushed
the
WFLY-14984
branch
from
254280b to
37b8680
Compare
PrarthonaPaul
force-pushed
the
WFLY-14984
branch
from
37b8680 to
959fa3e
Compare
Issue: https://issues.redhat.com/browse/WFLY-14984
Considerations based on preliminary discussions:
todo before implementation: