honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

SQL: Be more careful with break and eof (elastic/x-pack-elasticsearch#4092) 

The SQL CLI was being a bit cavalier about `null`, `ctrl-c`, and
`ctrl-d` while reading passwords to the point where it'd halt with
an exception if the user hit `ctrl-d` while typing a password. This
changes it so that the CLI will instead shut down if the user
`ctrl-c`s or `ctrl-d`s while on the password prompt with an
ENOPERM error code.

This also fixes a packaging test failure I caused by a copy and paste
error where the CLI was always enforcing things as though it was reading
a password all the time. This error was causing packaging test failures.

Original commit: elastic/x-pack-elasticsearch@a882c50fc7
This commit is contained in:
Nik Everett 2018-03-13 09:00:53 -04:00 committed by GitHub
parent 7f7ac08447
commit 47a2f63d5e
6 changed files with 167 additions and 58 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
plugin/sql/sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/CliTerminal.java
View File

@ -6,6 +6,7 @@
package org.elasticsearch.xpack.sql.cli;
import java.io.IOException;
import org.elasticsearch.cli.UserException;
/**
* Represents a terminal endpoint
@ -49,11 +50,20 @@ public interface CliTerminal extends AutoCloseable {
/**
* Prompts the user to enter the password and returns it.
*
* @throws UserException if there is a problem reading the password,
* for instance, the user {@code ctrl-c}s while we're waiting
* or they send an EOF
* @return the password the user typed, never null
*/
String readPassword(String prompt);
String readPassword(String prompt) throws UserException;
/**
* Reads the line from the terminal.
*
* @return {@code null} if the user closes the terminal while we're
* waiting for the line, {@code ""} if the use {@code ctrl-c}s while
* we're waiting, the line they typed otherwise
*/
String readLine(String prompt);

17
plugin/sql/sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/ConnectionBuilder.java
View File

@ -37,8 +37,8 @@ public class ConnectionBuilder {
*
* @param connectionStringArg the connection string to connect to
* @param keystoreLocation the location of the keystore to configure. If null then use the system keystore.
* @throws UserException if there is a problem with the information provided by the user
*/
@SuppressForbidden(reason = "cli application")
public ConnectionConfiguration buildConnection(String connectionStringArg, String keystoreLocation) throws UserException {
final URI uri;
final String connectionString;
@ -65,13 +65,9 @@ public class ConnectionBuilder {
if (false == "https".equals(uri.getScheme())) {
throw new UserException(ExitCodes.USAGE, "keystore file specified without https");
}
Path p = Paths.get(keystoreLocation);
Path p = getKeystorePath(keystoreLocation);
checkIfExists("keystore file", p);
String keystorePassword = cliTerminal.readPassword("keystore password: ");
if (keystorePassword == null) {
throw new FatalCliException("readPassword shouldn't ever return null but ["
+ cliTerminal + "#readPassword] did!");
}
/*
* Set both the keystore and truststore settings which is required
@ -91,10 +87,6 @@ public class ConnectionBuilder {
if (user != null) {
if (password == null) {
password = cliTerminal.readPassword("password: ");
if (password == null) {
throw new FatalCliException("readPassword shouldn't ever return null but ["
+ cliTerminal + "#readPassword] did!");
}
}
properties.setProperty(ConnectionConfiguration.AUTH_USER, user);
properties.setProperty(ConnectionConfiguration.AUTH_PASS, password);
@ -103,6 +95,11 @@ public class ConnectionBuilder {
return newConnectionConfiguration(uri, connectionString, properties);
}
@SuppressForbidden(reason = "cli application shouldn't depend on ES")
private Path getKeystorePath(String keystoreLocation) {
return Paths.get(keystoreLocation);
}
protected ConnectionConfiguration newConnectionConfiguration(URI uri, String connectionString, Properties properties) {
return new ConnectionConfiguration(uri, connectionString, properties);
}

33
plugin/sql/sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/JLineTerminal.java
View File

@ -5,6 +5,8 @@
*/
package org.elasticsearch.xpack.sql.cli;
import org.elasticsearch.cli.ExitCodes;
import org.elasticsearch.cli.UserException;
import org.jline.reader.EndOfFileException;
import org.jline.reader.LineReader;
import org.jline.reader.LineReaderBuilder;
@ -103,27 +105,30 @@ public class JLineTerminal implements CliTerminal {
}
@Override
public String readPassword(String prompt) {
String line = readLine(prompt, (char) 0);
if (line == null) {
throw new FatalCliException("Error reading password, terminal is closed");
public String readPassword(String prompt) throws UserException {
try {
String password = reader.readLine(prompt, (char) 0);
if (password == null) {
/*
* The docs say this can't return null but they lie. Lies, I tell you!
* This returns null when you pipe an empty file into the process.
* Since that is a lot like an EOF we throw the same exception.
*/
throw new UserException(ExitCodes.NOPERM, "password required");
}
return password;
} catch (UserInterruptException ex) {
throw new UserException(ExitCodes.NOPERM, "password required");
} catch (EndOfFileException ex) {
throw new UserException(ExitCodes.NOPERM, "password required");
}
return line;
}
@Override
public String readLine(String prompt) {
String attributedString = new AttributedString(prompt, DEFAULT.foreground(YELLOW)).toAnsi(terminal);
return readLine(attributedString, null);
}
private String readLine(String prompt, Character mask) {
try {
String line = reader.readLine(prompt, null, mask, null);
if (line == null) {
throw new FatalCliException("Error reading password, terminal is closed");
}
return line;
return reader.readLine(attributedString);
} catch (UserInterruptException ex) {
return "";
} catch (EndOfFileException ex) {

28
plugin/sql/sql-cli/src/test/java/org/elasticsearch/xpack/sql/cli/ConnectionBuilderTests.java
View File

@ -61,7 +61,7 @@ public class ConnectionBuilderTests extends ESTestCase {
verifyNoMoreInteractions(testTerminal);
}
public void testUserInteractiveConnection() throws Exception {
public void testAskUserForPassword() throws Exception {
CliTerminal testTerminal = mock(CliTerminal.class);
when(testTerminal.readPassword("password: ")).thenReturn("password");
ConnectionBuilder connectionBuilder = new ConnectionBuilder(testTerminal);
@ -74,7 +74,7 @@ public class ConnectionBuilderTests extends ESTestCase {
verifyNoMoreInteractions(testTerminal);
}
public void testKeystoreAndUserInteractiveConnection() throws Exception {
public void testAskUserForPasswordAndKeystorePassword() throws Exception {
CliTerminal testTerminal = mock(CliTerminal.class);
when(testTerminal.readPassword("keystore password: ")).thenReturn("keystore password");
when(testTerminal.readPassword("password: ")).thenReturn("password");
@ -105,5 +105,29 @@ public class ConnectionBuilderTests extends ESTestCase {
verifyNoMoreInteractions(testTerminal);
}
public void testUserGaveUpOnPassword() throws Exception {
CliTerminal testTerminal = mock(CliTerminal.class);
UserException ue = new UserException(randomInt(), randomAlphaOfLength(5));
when(testTerminal.readPassword("password: ")).thenThrow(ue);
ConnectionBuilder connectionBuilder = new ConnectionBuilder(testTerminal);
UserException actual = expectThrows(UserException.class, () ->
connectionBuilder.buildConnection("http://user@foobar:9242/", null));
assertSame(actual, ue);
}
public void testUserGaveUpOnKeystorePassword() throws Exception {
CliTerminal testTerminal = mock(CliTerminal.class);
UserException ue = new UserException(randomInt(), randomAlphaOfLength(5));
when(testTerminal.readPassword("keystore password: ")).thenThrow(ue);
when(testTerminal.readPassword("password: ")).thenReturn("password");
ConnectionBuilder connectionBuilder = new ConnectionBuilder(testTerminal) {
@Override
protected void checkIfExists(String name, Path p) {
// Stubbed so we don't need permission to read the file
}
};
UserException actual = expectThrows(UserException.class, () ->
connectionBuilder.buildConnection("https://user@foobar:9242/", "keystore_location"));
assertSame(actual, ue);
}
}

121
plugin/sql/sql-cli/src/test/java/org/elasticsearch/xpack/sql/cli/JLineTerminalTests.java
View File

@ -5,50 +5,109 @@
*/
package org.elasticsearch.xpack.sql.cli;
import org.elasticsearch.cli.ExitCodes;
import org.elasticsearch.cli.UserException;
import org.elasticsearch.test.ESTestCase;
import org.jline.reader.EndOfFileException;
import org.jline.reader.LineReader;
import org.jline.reader.UserInterruptException;
import org.jline.terminal.Terminal;
import java.io.IOException;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.eq;
import static org.mockito.Mockito.isNull;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import static org.mockito.Mockito.verify;
public class JLineTerminalTests extends ESTestCase {
public void testReadPasswordSuccess() throws IOException {
Terminal wrapped = mock(Terminal.class);
LineReader reader = mock(LineReader.class);
try (JLineTerminal terminal = new JLineTerminal(wrapped, reader, randomBoolean())) {
String prompt = randomAlphaOfLength(5);
String expected = randomAlphaOfLength(5);
when(reader.readLine(prompt, null, (char) 0, null)).thenReturn(expected);
String actual = terminal.readPassword(prompt);
assertEquals(expected, actual);
verify(reader).readLine(prompt, null, (char) 0, null);
}
}
public void testReadPasswordEof() throws IOException {
Terminal wrapped = mock(Terminal.class);
LineReader reader = mock(LineReader.class);
try (JLineTerminal terminal = new JLineTerminal(wrapped, reader, randomBoolean())) {
String prompt = randomAlphaOfLength(5);
Exception e = expectThrows(FatalCliException.class, () -> terminal.readPassword(prompt));
assertEquals("Error reading password, terminal is closed", e.getMessage());
verify(reader).readLine(prompt, null, (char) 0, null);
}
}
private final Terminal wrapped = mock(Terminal.class);
private final LineReader reader = mock(LineReader.class);
public void testDisableMatchBracket() throws IOException {
Terminal wrapped = mock(Terminal.class);
LineReader reader = mock(LineReader.class);
new JLineTerminal(wrapped, reader, false).close();
verify(reader).setVariable(LineReader.BLINK_MATCHING_PAREN, 0L);
}
public void testReadPasswordSuccess() throws IOException, UserException {
String prompt = randomAlphaOfLength(5);
String expected = randomAlphaOfLength(5);
when(reader.readLine(prompt, (char) 0)).thenReturn(expected);
try (JLineTerminal terminal = new JLineTerminal(wrapped, reader, randomBoolean())) {
String actual = terminal.readPassword(prompt);
assertEquals(expected, actual);
}
}
public void testReadPasswordNull() throws IOException {
String prompt = randomAlphaOfLength(5);
/*
* jLine documents readLine as not being able to return null but
* LineReader totally does sometimes. We should interpret that as
* "user hit ctrl-d on the password prompt" because that is similar
* to the situations where this comes up.
*/
when(reader.readLine(prompt, (char) 0)).thenReturn(null);
try (JLineTerminal terminal = new JLineTerminal(wrapped, reader, randomBoolean())) {
UserException e = expectThrows(UserException.class, () -> terminal.readPassword(prompt));
assertEquals(ExitCodes.NOPERM, e.exitCode);
assertEquals("password required", e.getMessage());
}
}
public void testReadPasswordInterrupted() throws IOException {
String prompt = randomAlphaOfLength(5);
when(reader.readLine(prompt, (char) 0)).thenThrow(new UserInterruptException(""));
try (JLineTerminal terminal = new JLineTerminal(wrapped, reader, randomBoolean())) {
UserException e = expectThrows(UserException.class, () -> terminal.readPassword(prompt));
assertEquals(ExitCodes.NOPERM, e.exitCode);
assertEquals("password required", e.getMessage());
}
}
public void testReadPasswordClosed() throws IOException {
String prompt = randomAlphaOfLength(5);
when(reader.readLine(prompt, (char) 0)).thenThrow(new EndOfFileException(""));
try (JLineTerminal terminal = new JLineTerminal(wrapped, reader, randomBoolean())) {
UserException e = expectThrows(UserException.class, () -> terminal.readPassword(prompt));
assertEquals(ExitCodes.NOPERM, e.exitCode);
assertEquals("password required", e.getMessage());
}
}
public void testReadLineSuccess() throws IOException {
String prompt = randomAlphaOfLength(5);
String expected = randomAlphaOfLength(5);
when(reader.readLine(any(String.class))).thenReturn(expected);
try (JLineTerminal terminal = new JLineTerminal(wrapped, reader, randomBoolean())) {
String actual = terminal.readLine(prompt);
assertEquals(expected, actual);
}
}
public void testReadLineInterrupted() throws IOException {
String prompt = randomAlphaOfLength(5);
when(reader.readLine(any(String.class))).thenThrow(new UserInterruptException(""));
try (JLineTerminal terminal = new JLineTerminal(wrapped, reader, randomBoolean())) {
assertEquals("", terminal.readLine(prompt));
}
}
public void testReadLineClosed() throws IOException {
String prompt = randomAlphaOfLength(5);
when(reader.readLine(any(String.class))).thenThrow(new EndOfFileException(""));
try (JLineTerminal terminal = new JLineTerminal(wrapped, reader, randomBoolean())) {
assertEquals(null, terminal.readLine(prompt));
}
}
}

14
qa/vagrant/src/test/resources/packaging/tests/bootstrap_password.bash
View File

@ -150,6 +150,20 @@ SQL
}
}
@test "[$GROUP] test sql-cli when user refuses password" {
# Run with empty stdin
run $ESHOME/bin/x-pack/sql-cli --debug "http://elastic@127.0.0.1:9200" <<SQL
SQL
[ "$status" -eq 77 ] || { #NOPERM
echo "SQL cli failed:\n$output"
false
}
[[ "$output" == *"password required"* ]] || {
echo "Failed to find author [password required] in error:$output"
false
}
}
@test "[$GROUP] stop Elasticsearch" {
stop_elasticsearch_service
}