[SHIRO-290] Implement BCrypt and Argon2 #280

bmarwell · 2021-01-13T15:08:53Z

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.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

fpapon

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

...s/argon2/src/test/groovy/org/apache/shiro/crypto/support/hashes/argon2/Argon2HashTest.groovy

bdemers

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

bdemers · 2021-04-05T17:01:04Z

crypto/hash/src/main/java/org/apache/shiro/crypto/hash/HashProvider.java

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


There might be some potential classloading issues here. We had to work around in JJWT:

Services now checks the contextClassLoader, Services.class.classLoader, and the system classloader jwtk/jjwt#574

ServiceLocator can't find an implementation in common-pool workers jwtk/jjwt#568

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.

crypto/hash/src/main/java/org/apache/shiro/crypto/hash/format/Shiro2CryptFormat.java

bdemers · 2021-04-05T17:20:07Z

...s/argon2/src/test/groovy/org/apache/shiro/crypto/support/hashes/argon2/Argon2HashTest.groovy

+
+import static org.junit.jupiter.api.Assertions.*
+
+class Argon2HashTest {


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

@bdemers Please review and merge: #290

[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 requested review from fpapon and bdemers January 13, 2021 15:31

bmarwell force-pushed the SHIRO-290 branch from 1e15994 to 78d6065 Compare January 20, 2021 20:55

fpapon approved these changes Feb 23, 2021

View reviewed changes

bmarwell commented Feb 26, 2021

View reviewed changes

bmarwell force-pushed the SHIRO-290 branch from 78d6065 to 6f22048 Compare February 26, 2021 20:30

bdemers reviewed Apr 5, 2021

View reviewed changes

bmarwell added the shiro-2.0.0 label Apr 7, 2021

fpapon requested a review from bdemers May 8, 2021 05:28

bmarwell added 3 commits May 10, 2021 20:19

[SHIRO-290] doc fix, remove backwards compatibility for SimpleHash.

d84faf8

[SHIRO-290] add review suggestions:

29dcaad

- use default parameters from IETF draft. - use bits as unit size which is more common for hashes.

bmarwell force-pushed the SHIRO-290 branch from 388df53 to 29dcaad Compare May 10, 2021 18:19

[SHIRO-290] remove obsolete legacy-tests.

eb8662d

bdemers approved these changes May 14, 2021

View reviewed changes

bmarwell merged commit 2e9cddc into main May 19, 2021

bmarwell deleted the SHIRO-290 branch May 19, 2021 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SHIRO-290] Implement BCrypt and Argon2 #280

[SHIRO-290] Implement BCrypt and Argon2 #280

bmarwell commented Jan 13, 2021

fpapon left a comment

bdemers left a comment

bdemers Apr 5, 2021

bmarwell Apr 7, 2021

bdemers Apr 5, 2021

bmarwell Apr 7, 2021


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

		class Argon2HashTest {

[SHIRO-290] Implement BCrypt and Argon2 #280

[SHIRO-290] Implement BCrypt and Argon2 #280

Conversation

bmarwell commented Jan 13, 2021

fpapon left a comment

Choose a reason for hiding this comment

bdemers left a comment

Choose a reason for hiding this comment

bdemers Apr 5, 2021

Choose a reason for hiding this comment

bmarwell Apr 7, 2021

Choose a reason for hiding this comment

bdemers Apr 5, 2021

Choose a reason for hiding this comment

bmarwell Apr 7, 2021

Choose a reason for hiding this comment