From 807fa1fca181046c727801a3b52c8fa8212fb240 Mon Sep 17 00:00:00 2001 From: Patrick Mackinlay Date: Tue, 19 May 2020 13:26:42 +0100 Subject: [PATCH] chore: questdb can be configured as globally read-only --- .../io/questdb/PropServerConfiguration.java | 7 + .../java/io/questdb/cairo/CairoEngine.java | 10 +- .../questdb/cairo/CairoSecurityContext.java | 8 ++ .../pool/ex/EntryUnavailableException.java | 6 +- .../AllowAllCairoSecurityContext.java | 5 + .../ReadOnlyCairoSecurityContext.java | 12 ++ .../http/DefaultHttpServerConfiguration.java | 5 + .../cutlass/http/HttpConnectionContext.java | 4 +- .../cutlass/http/HttpServerConfiguration.java | 3 + .../java/io/questdb/griffin/SqlCompiler.java | 5 +- .../griffin/AlterTableAddColumnTest.java | 2 +- .../griffin/AlterTableAlterColumnTest.java | 2 +- .../griffin/AlterTableDropColumnTest.java | 2 +- .../java/io/questdb/griffin/SecurityTest.java | 132 ++++++++++++++++++ 14 files changed, 194 insertions(+), 9 deletions(-) create mode 100644 core/src/main/java/io/questdb/cairo/security/ReadOnlyCairoSecurityContext.java create mode 100644 core/src/test/java/io/questdb/griffin/SecurityTest.java diff --git a/core/src/main/java/io/questdb/PropServerConfiguration.java b/core/src/main/java/io/questdb/PropServerConfiguration.java index e273dcb5c..1b514005a 100644 --- a/core/src/main/java/io/questdb/PropServerConfiguration.java +++ b/core/src/main/java/io/questdb/PropServerConfiguration.java @@ -183,6 +183,7 @@ public class PropServerConfiguration implements ServerConfiguration { private int jsonQueryDoubleScale; private int jsonQueryConnectionCheckFrequency; private boolean httpFrozenClock; + private boolean readOnlySecurityContext; public PropServerConfiguration(String root, Properties properties) throws ServerConfigurationException, JsonException { this.sharedWorkerCount = getInt(properties, "shared.worker.count", 2); @@ -255,6 +256,7 @@ public class PropServerConfiguration implements ServerConfiguration { this.jsonQueryConnectionCheckFrequency = getInt(properties, "http.json.query.connection.check.frequency", 1_000_000); this.jsonQueryFloatScale = getInt(properties, "http.json.query.float.scale", 4); this.jsonQueryDoubleScale = getInt(properties, "http.json.query.double.scale", 12); + this.readOnlySecurityContext = getBoolean(properties, "http.security.readonly", false); parseBindTo(properties, "http.bind.to", "0.0.0.0:9000", (a, p) -> { bindIPv4Address = a; @@ -865,6 +867,11 @@ public class PropServerConfiguration implements ServerConfiguration { public boolean haltOnError() { return httpWorkerHaltOnError; } + + @Override + public boolean readOnlySecurityContext() { + return readOnlySecurityContext; + } } private class PropCairoConfiguration implements CairoConfiguration { diff --git a/core/src/main/java/io/questdb/cairo/CairoEngine.java b/core/src/main/java/io/questdb/cairo/CairoEngine.java index b96080b5f..9d9d15715 100644 --- a/core/src/main/java/io/questdb/cairo/CairoEngine.java +++ b/core/src/main/java/io/questdb/cairo/CairoEngine.java @@ -82,6 +82,7 @@ public class CairoEngine implements Closeable { Path path, TableStructure struct ) { + securityContext.checkWritePermission(); TableUtils.createTable( configuration.getFilesFacade(), mem, @@ -155,6 +156,7 @@ public class CairoEngine implements Closeable { CairoSecurityContext securityContext, CharSequence tableName ) { + securityContext.checkWritePermission(); return writerPool.get(tableName); } @@ -163,7 +165,8 @@ public class CairoEngine implements Closeable { CharSequence tableName, CharSequence backupDirName ) { - // There is no point in pooling/caching these writers since they are only used once, backups are not incremental + securityContext.checkWritePermission(); + // There is no point in pooling/caching these writers since they are only used once, backups are not incremental return new TableWriter(configuration, tableName, messageBus, true, DefaultLifecycleManager.INSTANCE, backupDirName); } @@ -171,6 +174,7 @@ public class CairoEngine implements Closeable { CairoSecurityContext securityContext, CharSequence tableName ) { + securityContext.checkWritePermission(); if (writerPool.lock(tableName)) { boolean locked = readerPool.lock(tableName); if (locked) { @@ -226,6 +230,7 @@ public class CairoEngine implements Closeable { Path path, CharSequence tableName ) { + securityContext.checkWritePermission(); if (lock(securityContext, tableName)) { try { path.of(configuration.getRoot()).concat(tableName).$(); @@ -255,6 +260,7 @@ public class CairoEngine implements Closeable { Path otherPath, CharSequence newName ) { + securityContext.checkWritePermission(); if (lock(securityContext, tableName)) { try { rename0(path, tableName, otherPath, newName); @@ -316,7 +322,7 @@ public class CairoEngine implements Closeable { } protected boolean doRun() { - return releaseInactive(); + return releaseInactive(); } @Override diff --git a/core/src/main/java/io/questdb/cairo/CairoSecurityContext.java b/core/src/main/java/io/questdb/cairo/CairoSecurityContext.java index 236c9f99f..034475af5 100644 --- a/core/src/main/java/io/questdb/cairo/CairoSecurityContext.java +++ b/core/src/main/java/io/questdb/cairo/CairoSecurityContext.java @@ -25,4 +25,12 @@ package io.questdb.cairo; public interface CairoSecurityContext { + + default void checkWritePermission() { + if (!canWrite()) { + throw CairoException.instance(0).put("Write permission denied"); + } + } + + boolean canWrite(); } diff --git a/core/src/main/java/io/questdb/cairo/pool/ex/EntryUnavailableException.java b/core/src/main/java/io/questdb/cairo/pool/ex/EntryUnavailableException.java index aa76f20cc..def0e9aa6 100644 --- a/core/src/main/java/io/questdb/cairo/pool/ex/EntryUnavailableException.java +++ b/core/src/main/java/io/questdb/cairo/pool/ex/EntryUnavailableException.java @@ -27,5 +27,9 @@ package io.questdb.cairo.pool.ex; import io.questdb.cairo.CairoException; public class EntryUnavailableException extends CairoException { - public static final EntryUnavailableException INSTANCE = new EntryUnavailableException(); + public static final EntryUnavailableException INSTANCE; + static { + INSTANCE = new EntryUnavailableException(); + INSTANCE.put("table busy"); + } } diff --git a/core/src/main/java/io/questdb/cairo/security/AllowAllCairoSecurityContext.java b/core/src/main/java/io/questdb/cairo/security/AllowAllCairoSecurityContext.java index f13abe1ef..c474937df 100644 --- a/core/src/main/java/io/questdb/cairo/security/AllowAllCairoSecurityContext.java +++ b/core/src/main/java/io/questdb/cairo/security/AllowAllCairoSecurityContext.java @@ -28,4 +28,9 @@ import io.questdb.cairo.CairoSecurityContext; public class AllowAllCairoSecurityContext implements CairoSecurityContext { public static final AllowAllCairoSecurityContext INSTANCE = new AllowAllCairoSecurityContext(); + + @Override + public boolean canWrite() { + return true; + } } diff --git a/core/src/main/java/io/questdb/cairo/security/ReadOnlyCairoSecurityContext.java b/core/src/main/java/io/questdb/cairo/security/ReadOnlyCairoSecurityContext.java new file mode 100644 index 000000000..759ca56f3 --- /dev/null +++ b/core/src/main/java/io/questdb/cairo/security/ReadOnlyCairoSecurityContext.java @@ -0,0 +1,12 @@ +package io.questdb.cairo.security; + +import io.questdb.cairo.CairoSecurityContext; + +public class ReadOnlyCairoSecurityContext implements CairoSecurityContext { + public static final ReadOnlyCairoSecurityContext INSTANCE = new ReadOnlyCairoSecurityContext(); + + @Override + public boolean canWrite() { + return false; + } +} diff --git a/core/src/main/java/io/questdb/cutlass/http/DefaultHttpServerConfiguration.java b/core/src/main/java/io/questdb/cutlass/http/DefaultHttpServerConfiguration.java index 46af790f8..dd1c59278 100644 --- a/core/src/main/java/io/questdb/cutlass/http/DefaultHttpServerConfiguration.java +++ b/core/src/main/java/io/questdb/cutlass/http/DefaultHttpServerConfiguration.java @@ -209,4 +209,9 @@ class DefaultHttpServerConfiguration implements HttpServerConfiguration { public boolean haltOnError() { return false; } + + @Override + public boolean readOnlySecurityContext() { + return false; + } } diff --git a/core/src/main/java/io/questdb/cutlass/http/HttpConnectionContext.java b/core/src/main/java/io/questdb/cutlass/http/HttpConnectionContext.java index 2b58c74fc..f37003bef 100644 --- a/core/src/main/java/io/questdb/cutlass/http/HttpConnectionContext.java +++ b/core/src/main/java/io/questdb/cutlass/http/HttpConnectionContext.java @@ -26,6 +26,7 @@ package io.questdb.cutlass.http; import io.questdb.cairo.CairoSecurityContext; import io.questdb.cairo.security.AllowAllCairoSecurityContext; +import io.questdb.cairo.security.ReadOnlyCairoSecurityContext; import io.questdb.log.Log; import io.questdb.log.LogFactory; import io.questdb.network.*; @@ -50,7 +51,7 @@ public class HttpConnectionContext implements IOContext, Locality, Mutable { private final LocalValueMap localValueMap = new LocalValueMap(); private final NetworkFacade nf; private final long multipartIdleSpinCount; - private final CairoSecurityContext cairoSecurityContext = AllowAllCairoSecurityContext.INSTANCE; + private final CairoSecurityContext cairoSecurityContext; private final boolean dumpNetworkTraffic; private final boolean allowDeflateBeforeSend; private long fd; @@ -71,6 +72,7 @@ public class HttpConnectionContext implements IOContext, Locality, Mutable { this.multipartIdleSpinCount = configuration.getMultipartIdleSpinCount(); this.dumpNetworkTraffic = configuration.getDumpNetworkTraffic(); this.allowDeflateBeforeSend = configuration.allowDeflateBeforeSend(); + cairoSecurityContext = configuration.readOnlySecurityContext() ? ReadOnlyCairoSecurityContext.INSTANCE : AllowAllCairoSecurityContext.INSTANCE; } @Override diff --git a/core/src/main/java/io/questdb/cutlass/http/HttpServerConfiguration.java b/core/src/main/java/io/questdb/cutlass/http/HttpServerConfiguration.java index 595f3cffa..1571bbdc4 100644 --- a/core/src/main/java/io/questdb/cutlass/http/HttpServerConfiguration.java +++ b/core/src/main/java/io/questdb/cutlass/http/HttpServerConfiguration.java @@ -61,9 +61,12 @@ public interface HttpServerConfiguration extends WorkerPoolAwareConfiguration { int getSendBufferSize(); + @Override boolean isEnabled(); boolean getDumpNetworkTraffic(); boolean allowDeflateBeforeSend(); + + boolean readOnlySecurityContext(); } diff --git a/core/src/main/java/io/questdb/griffin/SqlCompiler.java b/core/src/main/java/io/questdb/griffin/SqlCompiler.java index dd9978722..bb1f5e968 100644 --- a/core/src/main/java/io/questdb/griffin/SqlCompiler.java +++ b/core/src/main/java/io/questdb/griffin/SqlCompiler.java @@ -697,8 +697,8 @@ public class SqlCompiler implements Closeable { throw SqlException.$(lexer.lastTokenPosition(), "'add' or 'drop' expected"); } } catch (CairoException e) { - LOG.info().$("failed to lock table for alter: ").$((Sinkable) e).$(); - throw SqlException.$(tableNamePosition, "table '").put(tableName).put("' is busy"); + LOG.info().$("failed to alter table: ").$((Sinkable) e).$(); + throw SqlException.$(tableNamePosition, "table '").put(tableName).put("' cannot be altered: ").put(e); } return compiledQuery.ofAlter(); @@ -1571,6 +1571,7 @@ public class SqlCompiler implements Closeable { } private CompiledQuery sqlBackup(SqlExecutionContext executionContext) throws SqlException { + executionContext.getCairoSecurityContext().checkWritePermission(); if (null == configuration.getBackupRoot()) { throw CairoException.instance(0).put("Backup is disabled, no backup root directory is configured in the server configuration ['cairo.sql.backup.root' property]"); } diff --git a/core/src/test/java/io/questdb/griffin/AlterTableAddColumnTest.java b/core/src/test/java/io/questdb/griffin/AlterTableAddColumnTest.java index cd1b9dec5..5e1f5a4e3 100644 --- a/core/src/test/java/io/questdb/griffin/AlterTableAddColumnTest.java +++ b/core/src/test/java/io/questdb/griffin/AlterTableAddColumnTest.java @@ -90,7 +90,7 @@ public class AlterTableAddColumnTest extends AbstractGriffinTest { } } catch (SqlException e) { Assert.assertEquals(12, e.getPosition()); - TestUtils.assertContains(e.getFlyweightMessage(), "table 'x' is busy"); + TestUtils.assertContains(e.getFlyweightMessage(), "table 'x' cannot be altered: [0]: table busy"); } allHaltLatch.await(2, TimeUnit.SECONDS); diff --git a/core/src/test/java/io/questdb/griffin/AlterTableAlterColumnTest.java b/core/src/test/java/io/questdb/griffin/AlterTableAlterColumnTest.java index 5db9a41c6..4a5e868c0 100644 --- a/core/src/test/java/io/questdb/griffin/AlterTableAlterColumnTest.java +++ b/core/src/test/java/io/questdb/griffin/AlterTableAlterColumnTest.java @@ -117,7 +117,7 @@ public class AlterTableAlterColumnTest extends AbstractGriffinTest { } } catch (SqlException e) { Assert.assertEquals(12, e.getPosition()); - TestUtils.assertContains(e.getFlyweightMessage(), "table 'x' is busy"); + TestUtils.assertContains(e.getFlyweightMessage(), "table 'x' cannot be altered: [0]: table busy"); } allHaltLatch.await(2, TimeUnit.SECONDS); }); diff --git a/core/src/test/java/io/questdb/griffin/AlterTableDropColumnTest.java b/core/src/test/java/io/questdb/griffin/AlterTableDropColumnTest.java index fc4f4e4a9..97dd606a2 100644 --- a/core/src/test/java/io/questdb/griffin/AlterTableDropColumnTest.java +++ b/core/src/test/java/io/questdb/griffin/AlterTableDropColumnTest.java @@ -92,7 +92,7 @@ public class AlterTableDropColumnTest extends AbstractGriffinTest { } } catch (SqlException e) { Assert.assertEquals(12, e.getPosition()); - TestUtils.assertContains(e.getFlyweightMessage(), "table 'x' is busy"); + TestUtils.assertContains(e.getFlyweightMessage(), "table 'x' cannot be altered: [0]: table busy"); } engine.releaseAllReaders(); diff --git a/core/src/test/java/io/questdb/griffin/SecurityTest.java b/core/src/test/java/io/questdb/griffin/SecurityTest.java new file mode 100644 index 000000000..594578097 --- /dev/null +++ b/core/src/test/java/io/questdb/griffin/SecurityTest.java @@ -0,0 +1,132 @@ +package io.questdb.griffin; + +import org.junit.Assert; +import org.junit.Test; + +import io.questdb.cairo.security.ReadOnlyCairoSecurityContext; +import io.questdb.cairo.sql.InsertMethod; +import io.questdb.cairo.sql.InsertStatement; + +public class SecurityTest extends AbstractGriffinTest { + protected static final SqlExecutionContext readOnlyExecutionContext = new SqlExecutionContextImpl( + configuration, + messageBus, + 1) + .with( + ReadOnlyCairoSecurityContext.INSTANCE, + bindVariableService, + null); + + @Test + public void testCreateTableDeniedOnNoWriteAccess() throws Exception { + assertMemoryLeak(() -> { + try { + compiler.compile("create table balances(cust_id int, ccy symbol, balance double)", readOnlyExecutionContext); + Assert.fail(); + } catch (Exception ex) { + Assert.assertTrue(ex.toString().contains("permission denied")); + } + try { + assertQuery("count\n1\n", "select count() from balances", null); + Assert.fail(); + } catch (SqlException ex) { + Assert.assertTrue(ex.toString().contains("table does not exist")); + } + }); + } + + @Test + public void testInsertDeniedOnNoWriteAccess() throws Exception { + assertMemoryLeak(() -> { + compiler.compile("create table balances(cust_id int, ccy symbol, balance double)", sqlExecutionContext); + assertQuery("count\n0\n", "select count() from balances", null); + + CompiledQuery cq = compiler.compile("insert into balances values (1, 'EUR', 140.6)", sqlExecutionContext); + InsertStatement insertStatement = cq.getInsertStatement(); + try (InsertMethod method = insertStatement.createMethod(sqlExecutionContext)) { + method.execute(); + method.commit(); + } + assertQuery("count\n1\n", "select count() from balances", null); + + try { + cq = compiler.compile("insert into balances values (2, 'ZAR', 140.6)", readOnlyExecutionContext); + insertStatement = cq.getInsertStatement(); + try (InsertMethod method = insertStatement.createMethod(readOnlyExecutionContext)) { + method.execute(); + method.commit(); + } + Assert.fail(); + } catch (Exception ex) { + Assert.assertTrue(ex.toString().contains("permission denied")); + } + + assertQuery("count\n1\n", "select count() from balances", null); + }); + } + + @Test + public void testDropTableDeniedOnNoWriteAccess() throws Exception { + assertMemoryLeak(() -> { + compiler.compile("create table balances(cust_id int, ccy symbol, balance double)", sqlExecutionContext); + try { + compiler.compile("drop table balances", readOnlyExecutionContext); + Assert.fail(); + } catch (Exception ex) { + Assert.assertTrue(ex.toString().contains("permission denied")); + } + assertQuery("count\n0\n", "select count() from balances", null); + }); + } + + @Test + public void testAlterTableDeniedOnNoWriteAccess() throws Exception { + assertMemoryLeak(() -> { + compiler.compile("create table balances(cust_id int, ccy symbol, balance double)", sqlExecutionContext); + CompiledQuery cq = compiler.compile("insert into balances values (1, 'EUR', 140.6)", sqlExecutionContext); + InsertStatement insertStatement = cq.getInsertStatement(); + try (InsertMethod method = insertStatement.createMethod(sqlExecutionContext)) { + method.execute(); + method.commit(); + } + assertQuery("cust_id\tccy\tbalance\n1\tEUR\t140.6\n", "select * from balances", null, true); + + try { + compiler.compile("alter table balances add column newcol int", readOnlyExecutionContext); + Assert.fail(); + } catch (Exception ex) { + Assert.assertTrue(ex.toString().contains("permission denied")); + } + + assertQuery("cust_id\tccy\tbalance\n1\tEUR\t140.6\n", "select * from balances", null, true); + }); + } + + @Test + public void testRenameTableDeniedOnNoWriteAccess() throws Exception { + assertMemoryLeak(() -> { + compiler.compile("create table balances(cust_id int, ccy symbol, balance double)", sqlExecutionContext); + try { + compiler.compile("rename table balances to newname", readOnlyExecutionContext); + Assert.fail(); + } catch (Exception ex) { + Assert.assertTrue(ex.toString().contains("permission denied")); + } + assertQuery("count\n0\n", "select count() from balances", null); + }); + } + + @Test + public void testBackupTableDeniedOnNoWriteAccess() throws Exception { + assertMemoryLeak(() -> { + compiler.compile("create table balances(cust_id int, ccy symbol, balance double)", sqlExecutionContext); + try { + compiler.compile("backup table balances", readOnlyExecutionContext); + Assert.fail(); + } catch (Exception ex) { + Assert.assertTrue(ex.toString().contains("permission denied")); + } + }); + } + +} -- GitLab