added jenkins.bouncycastle.prioritizeJceProvider system property #29
Conversation
Added a new (optional) boolean system property which affects how BouncyCastle is registered as a JCE provider. When `true`, the provider is inserted at the beginning of the list (in 2nd position), instead of being added at the end. It could help in situations where BC does a better job than the JDK. In particular, it should solve issues with SSH connections to some SunOS servers, which end up with this exception (with JSCH from the publish-over-ssh plugin): ``` java.security.InvalidAlgorithmParameterException: DH key size must be multiple of 64, and can only range from 512 to 4096 (inclusive). The specific key size 2047 is not supported ``` See https://stackoverflow.com/a/43121515, where this idea comes from. In this case, simply having BouncyCastle registered but as the last provider is not good enough to solve the issue. Probably it could have been a solution for [JENKINS-35231](https://issues.jenkins-ci.org/browse/JENKINS-35231) too. The system property added here is expected to be set on Jenkins main process. Registration on agents will follow the same preference, and not a property of the agent process.
|
Note: For the (unrelated) tests failure on Windows, see #30. |
|
FTR, I've finally found a significant regression with It's something which seems to be fixed already in BC 1.65 (current AFAIK, it happened only after upgrading OpenJDK from 8u265 to 8u272 (which is partly why I'm only seeing that now; the other part of the explanation being that we only have a handful of TCP Inbound agents on the Jenkins servers we manage). The BC issue mentions OpenJDK 11, but I guess the "offending" OpenJDK changes has been backported in 8u272 (see realease notes). Anyway, this issue does not invalidate this PR I think. It mainly shows that:
|
src/main/java/jenkins/bouncycastle/api/BcProviderRegistration.java
Outdated
Show resolved
Hide resolved
src/test/java/jenkins/bouncycastle/api/InstallBouncyCastleJCAProviderTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This looks good. I've been bitten by the SunOS issue before on a previous project, so it's nice to see something that manage it. The implementation looks good. I'm glad to see the tests.
I consider this an experimental feature for now. That's pretty well indicated by the system property to enable it. Retaining the original behavior as default is good.
| /** | ||
| * Utility class for registration of the BouncyCastle security provider. | ||
| */ | ||
| @Restricted(NoExternalUse.class) |
There was a problem hiding this comment.
Since the class isn't public this restriction shouldn't be necessary, but it doesn't hurt to be extra explicit about it.
|
Thanks for the submission! I didn't see any notification earlier so didn't realize this was here. |
|
Let's try a rebuild with #30 merged in. |
|
Try again to see if it's a flaky infra issue. I'm not seeing the failure locally. |
|
Try building on top of other plugin update PR. |
|
Finally |
Note: this is an experiment, hence the draft PR. The big TODO item is more testing in real-world conditions. So far, I only have one Jenkins server with this change enabled (where it was required to solve some SSH connection issues with a SunOS server), so we'll see how it goes there, and then at some point I will canary-test with more instances. Then, if no regression is detected, I will make it a real PR. For now, I'm only sharing in order to, maybe, get early feedback (if anyone has an opinion on the opportunity of such a change).Update on 2020/08/27: the experiment phase I was planning is over. I have actually deployed a patched
bouncycastle-apiplugin, and enabled-Djenkins.bouncycastle.prioritizeJceProvider=true, on a few hundreds of Jenkins installations, and got no negative feedback. Of course, this change is absolutely useless in most cases, but at least it was not harmful, which is what I was trying to verify. So I think this PR is safe and can now be considered for merging (keep in mind that it doesn't change anything anyway, as long as one doesn't setjenkins.bouncycastle.prioritizeJceProvider=true).Added a new (optional) boolean system property (
jenkins.bouncycastle.prioritizeJceProvider) which affects how BouncyCastle is registered as a JCE provider. Whentrue, the provider is inserted at the beginning of the list (in 2nd position), instead of being added at the end.It could help in situations where BC does a better job than the JDK. In particular, it should solve issues with SSH connections to some SunOS servers, which end up with this exception (with JSCH from the publish-over-ssh plugin):
See https://stackoverflow.com/a/43121515, where this idea comes from. In this case, simply having BouncyCastle registered, but as the last provider, was not good enough to solve the issue.
Probably it could also have been a solution for JENKINS-35231.
The system property added here is expected to be set on Jenkins controller process. Registration on agents will follow the same preference, and not a property of the agent process.