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

Remove unused bcrypt password hashing methods #852

Merged
merged 2 commits into from Jul 14, 2023

Conversation

exceptionfactory
Copy link
Contributor

This pull request removes unused bcrypt password hashing methods and associated unit tests.

The OpenSSH Private Key format uses a password-based key derivation function described in bcrypt_pbkdf documentation, which does not depend on the bcrypt password hashing methods. The BCrypt class retains methods necessary for key derivation, but does not need other methods related to password hashing and custom Base64 encoding.

@codecov-commenter
Copy link

codecov-commenter commented Mar 31, 2023

Codecov Report

Merging #852 (2962727) into master (0d16fbe) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #852      +/-   ##
============================================
- Coverage     65.06%   65.04%   -0.03%     
+ Complexity     1433     1412      -21     
============================================
  Files           209      209              
  Lines          8061     7918     -143     
  Branches        752      722      -30     
============================================
- Hits           5245     5150      -95     
+ Misses         2409     2383      -26     
+ Partials        407      385      -22     
Impacted Files Coverage Δ
...nomus/sshj/userauth/keyprovider/bcrypt/BCrypt.java 96.46% <100.00%> (+13.64%) ⬆️

... and 6 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@hierynomus
Copy link
Owner

Given that it's tested code, copied from a third party (which, if I remember correctly, does not provide a nice Maven dependency), I'd be tempted to keep the class intact. I don't think it hurts to have it in? Or do you disagree?

@exceptionfactory
Copy link
Contributor Author

Given that it's tested code, copied from a third party (which, if I remember correctly, does not provide a nice Maven dependency), I'd be tempted to keep the class intact. I don't think it hurts to have it in? Or do you disagree?

Thanks for the feedback @hierynomus.

Although the code is copied from an third party, I think it is better to retain only the elements that SSHJ actually uses. As evidenced by the methods removed, bcrypt key derivation is quite different from bcrypt password hashing. There is a well-maintained bcrypt password hashing library but it does not support bcrypt key derivation. From the perspective of only including code that actually applies to SSHJ, it seems like it would be better to remove the unused methods. On the other hand, if you think there is value is retaining the original source without any changes, I can see why that might be helpful for reference purposes. Less is more when it comes maintenance, and this improves some overall quality metrics, so that is another positive.

If you prefer to keep the original class unchanged, I can understand that approach.

@hierynomus
Copy link
Owner

Hmm Makes sense, can't remember I actually came across the bcrypt lib before you mentioned. Agreed. let's clean it up. (Actually the overall code coverage dropped ;) )

@hierynomus hierynomus merged commit 0783709 into hierynomus:master Jul 14, 2023
5 checks passed
@exceptionfactory
Copy link
Contributor Author

Thanks for merging! Removing those methods did close out a handful of minor code style issues according to the current Codacy settings.

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.

None yet

3 participants