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

Support ClientEndpointConfig#getSSLContext #767

Merged
merged 2 commits into from
Feb 10, 2022

Conversation

jansupol
Copy link
Contributor

Signed-off-by: jansupol [email protected]

Copy link
Member

@m0mus m0mus left a comment

Choose a reason for hiding this comment

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

LGTM

@jansupol jansupol added this to the 2.1.0 milestone Jan 27, 2022
@jansupol jansupol merged commit 5dd2a1d into eclipse-ee4j:2.x Feb 10, 2022
@jansupol jansupol deleted the getSSLContext branch February 10, 2022 19:31
@NilsRenaud
Copy link

NilsRenaud commented Sep 14, 2022

I'm facing an issue with this change, is there any reason you removed these lines ?

(containers/grizzly-client/src/main/java/org/glassfish/tyrus/container/grizzly/client/GrizzlyClientSocket.java#758-761)

if (configuratorObject instanceof SslEngineConfigurator) {
    return new ExtendedSSLEngineConfigurator((SslEngineConfigurator) configuratorObject, uri.getHost());
}

@jansupol

@jansupol
Copy link
Contributor Author

@NilsRenaud
As you can see the lines were duplicated and now Tyrus 2.1.0 contains just a single copy of that condition.

The change is only in EE10 branch. If you are using the EE10 branch, could your issue be caused by missing EE10 dependencies? Tyrus 1.19 (EE8), 2.0.4 (EE9) still contain the condition duplicated.

@NilsRenaud
Copy link

Nop that's not duplicated line, the first line is org.glassfish.tyrus.client.SslEngineConfigurator and the second one is org.glassfish.grizzly.ssl.SSLEngineConfigurator

The second one allowed us to disable TLS hostname verification.

@jansupol
Copy link
Contributor Author

oops, you are right. That needs to be fixed.

@NilsRenaud
Copy link

I've created an issue to track the fix : #804 :)

@jansupol
Copy link
Contributor Author

@jansupol 2.1.2 containing the fix is public now.

@NilsRenaud
Copy link

Thanks a lot @jansupol !

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

Successfully merging this pull request may close these issues.

3 participants