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

Replace PKCS5 Key File Class with PKCS8 #793

Merged
merged 3 commits into from
Jul 14, 2022

Conversation

exceptionfactory
Copy link
Contributor

This pull request removes the PKCS5KeyFile class and references in favor of parsing using PKCS8KeyFile. The Bouncy Castle PEMParser already used in PKCS8KeyFile supports both types of key files.

RFC 8018 describes PKCS5 as the Password-Based Cryptography Specification Version 2.0, including methods for key derivation using various algorithms. Although encrypted private keys make use of password-based cryptography, using PKCS5 as a type of key file does not describe the key format itself.

RFC 8017 describes PKCS1 as RSA Cryptography Specifications Version 2.2. Appendix A of RFC 8017 describes the ASN.1 syntax for RSA Public and Private Keys. The encoded binary contents of files starting with BEGIN RSA PRIVATE KEY or BEGIN RSA PUBLIC KEY align with the ASN.1 described in RFC 8017. For this reason, it is more accurate to describe the format of these files as PKCS1.

RFC 5958 describes PKCS8 Asymmetric Key Packages, including the ASN.1 syntax for different types of keys.

Although the naming of PKCS8KeyFile is accurate, the implementation leverages the Bouncy Castle PEMParser, which is capable of handling a variety of PEM encoded formats, including both PKCS8 and PKCS1 keys, with or without encryption. Removing the PKCS5KeyFile implementation and defaulting parsing to PKCS8KeyFile removes the need for custom PEM parsing in PKCS5KeyFile while maintaining support for the same types of keys.

Renaming PKCS8KeyFile to something more generalized, such as PEMKeyFile might be more accurate, but the initial approach avoids renaming to minimize the impact of changes.

Java supports unencrypted PKCS8 processing using PKCS8EncodedKeySpec and other classes. It might be possible to remove the dependency on Bouncy Castle with a custom PEM-parsing implementation, but it would be useful to streamline generalized PEM-encoded processing prior to considering those changes.

@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2022

Codecov Report

Merging #793 (c354dc7) into master (f33bfec) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #793      +/-   ##
============================================
+ Coverage     64.81%   64.86%   +0.05%     
+ Complexity     1480     1461      -19     
============================================
  Files           211      210       -1     
  Lines          8660     8516     -144     
  Branches        812      778      -34     
============================================
- Hits           5613     5524      -89     
+ Misses         2615     2587      -28     
+ Partials        432      405      -27     
Impacted Files Coverage Δ
src/main/java/net/schmizz/sshj/DefaultConfig.java 91.11% <ø> (ø)
src/main/java/net/schmizz/sshj/SSHClient.java 58.95% <ø> (ø)
...t/schmizz/sshj/userauth/keyprovider/KeyFormat.java 100.00% <ø> (ø)
...chmizz/sshj/userauth/keyprovider/PKCS8KeyFile.java 79.31% <ø> (+1.72%) ⬆️
...izz/sshj/userauth/keyprovider/KeyProviderUtil.java 58.33% <100.00%> (+0.64%) ⬆️
...in/java/net/schmizz/sshj/transport/digest/MD5.java 0.00% <0.00%> (-40.00%) ⬇️
...n/java/net/schmizz/sshj/common/ByteArrayUtils.java 53.06% <0.00%> (-16.33%) ⬇️
.../net/schmizz/sshj/transport/digest/BaseDigest.java 66.66% <0.00%> (-6.67%) ⬇️
...ain/java/net/schmizz/sshj/common/SSHException.java 67.74% <0.00%> (ø)
...net/schmizz/sshj/transport/TransportException.java 44.44% <0.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f33bfec...c354dc7. Read the comment docs.

- Moved tests for PEM-encoded PKCS1 files to PKCS8
- Removed PKCS5 Key File implementation
@exceptionfactory
Copy link
Contributor Author

Thanks for the feedback @hierynomus! I updated PKCS8KeyFileTest with a new test method to exercise retrying a password after initial failure.

@hierynomus hierynomus merged commit 5674072 into hierynomus:master Jul 14, 2022
ylangisc added a commit to iterate-ch/cyberduck that referenced this pull request Aug 11, 2022
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.

3 participants