[FIXED JENKINS-30109] Update to bouncycastle 1.53 #8
Conversation
|
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
|
@seryl Could you add a Jira issue about this? |
seryl
changed the title
|
@seryl Thanks so much. I'll try your proposal. |
|
@seryl If you are working on this ticket, it would be interesting you accept the Jira ticket. |
recena
changed the title
|
@recena I'm not sure what you mean? All of the work is done to get it finalized. Jenkins core also needed to be updated to 152, and as far as I'm aware jenkinsci/instance-identity-plugin#3 does that. I can assign the tickets to myself in the Jenkins Jira, but they are already done and I don't have merge permissions; so I'm not sure how that helps..? Let me know what further steps I need to do to pull these in. Eager to get this finished. |
|
@seryl I've update JENKINS-30109 status. |
|
@seryl I can merge this PR but I need to review it. |
|
@recena Any chance you could look at this? 36 additions. |
|
@seryl Could you squash the commits? |
| } finally { | ||
| r.close(); | ||
| } | ||
| } catch (Exception e) { | ||
| e.printStackTrace(listener.error(Messages.SSHAgentBuildWrapper_UnableToReadKey(e.getMessage()))); | ||
| e.printStackTrace(listener.getLogger()); |
There was a problem hiding this comment.
@seryl Why do you print listener.getLogger()?
There was a problem hiding this comment.
I did it to match the mirrored configuration on JNRRemoteAgent.java
If this is something you want, I assume you want it in both places.
If not, I assume you don't want it in either.
There was a problem hiding this comment.
They should both be using the listener.error form
There was a problem hiding this comment.
Did we not want to print the getlogger then?
There was a problem hiding this comment.
listener.error also sends to getLogger but with the prefix message and allows for the two to be associated.
seryl
force-pushed
the
update-bouncycastle-152
branch
from
4272145
to
f59f93b
Compare
|
squashed and should be ready |
|
@stephenc I agree with the changes. Your feedback will be great here. |
|
Minor issue on logging. Would be great if a test could be included to prevent regressions. Otherwise looking good |
|
@stephenc I'm not sure what you mean... do you want me to validate that it's printing errors when they occur? If that's the case, I'll just remove that. I have little to no interest in getting that logging information in, I just want the bouncycastle updates. I just did it since that seemed like what the initial intention was. |
|
@seryl I want you to test that ECDH keys do not blow up. A test with a test resource of an ECDH key that gets set as a credential and passed through the MinaAgent would suffice. Testing the logging information is not required. |
|
I feel like there's some confusion..? ECDH ssl is what we're adding support for here, not ecdsa keys. We're allowing the key agreement when connecting to openssl servers to use the ecdh cipher. This has nothing to do with openssh, so I don't understand why you are looking for a test on a new type of key? This is to support downstream changes in the identity module. Note: The original title was upgrade bouncycastle to 152; so that there would not be any confusion, which there seems to be. Link to identity PR: jenkinsci/instance-identity-plugin#3 When both of these are added, we'll be able to connect to apache and nginx servers which are using the openssl ciphers such as: ECDHE-ECDSA-AES256-GCM-SHA384. The only change provided here for openssh, is to use the new bouncycastle interfaces, nothing more. |
|
Ahh ok, so you just want to bump the BC version (with the usual hassle of method signature changes) OK... I'd prefer then if you update the PR title to reflect that as otherwise when we document the changes users will get confused just as I did |
seryl
changed the title
|
@stephenc Thanks for getting back to me, updated as requested |
|
@stephenc jenkinsci/instance-identity-plugin#3 is the associated change, this should be the last of them to allow the upgrade to 152 for vanilla jenkins |
|
Just like jenkinsci/instance-identity-plugin#3 this has been an issue for us as well and this fix works for us. Would be great to get this merged. |
seryl
changed the title
|
Still waiting for the requested changes to be made |
|
I'm not sure I follow. What requested changes? |
|
You removed the correct way of logging the stack trace in one of the lines you changed. I requested you to revert that change in the PR comment on that line Sent from my iPhone
|
|
@stephenc removed as requested. |
[FIXED JENKINS-30109] Update to bouncycastle 1.53
https://issues.jenkins-ci.org/browse/JENKINS-30109
Fixes issues connecting to ECDH ssl servers /w jdk8, perhaps other fixes.