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-2112] Automatic registration of client side / JVM wide default SSLContext #1509

Merged
merged 1 commit into from
Mar 17, 2022

Conversation

Skyllarr
Copy link
Contributor

@Skyllarr Skyllarr commented Mar 31, 2021

https://issues.redhat.com/browse/ELY-2112

Adds Elytron client provider that provides default SSLContext which can be returned by SSLContext.getDefault() call. The default SSLContext is considered to be the one that matches all rules.

public Object newInstance(Object ignored) throws NoSuchAlgorithmException {
Integer enteredCountTmp = entered.get();
entered.set(enteredCountTmp == null ? 1 : enteredCountTmp + 1);
if (entered.get() >= 2) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When default SSLContext is set to be default SSLContext in the elytron client configuration, the execution would loop. Entered variable serves as a counter to avoid this loop. It counts the number of entrances. When it is equal or higher than 2 it throws a NoSuchAlgorithmException. When this exception is encountered, JVM tries to obtain default SSLContext from providers of lower priority.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest include this info in a short comment in the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment added

}
// if we had an exception previously, then default ssl context was still returned from other security provider
// which is why we need to check entered variable again
if (entered.get() >= 2) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the exception was thrown to avoid loop, then the default SSL context was returned from another provider in the AuthenticationContextConfigurationClient. We do not want to wrap this returned SSL context that another provider provides, so we need to throw an exception again. Then the execution will move to another provider entirely.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also here, we may want to include this info in a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment added.

@Skyllarr
Copy link
Contributor Author

@fabiobrz This is ready for review

darranl
darranl previously requested changes Sep 1, 2021
Copy link
Contributor

@darranl darranl left a comment

Choose a reason for hiding this comment

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

No major issues, just some missing copyright headers and suggestions about comments.

@@ -0,0 +1,72 @@
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.

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. Thank you

@@ -0,0 +1,67 @@
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.

Another missing header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missing copyrights added.

public Object newInstance(Object ignored) throws NoSuchAlgorithmException {
Integer enteredCountTmp = entered.get();
entered.set(enteredCountTmp == null ? 1 : enteredCountTmp + 1);
if (entered.get() >= 2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest include this info in a short comment in the file.

if (node.getRule().matches(uri, abstractType, abstractTypeAuthority)) return node;
node = node.getNext();
if (uri == null) {
if (node.getRule().equals(MatchRule.ALL)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is needed here as the default SSLContext implementation doesn't know the destination it is being used for so we have no URI - are there any compatibility issues by matching ALL possibly sooner than we used to or do we feel this is safe?

Copy link
Contributor Author

@Skyllarr Skyllarr Sep 3, 2021

Choose a reason for hiding this comment

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

@darranl Matching ALL sooner than we used to would happen only if null URI was passed here. Which was not possible before my changes because this method is only called in getSSLContextFactory and there was a null check for URI. I removed this null check in this PR.

I think backwards incompatibility can happen only if users were relying on the IllegalArgumentException to happen by passing null to the getSSLContextFactory method. I don't think this is something users should have been doing?

Btw to avoid this I could introduce new config parameter for default SSL context meant only for this provider. But that would not leverage existing possibility to configure default SSLContext.

}
// if we had an exception previously, then default ssl context was still returned from other security provider
// which is why we need to check entered variable again
if (entered.get() >= 2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also here, we may want to include this info in a comment.

Copy link

@fabiobrz fabiobrz left a comment

Choose a reason for hiding this comment

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

Hi @Skyllarr - left my review with some comments.

On a general note, I couldn't find tests for the following cases (from the feature test plan security testing section). Am I missing something?

  • Test that exception will be thrown when trying to set invalid configuration path to the provider
  • Test that exception will NOT be thrown when not providing configuration path and wildfly-config.xml is not found on the classpath.

}

public ElytronClientDefaultSSLContextProvider(String configPath) {
super("ElytronClientDefaultSSLContextProvider", 1.0, "Elytron client provider for default SSLContext");
Copy link

Choose a reason for hiding this comment

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

Just asking here, is it okay to use a @Deprecated(since="9") API? Is there any reason for not using super(String, String, 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.

@fabiobrz The super(String, String, String) constructor was introduced in JDK 9, so I canot use it here because we have release configured for compiler to be 8. The deprecated constructor I used is still present in JDK 15 so I used it but good point, I will check today with the team whether the release can be changed to 9. If yes I can change it, if not I have to leave it as it is. I will let you know

Copy link

Choose a reason for hiding this comment

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

Thank you very much @Skyllarr - not a big issue, though. There is valid reasoning already for leaving it as is, which you provided.

I'll leave the final call to you here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabiobrz Yes we have to use the deprecated constructor here, as wildfly should work with JDK 8.

private static final String CONFIG_FILE = "file:./src/test/resources/org/wildfly/security/auth/client/test-wildfly-config-client-default-sslcontext.xml";

@Test
public void testDefaultSSLContextFromFileWorksAndHasPrecedence() {
Copy link

Choose a reason for hiding this comment

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

Can you please provide messages for assertion failures here and generally everywhere else in the PR tests? Or is there some reason that I am not aware of for this to be missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabiobrz Added assertion fail messages

Copy link

Choose a reason for hiding this comment

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

At least to most relevant ones 🙂 ...

But yeah, that makes sense, the remaining three assertions are clear enough and don't change among the tests, so I am okay with this.


@Test
public void testDefaultSSLContextFromFileWorksAndHasPrecedence() {
Security.insertProviderAt(new ElytronClientDefaultSSLContextProvider(CONFIG_FILE), 1);
Copy link

Choose a reason for hiding this comment

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

Looking at this and specifically at the ElytronClientDefaultSSLContextProvider ctor that takes a string as the path to config file, is it by design that it is not set to be a valid URL instead that a pure String?
I ask because I see that DefaultSSLContextSpi eventually does a new URI(String) call [1].

This also affects tests implementation, as in here, where my attention was caught by the fact that usually a getClass.getResource() call is the way [2] to go.

Same for DefaultSSLContextProviderDoesNotLoopTestCase, below.

I am aware that this would mean to change a couple more classes' constructors in this PR as well, WDYT?

[1]
https://github.com/wildfly-security/wildfly-elytron/pull/1509/files#diff-b812735a0a9a3eb737de44a71079bc61c0bb3c24109ff049f0dc26b392a416aeR46

[2]
https://github.com/wildfly-security/wildfly-elytron/blob/1.x/auth/client/src/test/java/org/wildfly/security/auth/client/ElytronXmlParserTest.java#L116

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabiobrz Provider takes string as an argument because this argument can be passed to the provider in java.security file like this:

java.security.provider.1=org.wildfly.security.auth.client.ElytronClientDefaultSSLContextProvider /path/to/config/file

In above cases the JVM is parsing the java.security file and the argument passed to the provider is considered to be String.

I can add another constructor with URI, but it would be usable only from code and I think this provider would usually be configured in the java.security. WDYT? I will check this with the team today as well.

Copy link

Choose a reason for hiding this comment

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

In above cases the JVM is parsing the java.security file and the argument passed to the provider is considered to be String.

Ah thanks, that sounds reasonable to me to me.

I can add another constructor with URI, but it would be usable only from code and I think this provider would usually be configured in the java.security. WDYT? I will check this with the team today as well

Eh... The only thing that I didn't actually feel to be correct is that usage of hard coded paths in test classes which would warrant for the overloaded ctor actually, but as said you'd have to overload it in a couple classes more, if I don't go wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabiobrz If I add the constuctor with URI (yes it would require overloading other classes as well), in tests I would still want to use the String ctor also, as that one would need testing the most anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabiobrz So since it would not help with the primary concern which is hard coded paths in tests, I will not add another ctor for now. Let me know if you agree with my reasoning.

Copy link

Choose a reason for hiding this comment

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

Yep, thanks, let's move on like this since it is a test use case.

Security.insertProviderAt(new ElytronClientDefaultSSLContextProvider(CONFIG_FILE), 1);
Assert.assertNotNull(Security.getProvider("ElytronClientDefaultSSLContextProvider"));
AuthenticationContext.empty().run(() -> { // provider will not use current auth context but will use authentication context from the file passed to provider. File passed to provider has precedence
SSLContext cip = null;
Copy link

Choose a reason for hiding this comment

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

Just for personal knowledge (but maybe worth a renaming), what does cip stand 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.

@fabiobrz sorry it was a bad name, originally it meant context in provider but that was just bad naming. I changed it to default SSLContext.

Copy link

Choose a reason for hiding this comment

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

Thanks

Assert.fail();
}
Assert.assertNotNull(cip);
Assert.assertEquals(cip.getProvider().getName(), ElytronClientDefaultSSLContextProvider.class.getSimpleName());
Copy link

Choose a reason for hiding this comment

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

IIUC, in case this is the "has precedence" part - and the following one is the "works" part - then I'd switch the two lines' order so that it matches the test method name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabiobrz switched

@Test
public void testDefaultSSLContextProviderWillDelegateIfNoConfigured() {
Security.insertProviderAt(new ElytronClientDefaultSSLContextProvider(), 1);
Assert.assertEquals(ElytronClientDefaultSSLContextProvider.class.getSimpleName(), Security.getProvider("ElytronClientDefaultSSLContextProvider").getName());
Copy link

Choose a reason for hiding this comment

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

IIUC, here you mean to check that the inserted provider is the ElytronClientDefaultSSLContextProvider, which seems right to me but in the previous test class you just checked for the named one not to be null. Can we have a single way to assess that the provider registration was successful and 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.

@fabiobrz If the provider is not null it is certain that this equality is true, so I left the assertNotNull check. And I changed this line to be consistent everywhere.

Copy link

Choose a reason for hiding this comment

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

Great, thanks!


@Test
public void testDefaultSSLContextProviderDoesNotLoopTestCase() throws GeneralSecurityException, URISyntaxException, ConfigXMLParseException {
Security.insertProviderAt(new ElytronClientDefaultSSLContextProvider(), 1);
Copy link

Choose a reason for hiding this comment

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

Isn't there the need to check that the registration went well as it is done in the previous test cases?

Same question about DefaultSSLContextProviderFromCurrentAuthContextTestCase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabiobrz Added, thanks.

@Test
public void testDefaultSSLContextTakenFromCurrentContext() throws GeneralSecurityException, URISyntaxException, ConfigXMLParseException {
Security.insertProviderAt(new ElytronClientDefaultSSLContextProvider(), 1);
AuthenticationContext authenticationContext = ElytronXmlParser.parseAuthenticationClientConfiguration(new URI(CONFIG_FILE)).create();
Copy link

Choose a reason for hiding this comment

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

I'd add a Javadoc comment to this test method so that it is clear what it does, as you did in the previous ones.

BTW could you please convert the above mentioned (existing) comments to Javadoc as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabiobrz I added javadoc on top of class in each test. I left in-line comments when I though it migh be useful to specify exact line. Thanks

@Skyllarr Skyllarr force-pushed the eap7-1120 branch 2 times, most recently from 35d3dd4 to ff960fd Compare September 7, 2021 10:08
@Skyllarr
Copy link
Contributor Author

Skyllarr commented Sep 7, 2021

Hi @Skyllarr - left my review with some comments.

On a general note, I couldn't find tests for the following cases (from the feature test plan security testing section). Am I missing something?

  • Test that exception will be thrown when trying to set invalid configuration path to the provider
  • Test that exception will NOT be thrown when not providing configuration path and wildfly-config.xml is not found on the classpath.

@fabiobrz I added missing test. For Test that exception will be thrown when trying to set invalid configuration path to the provider please see DefaultSSLContextNonexistentConfigFileTest.java
For Test that exception will NOT be thrown when not providing configuration path and wildfly-config.xml is not found on the classpath. please see DefaultSSLContextProviderIsIgnoredWhenConfigIsMissingTest.java

@fabiobrz
Copy link

fabiobrz commented Sep 7, 2021

@fabiobrz I added missing test. For Test that exception will be thrown when trying to set invalid configuration path to the provider please see DefaultSSLContextNonexistentConfigFileTest.java
For Test that exception will NOT be thrown when not providing configuration path and wildfly-config.xml is not found on the classpath. please see DefaultSSLContextProviderIsIgnoredWhenConfigIsMissingTest.java

ok, thanks - I somehow missed the expected exception declaration, my bad, sorry.

@Skyllarr
Copy link
Contributor Author

Skyllarr commented Sep 8, 2021

@fabiobrz Thanks for your review, I replied to the remaining comments. Let me know what you think.

Copy link

@fabiobrz fabiobrz left a comment

Choose a reason for hiding this comment

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

Hi @Skyllarr, and thanks for addressing my comments and questions.
The tests development part LGTM, so I am approving the PR with respect to that.

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

@fjuma fjuma Sep 8, 2021

Choose a reason for hiding this comment

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

Just minor, missing header here and in files below.

@@ -0,0 +1,34 @@
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.

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fjuma Thank you for review. I added missing headers. I also switched values in AssertEqual calls because expected and actual values were switched.

@Skyllarr Skyllarr force-pushed the eap7-1120 branch 2 times, most recently from a94264d to 2e3c885 Compare September 9, 2021 10:05
fjuma
fjuma previously approved these changes Sep 9, 2021
@Skyllarr
Copy link
Contributor Author

Skyllarr commented Sep 10, 2021

I have put this on hold now. During manual testing I found that the provider registration via java.security file does not work for me, not sure if the fact that this provider is unsigned is the reason.

@Skyllarr
Copy link
Contributor Author

I have put this on hold now. During manual testing I found that the provider registration via java.security file does not work for me, not sure if the fact that this provider is unsigned is the reason.

Just a quick update, this has nothing to do with signing, as the provider does not provide encryption it does not need to be signed. Reason it fails when registering it through java.security file is that if I add just wildfly-elytron-client jar to the JVM's external libraries the provider will throw NoClassDefFound error for class ConfigXMLParseException. But if I add wildfly-client-all jar to external libraries of JVM, the JVM is unable to start.

Btw I tried to remove scope provided for wildfly-client-config to avoid NoClassDefFound, but it did not help the Exception.

@darranl darranl dismissed their stale review September 10, 2021 16:40

Changes made

@Skyllarr Skyllarr force-pushed the eap7-1120 branch 3 times, most recently from 7cae61d to b23763a Compare March 2, 2022 14:44
authenticationContext.run(() -> {
SSLContext defaultSSLContext = null;
try {
defaultSSLContext = SSLContext.getDefault();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line throws the IllegalArgumentException, right?

}
Assert.assertNotNull(defaultSSLContext);
Assert.assertNotEquals(ElytronClientDefaultSSLContextProvider.class.getSimpleName(), defaultSSLContext.getProvider().getName()); // diff provider was used since elytron provider is looping
Assert.assertNotNull(defaultSSLContext.getSocketFactory());
Copy link
Contributor

Choose a reason for hiding this comment

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

So then these assertions won't actually be hit in this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, I removed those assertions

* Test that default SSLContext from provider can use programmatic configuration
*/
public class DefaultSSLContextProviderProgrammaticConfigurationTest {
private static final String CONFIG_FILE = "file:./src/test/resources/org/wildfly/security/auth/client/provider/test-wildfly-config-client-default-sslcontext.xml";
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, is the file: needed here? Or are we just trying the case where it is specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fjuma Yes exactly, file: is not needed. I just left it this way to test this variation as well

<?xml version="1.0" encoding="UTF-8"?>

<configuration>
<authentication-client xmlns="urn:elytron:client:1.5">
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the latest schema version is 1.7 so we should update this here and in the other files too.

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

@@ -230,7 +234,6 @@ public SSLContext getSSLContext(URI uri, AuthenticationContext authenticationCon
* @return the matching SSL context factory (not {@code null})
*/
public SecurityFactory<SSLContext> getSSLContextFactory(URI uri, AuthenticationContext authenticationContext, String abstractType, String abstractTypeAuthority) {
Assert.checkNotNullParam("uri", uri);
Copy link
Contributor

Choose a reason for hiding this comment

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

The javadoc for the parameter might need to be updated if it mentions that null is not allowed.

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

@Message(id = 14005, value = "Default SSL context in security provider creates infinite loop")
NoSuchAlgorithmException sslContextForSecurityProviderCreatesInfiniteLoop();

@Message(id = 14007, value = "Configuration file path passed to ElytronClientDefaultSSLContextProvider not found")
Copy link
Contributor

@fjuma fjuma Mar 2, 2022

Choose a reason for hiding this comment

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

Should this be 14006? It doesn't look like it's already 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.

Fixed

Assert.fail("Obtaining of default SSLContext with both config file and programmatic configuration present threw NoSuchAlgorithmException exception.");
}
Assert.assertNotNull(defaultSSLContext);
Assert.assertNotNull(defaultSSLContext.getSocketFactory());
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like we have any tests that validate the returned default SSLContext (e.g., Say we configured a default-ssl-context in wildfly-config.xml with specific cipher suites, protocols, etc. It would be good to have a test that validates the returned SSLContext to make sure it's correct).

Copy link
Contributor Author

@Skyllarr Skyllarr Mar 9, 2022

Choose a reason for hiding this comment

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

@fjuma Added protocol and cipher specification and test checks that. Thanks!

@@ -0,0 +1,23 @@
<assembly xmlns="http://maven.apache.org/plugins/maven-assembly-plugin/assembly/1.1.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing <?xml version="1.0" encoding="UTF-8"?>

</configuration>
</plugin>
<plugin>
<artifactId>maven-assembly-plugin</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, is there a reason to use this instead of maven-shade-plugin?

@fjuma
Copy link
Contributor

fjuma commented Mar 4, 2022

@Skyllarr Was taking a look at the wildfly-elytron-client-provider JAR and it contains a lot of stuff in it.

Currently, this JAR is only needed for the case where a user is registering the provider in the java.security file and attempting to add the JAR to the classpath manually.

What I'm wondering is if instead of producing this JAR with all these dependencies, could we just address this in the documentation instead? For example, we could mention that when registering this provider in the java.security file, the user needs to add a runtime dependency on wildfly-elytron-client to their client application.

@Skyllarr Skyllarr force-pushed the eap7-1120 branch 3 times, most recently from 2b8d8b9 to eb7f302 Compare March 9, 2022 22:09
@fjuma fjuma removed the hold label Mar 10, 2022
@@ -196,6 +196,10 @@ private static AuthenticationConfiguration initializeConfiguration(final URI uri
return configuration;
}

public SSLContext getSSLContext(AuthenticationContext authenticationContext) throws GeneralSecurityException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to add javadoc here.

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. Thanks!

@fjuma fjuma removed the +1 FJ label Mar 10, 2022
@fjuma fjuma dismissed their stale review March 10, 2022 20:28

Changes made.

@fjuma
Copy link
Contributor

fjuma commented Mar 10, 2022

Thanks for the updates @Skyllarr! I just did another pass and have only added a small comment now.

@darranl You had previously reviewed this a little while ago. There have been a bunch of updates since then. If you could take another pass through the updated changes, that would be great. Thanks.

@Skyllarr Skyllarr force-pushed the eap7-1120 branch 2 times, most recently from 7304f34 to 40ca1fb Compare March 11, 2022 15:41
if (e.getCause() instanceof FileNotFoundException) {
throw log.clientConfigurationFileNotFound();
}
throw new NoSuchAlgorithmException(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to add to ElytronMessages.

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 @Skyllarr!

*
* @param configPath path to Elytron client configuration path
*/
public Provider configure(String configPath) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My latest change was overriding this method so that JDK9+ can handle passing of the config path URL from java.security file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @Skyllarr! Looks good to me.

@fjuma fjuma merged commit b79b79f into wildfly-security:1.x Mar 17, 2022
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