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

[ELY-2711] Add support for encrypted expressions in the wildfly-config.xml #2084

Draft
wants to merge 9 commits into
base: 2.x
Choose a base branch
from

Conversation

PrarthonaPaul
Copy link
Contributor

@PrarthonaPaul PrarthonaPaul commented Jan 9, 2024

Issue: https://issues.redhat.com/browse/ELY-2711
Depends on: wildfly/wildfly-client-config#42
Note:

  • This PR will fail CI tests because it depends on some changes on wildfly-client-config that still need to be merged
  • The code for Encryption client has been moved to org.wildfly.security.encryption.client and org.wildfly.security.encryption has been moved to a folder called base. Module name for this stays the same.

@fjuma
Copy link
Contributor

fjuma commented Jan 9, 2024

@PrarthonaPaul Since this PR depends on changes in wildfly-client-config that haven't been pushed yet, I think it would be good to mark this as a draft PR.

Copy link
Contributor

@fjuma fjuma left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @PrarthonaPaul! I have started going through the analysis document and this PR and am adding some initial feedback. Let me know if you have any questions.


<dependency>
<groupId>org.wildfly.core</groupId>
<artifactId>wildfly-controller</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to be a bit careful here. This is a server dependency and it also depends on Elytron. We can't add a dependency on it here. Where it makes sense to do so, we could consider pulling out common pieces of code and adding it to Elytron and then updating the server code to make use of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed core dependency!

@@ -138,6 +141,11 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<environmentVariables>
<CREDSTORE_PATH_ENV>PATH/TO/ELYTRON/CLIENT/DIR/target/credstore/mycredstore.cs</CREDSTORE_PATH_ENV>
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this needed for? Is this for tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used for tests.
Since the absolute path needs to be specified for the location of the credential store, I added an environment variable to specify the path.
Maybe we can change it to something like what the OIDC tests do to resolve env variables.

@@ -0,0 +1,170 @@
package org.wildfly.security.auth.client;
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing copyright header (this can be copied from another .java file and then the year just needs to be updated).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added!

pom.xml Outdated
@@ -89,6 +89,7 @@
<version.org.wildfly.checkstyle-config>1.0.8.Final</version.org.wildfly.checkstyle-config>
<version.org.wildfly.client.config>1.0.1.Final</version.org.wildfly.client.config>
<version.org.wildfly.common>1.6.0.Final</version.org.wildfly.common>
<version.org.wildfly.core>23.0.0.Beta4</version.org.wildfly.core>
Copy link
Contributor

Choose a reason for hiding this comment

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

WildFly Core depends on Elytron. We can't add this dependency here (see my comment above).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed circular dependency. Introduced new exception classes instead and got rid of server specific code/classes.

Assert.assertEquals(clearText, decryptedExpression);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@PrarthonaPaul the tests could later include an wildfly-config.xml file that contains additional schemas that can be included there. You can find them here: https://docs.wildfly.org/30/WildFly_Elytron_Security.html#Configuring_other_clients_using_wildfly-config , there are also links to schema locations

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 have added a new test that uses encrypted expression with authentication client.
Please let me know if there are any other clients I should add tests for. So far there are tests for parsing only, but no integrated functionality tests. Please let me know if I should add those and where I can add those.
Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

@PrarthonaPaul we can add an integration tests that verify that the credentials used in SASL authentication are resolved as expected and authentication pass. See for example the MaskedPasswordSaslAuthenticationTest where there is a SASL client and SASL server communication and we test that their authentication is successful. We could add similar integraiton tests that uses encrypted password on the client side

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 have added a test for this. Currently looking to see if we need a programmatic one.
But the one for xml config should be good to go.
Thanks @Skyllarr

try {
throw new InvalidEncryptedExpressionConfigurationException(t);
} catch (InvalidEncryptedExpressionConfigurationException e) {
throw new RuntimeException(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

@PrarthonaPaul Just a minor, you can make the InvalidEncryptedExpressionConfigurationException extend IllegalArgumentException and then remove the RuntimeException here..similarly as it is done for DefaultAuthenticationContextProvider

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

* @author <a href="mailto:prpaul@redhat.com">Prarthona Paul</a>
*/

public final class EncryptedExpressionConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a total minor, we can name it without shortcut to keep it consistent with the rest of classes : EncryptedExpressionConfiguration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

return new EncryptedExpressionConfig.ClientCallbackHandler(this);
}

static class ClientCallbackHandler implements CallbackHandler {
Copy link
Contributor

Choose a reason for hiding this comment

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

@PrarthonaPaul just a minor, this callback handler is unused so you can remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed!

*/

public class EncryptedExpressionResolver {
private static final String CREDENTIAL_STORE_API_CAPABILITY = "org.wildfly.security.credential-store-api";
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a minor, you can remove the CAPABILITY string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed!

private static final String CREDENTIAL_STORE_API_CAPABILITY = "org.wildfly.security.credential-store-api";
private volatile boolean initialised = false;
private final ThreadLocal<String> initialisingFor = new ThreadLocal<>();
private volatile ExpressionResolutionException firstFailure = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

just a minor, these three variables are unused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed them!

private String resolveExpressionInternal(String fullExpression, EncryptedExpressionConfig config) {
assert config != null;

if (fullExpression.length() > 3) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@PrarthonaPaul most code in this class seems the same as in the wildfly-core repo. Can you extract the parts of the code used by both to some Utils class and make both wildfly-elytron-client and wildfly-core use the methods from Utils, so we avoid code duplication?

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 am not sure if we could do this (at least for some of the bigger ones, like createExpression or resolveExpressionInternal or resolveExpression since on the core side, they are server dependent).
But some of the smaller functions, like setResolverConfigurations or setDefaultResolver or setPrefix, we can add those to the utility.

Copy link
Contributor

@Skyllarr Skyllarr Feb 6, 2024

Choose a reason for hiding this comment

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

@PrarthonaPaul The methods that can be extracted to common Utils should be extracted, so that if a bug is found in the future, we can fix it on a single place. You can also refactor the methods on the server side to be able to introduce common Utils methods where it is possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Skyllarr
I have identified the common code between the two classes (ElytronExpressionResolver and the EncryptedExpressionResolver).
There are 3 functions whose contents are the same:

server side: https://github.com/wildfly/wildfly-core/blob/main/elytron/src/main/java/org/wildfly/extension/elytron/expression/ElytronExpressionResolver.java#L169-L186

Client side:

public EncryptedExpressionResolver setPrefix(final String prefix) {
this.prefix = prefix;
this.completePrefix = prefix + "::";
return this;
}
public EncryptedExpressionResolver setDefaultResolver(final String defaultResolver) {
if (defaultResolver == null) {
this.defaultResolver = getResolverConfiguration().entrySet().iterator().next().getKey();
} else {
this.defaultResolver = defaultResolver;
}
return this;
}
public EncryptedExpressionResolver setResolverConfigurations(final Map<String, ResolverConfiguration> resolverConfigurations) {
this.resolverConfigurations = Collections.unmodifiableMap(resolverConfigurations);
return this;
}

However, since the objects that invoke these functions, ElytronExpressionResolver and EncryptedExpressionResolver are different, I am not exactly sure how to combine them to use the same function. I could send them as Java Objects and then cast them using instanceof keyword.

Another thing is that I was thinking of adding it to org.wildfly.security.utils, but it turns out that would lead to a circular dependency. I could fix that using a service loader or add it to another module (maybe create org.wildfly.security.encryption.util and add it there)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another thing I realized is that the server side resolver lives inside WF-core and client side resolver is inside elytron. I think if we wanted to access the server-side resolver class from org.wildfly.security.utils, we would need to use a service loader. Or maybe we can place the util class for encryptedexoressionresolution inside the org.wildfly.common dependency?

return t -> second.apply(first.apply(t));
}

public static void configureExpressionResolver(EncryptedExpressionConfig encryptedExpressionConfig, EncryptedExpressionResolver resolver) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this method unused?

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, I have removed it.

@@ -213,4 +214,32 @@ ConfigXMLParseException xmlUnableToIdentifyProvider(@Param Location location, St

@Message(id = 14008, value = "WildFlyElytronClientDefaultSSLContextProvider could not obtain client default SSLContext")
NoSuchAlgorithmException couldNotObtainClientDefaultSSLContext();

@Message(id = 14009, value = "The resolver does not exist.")
IllegalArgumentException resolverNotFound();
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a minor, this exception is unused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed!

<xsd:complexType name="encrypted-expression-type">
<xsd:annotation>
<xsd:documentation>
Configuration for encrypted expressions and expression-resolvers to be used across different client configurations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a total minor, I would rename the file to encrypted-expressions-1_0.xsd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

Copy link
Contributor

Choose a reason for hiding this comment

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

@Skyllarr @PrarthonaPaul To be consistent with the other clients, just wondering if this should be encrypted-expressions-client-1_0.xsd or encryption-client-1_0.xsd with the root element updated accordingly as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fjuma Yes that is a good idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure! I can make changes to the schema and the elements.


SecurityFactory<AuthenticationContext> authClientConfiguration = ElytronXmlParser.parseAuthenticationClientConfiguration(config.toURI());
Assert.assertNotNull(clientConfiguration);
Assert.assertNotNull(authClientConfiguration);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add some test that verifies that the resolved expression is as expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking into this now

</xsd:sequence>
</xsd:complexType>

<xsd:complexType name="credential-store-type">
Copy link
Contributor

Choose a reason for hiding this comment

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

@PrarthonaPaul Just wondering, is it possible for this new schema to reference the types from the existing elytron-client xsd schema? With some include?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are there any examples of this?
If we can reuse credential-store-type from the elytron schema, it would make it much simpler.

Copy link
Contributor

Choose a reason for hiding this comment

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

@PrarthonaPaul I do not know of any examples, but maybe it is possible to do https://stackoverflow.com/questions/8194112/basics-of-referencing-a-xsd-schema-from-another-schema ? It needs some investigations of the schema possibilities

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 have added a reference to the elytron schema using the method mentioned in the post you shared. Thanks!

@PrarthonaPaul
Copy link
Contributor Author

Thanks, @Skyllarr for the comments!
I have addressed some of the comments and for the others, I have added some questions.

*
* @author <a href="mailto:prpaul@redhat.com">Prarthona Paul</a>
*/
class DefaultEncryptedExpressionContextProvider {
Copy link
Contributor

Choose a reason for hiding this comment

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

Very minor but some classes have EncryptedExpressions in the class name and others have just EncryptedExpression. We need to make sure that we are consistent with the schema throughout.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need "Context" in the class name? Just wondering if this was a copy/paste from DefaultAuthenticationContextProvider or if there is a reason to include it in the name here too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see now there's a corresponding EncryptedExpressionContext class.

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 have changed classes related to encryption client to have names like EncryptionClientContext


/**
* A lazily-initialized holder for the default encrypted expression context.
* If an error occurs setting up the default identity
Copy link
Contributor

Choose a reason for hiding this comment

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

"default identity context" -> This is likely a copy/paste from something else, should be reworded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

import static org.wildfly.security.auth.client._private.ElytronMessages.xmlLog;

/**
* An interface to get and check information about attributes in XML file.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/interface/utility class

Copy link
Contributor

@fjuma fjuma Feb 5, 2024

Choose a reason for hiding this comment

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

Since this is just a utility class with helper methods, this shouldn't be public API. Currently, this is a public class in the org.wildfly.security.auth.client package and that is public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing out!
Which of the following do you think would be the best approach:

  1. move it to org.wildfly.security.auth.util?
  2. keep it in the same directory and change the class and its functions to private or
  3. change the functions to protected and keep the class public or
  4. something else

} else {
this.defaultResolverName = original.defaultResolverName;
}
if (what == SET_USER_CALLBACK_HANDLER) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, I think i was trying to use it for something else but did not need it.
Should be removed now.

private static final int REMOVE_CREDENTIAL_STORE = 3;
private static final int SET_RESOLVER = 4;
private static final int SET_DEFAULT_RESOLVER = 5;
private static final int SET_USER_CALLBACK_HANDLER = 6;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed!

*
* @return the current thread's captured authentication context
*/
public static EncryptedExpressionContext captureCurrent() {
Copy link
Contributor

@fjuma fjuma Feb 5, 2024

Choose a reason for hiding this comment

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

"authentication context" references above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

* password to authenticate. The current encryption client configuration is loaded and is used to decrypt
* the encrypted password. If one does not exist, appropriate exception is thrown.
*
* @param encryptedPassword the password to use
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 added this function to decrypt a password before adding it so if users want to programmatically configure an auth client with encrypted expressions, they can do that.

To make it more general, we can replace it with a function called decryptAndUseValue() that takes in the expression to decrypt and the function to call afterwards. However, I am not sure if that is something we can do with templates (not sure if that's what they are called in java. But here is what I am talking about https://cplusplus.com/doc/oldtutorial/templates/).

If this is not supported, then we can send enumerated values like this function is doing. So, based on the enum value, we can call the appropriate function with the decrypted value.

@Skyllarr @fjuma please let me know what you think.

@PrarthonaPaul PrarthonaPaul force-pushed the WFLY-14984 branch 3 times, most recently from e00f3a8 to b19e179 Compare February 7, 2024 15:30
return value != null ? value : defVal;
}

private static int getOrDefault(int value, int defVal) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these getOrDefault methods used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed!

* @author <a href="mailto:prpaul@redhat.com">Prarthona Paul</a>
*/

public class ExpressionResolutionException extends RuntimeException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this thrown specifically for issues with encrypted expressions? If so, would be good to update the class name to reflect that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the name to EncryptedExpressionResolutionException

*/

@MetaInfServices(value = ResolverProvider.class)
public class WildFlyClientResolverProvider implements ResolverProvider{
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide some more details on what this class is used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! This is part of the service loader process.
The ResolverProvider interface lives inside the wildfly-client-config project and the WildFlyClientResolverProvider class implements that class in wildfly-elytron to give access to the classes and functions needed to resolve and decrypt the encrypted expression.

@PrarthonaPaul PrarthonaPaul force-pushed the WFLY-14984 branch 3 times, most recently from a8334dd to 2e3748e Compare February 13, 2024 20:41
// * @param encryptedPassword the password to use
// * @return the new configuration
// */
// public AuthenticationConfiguration decryptAndUsePassword(String encryptedPassword) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is here temporarily and will be uncommented if we decide to add support for programmatic configuration with encrypted expressions.

private String resolveExpressionInternal(String fullExpression, EncryptedExpressionConfig config) {
assert config != null;

if (fullExpression.length() > 3) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Skyllarr
I have identified the common code between the two classes (ElytronExpressionResolver and the EncryptedExpressionResolver).
There are 3 functions whose contents are the same:

server side: https://github.com/wildfly/wildfly-core/blob/main/elytron/src/main/java/org/wildfly/extension/elytron/expression/ElytronExpressionResolver.java#L169-L186

Client side:

public EncryptedExpressionResolver setPrefix(final String prefix) {
this.prefix = prefix;
this.completePrefix = prefix + "::";
return this;
}
public EncryptedExpressionResolver setDefaultResolver(final String defaultResolver) {
if (defaultResolver == null) {
this.defaultResolver = getResolverConfiguration().entrySet().iterator().next().getKey();
} else {
this.defaultResolver = defaultResolver;
}
return this;
}
public EncryptedExpressionResolver setResolverConfigurations(final Map<String, ResolverConfiguration> resolverConfigurations) {
this.resolverConfigurations = Collections.unmodifiableMap(resolverConfigurations);
return this;
}

However, since the objects that invoke these functions, ElytronExpressionResolver and EncryptedExpressionResolver are different, I am not exactly sure how to combine them to use the same function. I could send them as Java Objects and then cast them using instanceof keyword.

Another thing is that I was thinking of adding it to org.wildfly.security.utils, but it turns out that would lead to a circular dependency. I could fix that using a service loader or add it to another module (maybe create org.wildfly.security.encryption.util and add it there)?

private String resolveExpressionInternal(String fullExpression, EncryptedExpressionConfig config) {
assert config != null;

if (fullExpression.length() > 3) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another thing I realized is that the server side resolver lives inside WF-core and client side resolver is inside elytron. I think if we wanted to access the server-side resolver class from org.wildfly.security.utils, we would need to use a service loader. Or maybe we can place the util class for encryptedexoressionresolution inside the org.wildfly.common dependency?

@ropalka
Copy link

ropalka commented Apr 15, 2024

Just a friendly remainder @PrarthonaPaul there's a change required to move this PR forward.

@PrarthonaPaul
Copy link
Contributor Author

Just a friendly remainder @PrarthonaPaul there's a change required to move this PR forward.

Thanks @ropalka
I will resolve these conflicts and push soon.

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