From 0f2389735fd32e0bbc93ecde5d8c814b275b21b5 Mon Sep 17 00:00:00 2001 From: Dave Marion Date: Thu, 8 Jun 2023 16:10:20 -0400 Subject: [PATCH] Modify Shell to authenticate user on call to config (#3440) Closes #3433 --------- Co-authored-by: Christopher Tubbs --- .../java/org/apache/accumulo/shell/Shell.java | 66 +++++----- .../apache/accumulo/shell/ShellOptionsJC.java | 6 +- .../shell/commands/FateCommandTest.java | 9 +- .../test/shell/ShellAuthenticatorIT.java | 117 ++++++++++++++++++ 4 files changed, 166 insertions(+), 32 deletions(-) create mode 100644 test/src/main/java/org/apache/accumulo/test/shell/ShellAuthenticatorIT.java diff --git a/shell/src/main/java/org/apache/accumulo/shell/Shell.java b/shell/src/main/java/org/apache/accumulo/shell/Shell.java index 211f2b031bf..05dedd2d1bf 100644 --- a/shell/src/main/java/org/apache/accumulo/shell/Shell.java +++ b/shell/src/main/java/org/apache/accumulo/shell/Shell.java @@ -189,6 +189,7 @@ */ @AutoService(KeywordExecutable.class) public class Shell extends ShellOptions implements KeywordExecutable { + public static final Logger log = LoggerFactory.getLogger(Shell.class); private static final Logger audit = LoggerFactory.getLogger(Shell.class.getName() + ".audit"); @@ -257,6 +258,35 @@ public Shell(LineReader reader) { this.writer = terminal.writer(); } + // this is visible only for FateCommandTest, otherwise, should be private or inline + protected boolean authenticateUser(AccumuloClient client, AuthenticationToken token) + throws AccumuloException, AccumuloSecurityException { + return client.securityOperations().authenticateUser(client.whoami(), token); + } + + private AuthenticationToken getAuthenticationToken(String principal, String authenticationString, + String passwordPrompt) { + AuthenticationToken token = null; + if (authenticationString == null + && clientProperties.containsKey(ClientProperty.AUTH_TOKEN.getKey()) + && principal.equals(ClientProperty.AUTH_PRINCIPAL.getValue(clientProperties))) { + token = ClientProperty.getAuthenticationToken(clientProperties); + } + if (token == null) { + // Read password if the user explicitly asked for it, or didn't specify anything at all + if (PasswordConverter.STDIN.equals(authenticationString) || authenticationString == null) { + authenticationString = reader.readLine(passwordPrompt, '*'); + } + if (authenticationString == null) { + // User cancel, e.g. Ctrl-D pressed + throw new ParameterException("No password or token option supplied"); + } else { + token = new PasswordToken(authenticationString); + } + } + return token; + } + /** * Configures the shell using the provided options. Not for client use. * @@ -331,27 +361,13 @@ public boolean config(String... args) throws IOException { exitCode = 1; return false; } - String password = options.getPassword(); - AuthenticationToken token = null; - if (password == null && clientProperties.containsKey(ClientProperty.AUTH_TOKEN.getKey()) - && principal.equals(ClientProperty.AUTH_PRINCIPAL.getValue(clientProperties))) { - token = ClientProperty.getAuthenticationToken(clientProperties); - } - if (token == null) { - // Read password if the user explicitly asked for it, or didn't specify anything at all - if (PasswordConverter.STDIN.equals(password) || password == null) { - password = reader.readLine("Password: ", '*'); - } - if (password == null) { - // User cancel, e.g. Ctrl-D pressed - throw new ParameterException("No password or token option supplied"); - } else { - token = new PasswordToken(password); - } - } + String authenticationString = options.getPassword(); + final AuthenticationToken token = + getAuthenticationToken(principal, authenticationString, "Password: "); try { this.setTableName(""); accumuloClient = Accumulo.newClient().from(clientProperties).as(principal, token).build(); + authenticateUser(accumuloClient, token); context = (ClientContext) accumuloClient; } catch (Exception e) { printException(e); @@ -732,16 +748,10 @@ public void execCommand(String input, boolean ignoreAuthTimeout, boolean echoPro writer.println("Shell has been idle for too long. Please re-authenticate."); boolean authFailed = true; do { - String pwd = readMaskedLine( - "Enter current password for '" + accumuloClient.whoami() + "': ", '*'); - if (pwd == null) { - writer.println(); - return; - } // user canceled - + final AuthenticationToken authToken = getAuthenticationToken(accumuloClient.whoami(), + null, "Enter current password for '" + accumuloClient.whoami() + "': "); try { - authFailed = !accumuloClient.securityOperations() - .authenticateUser(accumuloClient.whoami(), new PasswordToken(pwd)); + authFailed = !authenticateUser(accumuloClient, authToken); } catch (Exception e) { ++exitCode; printException(e); @@ -1188,7 +1198,7 @@ public void updateUser(String principal, AuthenticationToken token) throws AccumuloException, AccumuloSecurityException { var newClient = Accumulo.newClient().from(clientProperties).as(principal, token).build(); try { - newClient.securityOperations().authenticateUser(principal, token); + authenticateUser(newClient, token); } catch (AccumuloSecurityException e) { // new client can't authenticate; close and discard newClient.close(); diff --git a/shell/src/main/java/org/apache/accumulo/shell/ShellOptionsJC.java b/shell/src/main/java/org/apache/accumulo/shell/ShellOptionsJC.java index 827e5637068..cb006fd438a 100644 --- a/shell/src/main/java/org/apache/accumulo/shell/ShellOptionsJC.java +++ b/shell/src/main/java/org/apache/accumulo/shell/ShellOptionsJC.java @@ -50,7 +50,7 @@ public class ShellOptionsJC { + " 'file:', 'env:', or stdin)", converter = ClientOpts.PasswordConverter.class) - private String password; + private String authenticationString; @DynamicParameter(names = {"-l"}, description = "command line properties in the format key=value. Reuse -l for each property") @@ -145,7 +145,7 @@ public String getUsername() throws Exception { } public String getPassword() { - return password; + return authenticationString; } public boolean isTabCompletionDisabled() { @@ -239,7 +239,7 @@ public Properties getClientProperties() { return props; } - static class PositiveInteger implements IParameterValidator { + public static class PositiveInteger implements IParameterValidator { @Override public void validate(String name, String value) throws ParameterException { int n = -1; diff --git a/shell/src/test/java/org/apache/accumulo/shell/commands/FateCommandTest.java b/shell/src/test/java/org/apache/accumulo/shell/commands/FateCommandTest.java index 191ffb926fc..0deacfb47a8 100644 --- a/shell/src/test/java/org/apache/accumulo/shell/commands/FateCommandTest.java +++ b/shell/src/test/java/org/apache/accumulo/shell/commands/FateCommandTest.java @@ -36,6 +36,8 @@ import java.nio.file.Files; import java.util.List; +import org.apache.accumulo.core.client.AccumuloClient; +import org.apache.accumulo.core.client.security.tokens.AuthenticationToken; import org.apache.accumulo.core.clientImpl.ClientContext; import org.apache.accumulo.core.fate.AdminUtil; import org.apache.accumulo.core.fate.ReadOnlyRepo; @@ -320,7 +322,12 @@ private Shell createShell(TestOutputStream output) throws IOException { Terminal terminal = new DumbTerminal(new FileInputStream(FileDescriptor.in), output); terminal.setSize(new Size(80, 24)); LineReader reader = LineReaderBuilder.builder().terminal(terminal).build(); - Shell shell = new Shell(reader); + Shell shell = new Shell(reader) { + @Override + protected boolean authenticateUser(AccumuloClient client, AuthenticationToken token) { + return true; + } + }; shell.setLogErrorsToConsole(); return shell; } diff --git a/test/src/main/java/org/apache/accumulo/test/shell/ShellAuthenticatorIT.java b/test/src/main/java/org/apache/accumulo/test/shell/ShellAuthenticatorIT.java new file mode 100644 index 00000000000..8dd631adea0 --- /dev/null +++ b/test/src/main/java/org/apache/accumulo/test/shell/ShellAuthenticatorIT.java @@ -0,0 +1,117 @@ +/* + * 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 + * + * https://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.accumulo.test.shell; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.util.TimeZone; + +import org.apache.accumulo.harness.SharedMiniClusterBase; +import org.apache.accumulo.shell.Shell; +import org.apache.accumulo.test.shell.ShellIT.StringInputStream; +import org.apache.accumulo.test.shell.ShellIT.TestOutputStream; +import org.jline.reader.LineReader; +import org.jline.reader.LineReaderBuilder; +import org.jline.terminal.Size; +import org.jline.terminal.Terminal; +import org.jline.terminal.impl.DumbTerminal; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class ShellAuthenticatorIT extends SharedMiniClusterBase { + + @BeforeAll + public static void setup() throws Exception { + SharedMiniClusterBase.startMiniCluster(); + } + + @AfterAll + public static void teardown() { + SharedMiniClusterBase.stopMiniCluster(); + } + + private StringInputStream input; + private TestOutputStream output; + private Shell shell; + public LineReader reader; + public Terminal terminal; + + @BeforeEach + public void setupShell() throws IOException { + TimeZone.setDefault(TimeZone.getTimeZone("UTC")); + output = new TestOutputStream(); + input = new StringInputStream(); + terminal = new DumbTerminal(input, output); + terminal.setSize(new Size(80, 24)); + reader = LineReaderBuilder.builder().terminal(terminal).build(); + } + + @AfterEach + public void tearDownShell() { + if (shell != null) { + shell.shutdown(); + } + } + + @Test + public void testClientPropertiesFile() throws IOException { + shell = new Shell(reader); + shell.setLogErrorsToConsole(); + assertTrue(shell.config("--config-file", getCluster().getClientPropsPath())); + } + + @Test + public void testClientProperties() throws IOException { + shell = new Shell(reader); + shell.setLogErrorsToConsole(); + assertTrue(shell.config("-u", getAdminPrincipal(), "-p", getRootPassword(), "-zi", + getCluster().getInstanceName(), "-zh", getCluster().getZooKeepers())); + } + + @Test + public void testClientPropertiesBadPassword() throws IOException { + shell = new Shell(reader); + shell.setLogErrorsToConsole(); + assertFalse(shell.config("-u", getAdminPrincipal(), "-p", "BADPW", "-zi", + getCluster().getInstanceName(), "-zh", getCluster().getZooKeepers())); + } + + @Test + public void testAuthTimeoutPropertiesFile() throws IOException, InterruptedException { + TimeZone.setDefault(TimeZone.getTimeZone("UTC")); + output = new TestOutputStream(); + input = new StringInputStream(); + terminal = new DumbTerminal(input, output); + terminal.setSize(new Size(80, 24)); + reader = LineReaderBuilder.builder().terminal(terminal).build(); + shell = new Shell(reader); + shell.setLogErrorsToConsole(); + assertTrue( + shell.config("--auth-timeout", "1", "--config-file", getCluster().getClientPropsPath())); + Thread.sleep(90000); + shell.execCommand("whoami", false, false); + assertTrue(output.get().contains("root")); + } + +}