From c53e1f4b1c119547f99435f966cff47160ffe182 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Thu, 1 Feb 2018 17:20:44 -0500 Subject: [PATCH] SQL: More tests and docs for permissions (elastic/x-pack-elasticsearch#3679) Adds tests for what works and what doesn't when we're missing some of SQL's required permissions. Adds required permissions to the documentation of each SQL access method. relates elastic/x-pack-elasticsearch#3552 Original commit: elastic/x-pack-elasticsearch@971dabb3b4536b0ccad916975788723c8b0315a0 --- docs/en/sql/endpoints/cli.asciidoc | 15 +++ docs/en/sql/endpoints/jdbc.asciidoc | 16 +++ docs/en/sql/endpoints/rest.asciidoc | 14 +++ docs/en/sql/endpoints/translate.asciidoc | 15 +++ qa/sql/security/roles.yml | 42 ++++++-- .../xpack/qa/sql/security/CliSecurityIT.java | 24 ++++- .../xpack/qa/sql/security/JdbcSecurityIT.java | 39 +++++-- .../qa/sql/security/RestSqlSecurityIT.java | 24 ++++- .../qa/sql/security/SqlSecurityTestCase.java | 23 +++- .../xpack/qa/sql/cli/RemoteCli.java | 101 +++++++++++------- 10 files changed, 255 insertions(+), 58 deletions(-) diff --git a/docs/en/sql/endpoints/cli.asciidoc b/docs/en/sql/endpoints/cli.asciidoc index 6f9785e254e..ae3e459aeef 100644 --- a/docs/en/sql/endpoints/cli.asciidoc +++ b/docs/en/sql/endpoints/cli.asciidoc @@ -37,3 +37,18 @@ James S.A. Corey |Leviathan Wakes |561 |1306972800000 -------------------------------------------------- // TODO it'd be lovely to be able to assert that this is correct but // that is probably more work then it is worth right now. + +[[sql-cli-permissions]] +[NOTE] +=============================== +If you are using Security you need to add a few permissions to +users so they can run SQL. To run SQL using the CLI a user needs +`read`, `indices:admin/get`, and `"cluster:monitor/main"`. The +following example configures a role that can run SQL in the CLI +for the `test` and `bort` indices: + +["source","yaml",subs="attributes,callouts,macros"] +-------------------------------------------------- +include-tagged::{sql-tests}/security/roles.yml[cli_jdbc] +-------------------------------------------------- +=============================== diff --git a/docs/en/sql/endpoints/jdbc.asciidoc b/docs/en/sql/endpoints/jdbc.asciidoc index b977f2ed388..1736b9633ea 100644 --- a/docs/en/sql/endpoints/jdbc.asciidoc +++ b/docs/en/sql/endpoints/jdbc.asciidoc @@ -50,3 +50,19 @@ connection. For example: -------------------------------------------------- include-tagged::{jdbc-tests}/SimpleExampleTestCase.java[simple_example] -------------------------------------------------- + +[[sql-jdbc-permissions]] +[NOTE] +=============================== +If you are using Security you need to add a few permissions to +users so they can run SQL. To run SQL a user needs `read` and +`indices:admin/get`. Some parts of the API require +`"cluster:monitor/main"`. The following example configures a +role that can run SQL in JDBC querying the `test` and `bort` +indices: + +["source","yaml",subs="attributes,callouts,macros"] +-------------------------------------------------- +include-tagged::{sql-tests}/security/roles.yml[cli_jdbc] +-------------------------------------------------- +=============================== diff --git a/docs/en/sql/endpoints/rest.asciidoc b/docs/en/sql/endpoints/rest.asciidoc index beb1cd2847a..383a420bc2f 100644 --- a/docs/en/sql/endpoints/rest.asciidoc +++ b/docs/en/sql/endpoints/rest.asciidoc @@ -183,3 +183,17 @@ or fewer results though. `time_zone` is the time zone to use for date functions and date parsing. `time_zone` defaults to `utc` and can take any values documented http://www.joda.org/joda-time/apidocs/org/joda/time/DateTimeZone.html[here]. + +[[sql-rest-permissions]] +[NOTE] +=============================== +If you are using Security you need to add a few permissions to +users so they can run SQL. To run SQL a user needs `read` and +`indices:admin/get`. The following example configures a role +that can run SQL against the `test` and `bort` indices: + +["source","yaml",subs="attributes,callouts,macros"] +-------------------------------------------------- +include-tagged::{sql-tests}/security/roles.yml[rest] +-------------------------------------------------- +=============================== diff --git a/docs/en/sql/endpoints/translate.asciidoc b/docs/en/sql/endpoints/translate.asciidoc index 5c00fe29c3c..27821141130 100644 --- a/docs/en/sql/endpoints/translate.asciidoc +++ b/docs/en/sql/endpoints/translate.asciidoc @@ -51,3 +51,18 @@ the normal <> API. The request body accepts all of the <> that the <> accepts except `cursor`. + +[[sql-translate-permissions]] +[NOTE] +=============================== +If you are using Security you need to add a few permissions to +users so they can run translate SQL. To translate SQL a user +needs `read` and `indices:admin/get`. The following example +configures a role that can run SQL against the `test` and +`bort` indices: + +["source","yaml",subs="attributes,callouts,macros"] +-------------------------------------------------- +include-tagged::{sql-tests}/security/roles.yml[rest] +-------------------------------------------------- +=============================== diff --git a/qa/sql/security/roles.yml b/qa/sql/security/roles.yml index ff7d8f1b035..a0e3d642495 100644 --- a/qa/sql/security/roles.yml +++ b/qa/sql/security/roles.yml @@ -1,22 +1,32 @@ -read_all: - cluster: - - "cluster:monitor/main" # Used by JDBC's MetaData +rest_minimal: indices: - names: test privileges: [read, "indices:admin/get"] - names: bort privileges: [read, "indices:admin/get"] +# end::rest + +# tag::cli_jdbc +cli_or_jdbc_minimal: + cluster: + - "cluster:monitor/main" + indices: + - names: test + privileges: [read, "indices:admin/get"] + - names: bort + privileges: [read, "indices:admin/get"] +# end::cli_jdbc read_something_else: cluster: - - "cluster:monitor/main" # Used by JDBC's MetaData + - "cluster:monitor/main" indices: - names: something_that_isnt_test privileges: [read, "indices:admin/get"] read_test_a: cluster: - - "cluster:monitor/main" # Used by JDBC's MetaData + - "cluster:monitor/main" indices: - names: test privileges: [read, "indices:admin/get"] @@ -25,7 +35,7 @@ read_test_a: read_test_a_and_b: cluster: - - "cluster:monitor/main" # Used by JDBC's MetaData + - "cluster:monitor/main" indices: - names: test privileges: [read, "indices:admin/get"] @@ -35,7 +45,7 @@ read_test_a_and_b: read_test_without_c_3: cluster: - - "cluster:monitor/main" # Used by JDBC's MetaData + - "cluster:monitor/main" indices: - names: test privileges: [read, "indices:admin/get"] @@ -54,7 +64,23 @@ read_test_without_c_3: read_bort: cluster: - - "cluster:monitor/main" # Used by JDBC's MetaData + - "cluster:monitor/main" indices: - names: bort privileges: [read, "indices:admin/get"] + +no_monitor_main: + indices: + - names: test + privileges: [read, "indices:admin/get"] + - names: bort + privileges: [read, "indices:admin/get"] + +no_get_index: + cluster: + - "cluster:monitor/main" + indices: + - names: test + privileges: [read] + - names: bort + privileges: [read] diff --git a/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/CliSecurityIT.java b/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/CliSecurityIT.java index 8124fd14dbf..aee0a473247 100644 --- a/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/CliSecurityIT.java +++ b/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/CliSecurityIT.java @@ -9,7 +9,7 @@ import org.elasticsearch.common.CheckedConsumer; import org.elasticsearch.common.io.PathUtils; import org.elasticsearch.xpack.qa.sql.cli.RemoteCli; import org.elasticsearch.xpack.qa.sql.cli.RemoteCli.SecurityConfig; - +import java.io.IOException; import java.net.URISyntaxException; import java.nio.file.Files; import java.nio.file.Path; @@ -50,6 +50,11 @@ public class CliSecurityIT extends SqlSecurityTestCase { * Perform security test actions using the CLI. */ private static class CliActions implements Actions { + @Override + public String minimalPermissionsForAllActions() { + return "cli_or_jdbc_minimal"; + } + private SecurityConfig userSecurity(String user) { SecurityConfig admin = adminSecurityConfig(); if (user == null) { @@ -135,6 +140,11 @@ public class CliSecurityIT extends SqlSecurityTestCase { assertEquals("---------------+---------------", cli.readLine()); for (String table : tables) { String line = null; + /* + * Security automatically creates either a `.security` or a + * `.security6` index but it might not have created the index + * by the time the test runs. + */ while (line == null || line.startsWith(".security")) { line = cli.readLine(); } @@ -170,6 +180,18 @@ public class CliSecurityIT extends SqlSecurityTestCase { assertThat(cli.readLine(), containsString("Unknown column [" + column + "][1;23;31m][0m")); } } + + @Override + public void checkNoMonitorMain(String user) throws Exception { + @SuppressWarnings("resource") // forceClose will close it + RemoteCli cli = new RemoteCli(elasticsearchAddress(), true, userSecurity(user)) { + @Override + protected void assertConnectionTest() throws IOException { + assertThat(readLine(), containsString("action [cluster:monitor/main] is unauthorized for user [" + user + "]")); + } + }; + cli.forceClose(); + } } public CliSecurityIT() { diff --git a/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcSecurityIT.java b/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcSecurityIT.java index d1494d5e4a8..113d31cff98 100644 --- a/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcSecurityIT.java +++ b/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcSecurityIT.java @@ -17,6 +17,7 @@ import java.sql.Connection; import java.sql.DriverManager; import java.sql.ResultSet; import java.sql.SQLException; +import java.sql.SQLInvalidAuthorizationSpecException; import java.sql.Statement; import java.util.ArrayList; import java.util.List; @@ -115,6 +116,11 @@ public class JdbcSecurityIT extends SqlSecurityTestCase { } private static class JdbcActions implements Actions { + @Override + public String minimalPermissionsForAllActions() { + return "cli_or_jdbc_minimal"; + } + @Override public void queryWorksAsAdmin() throws Exception { try (Connection h2 = LocalH2.anonymousDb(); @@ -182,9 +188,11 @@ public class JdbcSecurityIT extends SqlSecurityTestCase { try (Connection es = es(userProperties(user))) { ResultSet actual = es.createStatement().executeQuery("SHOW TABLES"); - // depending on whether security is enabled and a test is run in isolation or suite - // .security or .security6 index can appear - // to filter these out, the result set is flatten to a list + /* + * Security automatically creates either a `.security` or a + * `.security6` index but it might not have created the index + * by the time the test runs. + */ List actualList = new ArrayList<>(); while (actual.next()) { @@ -215,6 +223,25 @@ public class JdbcSecurityIT extends SqlSecurityTestCase { con -> con.createStatement().executeQuery(sql), column); } + + @Override + public void checkNoMonitorMain(String user) throws Exception { + // Most SQL actually works fine without monitor/main + expectMatchesAdmin("SELECT * FROM test", user, "SELECT * FROM test"); + expectMatchesAdmin("SHOW TABLES LIKE 'test'", user, "SHOW TABLES LIKE 'test'"); + expectMatchesAdmin("DESCRIBE test", user, "DESCRIBE test"); + + // But there are a few things that don't work + try (Connection es = es(userProperties(user))) { + expectUnauthorized("cluster:monitor/main", user, () -> es.getMetaData().getDatabaseMajorVersion()); + expectUnauthorized("cluster:monitor/main", user, () -> es.getMetaData().getDatabaseMinorVersion()); + } + } + + private void expectUnauthorized(String action, String user, ThrowingRunnable r) { + SQLInvalidAuthorizationSpecException e = expectThrows(SQLInvalidAuthorizationSpecException.class, r); + assertEquals("action [" + action + "] is unauthorized for user [" + user + "]", e.getMessage()); + } } public JdbcSecurityIT() { @@ -223,7 +250,7 @@ public class JdbcSecurityIT extends SqlSecurityTestCase { // Metadata methods only available to JDBC public void testMetaDataGetTablesWithFullAccess() throws Exception { - createUser("full_access", "read_all"); + createUser("full_access", "cli_or_jdbc_minimal"); expectActionMatchesAdmin( con -> con.getMetaData().getTables("%", "%", "%t", null), @@ -256,7 +283,7 @@ public class JdbcSecurityIT extends SqlSecurityTestCase { } public void testMetaDataGetColumnsWorksAsFullAccess() throws Exception { - createUser("full_access", "read_all"); + createUser("full_access", "cli_or_jdbc_minimal"); expectActionMatchesAdmin( con -> con.getMetaData().getColumns("%", "%", "%t", "%"), @@ -314,4 +341,4 @@ public class JdbcSecurityIT extends SqlSecurityTestCase { "no_3s", con -> con.getMetaData().getColumns("%", "%", "test", "%")); } -} \ No newline at end of file +} diff --git a/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/RestSqlSecurityIT.java b/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/RestSqlSecurityIT.java index dfe3c663e3c..478e6fe0edc 100644 --- a/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/RestSqlSecurityIT.java +++ b/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/RestSqlSecurityIT.java @@ -24,8 +24,10 @@ import java.sql.JDBCType; import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.TreeMap; import java.util.stream.Collectors; @@ -34,9 +36,15 @@ import static org.elasticsearch.xpack.qa.sql.rest.RestSqlTestCase.randomMode; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; +import static java.util.Collections.emptyMap; public class RestSqlSecurityIT extends SqlSecurityTestCase { private static class RestActions implements Actions { + @Override + public String minimalPermissionsForAllActions() { + return "rest_minimal"; + } + @Override public void queryWorksAsAdmin() throws Exception { String mode = randomMode(); @@ -121,6 +129,12 @@ public class RestSqlSecurityIT extends SqlSecurityTestCase { expected.put("rows", rows); Map actual = runSql(user, mode, "SHOW TABLES"); + /* + * Security automatically creates either a `.security` or a + * `.security6` index but it might not have created the index + * by the time the test runs. + */ + @SuppressWarnings("unchecked") List> rowsNoSecurity = ((List>) actual.get("rows")) .stream() .filter(ls -> ls.get(0).startsWith(".security") == false) @@ -149,6 +163,14 @@ public class RestSqlSecurityIT extends SqlSecurityTestCase { assertThat(e.getMessage(), containsString("Unknown column [" + column + "]")); } + @Override + public void checkNoMonitorMain(String user) throws Exception { + // Without monitor/main everything should work just fine + expectMatchesAdmin("SELECT * FROM test", user, "SELECT * FROM test"); + expectMatchesAdmin("SHOW TABLES LIKE 'test'", user, "SHOW TABLES LIKE 'test'"); + expectMatchesAdmin("DESCRIBE test", user, "DESCRIBE test"); + } + private static Map runSql(@Nullable String asUser, String mode, String sql) throws IOException { return runSql(asUser, mode, new StringEntity("{\"query\": \"" + sql + "\"}", ContentType.APPLICATION_JSON)); } @@ -190,7 +212,7 @@ public class RestSqlSecurityIT extends SqlSecurityTestCase { * paranoid we'd hack together something to test the others as well. */ public void testHijackScrollFails() throws Exception { - createUser("full_access", "read_all"); + createUser("full_access", "rest_minimal"); Map adminResponse = RestActions.runSql(null, randomMode(), new StringEntity("{\"query\": \"SELECT * FROM test\", \"fetch_size\": 1}", ContentType.APPLICATION_JSON)); diff --git a/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/SqlSecurityTestCase.java b/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/SqlSecurityTestCase.java index 87bf0e2a713..7a9e698633b 100644 --- a/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/SqlSecurityTestCase.java +++ b/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/SqlSecurityTestCase.java @@ -53,6 +53,7 @@ public abstract class SqlSecurityTestCase extends ESRestTestCase { * For methods that take {@code user} a {@code null} user means "use the admin". */ protected interface Actions { + String minimalPermissionsForAllActions(); void queryWorksAsAdmin() throws Exception; /** * Assert that running some sql as a user returns the same result as running it as @@ -69,6 +70,7 @@ public abstract class SqlSecurityTestCase extends ESRestTestCase { void expectForbidden(String user, String sql) throws Exception; void expectUnknownIndex(String user, String sql) throws Exception; void expectUnknownColumn(String user, String sql, String column) throws Exception; + void checkNoMonitorMain(String user) throws Exception; } protected static final String SQL_ACTION_NAME = "indices:data/read/sql"; @@ -196,7 +198,7 @@ public abstract class SqlSecurityTestCase extends ESRestTestCase { } public void testQueryWithFullAccess() throws Exception { - createUser("full_access", "read_all"); + createUser("full_access", actions.minimalPermissionsForAllActions()); actions.expectMatchesAdmin("SELECT * FROM test ORDER BY a", "full_access", "SELECT * FROM test ORDER BY a"); new AuditLogAsserter() @@ -206,7 +208,7 @@ public abstract class SqlSecurityTestCase extends ESRestTestCase { } public void testScrollWithFullAccess() throws Exception { - createUser("full_access", "read_all"); + createUser("full_access", actions.minimalPermissionsForAllActions()); actions.expectScrollMatchesAdmin("SELECT * FROM test ORDER BY a", "full_access", "SELECT * FROM test ORDER BY a"); new AuditLogAsserter() @@ -342,7 +344,7 @@ public abstract class SqlSecurityTestCase extends ESRestTestCase { } public void testShowTablesWorksAsFullAccess() throws Exception { - createUser("full_access", "read_all"); + createUser("full_access", actions.minimalPermissionsForAllActions()); actions.expectMatchesAdmin("SHOW TABLES LIKE '%t'", "full_access", "SHOW TABLES"); new AuditLogAsserter() @@ -394,7 +396,7 @@ public abstract class SqlSecurityTestCase extends ESRestTestCase { } public void testDescribeWorksAsFullAccess() throws Exception { - createUser("full_access", "read_all"); + createUser("full_access", actions.minimalPermissionsForAllActions()); actions.expectMatchesAdmin("DESCRIBE test", "full_access", "DESCRIBE test"); new AuditLogAsserter() @@ -455,6 +457,19 @@ public abstract class SqlSecurityTestCase extends ESRestTestCase { .assertLogs(); } + public void testNoMonitorMain() throws Exception { + createUser("no_monitor_main", "no_monitor_main"); + actions.checkNoMonitorMain("no_monitor_main"); + } + + public void testNoGetIndex() throws Exception { + createUser("no_get_index", "no_get_index"); + + actions.expectForbidden("no_get_index", "SELECT * FROM test"); + actions.expectForbidden("no_get_index", "SHOW TABLES LIKE 'test'"); + actions.expectForbidden("no_get_index", "DESCRIBE test"); + } + protected static void createUser(String name, String role) throws IOException { XContentBuilder user = JsonXContent.contentBuilder().prettyPrint().startObject(); { user.field("password", "testpass"); diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/RemoteCli.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/RemoteCli.java index b148a26fee6..12764338db0 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/RemoteCli.java +++ b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/RemoteCli.java @@ -85,44 +85,61 @@ public class RemoteCli implements Closeable { out = new PrintWriter(new OutputStreamWriter(socket.getOutputStream(), StandardCharsets.UTF_8), true); in = new BufferedReader(new InputStreamReader(socket.getInputStream(), StandardCharsets.UTF_8)); - // Start the CLI - String command; - if (security == null) { - command = elasticsearchAddress; - } else { - command = security.user + "@" + elasticsearchAddress; - if (security.https) { - command = "https://" + command; - } else if (randomBoolean()) { - command = "http://" + command; + try { + // Start the CLI + String command; + if (security == null) { + command = elasticsearchAddress; + } else { + command = security.user + "@" + elasticsearchAddress; + if (security.https) { + command = "https://" + command; + } else if (randomBoolean()) { + command = "http://" + command; + } + if (security.keystoreLocation != null) { + command = command + " -keystore_location " + security.keystoreLocation; + } } - if (security.keystoreLocation != null) { - command = command + " -keystore_location " + security.keystoreLocation; + if (false == checkConnectionOnStartup) { + command += " -check false"; } - } - if (false == checkConnectionOnStartup) { - command += " -check false"; - } - /* Don't use println because it emits \r\n on windows but we put the - * terminal in unix mode to make the tests consistent. */ - out.print(command + "\n"); - out.flush(); - - // Feed it passwords if needed - if (security != null && security.keystoreLocation != null) { - assertEquals("keystore password: ", readUntil(s -> s.endsWith(": "))); - out.print(security.keystorePassword + "\n"); + /* Don't use println because it emits \r\n on windows but we put the + * terminal in unix mode to make the tests consistent. */ + out.print(command + "\n"); out.flush(); - } - if (security != null) { - assertEquals("password: ", readUntil(s -> s.endsWith(": "))); - out.print(security.password + "\n"); - out.flush(); - } - // Throw out the logo and warnings about making a dumb terminal - while (false == readLine().contains("SQL")); - // Throw out the empty line before all the good stuff + // Feed it passwords if needed + if (security != null && security.keystoreLocation != null) { + assertEquals("keystore password: ", readUntil(s -> s.endsWith(": "))); + out.print(security.keystorePassword + "\n"); + out.flush(); + } + if (security != null) { + assertEquals("password: ", readUntil(s -> s.endsWith(": "))); + out.print(security.password + "\n"); + out.flush(); + } + + // Throw out the logo and warnings about making a dumb terminal + while (false == readLine().contains("SQL")); + + assertConnectionTest(); + } catch (AssertionError | Exception e) { + /* If there is an error during connection then try and + * force the socket shut. */ + forceClose(); + throw e; + } + } + + /** + * Assert that result of the connection test. Default implementation + * asserts that the test passes but overridden to check places where + * we want to assert that it fails. + */ + protected void assertConnectionTest() throws IOException { + // After the connection test passess we emit an empty line and then the prompt assertEquals("", readLine()); } @@ -146,13 +163,21 @@ public class RemoteCli implements Closeable { } assertThat("unconsumed lines", nonQuit, empty()); } finally { - out.close(); - in.close(); - // Most importantly, close the socket so the next test can use the fixture - socket.close(); + forceClose(); } } + /** + * Shutdown the connection to the remote CLI without attempting to shut + * the remote down in an orderly way. + */ + public void forceClose() throws IOException { + out.close(); + in.close(); + // Most importantly, close the socket so the next test can use the fixture + socket.close(); + } + /** * Send a command and assert the echo. */