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-1996] SSLContext to support delegation to alternate instances based on peer information. #1488

Closed

Conversation

Skyllarr
Copy link
Contributor

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

This is PR is meant to be merged before we have a QE pre-check.

Currently it does change public API because as it adds 2 public methods to AuthenticationContextConfigurationClient.java, please see the comment below and let me know if that is ok or I should use a reflection instead.

* @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
*/
public List<SSLContext> getConfiguredSSLContexts(AuthenticationContext authenticationContext) throws GeneralSecurityException {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@darranl @fjuma I did not realize before, but when I move the public class DynamicSSLContextSpiImpl from elytron client to dynamic-ssl module, then I can't use these methods without them being public.

So we either add them as public methods and use a disclaimer saying it is not part of the public API (in javadoc/blog etc.), or use a reflection. I decided to do the former but let me know if you think otherwise. Thanks.

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 looking at the two methods we should just accept them in a new public API, I don't think this will affect our plans to include in Elytron 1.16 as the methods have a well defined scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@wildfly-ci
Copy link

Windows Build 626 outcome was FAILURE using a merge of 0b39596
Summary: Exit code 1 (Step: Maven) (new) Build time: 00:00:20

@wildfly-ci
Copy link

Linux - JDK11 EA 28 Build 632 outcome was UNKNOWN using a merge of 0b39596
Summary: Canceled (Error while applying patch; cannot find commit a4b7686 in the https://github.com/wildfly-security/wildfly-elytron.git repository, possible reason: refs/pull/1488/merge branch was updated and the commit selec... Build time: 00:00:53

@Skyllarr Skyllarr force-pushed the dynamic-ssl-elytron-part branch 4 times, most recently from bfdc8a5 to 5d759e3 Compare February 16, 2021 19:38
@wildfly-ci
Copy link

Linux - JDK11 EA 28 Build 637 outcome was FAILURE using a merge of 5d759e3
Summary: Tests passed: 484, ignored: 2; exit code 1 (Step: Maven) (new) Build time: 00:01:51

@wildfly-ci
Copy link

Windows Build 631 outcome was FAILURE using a merge of 5d759e3
Summary: Tests passed: 484, ignored: 2; exit code 1 (Step: Maven) (new) Build time: 00:02:51

@Skyllarr Skyllarr force-pushed the dynamic-ssl-elytron-part branch 4 times, most recently from 65886f8 to 46cfe35 Compare February 16, 2021 21:12
@fjuma
Copy link
Contributor

fjuma commented Feb 16, 2021

@Skyllarr Just noticed that some of the error messages from the GitHub CI jobs look similar to the some of the issues we ran into recently with Zulu JDK. Darran had submitted a PR to ensure that all SSLContext instances in the tests exclusively use the installed providers:

#1474

We'll likely need similar changes in this PR.

@Skyllarr
Copy link
Contributor Author

@Skyllarr Just noticed that some of the error messages from the GitHub CI jobs look similar to the some of the issues we ran into recently with Zulu JDK. Darran had submitted a PR to ensure that all SSLContext instances in the tests exclusively use the installed providers:

#1474

We'll likely need similar changes in this PR.

@fjuma yes that was the problem. Thanks a lot!

<parent>
<groupId>org.wildfly.security</groupId>
<artifactId>wildfly-elytron-parent</artifactId>
<version>1.14.3.CR1-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

This version now 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.

fixed

@wildfly-ci
Copy link

Linux - JDK11 EA 28 Build 657 outcome was FAILURE using a merge of f37301d
Summary: Exit code 1 (Step: Maven) (new) Build time: 00:00:21

@wildfly-ci
Copy link

Windows Build 649 outcome was FAILURE using a merge of f37301d
Summary: Exit code 1 (Step: Maven) (new) Build time: 00:00:23

<groupId>org.kohsuke.metainf-services</groupId>
<artifactId>metainf-services</artifactId>
<scope>provided</scope>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

@Skyllarr Just wondering why this is being removed. It looks like it was added for RESTEasy integration:

#1340

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 it maybe does not belong to this PR but I noticed it is a double dependency. The same dependency is in this file on line 118.

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, I didn't realize it was a duplicate.

fjuma
fjuma previously approved these changes Mar 5, 2021
@fjuma fjuma added the +1 FJ label Mar 5, 2021
<parent>
<groupId>org.wildfly.security</groupId>
<artifactId>wildfly-elytron-parent</artifactId>
<version>1.15.1.CR1-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

This version will need to be updated.

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