From 4113fffc993b715629b484fe8f5a06d7efb9f644 Mon Sep 17 00:00:00 2001 From: Victor Shyba Date: Tue, 15 Sep 2015 19:51:49 -0300 Subject: [feat] Validate and execute commands by subprocess This commit adds a way to validate and execute commands using an argument validator. Commands are executed via subprocess. --- common/src/leap/soledad/common/command.py | 54 +++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 common/src/leap/soledad/common/command.py (limited to 'common') diff --git a/common/src/leap/soledad/common/command.py b/common/src/leap/soledad/common/command.py new file mode 100644 index 00000000..978cec91 --- /dev/null +++ b/common/src/leap/soledad/common/command.py @@ -0,0 +1,54 @@ +# -*- coding: utf-8 -*- +# command.py +# Copyright (C) 2015 LEAP +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + + +""" +Utility to sanitize and run shell commands. +""" + + +import subprocess + + +def exec_validated_cmd(cmd, args, validator=None): + """ + Executes cmd, validating args with validator. + + :param cmd: command. + :type dbname: str + :param args: arguments. + :type args: str + :param validator: optional function to validate args + :type validator: function + + :return: exit code and stdout or stderr (if code != 0) + :rtype: (int, str) + """ + if validator and not validator(args): + return 1, "invalid argument" + command = cmd.split(' ') + command.append(args) + try: + process = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + except OSError, e: + return 1, e + (out, err) = process.communicate() + code = process.wait() + if code is not 0: + return code, err + else: + return code, out -- cgit v1.2.3 From 66baa09a1760e911a6e58c87bce4f0062bd95805 Mon Sep 17 00:00:00 2001 From: Victor Shyba Date: Tue, 15 Sep 2015 19:53:25 -0300 Subject: [tests] Tests for command validation and execution Checks if arguments validation occurs properly and command execution brings back status code and stdout or stderr on some scenarios. --- .../src/leap/soledad/common/tests/test_command.py | 53 ++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 common/src/leap/soledad/common/tests/test_command.py (limited to 'common') diff --git a/common/src/leap/soledad/common/tests/test_command.py b/common/src/leap/soledad/common/tests/test_command.py new file mode 100644 index 00000000..af4903eb --- /dev/null +++ b/common/src/leap/soledad/common/tests/test_command.py @@ -0,0 +1,53 @@ +# -*- coding: utf-8 -*- +# test_command.py +# Copyright (C) 2015 LEAP +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +""" +Tests for command execution using a validator function for arguments. +""" +from twisted.trial import unittest +from leap.soledad.common.command import exec_validated_cmd + + +class ExecuteValidatedCommandTest(unittest.TestCase): + + def test_argument_validation(self): + validator = lambda arg: True if arg is 'valid' else False + status, out = exec_validated_cmd("command", "invalid arg", validator) + self.assertEquals(status, 1) + self.assertEquals(out, "invalid argument") + status, out = exec_validated_cmd("echo", "valid", validator) + self.assertEquals(status, 0) + self.assertEquals(out, "valid\n") + + def test_return_status_code_success(self): + status, out = exec_validated_cmd("echo", "arg") + self.assertEquals(status, 0) + self.assertEquals(out, "arg\n") + + def test_handle_command_with_spaces(self): + status, out = exec_validated_cmd("echo I am", "an argument") + self.assertEquals(status, 0, out) + self.assertEquals(out, "I am an argument\n") + + def test_handle_oserror_on_invalid_command(self): + status, out = exec_validated_cmd("inexistent command with", "args") + self.assertEquals(status, 1) + self.assertIn("No such file or directory", out) + + def test_return_status_code_number_on_failure(self): + status, out = exec_validated_cmd("ls", "user-bebacafe") + self.assertEquals(status, 2) + self.assertIn('ls: cannot access user-bebacafe: No such file or directory\n', out) -- cgit v1.2.3 From eb6b66da6aa81ade4e61ef153ebbe8fba78cd335 Mon Sep 17 00:00:00 2001 From: Victor Shyba Date: Tue, 15 Sep 2015 19:54:34 -0300 Subject: [feat] ensure_database is now able to call a cmd If CouchServerState is created with a create_cmd parameter, it can now use this parameter to invoke a command to create databases. A validator for database name is also used to ensure that command injection is not possible if user manages to manipulate database name argument. --- common/src/leap/soledad/common/couch.py | 46 +++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 11 deletions(-) (limited to 'common') diff --git a/common/src/leap/soledad/common/couch.py b/common/src/leap/soledad/common/couch.py index 38041c09..cd5185eb 100644 --- a/common/src/leap/soledad/common/couch.py +++ b/common/src/leap/soledad/common/couch.py @@ -60,6 +60,7 @@ from u1db.remote.server_state import ServerState from leap.soledad.common import ddocs, errors +from leap.soledad.common.command import exec_validated_cmd from leap.soledad.common.document import SoledadDocument @@ -1374,13 +1375,29 @@ class CouchSyncTarget(CommonSyncTarget): source_replica_transaction_id) +DB_NAME_MASK = "^user-[a-f0-9]+$" + + +def is_db_name_valid(name): + """ + Validate a user database using DB_NAME_MASK. + + :param name: database name. + :type name: str + + :return: boolean for name vailidity + :rtype: bool + """ + return re.match(DB_NAME_MASK, name) is not None + + class CouchServerState(ServerState): """ Inteface of the WSGI server with the CouchDB backend. """ - def __init__(self, couch_url): + def __init__(self, couch_url, create_cmd=None): """ Initialize the couch server state. @@ -1388,6 +1405,7 @@ class CouchServerState(ServerState): :type couch_url: str """ self.couch_url = couch_url + self.create_cmd = create_cmd def open_database(self, dbname): """ @@ -1409,20 +1427,26 @@ class CouchServerState(ServerState): """ Ensure couch database exists. - Usually, this method is used by the server to ensure the existence of - a database. In our setup, the Soledad user that accesses the underlying - couch server should never have permission to create (or delete) - databases. But, in case it ever does, by raising an exception here we - have one more guarantee that no modified client will be able to - enforce creation of a database when syncing. - :param dbname: The name of the database to ensure. :type dbname: str - :raise Unauthorized: Always, because Soledad server is not allowed to - create databases. + :raise Unauthorized: If disabled or other error was raised. + + :return: The CouchDatabase object and its replica_uid. + :rtype: (CouchDatabase, str) """ - raise Unauthorized() + if not self.create_cmd: + raise Unauthorized() + else: + code, out = exec_validated_cmd(self.create_cmd, dbname, + validator=is_db_name_valid) + if code is not 0: + logger.error(""" + Error while creating database (%s) with (%s) command. + Output: %s + Exit code: %d + """ % (dbname, self.create_cmd, out, code)) + raise Unauthorized() def delete_database(self, dbname): """ -- cgit v1.2.3 From 7591c95951e4618f7775c52340f4d170a1bdd961 Mon Sep 17 00:00:00 2001 From: Victor Shyba Date: Tue, 15 Sep 2015 19:56:43 -0300 Subject: [tests] CouchServerState tests for ensure_database Tests that Unauthorized is raised in any failure scenario, leaving user blind for tips on what happened during execution. This should lower chances of information disclosure on execution failure. --- common/src/leap/soledad/common/tests/test_couch.py | 25 ++++++++++++++++++++++ 1 file changed, 25 insertions(+) (limited to 'common') diff --git a/common/src/leap/soledad/common/tests/test_couch.py b/common/src/leap/soledad/common/tests/test_couch.py index c8d13667..d0a9dc3c 100644 --- a/common/src/leap/soledad/common/tests/test_couch.py +++ b/common/src/leap/soledad/common/tests/test_couch.py @@ -28,6 +28,7 @@ from couchdb.client import Server from uuid import uuid4 from testscenarios import TestWithScenarios +from twisted.trial import unittest from u1db import errors as u1db_errors from u1db import SyncTarget @@ -1498,3 +1499,27 @@ class CouchDatabaseExceptionsTests(CouchDBTestCase): self.db._get_transaction_log) self.create_db(ensure=True, dbname=self.db._dbname) self.db._get_transaction_log() + + +class DatabaseNameValidationTest(unittest.TestCase): + + def test_database_name_validation(self): + self.assertFalse(couch.is_db_name_valid("user-deadbeef | cat /secret")) + self.assertTrue(couch.is_db_name_valid("user-cafe1337")) + + +class CommandBasedDBCreationTest(unittest.TestCase): + + def test_ensure_db_using_custom_command(self): + state = couch.CouchServerState("url", create_cmd="echo") + state.ensure_database("user-1337") # works + + def test_raises_unauthorized_on_failure(self): + state = couch.CouchServerState("url", create_cmd="inexistent") + self.assertRaises(u1db_errors.Unauthorized, + state.ensure_database, "user-1337") + + def test_raises_unauthorized_by_default(self): + state = couch.CouchServerState("url") + self.assertRaises(u1db_errors.Unauthorized, + state.ensure_database, "user-1337") -- cgit v1.2.3 From f7ff2e014e25b5c201f4e6209549518b53fc36b2 Mon Sep 17 00:00:00 2001 From: Victor Shyba Date: Tue, 15 Sep 2015 20:12:01 -0300 Subject: [bug] ensure_database returns db and replica_uid ensure database needs to return a db and its replica_uid. Updated tests, doc and code to reflect that. --- common/src/leap/soledad/common/couch.py | 2 ++ common/src/leap/soledad/common/tests/test_couch.py | 8 +++++++- 2 files changed, 9 insertions(+), 1 deletion(-) (limited to 'common') diff --git a/common/src/leap/soledad/common/couch.py b/common/src/leap/soledad/common/couch.py index cd5185eb..36f6239e 100644 --- a/common/src/leap/soledad/common/couch.py +++ b/common/src/leap/soledad/common/couch.py @@ -1447,6 +1447,8 @@ class CouchServerState(ServerState): Exit code: %d """ % (dbname, self.create_cmd, out, code)) raise Unauthorized() + db = self.open_database(dbname) + return db, db.replica_uid def delete_database(self, dbname): """ diff --git a/common/src/leap/soledad/common/tests/test_couch.py b/common/src/leap/soledad/common/tests/test_couch.py index d0a9dc3c..a56cea21 100644 --- a/common/src/leap/soledad/common/tests/test_couch.py +++ b/common/src/leap/soledad/common/tests/test_couch.py @@ -29,6 +29,7 @@ from uuid import uuid4 from testscenarios import TestWithScenarios from twisted.trial import unittest +from mock import Mock from u1db import errors as u1db_errors from u1db import SyncTarget @@ -1512,7 +1513,12 @@ class CommandBasedDBCreationTest(unittest.TestCase): def test_ensure_db_using_custom_command(self): state = couch.CouchServerState("url", create_cmd="echo") - state.ensure_database("user-1337") # works + mock_db = Mock() + mock_db.replica_uid = 'replica_uid' + state.open_database = Mock(return_value=mock_db) + db, replica_uid = state.ensure_database("user-1337") # works + self.assertEquals(mock_db, db) + self.assertEquals(mock_db.replica_uid, replica_uid) def test_raises_unauthorized_on_failure(self): state = couch.CouchServerState("url", create_cmd="inexistent") -- cgit v1.2.3 From a660f60b9644836b0dbdf54cd04b15f4d4654d0f Mon Sep 17 00:00:00 2001 From: Victor Shyba Date: Fri, 18 Sep 2015 17:30:19 -0300 Subject: [feat] ensure security document Beyond ensuring ddocs, it is also necessary to ensure _security doc presence while creating a database. This document will tell couchdb to grant access to 'soledad' user as a member role and no one as admin. --- common/src/leap/soledad/common/couch.py | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 'common') diff --git a/common/src/leap/soledad/common/couch.py b/common/src/leap/soledad/common/couch.py index 36f6239e..d9ed5026 100644 --- a/common/src/leap/soledad/common/couch.py +++ b/common/src/leap/soledad/common/couch.py @@ -435,6 +435,7 @@ class CouchDatabase(CommonBackend): self._set_replica_uid(replica_uid) if ensure_ddocs: self.ensure_ddocs_on_db() + self.ensure_security() self._cache = None @property @@ -467,6 +468,16 @@ class CouchDatabase(CommonBackend): getattr(ddocs, ddoc_name))) self._database.save(ddoc) + def ensure_security(self): + """ + Make sure that only soledad user is able to access this database as + a member. + """ + security = self._database.security + security['members'] = {'names': ['soledad'], 'roles': []} + security['admins'] = {'names': [], 'roles': []} + self._database.security = security + def get_sync_target(self): """ Return a SyncTarget object, for another u1db to synchronize with. -- cgit v1.2.3 From 7b1591e3d561aa6525318235e702eebeb6708560 Mon Sep 17 00:00:00 2001 From: Victor Shyba Date: Fri, 18 Sep 2015 17:31:37 -0300 Subject: [tests] tests for ensure_security As the other tests does. Make sure that a fresh database gets proper security doc after calling ensure_security method. --- common/src/leap/soledad/common/tests/test_couch.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 'common') diff --git a/common/src/leap/soledad/common/tests/test_couch.py b/common/src/leap/soledad/common/tests/test_couch.py index a56cea21..3622bb56 100644 --- a/common/src/leap/soledad/common/tests/test_couch.py +++ b/common/src/leap/soledad/common/tests/test_couch.py @@ -1501,6 +1501,20 @@ class CouchDatabaseExceptionsTests(CouchDBTestCase): self.create_db(ensure=True, dbname=self.db._dbname) self.db._get_transaction_log() + def test_ensure_security_doc(self): + """ + Ensure_security creates a _security ddoc to ensure that only soledad + will have member access to a db. + """ + self.create_db(ensure=False) + self.assertFalse(self.db._database.security) + self.db.ensure_security() + security_ddoc = self.db._database.security + self.assertIn('admins', security_ddoc) + self.assertFalse(security_ddoc['admins']['names']) + self.assertIn('members', security_ddoc) + self.assertIn('soledad', security_ddoc['members']['names']) + class DatabaseNameValidationTest(unittest.TestCase): -- cgit v1.2.3 From de0cf00b4412e253a481ff19803bab66ffc4443e Mon Sep 17 00:00:00 2001 From: Victor Shyba Date: Thu, 24 Sep 2015 21:57:26 -0300 Subject: [refactor] kaliy's review and pep8 fixes README with information about latest change, missing docs and licenses, variable naming and pep8. --- common/changes/create_db_cmd | 2 ++ common/src/leap/soledad/common/command.py | 17 +++++++++-------- common/src/leap/soledad/common/couch.py | 19 +++++++++++-------- common/src/leap/soledad/common/tests/test_command.py | 4 +++- common/src/leap/soledad/common/tests/test_couch.py | 4 ++-- 5 files changed, 27 insertions(+), 19 deletions(-) create mode 100644 common/changes/create_db_cmd (limited to 'common') diff --git a/common/changes/create_db_cmd b/common/changes/create_db_cmd new file mode 100644 index 00000000..00bbdf71 --- /dev/null +++ b/common/changes/create_db_cmd @@ -0,0 +1,2 @@ + o Add a sanitized command executor for database creation and re-enable + user database creation on CouchServerState via command line. diff --git a/common/src/leap/soledad/common/command.py b/common/src/leap/soledad/common/command.py index 978cec91..811bf135 100644 --- a/common/src/leap/soledad/common/command.py +++ b/common/src/leap/soledad/common/command.py @@ -24,26 +24,27 @@ Utility to sanitize and run shell commands. import subprocess -def exec_validated_cmd(cmd, args, validator=None): +def exec_validated_cmd(cmd, argument, validator=None): """ - Executes cmd, validating args with validator. + Executes cmd, validating argument with a validator function. :param cmd: command. :type dbname: str - :param args: arguments. - :type args: str - :param validator: optional function to validate args + :param argument: argument. + :type argument: str + :param validator: optional function to validate argument :type validator: function :return: exit code and stdout or stderr (if code != 0) :rtype: (int, str) """ - if validator and not validator(args): + if validator and not validator(argument): return 1, "invalid argument" command = cmd.split(' ') - command.append(args) + command.append(argument) try: - process = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + process = subprocess.Popen(command, stdout=subprocess.PIPE, + stderr=subprocess.PIPE) except OSError, e: return 1, e (out, err) = process.communicate() diff --git a/common/src/leap/soledad/common/couch.py b/common/src/leap/soledad/common/couch.py index d9ed5026..4c5f6400 100644 --- a/common/src/leap/soledad/common/couch.py +++ b/common/src/leap/soledad/common/couch.py @@ -435,7 +435,7 @@ class CouchDatabase(CommonBackend): self._set_replica_uid(replica_uid) if ensure_ddocs: self.ensure_ddocs_on_db() - self.ensure_security() + self.ensure_security_ddoc() self._cache = None @property @@ -468,10 +468,15 @@ class CouchDatabase(CommonBackend): getattr(ddocs, ddoc_name))) self._database.save(ddoc) - def ensure_security(self): + def ensure_security_ddoc(self): """ Make sure that only soledad user is able to access this database as - a member. + an unprivileged member, meaning that administration access will + be forbidden even inside an user database. + The goal is to make sure that only the lowest access level is given + to the unprivileged CouchDB user set on the server process. + This is achieved by creating a _security design document, see: + http://docs.couchdb.org/en/latest/api/database/security.html """ security = self._database.security security['members'] = {'names': ['soledad'], 'roles': []} @@ -1386,12 +1391,9 @@ class CouchSyncTarget(CommonSyncTarget): source_replica_transaction_id) -DB_NAME_MASK = "^user-[a-f0-9]+$" - - def is_db_name_valid(name): """ - Validate a user database using DB_NAME_MASK. + Validate a user database using a regular expression. :param name: database name. :type name: str @@ -1399,7 +1401,8 @@ def is_db_name_valid(name): :return: boolean for name vailidity :rtype: bool """ - return re.match(DB_NAME_MASK, name) is not None + db_name_regex = "^user-[a-f0-9]+$" + return re.match(db_name_regex, name) is not None class CouchServerState(ServerState): diff --git a/common/src/leap/soledad/common/tests/test_command.py b/common/src/leap/soledad/common/tests/test_command.py index af4903eb..420f91ae 100644 --- a/common/src/leap/soledad/common/tests/test_command.py +++ b/common/src/leap/soledad/common/tests/test_command.py @@ -50,4 +50,6 @@ class ExecuteValidatedCommandTest(unittest.TestCase): def test_return_status_code_number_on_failure(self): status, out = exec_validated_cmd("ls", "user-bebacafe") self.assertEquals(status, 2) - self.assertIn('ls: cannot access user-bebacafe: No such file or directory\n', out) + self.assertIn( + 'ls: cannot access user-bebacafe: No such file or directory\n', + out) diff --git a/common/src/leap/soledad/common/tests/test_couch.py b/common/src/leap/soledad/common/tests/test_couch.py index 3622bb56..d1a07a3a 100644 --- a/common/src/leap/soledad/common/tests/test_couch.py +++ b/common/src/leap/soledad/common/tests/test_couch.py @@ -1504,11 +1504,11 @@ class CouchDatabaseExceptionsTests(CouchDBTestCase): def test_ensure_security_doc(self): """ Ensure_security creates a _security ddoc to ensure that only soledad - will have member access to a db. + will have the lowest privileged access to an user db. """ self.create_db(ensure=False) self.assertFalse(self.db._database.security) - self.db.ensure_security() + self.db.ensure_security_ddoc() security_ddoc = self.db._database.security self.assertIn('admins', security_ddoc) self.assertFalse(security_ddoc['admins']['names']) -- cgit v1.2.3 From 01314341c8ef1e947b8f921ba023ac67d89a9ce7 Mon Sep 17 00:00:00 2001 From: Victor Shyba Date: Mon, 28 Sep 2015 15:43:49 -0300 Subject: [test] remove old mocks Those hardcoded mocks are leaking into other tests and are unnecessary. --- .../soledad/common/tests/test_couch_operations_atomicity.py | 6 ------ common/src/leap/soledad/common/tests/test_server.py | 12 ------------ 2 files changed, 18 deletions(-) (limited to 'common') diff --git a/common/src/leap/soledad/common/tests/test_couch_operations_atomicity.py b/common/src/leap/soledad/common/tests/test_couch_operations_atomicity.py index 3e8e8cce..507f2984 100644 --- a/common/src/leap/soledad/common/tests/test_couch_operations_atomicity.py +++ b/common/src/leap/soledad/common/tests/test_couch_operations_atomicity.py @@ -35,17 +35,11 @@ from leap.soledad.common.tests.util import ( ) from leap.soledad.common.tests.test_couch import CouchDBTestCase from leap.soledad.common.tests.u1db_tests import TestCaseWithServer -from leap.soledad.common.tests.test_server import _couch_ensure_database REPEAT_TIMES = 20 -# monkey path CouchServerState so it can ensure databases. - -CouchServerState.ensure_database = _couch_ensure_database - - class CouchAtomicityTestCase(CouchDBTestCase, TestCaseWithServer): @staticmethod diff --git a/common/src/leap/soledad/common/tests/test_server.py b/common/src/leap/soledad/common/tests/test_server.py index f512d6c1..19d2907d 100644 --- a/common/src/leap/soledad/common/tests/test_server.py +++ b/common/src/leap/soledad/common/tests/test_server.py @@ -46,18 +46,6 @@ from leap.soledad.server import LockResource from leap.soledad.server.auth import URLToAuthorization -# monkey path CouchServerState so it can ensure databases. - -def _couch_ensure_database(self, dbname): - db = CouchDatabase.open_database( - self.couch_url + '/' + dbname, - create=True, - ensure_ddocs=True) - return db, db._replica_uid - -CouchServerState.ensure_database = _couch_ensure_database - - class ServerAuthorizationTestCase(BaseSoledadTest): """ -- cgit v1.2.3 From 6427b73247e327c27d5f8e5c2036281fb77205b2 Mon Sep 17 00:00:00 2001 From: Gislene Pereira Date: Mon, 28 Sep 2015 16:04:46 -0300 Subject: [test] Making test_command pass on Mac OS X. --- common/src/leap/soledad/common/tests/test_command.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'common') diff --git a/common/src/leap/soledad/common/tests/test_command.py b/common/src/leap/soledad/common/tests/test_command.py index 420f91ae..c386bdd2 100644 --- a/common/src/leap/soledad/common/tests/test_command.py +++ b/common/src/leap/soledad/common/tests/test_command.py @@ -49,7 +49,5 @@ class ExecuteValidatedCommandTest(unittest.TestCase): def test_return_status_code_number_on_failure(self): status, out = exec_validated_cmd("ls", "user-bebacafe") - self.assertEquals(status, 2) - self.assertIn( - 'ls: cannot access user-bebacafe: No such file or directory\n', - out) + self.assertNotEquals(status, 0) + self.assertIn('No such file or directory\n', out) -- cgit v1.2.3