Skip to content

Conversation

alonbl
Copy link
Contributor

@alonbl alonbl commented Sep 2, 2015

Adding a security provider at runtime should be avoided as it effects
the entire JVM and may effect other efforts.

Constructing com.sun.net.ssl.internal is not part of public API.

This change performs the construction and registration of sun internal
JSSE provider only if SSLContext cannot be established, it solves only
the issue of not constructing and registering JSSE provider if already
available.

Signed-off-by: Alon Bar-Lev alon.barlev@gmail.com

@ronsigal
Copy link
Collaborator

ronsigal commented Feb 8, 2016

Hi Alon,

  1. Why does this matter? tjws is just used for Resteasy testing.
  2. Have you looked into the netty test failure (https://s3.amazonaws.com/archive.travis-ci.org/jobs/78357698/log.txt)?

-Ron

@alonbl
Copy link
Contributor Author

alonbl commented Feb 9, 2016

@ronsigal: why do you think it is used only for testing? we got this error in our implementation.

I do not see how this test failure is related to this change, I will try to rebase.

…uired

Adding a security provider at runtime should be avoided as it effects
the entire JVM and may effect other efforts.

Constructing com.sun.net.ssl.internal is not part of public API.

This change performs the construction and registration of sun internal
JSSE provider only if SSLContext cannot be established, it solves only
the issue of not constructing and registering JSSE provider if already
available.

Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>
@ronsigal
Copy link
Collaborator

ronsigal commented Feb 9, 2016

re: "why do you think it is used only for testing? we got this error in our implementation."

Ah, you're using tjws in production? I didn't know anyone did that.

@alonbl
Copy link
Contributor Author

alonbl commented Feb 9, 2016

@ronsigal: ci is ok, this change will be applied only if there is no ssl implementation what-so-ever, which is a very rare case, when falling back to "manual" injection (left for backward compatibility) the load of the internal sun provide may or may not be in classpath, which is fine.

Example: jboss module does not import the internal sun classes, it can use TLS protocol but cannot load the class directly.

@ronsigal
Copy link
Collaborator

Hi Alon,

Ok, looks good. I'm going to ask you for two things:

  1. A Resteasy JIRA issue.
  2. A test case - I can't see anywhere in Resteasy where Acme.Serve.Serve.Acceptor.SSLAcceptor is used.

Thanks,
Ron

@alonbl
Copy link
Contributor Author

alonbl commented Feb 10, 2016

Hi Ron,

I am sorry, but I do not have access to the environment in which this happened. But again, what happened is that this class was somehow called and modified the system properties to try and load a class that was not set in the jboss module classpath.

A library should not modify the state of the entire jvm, this class is doing that and invalidate settings of the container and other classes.
You can look at the SSLAcceptor static contact, in which it calls an initHandler() that also modifies the jvm setting.

I am unsure what bug should I open... I suggest: "resteasy should not modify jvm context nor attempt to dynamic load any class outside of resteasy package".

What do you think?

Thanks,
Alon

@ronsigal
Copy link
Collaborator

  1. For the test case, I wouldn't expect much. Just a test in which SSLAcceptor is used without any problems.
  2. For the JIRA issue, maybe something more specific, like "SSLAcceptor should add security provider only when none exists", or something like that.

Thanks.

@liweinan
Copy link
Collaborator

liweinan commented Mar 8, 2016

Let me work on a test case

@liweinan liweinan changed the title jaxrs: ssl: construct and register java internal JSSE provider if required RESTEASY-1318: TJWS construct and register java internal JSSE provider if required Mar 9, 2016
@liweinan
Copy link
Collaborator

I have made some progress on this. I'll try to update the PR next week.

@liweinan liweinan merged commit 7155468 into resteasy:master Mar 22, 2016
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