-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Refactor SSLUtils.loadKey using PEMParser #2441
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
Refactor SSLUtils.loadKey using PEMParser #2441
Conversation
|
|
|
Welcome @exceptionfactory! |
- Removed custom DER parsing and Base64 decoding in favor of Bouncy Castle PEM Parser - Refactored SSLUtilsTest methods with reusable assertion method and algorithm checks
c634da3 to
37cbbe0
Compare
|
Thanks for the PR. A couple of questions: Second can I request that this is split into two PRs:
The reason for this is that I want to make sure that the existing tests pass with the refactored SSL code. I worry that in refactoring both the implementation and the test at the same time it is possible for behaviors to regress. Thanks! |
|
Thanks for the quick reply @brendandburns! I was reviewing the client code for its approach to loading private keys and came across issue #1724. I also noted the code comments with the try-catch approach to handling different algorithms with PKCS8. I haven't noticed any runtime issues, but I have worked on projects such as sshj to improve key loading. I understand the concern about refactoring both the implementation and tests together. I will update this PR to remove the unit test updates and create a new PR for just those changes. |
|
/lgtm (assuming tests pass :) |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, exceptionfactory The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This pull request implements private key loading refactoring proposed in #2440.
The implementation refactors the
SSLUtils.loadKeymethod to use the Bouncy CastlePEMParserfor all types of private keys. The existing implementation usesPEMParserforECDSAPrivate Keys, and the proposed changes eliminate the need for custom DER and PKCS8 parsing. Changes include updates to the SSLUtils test class, providing a reusable test method and also asserting expected algorithm values for parsed Private Keys.