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

[SHIRO-290] Implement BCrypt and Argon2 #280

Merged
merged 4 commits into from
May 19, 2021
Merged

[SHIRO-290] Implement BCrypt and Argon2 #280

merged 4 commits into from
May 19, 2021

Conversation

bmarwell
Copy link
Contributor

superseeds #273

[SHIRO-290] WIP: Implement Unix crypt format, starting with bcrypt.

  • TBD: HashRequest
  • TBD: PasswortMatcher doesn’t know about the new hash format yet

[SHIRO-290] Rework to use existing Shiro1CryptFormat.

  • Hashes can now compare themselves to a given password.
    Reviwers: Review method placement and HAsh class description.
  • removed hashrequest
  • removed UnixCryptFormat
  • API change: made salt not-nullable.
    Additional constructor is supplied for hashing without or with
    default salt, the former and other methods/fields using
    SimpleByteSource.empty().
    Reviewers: Pay attention to method logic, so no empty salt is
    being used where a former null value would have created a
    new, random salt.
  • Modified tests to not expect exceptions in certain cases.
  • Modified tests to not expect passwordService calls when supplying an
    existing hash.
  • TBD: Fix Javadocs
  • TBD: Fix Hasher utility
  • TBD: Deprecate old non-KDF hash classes

[SHIRO-290] Prepare argon2 implementation.

  • BCrypt iterations vs cost: make iterations return iterations
  • add validate methods

[SHIRO-290] Implement Argon2Hash.java.

  • expand iterations field to take a comma separated list. Maybe just create a Shiro2CryptFormat instead?
  • Hex and Base64 formats are not fixed. Maybe we can drop them?
  • Fixed parameter "algorithm name" not taken into account for bcrypt.
  • Allow Hasher to read from stdin
  • Added a short test for Hasher.java.
  • Changed default DefaultPasswordService.java algorithm to "Argon2id".

[SHIRO-290] Implement Shiro2CryptFormat.java.

  • Only fields 1 and two are defined, rest is defined by the hash implementation
  • Therefore fully backwards-compatible to Shiro1CryptFormat.java.
  • Loads formats from ProvidedKdfHashes.java.
    We could also think of a pluggable mechanism, like using service loaders to hide classes
    like OpenBSDBase64.
  • In AbstractCryptHash.java, renamed version to algorithmName.
  • Removed iterations from AbstractCryptHash.java, they are possibly an implementation detail not
    present in other implementations (like bcrypt).
  • Signature change: PasswordService.encryptPassword(Object plaintext) will now throw
    a NullPointerException on null parameter. It was never specified how this method would behave.

[SHIRO-290] Add hasher tests

  • fix invalid cost factor for bcrypt when input is 0.
  • output Hasher messages using slf4j.

[SHIRO-290] ServiceLoadable KDF algorithms.

  • Move BCrypt and Argon2 into their own modules
  • Add a SPI
  • Remove hardcoded parameters, replace with ParameterMap for the hashRequest

[SHIRO-290] implemented review comments

  • remove at least MD2, MD5 and Sha1
  • Remove unused support-hashes module
  • changed group and artifact-ids for new modules
  • fixed compilation issue in Hasher (needs more work though)
  • add "since 2.0" comments

[SHIRO-290] add some javadoc, make implementation classes package-private.

[SHIRO-290] doc updates


Following this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a JIRA issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [SHIRO-XXX] - Fixes bug in SessionManager,
    where you replace SHIRO-XXX with the appropriate JIRA issue. Best practice
    is to use the JIRA issue title in the pull request title and in the first line of the commit message.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean install apache-rat:check to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • If you have a group of commits related to the same change, please squash your commits into one and force push your branch using git rebase -i.

Trivial changes like typos do not require a JIRA issue (javadoc, comments...).
In this case, just format the pull request title like (DOC) - Add javadoc in SessionManager.

If this is your first contribution, you have to read the Contribution Guidelines

If your pull request is about ~20 lines of code you don't need to sign an Individual Contributor License Agreement
if you are unsure please ask on the developers list.

To make clear that you license your contribution under the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

Copy link
Member

@fpapon fpapon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of files to review but after a first look, it's ok for me.
LGTM

Copy link
Member

@bdemers bdemers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a quick pass-through. It looks good to me :)

A few minor comments in there from me, but great work, and (sorry for not taking a pass-through this earlier)

It might also be worth mentioning the OWASP cheat sheet (though that might be a site/doc PR once we get a 2.x release out), I'm just adding it here as it is top of mind: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html


@SuppressWarnings("unchecked")
private static ServiceLoader<HashSpi> load() {
return ServiceLoader.load(HashSpi.class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, this should not happen in managed environments (like java web containers / app servers), unless the app is not using a managed thread.
Since this is an isolated point, we will fix it as soon as we hit a problem.


import static org.junit.jupiter.api.Assertions.*

class Argon2HashTest {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: we may want to include the argon2 test vectors from the draft rfc too: https://datatracker.ietf.org/doc/draft-irtf-cfrg-argon2/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bdemers Please review and merge: #290

@fpapon fpapon requested a review from bdemers May 8, 2021 05:28
bmarwell added 3 commits May 10, 2021 20:19
[SHIRO-290] WIP: Implement Unix crypt format, starting with bcrypt.

  - TBD: HashRequest
  - TBD: PasswortMatcher doesn’t know about the new hash format yet

[SHIRO-290] Rework to use existing Shiro1CryptFormat.

  - Hashes can now compare themselves to a given password.
    Reviwers: Review method placement and HAsh class description.
  - removed hashrequest
  - removed UnixCryptFormat
  - API change: made salt not-nullable.
    Additional constructor is supplied for hashing without or with
    default salt, the former and other methods/fields using
    SimpleByteSource.empty().
    Reviewers: Pay attention to method logic, so no empty salt is
    being used where a former `null` value would have created a
    new, random salt.
  - Modified tests to not expect exceptions in certain cases.
  - Modified tests to not expect passwordService calls when supplying an
    existing hash.
  - TBD: Fix Javadocs
  - TBD: Fix Hasher utility
  - TBD: Deprecate old non-KDF hash classes

[SHIRO-290] Prepare argon2 implementation.

 - BCrypt iterations vs cost: make iterations return iterations
 - add validate methods

[SHIRO-290] Implement Argon2Hash.java.

 - expand iterations field to take a comma separated list. Maybe just create a Shiro2CryptFormat instead?
 - Hex and Base64 formats are not fixed. Maybe we can drop them?
 - Fixed parameter "algorithm name" not taken into account for bcrypt.
 - Allow Hasher to read from stdin
 - Added a short test for Hasher.java.
 - Changed default DefaultPasswordService.java algorithm to "Argon2id".

[SHIRO-290] Implement Shiro2CryptFormat.java.

 - Only fields 1 and two are defined, rest is defined by the hash implementation
 - Therefore fully backwards-compatible to Shiro1CryptFormat.java.
 - Loads formats from ProvidedKdfHashes.java.
   We could also think of a pluggable mechanism, like using service loaders to hide classes
   like OpenBSDBase64.
 - In AbstractCryptHash.java, renamed `version` to `algorithmName`.
 - Removed iterations from AbstractCryptHash.java, they are possibly an implementation detail not
   present in other implementations (like bcrypt).
 - Signature change: `PasswordService.encryptPassword(Object plaintext)` will now throw
   a NullPointerException on `null` parameter. It was never specified how this method would behave.

[SHIRO-290] Add hasher tests

 - fix invalid cost factor for bcrypt when input is 0.
 - output Hasher messages using slf4j.

[SHIRO-290] ServiceLoadable KDF algorithms.

  - Move BCrypt and Argon2 into their own modules
  - Add a SPI
  - Remove hardcoded parameters, replace with ParameterMap for the hashRequest

[SHIRO-290] implemented review comments

 - remove at least MD2, MD5 and Sha1
 - Remove unused support-hashes module
 - changed group and artifact-ids for new modules
 - fixed compilation issue in Hasher (needs more work though)
 - add "since 2.0" comments

[SHIRO-290] add some javadoc, make implementation classes package-private.

[SHIRO-290] doc updates
 - use default parameters from IETF draft.
 - use bits as unit size which is more common for hashes.
@bmarwell bmarwell merged commit 2e9cddc into main May 19, 2021
@bmarwell bmarwell deleted the SHIRO-290 branch May 19, 2021 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants