bmarwell commented on a change in pull request #273:
URL: https://github.com/apache/shiro/pull/273#discussion_r554342422
##########
File path:
crypto/hash/src/main/java/org/apache/shiro/crypto/hash/ConfigurableHashService.java
##########
@@ -18,44 +18,20 @@
*/
package org.apache.shiro.crypto.hash;
-import org.apache.shiro.crypto.RandomNumberGenerator;
-import org.apache.shiro.lang.util.ByteSource;
-
/**
* A {@code HashService} that allows configuration of its strategy via
JavaBeans-compatible setter methods.
*
* @since 1.2
*/
public interface ConfigurableHashService extends HashService {
- /**
- * Sets the 'private' (internal) salt to be paired with a 'public' (random
or supplied) salt during hash computation.
- *
- * @param privateSalt the 'private' internal salt to be paired with a
'public' (random or supplied) salt during
- * hash computation.
- */
- void setPrivateSalt(ByteSource privateSalt);
-
- /**
- * Sets the number of hash iterations that will be performed during hash
computation.
- *
- * @param iterations the number of hash iterations that will be performed
during hash computation.
- */
- void setHashIterations(int iterations);
-
/**
* Sets the name of the {@link java.security.MessageDigest MessageDigest}
algorithm that will be used to compute
* hashes.
*
* @param name the name of the {@link java.security.MessageDigest
MessageDigest} algorithm that will be used to
* compute hashes.
*/
- void setHashAlgorithmName(String name);
+ void setDefaultAlgorithmName(String name);
Review comment:
Yes, the javadocs are the unchecked checkbox ;) Next commit is on the
way.
The default is being used when a `HashRequest` does not carry an algorithm
name. Its old behaviour/semantics was at least similar, but I was not looking
into making it a 100% compatible here.
##########
File path:
crypto/hash/src/main/java/org/apache/shiro/crypto/hash/DefaultHashService.java
##########
@@ -123,222 +71,52 @@ public DefaultHashService() {
* <p/>
* A salt will be generated and used to compute the hash. The salt is
generated as follows:
* <ol>
- * <li>Use the {@link #getRandomNumberGenerator() randomNumberGenerator}
to generate a new random number.</li>
- * <li>{@link #combine(ByteSource, ByteSource) combine} this random salt
with any configured
- * {@link #getPrivateSalt() privateSalt}
- * </li>
* <li>Use the combined value as the salt used during hash computation</li>
* </ol>
* </li>
* <li>
- * If the request salt is not null:
- * <p/>
- * This indicates that the hash computation is for comparison purposes (of
a
- * previously computed hash). The request salt will be {@link
#combine(ByteSource, ByteSource) combined} with any
- * configured {@link #getPrivateSalt() privateSalt} and used as the
complete salt during hash computation.
- * </li>
- * </ul>
- * <p/>
- * The returned {@code Hash}'s {@link Hash#getSalt() salt} property
- * will contain <em>only</em> the 'public' part of the salt and
<em>NOT</em> the privateSalt. See the class-level
- * JavaDoc explanation for more info.
*
* @param request the request to process
* @return the response containing the result of the hash computation, as
well as any hash salt used that should be
* exposed to the caller.
*/
+ @Override
public Hash computeHash(HashRequest request) {
if (request == null || request.getSource() == null ||
request.getSource().isEmpty()) {
return null;
}
String algorithmName = getAlgorithmName(request);
- ByteSource source = request.getSource();
- int iterations = getIterations(request);
-
- ByteSource publicSalt = getPublicSalt(request);
- ByteSource privateSalt = getPrivateSalt();
- ByteSource salt = combine(privateSalt, publicSalt);
-
- Hash computed = new SimpleHash(algorithmName, source, salt,
iterations);
-
- SimpleHash result = new SimpleHash(algorithmName);
- result.setBytes(computed.getBytes());
- result.setIterations(iterations);
- //Only expose the public salt - not the real/combined salt that might
have been used:
- result.setSalt(publicSalt);
-
- return result;
- }
-
- protected String getAlgorithmName(HashRequest request) {
- String name = request.getAlgorithmName();
- if (name == null) {
- name = getHashAlgorithmName();
- }
- return name;
- }
-
- protected int getIterations(HashRequest request) {
- int iterations = Math.max(0, request.getIterations());
- if (iterations < 1) {
- iterations = Math.max(1, getHashIterations());
- }
- return iterations;
- }
-
- /**
- * Returns the public salt that should be used to compute a hash based on
the specified request or
- * {@code null} if no public salt should be used.
- * <p/>
- * This implementation functions as follows:
- * <ol>
- * <li>If the request salt is not null and non-empty, this will be used,
return it.</li>
- * <li>If the request salt is null or empty:
- * <ol>
- * <li>If a private salt has been set <em>OR</em> {@link
#isGeneratePublicSalt()} is {@code true},
- * auto generate a random public salt via the configured
- * {@link #getRandomNumberGenerator() randomNumberGenerator}.</li>
- * <li>If a private salt has not been configured and {@link
#isGeneratePublicSalt()} is {@code false},
- * do nothing - return {@code null} to indicate a salt should not be used
during hash computation.</li>
- * </ol>
- * </li>
- * </ol>
- *
- * @param request request the request to process
- * @return the public salt that should be used to compute a hash based on
the specified request or
- * {@code null} if no public salt should be used.
- */
- protected ByteSource getPublicSalt(HashRequest request) {
-
- ByteSource publicSalt = request.getSalt();
-
- if (publicSalt != null && !publicSalt.isEmpty()) {
- //a public salt was explicitly requested to be used - go ahead and
use it:
- return publicSalt;
- }
-
- publicSalt = null;
- //check to see if we need to generate one:
- ByteSource privateSalt = getPrivateSalt();
- boolean privateSaltExists = privateSalt != null &&
!privateSalt.isEmpty();
+ Optional<HashSpi<? extends Hash>> kdfHash =
HashProvider.getByAlgorithmName(algorithmName);
+ if (kdfHash.isPresent()) {
+ HashSpi<?> hashSpi =
kdfHash.orElseThrow(NoSuchElementException::new);
- //If a private salt exists, we must generate a public salt to protect
the integrity of the private salt.
- //Or generate it if the instance is explicitly configured to do so:
- if (privateSaltExists || isGeneratePublicSalt()) {
- publicSalt = getRandomNumberGenerator().nextBytes();
+ return hashSpi.newHashFactory(random).generate(request);
}
- return publicSalt;
+ throw new UnsupportedOperationException("Cannot create a hash with the
given algorithm: " + algorithmName);
}
- /**
- * Combines the specified 'private' salt bytes with the specified
additional extra bytes to use as the
- * total salt during hash computation. {@code privateSaltBytes} will be
{@code null} }if no private salt has been
- * configured.
- *
- * @param privateSalt the (possibly {@code null}) 'private' salt to
combine with the specified extra bytes
- * @param publicSalt the extra bytes to use in addition to the given
private salt.
- * @return a combination of the specified private salt bytes and extra
bytes that will be used as the total
- * salt during hash computation.
- */
- protected ByteSource combine(ByteSource privateSalt, ByteSource
publicSalt) {
-
- byte[] privateSaltBytes = privateSalt != null ? privateSalt.getBytes()
: null;
- int privateSaltLength = privateSaltBytes != null ?
privateSaltBytes.length : 0;
-
- byte[] publicSaltBytes = publicSalt != null ? publicSalt.getBytes() :
null;
- int extraBytesLength = publicSaltBytes != null ?
publicSaltBytes.length : 0;
-
- int length = privateSaltLength + extraBytesLength;
- if (length <= 0) {
- return null;
- }
-
- byte[] combined = new byte[length];
-
- int i = 0;
- for (int j = 0; j < privateSaltLength; j++) {
- assert privateSaltBytes != null;
- combined[i++] = privateSaltBytes[j];
- }
- for (int j = 0; j < extraBytesLength; j++) {
- assert publicSaltBytes != null;
- combined[i++] = publicSaltBytes[j];
- }
-
- return ByteSource.Util.bytes(combined);
- }
-
- public void setHashAlgorithmName(String name) {
- this.algorithmName = name;
- }
-
- public String getHashAlgorithmName() {
- return this.algorithmName;
- }
-
- public void setPrivateSalt(ByteSource privateSalt) {
- this.privateSalt = privateSalt;
- }
-
- public ByteSource getPrivateSalt() {
- return this.privateSalt;
- }
-
- public void setHashIterations(int count) {
- this.iterations = count;
- }
-
- public int getHashIterations() {
- return this.iterations;
+ protected String getAlgorithmName(HashRequest request) {
+ return
request.getAlgorithmName().orElseGet(this::getDefaultAlgorithmName);
}
- public void setRandomNumberGenerator(RandomNumberGenerator rng) {
- this.rng = rng;
+ @Override
+ public void setDefaultAlgorithmName(String name) {
+ this.defaultAlgorithmName = name;
}
- public RandomNumberGenerator getRandomNumberGenerator() {
- return this.rng;
+ public String getDefaultAlgorithmName() {
+ return this.defaultAlgorithmName;
}
- /**
- * Returns {@code true} if a public salt should be randomly generated and
used to compute a hash if a
- * {@link HashRequest} does not specify a salt, {@code false} otherwise.
- * <p/>
- * The default value is {@code false} but should definitely be set to
{@code true} if the
- * {@code HashService} instance is being used for password hashing.
- * <p/>
- * <b>NOTE:</b> this property only has an effect if a {@link
#getPrivateSalt() privateSalt} is NOT configured. If a
- * private salt has been configured and a request does not provide a salt,
a random salt will always be generated
- * to protect the integrity of the private salt (without a public salt,
the private salt would be exposed as-is,
- * which is undesirable).
- *
- * @return {@code true} if a public salt should be randomly generated and
used to compute a hash if a
- * {@link HashRequest} does not specify a salt, {@code false}
otherwise.
- */
- public boolean isGeneratePublicSalt() {
- return generatePublicSalt;
+ public Random getRandom() {
+ return random;
}
- /**
- * Sets whether or not a public salt should be randomly generated and used
to compute a hash if a
- * {@link HashRequest} does not specify a salt.
- * <p/>
- * The default value is {@code false} but should definitely be set to
{@code true} if the
- * {@code HashService} instance is being used for password hashing.
- * <p/>
- * <b>NOTE:</b> this property only has an effect if a {@link
#getPrivateSalt() privateSalt} is NOT configured. If a
- * private salt has been configured and a request does not provide a salt,
a random salt will always be generated
- * to protect the integrity of the private salt (without a public salt,
the private salt would be exposed as-is,
- * which is undesirable).
- *
- * @param generatePublicSalt whether or not a public salt should be
randomly generated and used to compute a hash
- * if a {@link HashRequest} does not specify a
salt.
- */
- public void setGeneratePublicSalt(boolean generatePublicSalt) {
- this.generatePublicSalt = generatePublicSalt;
+ public void setRandom(Random random) {
+ this.random = random;
Review comment:
Not even for testing. Removing it until we need it.
##########
File path: crypto/hash/src/main/java/org/apache/shiro/crypto/hash/Md2Hash.java
##########
@@ -30,10 +30,13 @@
* techniques and how the overloaded constructors function.
*
* @since 0.9
+ * @deprecated and will throw exceptions since 2.0.0, to be removed in 2.1.0.
Review comment:
+1 for ripping out. We should nag the users in 1.7+ then.
And not just for MD2. For all iteration+salt+pepper based hashes, even if
there is no alternative in shiro 1. Seriously.
##########
File path: tools/hasher/pom.xml
##########
@@ -50,7 +50,11 @@
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
- <artifactId>slf4j-simple</artifactId>
+ <artifactId>slf4j-api</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
Review comment:
It is not so funny actually. See the chat in slack. To be able test
things written to `System.out`, we need `logback-classic`, as the
`simple-logger` cannot be intercepted.
Romain suggested to refactor the hasher utility a bit further, and I would
like to do that in another PR.
##########
File path:
crypto/hash/src/main/java/org/apache/shiro/crypto/hash/HashProvider.java
##########
@@ -0,0 +1,60 @@
+/*
+ * 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>
+ */
+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<? extends Hash>> getByAlgorithmName(String
algorithmName) {
+ requireNonNull(algorithmName, "algorithmName in
HashProvider.getByAlgorithmName");
+ ServiceLoader<HashSpi<? extends Hash>> hashSpis = load();
+
+ return StreamSupport.stream(hashSpis.spliterator(), false)
+ .filter(hashSpi ->
hashSpi.getImplementedAlgorithms().contains(algorithmName))
+ .findAny();
+ }
+
+ @SuppressWarnings("unchecked")
+ private static ServiceLoader<HashSpi<? extends Hash>> load() {
+ return (ServiceLoader<HashSpi<? extends Hash>>) (Object)
ServiceLoader.load(HashSpi.class);
Review comment:
What bugs me most is the ugly casting to object and back. But with what
I saw from your `jjwt` example, adding a parameter might help with that!
----------------------------------------------------------------
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:
[email protected]
