Copilot commented on code in PR #10968: URL: https://github.com/apache/gravitino/pull/10968#discussion_r3192837861
########## authenticators/authenticator-basic/build.gradle.kts: ########## @@ -0,0 +1,39 @@ +/* + * 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. + */ + +plugins { + `maven-publish` + id("java") + id("idea") +} + +dependencies { + implementation(libs.bcprov.jdk18on) + implementation(libs.commons.lang3) + implementation(libs.guava) Review Comment: PR description says it adds the `argon2-jvm` dependency, but this module actually depends on Bouncy Castle (`bcprov-jdk18on`). Please either update the PR description to reflect the actual dependency choice, or switch the implementation back to the intended `argon2-jvm` library so the change matches the stated design/testing expectations. ########## authenticators/authenticator-basic/src/main/java/org/apache/gravitino/auth/local/password/Argon2Parameters.java: ########## @@ -0,0 +1,35 @@ +/* + * 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.gravitino.auth.local.password; + +/** Default parameters for the built-in Argon2id password hasher. */ +public final class Argon2Parameters { + + public static final int DEFAULT_VERSION = + org.bouncycastle.crypto.params.Argon2Parameters.ARGON2_VERSION_13; + public static final int DEFAULT_TYPE = org.bouncycastle.crypto.params.Argon2Parameters.ARGON2_id; + public static final int DEFAULT_HASH_LENGTH = 32; + public static final int DEFAULT_MEMORY_KB = 1 << 16; + public static final int DEFAULT_ITERATIONS = 3; + public static final int DEFAULT_PARALLELISM = 1; Review Comment: The helper class name `Argon2Parameters` collides with Bouncy Castle's `org.bouncycastle.crypto.params.Argon2Parameters`, which forces repeated fully-qualified references and makes the code harder to read. Renaming this class (e.g., to `Argon2Defaults`/`Argon2idDefaults`) would remove the collision and simplify imports/usages. ########## authenticators/authenticator-basic/src/main/java/org/apache/gravitino/auth/local/password/Argon2idPasswordHasher.java: ########## @@ -0,0 +1,178 @@ +/* + * 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.gravitino.auth.local.password; + +import com.google.common.base.Preconditions; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.security.SecureRandom; +import java.util.Base64; +import org.apache.commons.lang3.StringUtils; +import org.bouncycastle.crypto.generators.Argon2BytesGenerator; +import org.bouncycastle.util.Arrays; + +/** Argon2id-based password hasher. */ +public class Argon2idPasswordHasher implements PasswordHasher { + + private static final String PHC_PREFIX = "$argon2id$"; + private static final SecureRandom SECURE_RANDOM = new SecureRandom(); + + @Override + public String hash(String plainPassword) { + Preconditions.checkArgument( + StringUtils.isNotBlank(plainPassword), "Plain password must not be blank"); + + byte[] salt = new byte[Argon2Parameters.DEFAULT_SALT_LENGTH]; + SECURE_RANDOM.nextBytes(salt); + byte[] passwordBytes = plainPassword.getBytes(StandardCharsets.UTF_8); + byte[] hash = new byte[Argon2Parameters.DEFAULT_HASH_LENGTH]; + try { + generateHash( + passwordBytes, + salt, + Argon2Parameters.DEFAULT_ITERATIONS, + Argon2Parameters.DEFAULT_MEMORY_KB, + Argon2Parameters.DEFAULT_PARALLELISM, + Argon2Parameters.DEFAULT_VERSION, + hash); + return toPhcString( + salt, + hash, + Argon2Parameters.DEFAULT_ITERATIONS, + Argon2Parameters.DEFAULT_MEMORY_KB, + Argon2Parameters.DEFAULT_PARALLELISM, + Argon2Parameters.DEFAULT_VERSION); + } finally { + Arrays.clear(passwordBytes); + Arrays.clear(salt); + Arrays.clear(hash); + } + } + + @Override + public boolean verify(String plainPassword, String hashedPassword) { + Preconditions.checkArgument( + StringUtils.isNotBlank(plainPassword), "Plain password must not be blank"); + Preconditions.checkArgument( + StringUtils.isNotBlank(hashedPassword), "Hashed password must not be blank"); + + ParsedHash parsedHash = parse(hashedPassword); + byte[] passwordBytes = plainPassword.getBytes(StandardCharsets.UTF_8); + byte[] actualHash = new byte[parsedHash.hash.length]; + try { + generateHash( + passwordBytes, + parsedHash.salt, + parsedHash.iterations, + parsedHash.memoryKb, + parsedHash.parallelism, + parsedHash.version, + actualHash); Review Comment: `verify()` trusts cost parameters parsed from the stored PHC string (`memoryKb`, `iterations`, `parallelism`) and feeds them directly into Argon2. If a corrupted/attacker-controlled hash is ever processed, this can force extremely expensive hashing (CPU/RAM) and become a DoS vector. Consider enforcing reasonable upper bounds (or only accepting the known built-in parameters) before calling `generateHash()`. ########## authenticators/authenticator-basic/src/test/java/org/apache/gravitino/auth/local/password/TestArgon2idPasswordHasher.java: ########## @@ -0,0 +1,48 @@ +/* + * 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.gravitino.auth.local.password; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class TestArgon2idPasswordHasher { + + private final PasswordHasher passwordHasher = new Argon2idPasswordHasher(); + + @Test + public void testHashProducesArgon2idPhcString() { + String hashedPassword = passwordHasher.hash("test-password"); + + Assertions.assertTrue(hashedPassword.startsWith("$argon2id$")); + } Review Comment: The PHC-format test only checks the `$argon2id$` prefix, so regressions in the required PHC structure/parameters (e.g., missing/incorrect `v=`, `m=`, `t=`, `p=` or salt/hash components) may go unnoticed. Strengthen the assertion to validate the full PHC string structure and the expected default cost parameters. ########## authenticators/authenticator-basic/src/main/java/org/apache/gravitino/auth/local/password/Argon2idPasswordHasher.java: ########## @@ -0,0 +1,178 @@ +/* + * 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.gravitino.auth.local.password; + +import com.google.common.base.Preconditions; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.security.SecureRandom; +import java.util.Base64; +import org.apache.commons.lang3.StringUtils; +import org.bouncycastle.crypto.generators.Argon2BytesGenerator; +import org.bouncycastle.util.Arrays; + +/** Argon2id-based password hasher. */ +public class Argon2idPasswordHasher implements PasswordHasher { + + private static final String PHC_PREFIX = "$argon2id$"; + private static final SecureRandom SECURE_RANDOM = new SecureRandom(); + + @Override + public String hash(String plainPassword) { + Preconditions.checkArgument( + StringUtils.isNotBlank(plainPassword), "Plain password must not be blank"); + + byte[] salt = new byte[Argon2Parameters.DEFAULT_SALT_LENGTH]; + SECURE_RANDOM.nextBytes(salt); + byte[] passwordBytes = plainPassword.getBytes(StandardCharsets.UTF_8); + byte[] hash = new byte[Argon2Parameters.DEFAULT_HASH_LENGTH]; + try { + generateHash( + passwordBytes, + salt, + Argon2Parameters.DEFAULT_ITERATIONS, + Argon2Parameters.DEFAULT_MEMORY_KB, + Argon2Parameters.DEFAULT_PARALLELISM, + Argon2Parameters.DEFAULT_VERSION, + hash); + return toPhcString( + salt, + hash, + Argon2Parameters.DEFAULT_ITERATIONS, + Argon2Parameters.DEFAULT_MEMORY_KB, + Argon2Parameters.DEFAULT_PARALLELISM, + Argon2Parameters.DEFAULT_VERSION); + } finally { + Arrays.clear(passwordBytes); + Arrays.clear(salt); + Arrays.clear(hash); + } + } + + @Override + public boolean verify(String plainPassword, String hashedPassword) { + Preconditions.checkArgument( + StringUtils.isNotBlank(plainPassword), "Plain password must not be blank"); + Preconditions.checkArgument( + StringUtils.isNotBlank(hashedPassword), "Hashed password must not be blank"); + + ParsedHash parsedHash = parse(hashedPassword); + byte[] passwordBytes = plainPassword.getBytes(StandardCharsets.UTF_8); + byte[] actualHash = new byte[parsedHash.hash.length]; + try { + generateHash( + passwordBytes, + parsedHash.salt, + parsedHash.iterations, + parsedHash.memoryKb, + parsedHash.parallelism, + parsedHash.version, + actualHash); + return MessageDigest.isEqual(actualHash, parsedHash.hash); + } finally { + Arrays.clear(passwordBytes); + Arrays.clear(actualHash); + } + } + + private static void generateHash( + byte[] passwordBytes, + byte[] salt, + int iterations, + int memoryKb, + int parallelism, + int version, + byte[] output) { + org.bouncycastle.crypto.params.Argon2Parameters parameters = + new org.bouncycastle.crypto.params.Argon2Parameters.Builder(Argon2Parameters.DEFAULT_TYPE) + .withVersion(version) + .withIterations(iterations) + .withMemoryAsKB(memoryKb) + .withParallelism(parallelism) + .withSalt(salt) + .build(); + Argon2BytesGenerator generator = new Argon2BytesGenerator(); + generator.init(parameters); + generator.generateBytes(passwordBytes, output); + } + + private static String toPhcString( + byte[] salt, byte[] hash, int iterations, int memoryKb, int parallelism, int version) { + Base64.Encoder encoder = Base64.getEncoder().withoutPadding(); + return PHC_PREFIX + + "v=" + + version + + "$m=" + + memoryKb + + ",t=" + + iterations + + ",p=" + + parallelism + + "$" + + encoder.encodeToString(salt) + + "$" + + encoder.encodeToString(hash); + } + + private static ParsedHash parse(String hashedPassword) { + Preconditions.checkArgument( + hashedPassword.startsWith(PHC_PREFIX), "Invalid Argon2id hash format"); + String[] parts = hashedPassword.split("\\$"); + Preconditions.checkArgument(parts.length == 6, "Invalid Argon2id hash format"); + Preconditions.checkArgument("argon2id".equals(parts[1]), "Invalid Argon2id hash format"); + Preconditions.checkArgument(parts[2].startsWith("v="), "Invalid Argon2id hash format"); + Preconditions.checkArgument(parts[3].startsWith("m="), "Invalid Argon2id hash format"); + + String[] parameterParts = parts[3].split(","); + Preconditions.checkArgument(parameterParts.length == 3, "Invalid Argon2id hash format"); + return new ParsedHash( + Integer.parseInt(parts[2].substring(2)), + Integer.parseInt(parameterParts[0].substring(2)), + Integer.parseInt(parameterParts[1].substring(2)), + Integer.parseInt(parameterParts[2].substring(2)), + decodeBase64(parts[4]), + decodeBase64(parts[5])); Review Comment: `parse()` validates some prefixes but then assumes the `m=...,t=...,p=...` segments are present and numeric. For malformed hashes, this can currently throw `NumberFormatException`/`ArrayIndexOutOfBoundsException` instead of consistently failing with the intended "Invalid Argon2id hash format" message. Add explicit prefix checks for `t=` and `p=` (and handle parse errors) so invalid inputs are rejected deterministically. ########## authenticators/authenticator-basic/src/main/java/org/apache/gravitino/auth/local/password/Argon2idPasswordHasher.java: ########## @@ -0,0 +1,178 @@ +/* + * 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.gravitino.auth.local.password; + +import com.google.common.base.Preconditions; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.security.SecureRandom; +import java.util.Base64; +import org.apache.commons.lang3.StringUtils; +import org.bouncycastle.crypto.generators.Argon2BytesGenerator; +import org.bouncycastle.util.Arrays; + +/** Argon2id-based password hasher. */ +public class Argon2idPasswordHasher implements PasswordHasher { + + private static final String PHC_PREFIX = "$argon2id$"; + private static final SecureRandom SECURE_RANDOM = new SecureRandom(); + + @Override + public String hash(String plainPassword) { + Preconditions.checkArgument( + StringUtils.isNotBlank(plainPassword), "Plain password must not be blank"); + + byte[] salt = new byte[Argon2Parameters.DEFAULT_SALT_LENGTH]; + SECURE_RANDOM.nextBytes(salt); + byte[] passwordBytes = plainPassword.getBytes(StandardCharsets.UTF_8); + byte[] hash = new byte[Argon2Parameters.DEFAULT_HASH_LENGTH]; + try { + generateHash( + passwordBytes, + salt, + Argon2Parameters.DEFAULT_ITERATIONS, + Argon2Parameters.DEFAULT_MEMORY_KB, + Argon2Parameters.DEFAULT_PARALLELISM, + Argon2Parameters.DEFAULT_VERSION, + hash); + return toPhcString( + salt, + hash, + Argon2Parameters.DEFAULT_ITERATIONS, + Argon2Parameters.DEFAULT_MEMORY_KB, + Argon2Parameters.DEFAULT_PARALLELISM, + Argon2Parameters.DEFAULT_VERSION); + } finally { + Arrays.clear(passwordBytes); + Arrays.clear(salt); + Arrays.clear(hash); + } + } + + @Override + public boolean verify(String plainPassword, String hashedPassword) { + Preconditions.checkArgument( + StringUtils.isNotBlank(plainPassword), "Plain password must not be blank"); + Preconditions.checkArgument( + StringUtils.isNotBlank(hashedPassword), "Hashed password must not be blank"); + + ParsedHash parsedHash = parse(hashedPassword); + byte[] passwordBytes = plainPassword.getBytes(StandardCharsets.UTF_8); + byte[] actualHash = new byte[parsedHash.hash.length]; + try { + generateHash( + passwordBytes, + parsedHash.salt, + parsedHash.iterations, + parsedHash.memoryKb, + parsedHash.parallelism, + parsedHash.version, + actualHash); + return MessageDigest.isEqual(actualHash, parsedHash.hash); + } finally { + Arrays.clear(passwordBytes); + Arrays.clear(actualHash); + } + } + + private static void generateHash( + byte[] passwordBytes, + byte[] salt, + int iterations, + int memoryKb, + int parallelism, + int version, + byte[] output) { + org.bouncycastle.crypto.params.Argon2Parameters parameters = + new org.bouncycastle.crypto.params.Argon2Parameters.Builder(Argon2Parameters.DEFAULT_TYPE) + .withVersion(version) + .withIterations(iterations) + .withMemoryAsKB(memoryKb) + .withParallelism(parallelism) + .withSalt(salt) + .build(); + Argon2BytesGenerator generator = new Argon2BytesGenerator(); + generator.init(parameters); + generator.generateBytes(passwordBytes, output); + } + + private static String toPhcString( + byte[] salt, byte[] hash, int iterations, int memoryKb, int parallelism, int version) { + Base64.Encoder encoder = Base64.getEncoder().withoutPadding(); + return PHC_PREFIX + + "v=" + + version + + "$m=" + + memoryKb + + ",t=" + + iterations + + ",p=" + + parallelism + + "$" + + encoder.encodeToString(salt) + + "$" + + encoder.encodeToString(hash); + } + + private static ParsedHash parse(String hashedPassword) { + Preconditions.checkArgument( + hashedPassword.startsWith(PHC_PREFIX), "Invalid Argon2id hash format"); + String[] parts = hashedPassword.split("\\$"); + Preconditions.checkArgument(parts.length == 6, "Invalid Argon2id hash format"); + Preconditions.checkArgument("argon2id".equals(parts[1]), "Invalid Argon2id hash format"); + Preconditions.checkArgument(parts[2].startsWith("v="), "Invalid Argon2id hash format"); + Preconditions.checkArgument(parts[3].startsWith("m="), "Invalid Argon2id hash format"); + + String[] parameterParts = parts[3].split(","); + Preconditions.checkArgument(parameterParts.length == 3, "Invalid Argon2id hash format"); + return new ParsedHash( + Integer.parseInt(parts[2].substring(2)), + Integer.parseInt(parameterParts[0].substring(2)), + Integer.parseInt(parameterParts[1].substring(2)), + Integer.parseInt(parameterParts[2].substring(2)), + decodeBase64(parts[4]), + decodeBase64(parts[5])); + } + + private static byte[] decodeBase64(String value) { + int remainder = value.length() % 4; Review Comment: `decodeBase64()` currently pads any non-multiple-of-4 length, including the invalid case where `value.length() % 4 == 1`. In Base64 this remainder is not representable and should be rejected as an invalid hash rather than silently padded/decoded. Add a precondition to fail fast when `remainder == 1` (and ideally propagate a consistent "Invalid Argon2id hash format" error). -- 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]
