bmarwell commented on code in PR #2475:
URL: https://github.com/apache/shiro/pull/2475#discussion_r2728762363
##########
crypto/hash/src/main/java/org/apache/shiro/crypto/hash/DefaultHashService.java:
##########
@@ -112,4 +115,13 @@ public String getDefaultAlgorithmName() {
return this.defaultAlgorithmName;
}
+ @Override
+ public Map<String, Object> getParameters() {
+ return Collections.unmodifiableMap(this.parameters);
+ }
+
+ @Override
+ public void setParameters(Map<String, Object> parameters) {
+ this.parameters = parameters;
Review Comment:
Wrap with Map.copyOf() as well, so it can't be modified from the outside,
causing race conditions etc
##########
crypto/hash/src/main/java/org/apache/shiro/crypto/hash/DefaultHashService.java:
##########
@@ -112,4 +115,13 @@ public String getDefaultAlgorithmName() {
return this.defaultAlgorithmName;
}
+ @Override
+ public Map<String, Object> getParameters() {
+ return Collections.unmodifiableMap(this.parameters);
Review Comment:
Map.copyOf() instead? Shorter and also unmodifiable and doesn't create deep
copies
##########
crypto/hash/src/main/java/org/apache/shiro/crypto/hash/HashService.java:
##########
@@ -80,4 +82,20 @@ public interface HashService {
* @since 2.0
*/
String getDefaultAlgorithmName();
+
+ /**
+ * Returns the various parameters that will be used when computing hashes.
+ *
+ * @return the various parameters that will be used when computing hashes.
+ */
+ default Map<String, Object> getParameters() {
Review Comment:
Should document what can go in there, when to use it and why we have it.
##########
crypto/hash/src/main/java/org/apache/shiro/crypto/hash/HashService.java:
##########
@@ -80,4 +82,20 @@ public interface HashService {
* @since 2.0
*/
String getDefaultAlgorithmName();
+
+ /**
+ * Returns the various parameters that will be used when computing hashes.
+ *
+ * @return the various parameters that will be used when computing hashes.
+ */
+ default Map<String, Object> getParameters() {
+ return Map.of();
+ }
+
+ /**
+ * Sets the various parameters that will be used when computing hashes.
+ *
+ * @param parameters the various parameters that will be used when
computing hashes.
+ */
+ default void setParameters(Map<String, Object> parameters) { }
Review Comment:
Also, define why this is empty by default and it can be a no-op
##########
core/src/main/java/org/apache/shiro/authc/credential/DefaultPasswordService.java:
##########
@@ -146,6 +157,20 @@ protected HashRequest createHashRequest(ByteSource
plaintext) {
.build();
}
+ protected HashRequest createHashRequest(ByteSource plaintext, Hash saved) {
+ Map<String, Object> parameters =
Stream.concat(getHashService().getParameters().entrySet().stream(),
+ Map.of(PARAMETER_ITERATIONS,
saved.getIterations()).entrySet().stream())
+ .collect(Collectors.toUnmodifiableMap(Map.Entry::getKey,
Map.Entry::getValue,
Review Comment:
I think there's a shortcut for that
##########
crypto/hash/src/main/java/org/apache/shiro/crypto/hash/HashService.java:
##########
@@ -80,4 +82,20 @@ public interface HashService {
* @since 2.0
*/
String getDefaultAlgorithmName();
+
+ /**
+ * Returns the various parameters that will be used when computing hashes.
+ *
+ * @return the various parameters that will be used when computing hashes.
+ */
+ default Map<String, Object> getParameters() {
+ return Map.of();
+ }
+
+ /**
+ * Sets the various parameters that will be used when computing hashes.
+ *
+ * @param parameters the various parameters that will be used when
computing hashes.
+ */
+ default void setParameters(Map<String, Object> parameters) { }
Review Comment:
See getter
##########
crypto/hash/src/main/java/org/apache/shiro/crypto/hash/ConfigurableHashService.java:
##########
@@ -33,5 +33,4 @@ public interface ConfigurableHashService extends HashService {
* compute secure hashes for passwords.
*/
void setDefaultAlgorithmName(String name);
Review Comment:
..?
##########
core/src/main/java/org/apache/shiro/authc/credential/DefaultPasswordService.java:
##########
@@ -146,6 +157,20 @@ protected HashRequest createHashRequest(ByteSource
plaintext) {
.build();
}
+ protected HashRequest createHashRequest(ByteSource plaintext, Hash saved) {
+ Map<String, Object> parameters =
Stream.concat(getHashService().getParameters().entrySet().stream(),
+ Map.of(PARAMETER_ITERATIONS,
saved.getIterations()).entrySet().stream())
+ .collect(Collectors.toUnmodifiableMap(Map.Entry::getKey,
Map.Entry::getValue,
+ (value1, value2) -> value2));
+ //keep everything from the saved hash except for the source:
+ return new HashRequest.Builder().setSource(plaintext)
+ //now use the existing saved data:
+ .setAlgorithmName(saved.getAlgorithmName())
+ .setSalt(saved.getSalt())
+ .withParameters(parameters)
Review Comment:
We really need to add javasoc that implementations can choose to ignore
those at will, eg salt in modern KDF (password hashes)
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
