-
Notifications
You must be signed in to change notification settings - Fork 280
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-1996] [Community] SSLContext to support delegation to alternate instances based on peer information. #1382
Conversation
42cb2b0
to
6566eed
Compare
822cd3b
to
ced5fb0
Compare
Linux - JDK11 EA 28 Build 463 outcome was FAILURE using a merge of d7c3dd4 Failed tests
|
Linux - JDK11 EA 28 Build 468 outcome was FAILURE using a merge of 90a930f Failed tests
|
Windows Build 464 outcome was FAILURE using a merge of 90a930f Failed tests
|
bcdc493
to
0aae7a3
Compare
Just FYI, I have updated the base branch for this PR to 2.x. |
c01ea92
to
32aedbc
Compare
@@ -196,6 +198,42 @@ private static AuthenticationConfiguration initializeConfiguration(final URI uri | |||
return configuration; | |||
} | |||
|
|||
/** | |||
* Get all SSL contexts configured for this authentication context. | |||
* This method is not part of the public API. |
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 org.wildfly.security.auth.client.AuthenticationContextConfigurationClient
class is public API so this method is also public API as discussed on Zulip. Should the sentence "This method is not part of the public API." be removed?
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 this should be removed, good catch I removed it thank you!
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.
@Skyllarr This is looking good! Just some small questions.
|
||
/** | ||
* Get the default SSL context that should be used when no other rules match, or {@link SSLContext#getDefault()} if there is none configured. | ||
* This method is not part of the public API. |
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.
Sentence is removed now
* This method is not part of the public API. | ||
* | ||
* @param authenticationContext the authentication context to examine (must not be {@code null}) | ||
* @return List of all configured SSL context belonging to the provided authentication context |
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.
s/context/contexts
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, thanks! I also added null check for authenticationContext to both methods
@darranl Please review, you can also check out the related wildfly-core PR wildfly/wildfly-core#4311 and wildfly PR wildfly/wildfly#13687 |
...src/main/java/org/wildfly/security/auth/client/AuthenticationContextConfigurationClient.java
Show resolved
Hide resolved
...src/main/java/org/wildfly/security/auth/client/AuthenticationContextConfigurationClient.java
Show resolved
Hide resolved
dynamic-ssl/src/main/java/org/wildfly/security/dynamic/ssl/DynamicSSLContextImpl.java
Outdated
Show resolved
Hide resolved
dynamic-ssl/src/test/java/org/wildfly/security/dynamic/ssl/DynamicSSLTestUtils.java
Show resolved
Hide resolved
.../wildfly/security/dynamic/ssl/wildfly-config-dynamic-ssl-test-without-default-sslcontext.xml
Outdated
Show resolved
Hide resolved
...-ssl/src/test/resources/org/wildfly/security/dynamic/ssl/wildfly-config-dynamic-ssl-test.xml
Outdated
Show resolved
Hide resolved
Do we consider any of the DynamicSSLContext classes as public api or is this only usable via the authentication client APIs? |
…instances based on peer information.
@darranl No it is not considered public API and that's why it is in a separate module. I also did not add it to the javadocs for public/supported APIs. On the client side it can be imported and initialized with the authentication context, see included tests, for example:
but it is not considered supported and so it is only for community users. The authentication client APIs do not have it as a dependency. On the server side for outgoing connections it is only available via dynamic-client-ssl-context resource in the community stability level |
This does not change any approvals here but I think we need to think at some point about "public API", I think at some point we may need to consider some APIs as public API in the context of community so they can be documented / have their APIs preserved. |
This PR is just a place to discuss the approach.
https://issues.redhat.com/browse/EAP7-1121