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

Don't depend on sun.security package #99

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

multani
Copy link

@multani multani commented May 25, 2021

This is not portable, and fails depending on the JVM used, see https://www.oracle.com/java/technologies/faq-sun-packages.html:

The sun.* packages are not part of the supported, public interface.
A Java program that directly calls into sun.* packages is not
guaranteed to work on all Java-compatible platforms. In fact, such a
program is not guaranteed to work even in future versions on the same
platform.

Also:

$ docker run --rm -ti -v $PWD:/src -w /src maven:3-openjdk-11 mvn package
[...]
[ERROR] COMPILATION ERROR :
[INFO] -------------------------------------------------------------
[ERROR] /src/common/src/test/java/com/zegelin/cassandra/exporter/netty/ssl/TestSslContextFactory.java:[10,24] sun.security.ssl.SSLEngineImpl is not public in sun.security.ssl; cannot be accessed from outside package
[ERROR] /src/common/src/test/java/com/zegelin/cassandra/exporter/netty/ssl/TestSslContextFactory.java:[49,78] sun.security.ssl.SSLEngineImpl is not public in sun.security.ssl; cannot be accessed from outside package
[ERROR] /src/common/src/test/java/com/zegelin/cassandra/exporter/netty/ssl/TestSslContextFactory.java:[166,78] sun.security.ssl.SSLEngineImpl is not public in sun.security.ssl; cannot be accessed from outside package
[ERROR] /src/common/src/test/java/com/zegelin/cassandra/exporter/netty/ssl/TestSslContextFactory.java:[178,78] sun.security.ssl.SSLEngineImpl is not public in sun.security.ssl; cannot be accessed from outside package
[...]

For some reason, the tests pass with the CircleCI Java Docker image; I tried with various combination of the JDK (dockerized Maven's maven:3-openjdk-11 and maven:3-openjdk-8 and the JDK from Oracle's own website) and none work.

This is not portable, and fails depending on the JVM used, see https://www.oracle.com/java/technologies/faq-sun-packages.html:

> The sun.* packages are not part of the supported, public interface.
> A Java program that directly calls into sun.* packages is not
> guaranteed to work on all Java-compatible platforms. In fact, such a
> program is not guaranteed to work even in future versions on the same
> platform.

Also:

    $ docker run --rm -ti -v $PWD:/src -w /src maven:3-openjdk-11 mvn package
    [...]
    [ERROR] COMPILATION ERROR :
    [INFO] -------------------------------------------------------------
    [ERROR] /src/common/src/test/java/com/zegelin/cassandra/exporter/netty/ssl/TestSslContextFactory.java:[10,24] sun.security.ssl.SSLEngineImpl is not public in sun.security.ssl; cannot be accessed from outside package
    [ERROR] /src/common/src/test/java/com/zegelin/cassandra/exporter/netty/ssl/TestSslContextFactory.java:[49,78] sun.security.ssl.SSLEngineImpl is not public in sun.security.ssl; cannot be accessed from outside package
    [ERROR] /src/common/src/test/java/com/zegelin/cassandra/exporter/netty/ssl/TestSslContextFactory.java:[166,78] sun.security.ssl.SSLEngineImpl is not public in sun.security.ssl; cannot be accessed from outside package
    [ERROR] /src/common/src/test/java/com/zegelin/cassandra/exporter/netty/ssl/TestSslContextFactory.java:[178,78] sun.security.ssl.SSLEngineImpl is not public in sun.security.ssl; cannot be accessed from outside package
    [...]
@itskarlsson
Copy link

I agree that depending on sun.* packages should be fixed, but the proposed fix you uploaded doesn't actually test anything since you are just checking the interface which both are a part of SSL implementations use. Those tests will pass even when the incorrect implementation is passed.

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.

2 participants