You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/03/17 12:45:38 UTC
[GitHub] [cassandra] adelapena commented on a change in pull request #1478: CASSANDRA-17334 Pre hashed passwords in CQL for trunk
adelapena commented on a change in pull request #1478:
URL: https://github.com/apache/cassandra/pull/1478#discussion_r829046847
##########
File path: src/java/org/apache/cassandra/auth/RoleOptions.java
##########
@@ -89,6 +90,15 @@ public boolean isEmpty()
return Optional.fromNullable((String)options.get(IRoleManager.Option.PASSWORD));
}
+ /**
+ * Return the string value of the hashed password option
+ * @return hashed password option value
+ */
+ public Optional<String> getHashedPassword()
+ {
+ return Optional.fromNullable((String)options.get(IRoleManager.Option.HASHED_PASSWORD));
Review comment:
Nit: missed whitespace
```suggestion
return Optional.fromNullable((String) options.get(IRoleManager.Option.HASHED_PASSWORD));
```
##########
File path: src/java/org/apache/cassandra/auth/CassandraRoleManager.java
##########
@@ -172,14 +172,14 @@ protected final void loadRoleStatement()
public void createRole(AuthenticatedUser performer, RoleResource role, RoleOptions options)
throws RequestValidationException, RequestExecutionException
{
- String insertCql = options.getPassword().isPresent()
+ String insertCql = options.getPassword().isPresent() || options.getHashedPassword().isPresent()
? String.format("INSERT INTO %s.%s (role, is_superuser, can_login, salted_hash) VALUES ('%s', %s, %s, '%s')",
SchemaConstants.AUTH_KEYSPACE_NAME,
AuthKeyspace.ROLES,
escape(role.getRoleName()),
options.getSuperuser().or(false),
options.getLogin().or(false),
- escape(hashpw(options.getPassword().get())))
+ options.getHashedPassword().or(() -> escape(hashpw(options.getPassword().get()))))
Review comment:
Guava's `Optional#or` has been marked as beta for a while. I think that `RoleOptions` uses Guava's `Optional` because it was created before Java 8, now we can easily replace it with `java.util.Optional`. I gave it a try [here](https://github.com/adelapena/cassandra/commit/f6bf02fd02aa58ae99f15b4865a212204d2ee907) and the change seems pretty straightforward. I know this is not strictly related to the patch but since we are here...
##########
File path: src/java/org/apache/cassandra/cql3/PasswordObfuscator.java
##########
@@ -62,10 +62,15 @@ public static String obfuscate(String query, RoleOptions opts)
return query;
Optional<String> pass = opts.getPassword();
+ if (!pass.isPresent() || pass.get().isEmpty())
+ pass = opts.getHashedPassword();
if (!pass.isPresent() || pass.get().isEmpty())
return query;
- // match new line, case insensitive (?si), and PASSWORD_TOKEN up to the actual password greedy. Group that and replace the password
- return query.replaceAll("((?si)"+ PASSWORD_TOKEN + ".+?)" + pass.get(), "$1" + PasswordObfuscator.OBFUSCATION_TOKEN);
+ // Regular expression:
+ // - Match new line and case insensitive (?si), and PASSWORD_TOKEN with greedy mode up to the start of the actual password and group it.
+ // - Quote the password between \Q and \E so any potential special characters are ignored
+ // - Replace the match with the grouped data + the obfuscated token
+ return query.replaceAll("((?si)"+ PASSWORD_TOKEN + ".+?)\\Q" + pass.get() + "\\E", "$1" + PasswordObfuscator.OBFUSCATION_TOKEN);
Review comment:
Nit: I would split the arguments for better readability, feel free to ignore if you don't agree:
```suggestion
return query.replaceAll("((?si)"+ PASSWORD_TOKEN + ".+?)\\Q" + pass.get() + "\\E",
"$1" + PasswordObfuscator.OBFUSCATION_TOKEN);
```
##########
File path: test/unit/org/apache/cassandra/tools/ToolRunner.java
##########
@@ -119,11 +119,13 @@ public void checkPermission(Permission perm, Object context)
private final InputStream input;
private final T out;
+ private boolean autoCloseOut;
Review comment:
Nit
```suggestion
private final boolean autoCloseOut;
```
##########
File path: test/unit/org/apache/cassandra/tools/ToolRunner.java
##########
@@ -119,11 +119,13 @@ public void checkPermission(Permission perm, Object context)
private final InputStream input;
private final T out;
+ private boolean autoCloseOut;
- private StreamGobbler(InputStream input, T out)
+ private StreamGobbler(InputStream input, T out, boolean autoCloseOut)
Review comment:
Is this one of the two bugs mentioned on the ticket?
##########
File path: src/java/org/apache/cassandra/tools/HashPassword.java
##########
@@ -0,0 +1,195 @@
+/*
+ * 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.cassandra.tools;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+
+import org.apache.commons.cli.CommandLine;
+import org.apache.commons.cli.CommandLineParser;
+import org.apache.commons.cli.GnuParser;
+import org.apache.commons.cli.HelpFormatter;
+import org.apache.commons.cli.Option;
+import org.apache.commons.cli.OptionGroup;
+import org.apache.commons.cli.Options;
+import org.apache.commons.cli.ParseException;
+import org.apache.commons.lang3.StringUtils;
+import org.mindrot.jbcrypt.BCrypt;
+
+public class HashPassword
+{
+ private static final String LOGROUNDS_OPTION = "logrounds";
+ private static final String HELP_OPTION = "help";
+ private static final String ENV_VAR = "environment-var";
+ private static final String PLAIN = "plain";
+ private static final String INPUT = "input";
+
+ private static final int LOGROUNDS_DEFAULT = 10;
+
+ public static void main(String[] args)
+ {
+ try
+ {
+ Options options = getOptions();
+ CommandLine cmd = parseCommandLine(args, options);
+
+ String password = null;
+ if (cmd.hasOption(ENV_VAR))
+ {
+ password = System.getenv(cmd.getOptionValue(ENV_VAR));
+ if (password == null)
+ {
+ System.err.println(String.format("Environment variable '%s' is undefined.", cmd.getOptionValue(ENV_VAR)));
+ System.exit(1);
+ }
+ }
+ else if (cmd.hasOption(PLAIN))
+ {
+ password = cmd.getOptionValue(PLAIN);
+ }
+ else if (cmd.hasOption(INPUT))
+ {
+ String input = cmd.getOptionValue(INPUT);
+ byte[] fileInput = null;
+ if ("-".equals(input))
+ {
+ ByteArrayOutputStream os = new ByteArrayOutputStream();
+ int rd;
+ while ((rd = System.in.read()) != -1)
+ os.write(rd);
+ fileInput = os.toByteArray();
+ }
+ else
+ {
+ try
+ {
+ Path file = Paths.get(input);
+ fileInput = Files.readAllBytes(file);
+ }
+ catch (IOException e)
+ {
+ System.err.println(String.format("Failed to read from '%s': %s",
+ input, e));
+ System.exit(1);
+ }
+ }
+ password = new String(fileInput, StandardCharsets.UTF_8);
+ }
+ else
+ {
+ System.err.println(String.format("One of the options --%s, --%s or --%s must be used.",
+ ENV_VAR, PLAIN, INPUT));
+ printUsage(options);
+ System.exit(1);
+ }
+
+ if (password.chars().anyMatch(i -> i < 32))
+ System.err.println("WARNING: The provided plain text password contains non-printable characters (ASCII<32).");
+
+ if (password.length() < 4)
Review comment:
I would place that `4` in a class constant. We don't check password lengths when creating a user/role with a plain password, do we?
##########
File path: test/unit/org/apache/cassandra/tools/ToolRunner.java
##########
@@ -136,6 +138,8 @@ public void run()
int read = input.read(buffer);
if (read == -1)
{
+ if(autoCloseOut)
Review comment:
Nit: missed whitespace
```suggestion
if (autoCloseOut)
```
##########
File path: src/java/org/apache/cassandra/tools/HashPassword.java
##########
@@ -0,0 +1,195 @@
+/*
+ * 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.cassandra.tools;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+
+import org.apache.commons.cli.CommandLine;
+import org.apache.commons.cli.CommandLineParser;
+import org.apache.commons.cli.GnuParser;
+import org.apache.commons.cli.HelpFormatter;
+import org.apache.commons.cli.Option;
+import org.apache.commons.cli.OptionGroup;
+import org.apache.commons.cli.Options;
+import org.apache.commons.cli.ParseException;
+import org.apache.commons.lang3.StringUtils;
+import org.mindrot.jbcrypt.BCrypt;
+
+public class HashPassword
+{
+ private static final String LOGROUNDS_OPTION = "logrounds";
+ private static final String HELP_OPTION = "help";
+ private static final String ENV_VAR = "environment-var";
+ private static final String PLAIN = "plain";
+ private static final String INPUT = "input";
+
+ private static final int LOGROUNDS_DEFAULT = 10;
+
+ public static void main(String[] args)
+ {
+ try
+ {
+ Options options = getOptions();
+ CommandLine cmd = parseCommandLine(args, options);
+
+ String password = null;
+ if (cmd.hasOption(ENV_VAR))
+ {
+ password = System.getenv(cmd.getOptionValue(ENV_VAR));
+ if (password == null)
+ {
+ System.err.println(String.format("Environment variable '%s' is undefined.", cmd.getOptionValue(ENV_VAR)));
+ System.exit(1);
+ }
+ }
+ else if (cmd.hasOption(PLAIN))
+ {
+ password = cmd.getOptionValue(PLAIN);
+ }
+ else if (cmd.hasOption(INPUT))
+ {
+ String input = cmd.getOptionValue(INPUT);
+ byte[] fileInput = null;
+ if ("-".equals(input))
+ {
+ ByteArrayOutputStream os = new ByteArrayOutputStream();
+ int rd;
+ while ((rd = System.in.read()) != -1)
+ os.write(rd);
+ fileInput = os.toByteArray();
+ }
+ else
+ {
+ try
+ {
+ Path file = Paths.get(input);
+ fileInput = Files.readAllBytes(file);
+ }
+ catch (IOException e)
+ {
+ System.err.println(String.format("Failed to read from '%s': %s",
Review comment:
No need to break the line. Also, we could use `System.err.printf("Failed to read from '%s': %s%n", input, e);`.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org