-
Notifications
You must be signed in to change notification settings - Fork 282
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
Conversation
a6d7666
to
1cfcd71
Compare
public Object newInstance(Object ignored) throws NoSuchAlgorithmException { | ||
Integer enteredCountTmp = entered.get(); | ||
entered.set(enteredCountTmp == null ? 1 : enteredCountTmp + 1); | ||
if (entered.get() >= 2) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment added.
@fabiobrz This is ready for review |
There was a problem hiding this 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing copyright header.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another missing header.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this 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"); |
There was a problem hiding this comment.
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)
?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
35d3dd4
to
ff960fd
Compare
@fabiobrz I added missing test. For |
ok, thanks - I somehow missed the expected exception declaration, my bad, sorry. |
@fabiobrz Thanks for your review, I replied to the remaining comments. Let me know what you think. |
There was a problem hiding this 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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
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.
a94264d
to
2e3c885
Compare
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 Btw I tried to remove scope provided for |
7cae61d
to
b23763a
Compare
authenticationContext.run(() -> { | ||
SSLContext defaultSSLContext = null; | ||
try { | ||
defaultSSLContext = SSLContext.getDefault(); |
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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!
auth/client-provider/assembly.xml
Outdated
@@ -0,0 +1,23 @@ | |||
<assembly xmlns="http://maven.apache.org/plugins/maven-assembly-plugin/assembly/1.1.3" |
There was a problem hiding this comment.
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"?>
auth/client-provider/pom.xml
Outdated
</configuration> | ||
</plugin> | ||
<plugin> | ||
<artifactId>maven-assembly-plugin</artifactId> |
There was a problem hiding this comment.
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
?
@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 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 |
2b8d8b9
to
eb7f302
Compare
@@ -196,6 +196,10 @@ private static AuthenticationConfiguration initializeConfiguration(final URI uri | |||
return configuration; | |||
} | |||
|
|||
public SSLContext getSSLContext(AuthenticationContext authenticationContext) throws GeneralSecurityException { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. Thanks!
7304f34
to
40ca1fb
Compare
if (e.getCause() instanceof FileNotFoundException) { | ||
throw log.clientConfigurationFileNotFound(); | ||
} | ||
throw new NoSuchAlgorithmException(e); |
There was a problem hiding this comment.
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.
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.