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

ZOOKEEPER-3404. Downgrade BouncyCastle to 1.60 #961

Closed
wants to merge 1 commit into from

Conversation

anmolnar
Copy link
Contributor

I've seen a lot of test timeout errors with QuorumSSL tests since I upgraded master to BouncyCastle 1.61 due to a Java 9 warning. The warning has been reported by Enrico Olivelli which we tried to solve by the upgrade, but the warning message is still present so I don't see any harm in downgrading to the previous version.

https://issues.apache.org/jira/browse/ZOOKEEPER-3404

@eolivelli
Copy link
Contributor

As we are using BC only for tests it is okay to downgrade in order to make tests more stable.

btw if we have these problems now someday we will see them again when we will need to upgrade.
Aren't we using BC only for generating certs and keys ? it is not used by the runtime.

BC comes with its own Security Providers, I am afraid that it not polluting the classpath during tests executions. The JVM (Javax Crypto) selects Security Providers by using what is on the classpath.
It is a problem if during tests execution we are using a Security Provider that it is not used in production.

We should add debug in every security-related utility and dump which Security Provider is in use.
In order to be sure about the security provider we are using every Javax Crypto utility has a way to force the provider without using auto discovery.

We should also add Netty (Google) Boring SSL library in order to be sure about the SSL implementation we are using.

Unfortunately we are not using Netty yet on server to server communication, as so I guess we are more fragile in this Security Provider selection.

cc @enixon

@anmolnar
Copy link
Contributor Author

btw if we have these problems now someday we will see them again when we will need to upgrade.
Aren't we using BC only for generating certs and keys ? it is not used by the runtime.

@eolivelli This is valid. BC is for testing only currently and we'll face this problem again once we need to upgrade to address a CVE for example. However that could be a more recent version of BC where they identify and fix the performance regression.

Other option could be to improve QuorumSSL tests: it's already running for 5-6 minutes which is kinda odd for unit tests. I'll do some research on that side to find where the bottleneck is.

Wrt to Security Providers I think you're right, though I'm not an expert of Java Security, so not sure how to address the problem properly. I suggest to keep this patch for the performance problem only for now.

Please see my comments / evidence in Jira too.

Copy link
Contributor

@nkalmar nkalmar left a comment

Choose a reason for hiding this comment

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

Then I think we have an agreement for the downgrade. For now, this will solve the performance issue on Quorum SSL tests. Thanks Andor.

@asfgit asfgit closed this in 78b3d1a May 28, 2019
asfgit pushed a commit that referenced this pull request May 28, 2019
I've seen a lot of test timeout errors with QuorumSSL tests since I upgraded master to BouncyCastle 1.61 due to a Java 9 warning. The warning has been reported by Enrico Olivelli which we tried to solve by the upgrade, but the warning message is still present so I don't see any harm in downgrading to the previous version.

https://issues.apache.org/jira/browse/ZOOKEEPER-3404

Author: Andor Molnar <andor@apache.org>

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Norbert Kalmar <nkalmar@apache.org>

Closes #961 from anmolnar/ZOOKEEPER-3404

(cherry picked from commit 78b3d1a)
Signed-off-by: Norbert Kalmar <nkalmar@apache.org>
@nkalmar
Copy link
Contributor

nkalmar commented May 28, 2019

Thanks @anmolnar , merged to master and 3.5

@anmolnar anmolnar deleted the ZOOKEEPER-3404 branch May 28, 2019 16:25
@enixon
Copy link

enixon commented May 30, 2019

@eolivelli @anmolnar - one of you want to open a ticket for the improvements mentioned above? Getting the dependencies clarified and reducing the running time of the SSL tests are good improvements.

RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Sep 3, 2022
I've seen a lot of test timeout errors with QuorumSSL tests since I upgraded master to BouncyCastle 1.61 due to a Java 9 warning. The warning has been reported by Enrico Olivelli which we tried to solve by the upgrade, but the warning message is still present so I don't see any harm in downgrading to the previous version.

https://issues.apache.org/jira/browse/ZOOKEEPER-3404

Author: Andor Molnar <andor@apache.org>

Reviewers: Enrico Olivelli <eolivelli@apache.org>, Norbert Kalmar <nkalmar@apache.org>

Closes apache#961 from anmolnar/ZOOKEEPER-3404
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants