You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@shiro.apache.org by GitBox <gi...@apache.org> on 2021/01/13 15:09:02 UTC

[GitHub] [shiro] bmarwell opened a new pull request #280: [SHIRO-290] Implement BCrypt and Argon2

bmarwell opened a new pull request #280:
URL: https://github.com/apache/shiro/pull/280


   superseeds https://github.com/apache/shiro/pull/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:
   
    - [X] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/SHIRO) 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.
    - [X] Each commit in the pull request should have a meaningful subject line and body.
    - [X] 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.
    - [X] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
    - [X] 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.
    - [X] 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](https://github.com/apache/shiro/blob/master/CONTRIBUTING.md)
   
   If your pull request is about ~20 lines of code you don't need to sign an [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) 
   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](http://www.apache.org/licenses/LICENSE-2.0)
   you have to acknowledge this by using the following check-box.
   
    - [X] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
    - [X] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shiro] bdemers commented on a change in pull request #280: [SHIRO-290] Implement BCrypt and Argon2

Posted by GitBox <gi...@apache.org>.
bdemers commented on a change in pull request #280:
URL: https://github.com/apache/shiro/pull/280#discussion_r607203282



##########
File path: crypto/hash/src/main/java/org/apache/shiro/crypto/hash/HashProvider.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.shiro.crypto.hash;
+
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.stream.StreamSupport;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Hashes used by the Shiro2CryptFormat class.
+ *
+ * <p>Instead of maintaining them as an {@code Enum}, ServiceLoaders would provide a pluggable alternative.</p>
+ *
+ * @since 2.0
+ */
+public final class HashProvider {
+
+    private HashProvider() {
+        // utility class
+    }
+
+    /**
+     * Find a KDF implementation by searching the algorithms.
+     *
+     * @param algorithmName the algorithmName to match. This is case-sensitive.
+     * @return an instance of {@link HashProvider} if found, otherwise {@link Optional#empty()}.
+     * @throws NullPointerException if the given parameter algorithmName is {@code null}.
+     */
+    public static Optional<HashSpi> getByAlgorithmName(String algorithmName) {
+        requireNonNull(algorithmName, "algorithmName in HashProvider.getByAlgorithmName");
+        ServiceLoader<HashSpi> hashSpis = load();
+
+        return StreamSupport.stream(hashSpis.spliterator(), false)
+                .filter(hashSpi -> hashSpi.getImplementedAlgorithms().contains(algorithmName))
+                .findAny();
+    }
+
+    @SuppressWarnings("unchecked")
+    private static ServiceLoader<HashSpi> load() {
+        return ServiceLoader.load(HashSpi.class);

Review comment:
       There might be some potential classloading issues here.  We had to work around in JJWT: 
   * https://github.com/jwtk/jjwt/pull/574
   * https://github.com/jwtk/jjwt/issues/568
   

##########
File path: crypto/hash/src/main/java/org/apache/shiro/crypto/hash/format/Shiro2CryptFormat.java
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.shiro.crypto.hash.format;
+
+import org.apache.shiro.crypto.hash.AbstractCryptHash;
+import org.apache.shiro.crypto.hash.Hash;
+import org.apache.shiro.crypto.hash.HashProvider;
+import org.apache.shiro.crypto.hash.HashSpi;
+import org.apache.shiro.crypto.hash.SimpleHash;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * The {@code Shiro1CryptFormat} is a fully reversible

Review comment:
       Should this be `Shiro2CryptFormat` ?

##########
File path: crypto/support/hashes/argon2/src/test/groovy/org/apache/shiro/crypto/support/hashes/argon2/Argon2HashTest.groovy
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.shiro.crypto.support.hashes.argon2
+
+import org.apache.shiro.crypto.hash.format.Shiro1CryptFormat
+import org.apache.shiro.crypto.hash.format.Shiro2CryptFormat
+import org.apache.shiro.lang.util.SimpleByteSource
+import org.bouncycastle.crypto.params.Argon2Parameters
+import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.function.Executable
+
+import static org.junit.jupiter.api.Assertions.*
+
+class Argon2HashTest {

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

##########
File path: crypto/hash/src/main/java/org/apache/shiro/crypto/hash/format/Shiro2CryptFormat.java
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.shiro.crypto.hash.format;
+
+import org.apache.shiro.crypto.hash.AbstractCryptHash;
+import org.apache.shiro.crypto.hash.Hash;
+import org.apache.shiro.crypto.hash.HashProvider;
+import org.apache.shiro.crypto.hash.HashSpi;
+import org.apache.shiro.crypto.hash.SimpleHash;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * The {@code Shiro1CryptFormat} is a fully reversible
+ * <a href="http://packages.python.org/passlib/modular_crypt_format.html">Modular Crypt Format</a> (MCF). It is based
+ * on the posix format for storing KDF-hashed passwords in {@code /etc/shadow} files on linux and unix-alike systems.
+ * <h2>Format</h2>
+ * <p>Hash instances formatted with this implementation will result in a String with the following dollar-sign ($)
+ * delimited format:</p>
+ * <pre>
+ * <b>$</b>mcfFormatId<b>$</b>algorithmName<b>$</b>algorithm-specific-data.
+ * </pre>
+ * <p>Each token is defined as follows:</p>
+ * <table>
+ *     <tr>
+ *         <th>Position</th>
+ *         <th>Token</th>
+ *         <th>Description</th>
+ *         <th>Required?</th>
+ *     </tr>
+ *     <tr>
+ *         <td>1</td>
+ *         <td>{@code mcfFormatId}</td>
+ *         <td>The Modular Crypt Format identifier for this implementation, equal to <b>{@code shiro2}</b>.
+ *             ( This implies that all {@code shiro2} MCF-formatted strings will always begin with the prefix
+ *             {@code $shiro2$} ).</td>
+ *         <td>true</td>
+ *     </tr>
+ *     <tr>
+ *         <td>2</td>
+ *         <td>{@code algorithmName}</td>
+ *         <td>The name of the hash algorithm used to perform the hash. Either a hash class exists, or
+ *         otherwise a {@link UnsupportedOperationException} will be thrown.
+ *         <td>true</td>
+ *     </tr>
+ *     <tr>
+ *         <td>3</td>
+ *         <td>{@code algorithm-specific-data}</td>
+ *         <td>In contrast to the previous {@code shiro1} format, the shiro2 format does not make any assumptions
+ *         about how an algorithm stores its data. Therefore, everything beyond the first token is handled over
+ *         to the Hash implementation.</td>
+ *     </tr>
+ * </table>
+ *
+ * @see ModularCryptFormat
+ * @see ParsableHashFormat
+ * @since 2.0
+ */
+public class Shiro2CryptFormat implements ModularCryptFormat, ParsableHashFormat {
+
+    /**
+     * Identifier for the shiro2 crypt format.
+     */
+    public static final String ID = "shiro2";
+    /**
+     * Enclosed identifier of the shiro2 crypt format.
+     */
+    public static final String MCF_PREFIX = TOKEN_DELIMITER + ID + TOKEN_DELIMITER;
+
+    public Shiro2CryptFormat() {
+    }
+
+    @Override
+    public String getId() {
+        return ID;
+    }
+
+    /**
+     * Converts a Hash-extending class to a string understood by the hash class. Usually this string will follow
+     * posix standards for passwords stored in {@code /etc/passwd}.
+     *
+     * <p>This method should only delegate to the corresponding formatter and prepend {@code $shiro2$}.</p>
+     *
+     * @param hash the hash instance to format into a String.
+     * @return a string representing the hash.
+     */
+    @Override
+    public String format(final Hash hash) {
+        requireNonNull(hash, "hash in Shiro2CryptFormat.format(Hash hash)");
+
+        // backwards compatibility until Shiro 2.1.0.
+        if (hash instanceof SimpleHash) {
+            return new Shiro1CryptFormat().format(hash);
+        }
+
+        if (!(hash instanceof AbstractCryptHash)) {
+            throw new UnsupportedOperationException("Shiro2CryptFormat can only format classes extending AbstractCryptHash.");
+        }
+
+        AbstractCryptHash cryptHash = (AbstractCryptHash) hash;
+        return TOKEN_DELIMITER + ID + cryptHash.formatToCryptString();
+    }
+
+    @Override
+    public Hash parse(final String formatted) {
+        requireNonNull(formatted, "formatted in Shiro2CryptFormat.parse(String formatted)");
+
+        // backwards compatibility until Shiro 2.1.0.

Review comment:
       IMHO we could make a clean break for 2.x (or we could keep the compat and release milestones for 2.x 🤔 )




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shiro] bmarwell edited a comment on pull request #280: [SHIRO-290] Implement BCrypt and Argon2

Posted by GitBox <gi...@apache.org>.
bmarwell edited a comment on pull request #280:
URL: https://github.com/apache/shiro/pull/280#issuecomment-759525942


   TODO: add test
   https://github.com/apache/shiro/blob/3008045470f302530d3db61eb075e5539132fae6/config/ogdl/src/test/groovy/org/apache/shiro/config/ogdl/ReflectionBuilderTest.groovy#L98


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shiro] bmarwell commented on a change in pull request #280: [SHIRO-290] Implement BCrypt and Argon2

Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #280:
URL: https://github.com/apache/shiro/pull/280#discussion_r583430952



##########
File path: crypto/support/hashes/argon2/src/test/groovy/org/apache/shiro/crypto/support/hashes/argon2/Argon2HashTest.groovy
##########
@@ -0,0 +1,88 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.shiro.crypto.support.hashes.argon2
+
+import org.apache.shiro.crypto.hash.format.Shiro1CryptFormat
+import org.apache.shiro.crypto.hash.format.Shiro2CryptFormat
+import org.apache.shiro.lang.util.SimpleByteSource
+import org.bouncycastle.crypto.params.Argon2Parameters
+import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.function.Executable
+
+import static org.junit.jupiter.api.Assertions.*
+
+class Argon2HashTest {
+
+    private static final TEST_PASSWORD = "secret#shiro,password;Jo8opech";
+    private static final TEST_PASSWORD_BS = new SimpleByteSource(TEST_PASSWORD)
+
+    @Test
+    void testArgon2Hash() {
+        // given
+        def shiro2Format = '$shiro2$argon2id$v=19$m=4096,t=3,p=4$MTIzNDU2Nzg5MDEyMzQ1Ng$bjcHqfb0LPHyS13eVaNcBga9LF12I3k34H5ULt2gyoI'
+        def expectedPassword = new SimpleByteSource('secret#shiro,password;Jo8opech')

Review comment:
       replace with constant

##########
File path: crypto/support/hashes/argon2/src/test/groovy/org/apache/shiro/crypto/support/hashes/argon2/Argon2HashTest.groovy
##########
@@ -0,0 +1,88 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.shiro.crypto.support.hashes.argon2
+
+import org.apache.shiro.crypto.hash.format.Shiro1CryptFormat
+import org.apache.shiro.crypto.hash.format.Shiro2CryptFormat
+import org.apache.shiro.lang.util.SimpleByteSource
+import org.bouncycastle.crypto.params.Argon2Parameters
+import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.function.Executable
+
+import static org.junit.jupiter.api.Assertions.*
+
+class Argon2HashTest {
+
+    private static final TEST_PASSWORD = "secret#shiro,password;Jo8opech";
+    private static final TEST_PASSWORD_BS = new SimpleByteSource(TEST_PASSWORD)
+
+    @Test
+    void testArgon2Hash() {
+        // given
+        def shiro2Format = '$shiro2$argon2id$v=19$m=4096,t=3,p=4$MTIzNDU2Nzg5MDEyMzQ1Ng$bjcHqfb0LPHyS13eVaNcBga9LF12I3k34H5ULt2gyoI'
+        def expectedPassword = new SimpleByteSource('secret#shiro,password;Jo8opech')
+
+        // when
+        def hash = new Shiro2CryptFormat().parse(shiro2Format) as Argon2Hash;
+        System.out.println("Hash: " + hash)

Review comment:
       println

##########
File path: crypto/support/hashes/argon2/src/test/groovy/org/apache/shiro/crypto/support/hashes/argon2/Argon2HashTest.groovy
##########
@@ -0,0 +1,88 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.shiro.crypto.support.hashes.argon2
+
+import org.apache.shiro.crypto.hash.format.Shiro1CryptFormat
+import org.apache.shiro.crypto.hash.format.Shiro2CryptFormat
+import org.apache.shiro.lang.util.SimpleByteSource
+import org.bouncycastle.crypto.params.Argon2Parameters
+import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.function.Executable
+
+import static org.junit.jupiter.api.Assertions.*
+
+class Argon2HashTest {
+
+    private static final TEST_PASSWORD = "secret#shiro,password;Jo8opech";
+    private static final TEST_PASSWORD_BS = new SimpleByteSource(TEST_PASSWORD)
+
+    @Test
+    void testArgon2Hash() {
+        // given
+        def shiro2Format = '$shiro2$argon2id$v=19$m=4096,t=3,p=4$MTIzNDU2Nzg5MDEyMzQ1Ng$bjcHqfb0LPHyS13eVaNcBga9LF12I3k34H5ULt2gyoI'
+        def expectedPassword = new SimpleByteSource('secret#shiro,password;Jo8opech')
+
+        // when
+        def hash = new Shiro2CryptFormat().parse(shiro2Format) as Argon2Hash;
+        System.out.println("Hash: " + hash)
+        def matchesPassword = hash.matchesPassword expectedPassword;
+
+        // then
+        assertEquals Argon2Parameters.ARGON2_VERSION_13, hash.argonVersion
+        assertEquals 3, hash.iterations
+        assertEquals 4096, hash.memoryKiB
+        assertEquals 4, hash.parallelism
+        assertTrue matchesPassword
+    }
+
+    @Test
+    void testArgon2HashShiro1Format() {
+        // given
+        def shiro1Format = '$shiro1$argon2id$v=19$t=2,m=131072,p=4$7858qTJTreh61AzFV2XMOw==$lLzl2VNNbyFcuJo0Hp7JQpguKCDoQwxo91AWobcHzeo='
+

Review comment:
       document why this tests exist




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shiro] bmarwell commented on a change in pull request #280: [SHIRO-290] Implement BCrypt and Argon2

Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #280:
URL: https://github.com/apache/shiro/pull/280#discussion_r609047335



##########
File path: crypto/hash/src/main/java/org/apache/shiro/crypto/hash/HashProvider.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.shiro.crypto.hash;
+
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.stream.StreamSupport;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Hashes used by the Shiro2CryptFormat class.
+ *
+ * <p>Instead of maintaining them as an {@code Enum}, ServiceLoaders would provide a pluggable alternative.</p>
+ *
+ * @since 2.0
+ */
+public final class HashProvider {
+
+    private HashProvider() {
+        // utility class
+    }
+
+    /**
+     * Find a KDF implementation by searching the algorithms.
+     *
+     * @param algorithmName the algorithmName to match. This is case-sensitive.
+     * @return an instance of {@link HashProvider} if found, otherwise {@link Optional#empty()}.
+     * @throws NullPointerException if the given parameter algorithmName is {@code null}.
+     */
+    public static Optional<HashSpi> getByAlgorithmName(String algorithmName) {
+        requireNonNull(algorithmName, "algorithmName in HashProvider.getByAlgorithmName");
+        ServiceLoader<HashSpi> hashSpis = load();
+
+        return StreamSupport.stream(hashSpis.spliterator(), false)
+                .filter(hashSpi -> hashSpi.getImplementedAlgorithms().contains(algorithmName))
+                .findAny();
+    }
+
+    @SuppressWarnings("unchecked")
+    private static ServiceLoader<HashSpi> load() {
+        return ServiceLoader.load(HashSpi.class);

Review comment:
       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.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shiro] bmarwell merged pull request #280: [SHIRO-290] Implement BCrypt and Argon2

Posted by GitBox <gi...@apache.org>.
bmarwell merged pull request #280:
URL: https://github.com/apache/shiro/pull/280


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shiro] bmarwell removed a comment on pull request #280: [SHIRO-290] Implement BCrypt and Argon2

Posted by GitBox <gi...@apache.org>.
bmarwell removed a comment on pull request #280:
URL: https://github.com/apache/shiro/pull/280#issuecomment-759525942


   TODO: add test
   https://github.com/apache/shiro/blob/3008045470f302530d3db61eb075e5539132fae6/config/ogdl/src/test/groovy/org/apache/shiro/config/ogdl/ReflectionBuilderTest.groovy#L98


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shiro] bmarwell commented on a change in pull request #280: [SHIRO-290] Implement BCrypt and Argon2

Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #280:
URL: https://github.com/apache/shiro/pull/280#discussion_r609046448



##########
File path: crypto/support/hashes/argon2/src/test/groovy/org/apache/shiro/crypto/support/hashes/argon2/Argon2HashTest.groovy
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.shiro.crypto.support.hashes.argon2
+
+import org.apache.shiro.crypto.hash.format.Shiro1CryptFormat
+import org.apache.shiro.crypto.hash.format.Shiro2CryptFormat
+import org.apache.shiro.lang.util.SimpleByteSource
+import org.bouncycastle.crypto.params.Argon2Parameters
+import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.function.Executable
+
+import static org.junit.jupiter.api.Assertions.*
+
+class Argon2HashTest {

Review comment:
       @bdemers Please review and merge: https://github.com/apache/shiro/pull/290




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shiro] bmarwell commented on pull request #280: [SHIRO-290] Implement BCrypt and Argon2

Posted by GitBox <gi...@apache.org>.
bmarwell commented on pull request #280:
URL: https://github.com/apache/shiro/pull/280#issuecomment-759525942


   TBD: Can someone please write a test if we can actually define the default parameters in an ini file?
   Something like `defaultHashFactory.parameters['Argon2.iterations'] = 10`?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org