From 02ec57fc4915fc611cbf0f1df1a939142ea3a1bf Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Tue, 7 Nov 2023 15:23:34 +0200 Subject: [PATCH 1/9] chmod the storage to 600 after opening it. --- ops/storage.py | 3 +++ test/test_storage.py | 11 +++++++++++ 2 files changed, 14 insertions(+) diff --git a/ops/storage.py b/ops/storage.py index 5dda21d0e..ae2c3a76e 100755 --- a/ops/storage.py +++ b/ops/storage.py @@ -18,6 +18,7 @@ import pickle import shutil import sqlite3 +import stat import subprocess from datetime import timedelta from pathlib import Path @@ -62,6 +63,8 @@ def __init__(self, filename: Union['Path', str]): self._db = sqlite3.connect(str(filename), isolation_level=None, timeout=self.DB_LOCK_TIMEOUT.total_seconds()) + if filename != ":memory:": + os.chmod(filename, stat.S_IRUSR | stat.S_IWUSR) self._setup() def _setup(self): diff --git a/test/test_storage.py b/test/test_storage.py index 2d58b7532..f0e0b2ccf 100755 --- a/test/test_storage.py +++ b/test/test_storage.py @@ -18,6 +18,7 @@ import os import pathlib import sys +import stat import tempfile import typing import unittest @@ -218,6 +219,16 @@ class TestSQLiteStorage(StoragePermutations, BaseTestCase): def create_storage(self): return ops.storage.SQLiteStorage(':memory:') + def test_permissions(self): + fd, filename = tempfile.mkstemp() + try: + os.close(fd) + os.remove(filename) + storage = ops.storage.SQLiteStorage(filename) + self.assertEqual(stat.S_IMODE(os.stat(filename).st_mode), stat.S_IREAD | stat.S_IWRITE) + storage.close() + finally: + os.remove(filename) def setup_juju_backend(test_case: unittest.TestCase, state_file: pathlib.Path): """Create fake scripts for pretending to be state-set and state-get.""" From 46f942fa3629d997a110e20de5bb09c423cfcc71 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Tue, 7 Nov 2023 15:29:18 +0200 Subject: [PATCH 2/9] Use consistent names. --- test/test_storage.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_storage.py b/test/test_storage.py index f0e0b2ccf..6c6c31295 100755 --- a/test/test_storage.py +++ b/test/test_storage.py @@ -225,7 +225,7 @@ def test_permissions(self): os.close(fd) os.remove(filename) storage = ops.storage.SQLiteStorage(filename) - self.assertEqual(stat.S_IMODE(os.stat(filename).st_mode), stat.S_IREAD | stat.S_IWRITE) + self.assertEqual(stat.S_IMODE(os.stat(filename).st_mode), stat.S_IRUSR | stat.S_IWUSR) storage.close() finally: os.remove(filename) From 9f72641ed2249916e4a54d65c5d53543fa19bc30 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Tue, 7 Nov 2023 15:30:16 +0200 Subject: [PATCH 3/9] Fix style. --- test/test_storage.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/test_storage.py b/test/test_storage.py index 6c6c31295..8b4e29dce 100755 --- a/test/test_storage.py +++ b/test/test_storage.py @@ -17,8 +17,8 @@ import io import os import pathlib -import sys import stat +import sys import tempfile import typing import unittest @@ -230,6 +230,7 @@ def test_permissions(self): finally: os.remove(filename) + def setup_juju_backend(test_case: unittest.TestCase, state_file: pathlib.Path): """Create fake scripts for pretending to be state-set and state-get.""" template_args = { From d00969eb9509fca3fb8ce6ce5e7f8d120350b7c2 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Fri, 10 Nov 2023 10:27:46 +0200 Subject: [PATCH 4/9] Use TemporaryDirectory rather than mkstemp. --- test/test_storage.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/test/test_storage.py b/test/test_storage.py index 8b4e29dce..fb0b0f2a3 100755 --- a/test/test_storage.py +++ b/test/test_storage.py @@ -220,15 +220,11 @@ def create_storage(self): return ops.storage.SQLiteStorage(':memory:') def test_permissions(self): - fd, filename = tempfile.mkstemp() - try: - os.close(fd) - os.remove(filename) + with tempfile.TemporaryDirectory() as temp_dir: + filename = os.path.join(temp_dir, "framework.data") storage = ops.storage.SQLiteStorage(filename) self.assertEqual(stat.S_IMODE(os.stat(filename).st_mode), stat.S_IRUSR | stat.S_IWUSR) storage.close() - finally: - os.remove(filename) def setup_juju_backend(test_case: unittest.TestCase, state_file: pathlib.Path): From 37004f1d7c3eb629ef3749e328e2592ca36b7e9b Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 15 Nov 2023 11:53:40 +1300 Subject: [PATCH 5/9] Avoid race conditions fixing the permissions. --- ops/storage.py | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/ops/storage.py b/ops/storage.py index ae2c3a76e..5a13158c9 100644 --- a/ops/storage.py +++ b/ops/storage.py @@ -60,13 +60,30 @@ def __init__(self, filename: Union['Path', str]): # sqlite3.connect creates the file silently if it does not exist logger.debug(f"Initializing SQLite local storage: {filename}.") + if filename != ":memory:": + self._ensure_db_permissions(filename) self._db = sqlite3.connect(str(filename), isolation_level=None, timeout=self.DB_LOCK_TIMEOUT.total_seconds()) - if filename != ":memory:": - os.chmod(filename, stat.S_IRUSR | stat.S_IWUSR) self._setup() + def _ensure_db_permissions(self, filename: Union[Path, str]): + """Make sure that the DB file has appropriately secure permissions.""" + mode = stat.S_IRUSR | stat.S_IWUSR + filepath = Path(filename) + if filepath.exists(): + try: + os.chmod(filename, mode) + except OSError as e: + logger.warning(f"Unable to adjust permissions of storage file {filename}: {e}") + return + try: + fd = os.open(filename, os.O_CREAT | os.O_EXCL, mode=mode) + except OSError as e: + logger.warning(f"Unable to ensure permissions of storage file {filename}: {e}") + else: + os.close(fd) + def _setup(self): """Make the database ready to be used as storage.""" # Make sure that the database is locked until the connection is closed, From b9af52bcd88c64388dc2422da5c83fdfba48d573 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 15 Nov 2023 12:09:04 +1300 Subject: [PATCH 6/9] Avoid converting to a Path just for exists. --- ops/storage.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/ops/storage.py b/ops/storage.py index 5a13158c9..2cfb1d94a 100644 --- a/ops/storage.py +++ b/ops/storage.py @@ -61,26 +61,25 @@ def __init__(self, filename: Union['Path', str]): logger.debug(f"Initializing SQLite local storage: {filename}.") if filename != ":memory:": - self._ensure_db_permissions(filename) + self._ensure_db_permissions(str(filename)) self._db = sqlite3.connect(str(filename), isolation_level=None, timeout=self.DB_LOCK_TIMEOUT.total_seconds()) self._setup() - def _ensure_db_permissions(self, filename: Union[Path, str]): + def _ensure_db_permissions(self, filename: str): """Make sure that the DB file has appropriately secure permissions.""" mode = stat.S_IRUSR | stat.S_IWUSR - filepath = Path(filename) - if filepath.exists(): + if os.path.exists(filename): try: os.chmod(filename, mode) except OSError as e: - logger.warning(f"Unable to adjust permissions of storage file {filename}: {e}") + logger.warning(f"Unable to adjust permissions of storage file {filename!r}: {e}") return try: fd = os.open(filename, os.O_CREAT | os.O_EXCL, mode=mode) except OSError as e: - logger.warning(f"Unable to ensure permissions of storage file {filename}: {e}") + logger.warning(f"Unable to ensure permissions of storage file {filename!r}: {e}") else: os.close(fd) From d5ca418ba4999d1a686333eb7a5debdd1a2a3215 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 16 Nov 2023 11:10:08 +1300 Subject: [PATCH 7/9] Hard-fail if we cannot properly secure the DB file. --- ops/storage.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ops/storage.py b/ops/storage.py index 2cfb1d94a..787dea02b 100644 --- a/ops/storage.py +++ b/ops/storage.py @@ -74,12 +74,13 @@ def _ensure_db_permissions(self, filename: str): try: os.chmod(filename, mode) except OSError as e: - logger.warning(f"Unable to adjust permissions of storage file {filename!r}: {e}") + raise RuntimeError(f"Unable to adjust access permission of {filename!r}") from e return + try: fd = os.open(filename, os.O_CREAT | os.O_EXCL, mode=mode) except OSError as e: - logger.warning(f"Unable to ensure permissions of storage file {filename!r}: {e}") + raise RuntimeError(f"Unable to adjust access permission of {filename!r}") from e else: os.close(fd) From 76d62c310c7ca01b223f6bf3f31552b5c540fa18 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 16 Nov 2023 11:11:10 +1300 Subject: [PATCH 8/9] Slightly simplify code. --- ops/storage.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ops/storage.py b/ops/storage.py index 787dea02b..9f259f452 100644 --- a/ops/storage.py +++ b/ops/storage.py @@ -81,8 +81,7 @@ def _ensure_db_permissions(self, filename: str): fd = os.open(filename, os.O_CREAT | os.O_EXCL, mode=mode) except OSError as e: raise RuntimeError(f"Unable to adjust access permission of {filename!r}") from e - else: - os.close(fd) + os.close(fd) def _setup(self): """Make the database ready to be used as storage.""" From 104a88b7efde4549f1c6cd32da16b3619e59afce Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 16 Nov 2023 11:29:20 +1300 Subject: [PATCH 9/9] Extend the tests to cover additional scenarios. --- test/test_storage.py | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/test/test_storage.py b/test/test_storage.py index fb0b0f2a3..b8467c8bb 100644 --- a/test/test_storage.py +++ b/test/test_storage.py @@ -22,6 +22,7 @@ import tempfile import typing import unittest +import unittest.mock from test.test_helpers import BaseTestCase, fake_script, fake_script_calls from textwrap import dedent @@ -219,13 +220,41 @@ class TestSQLiteStorage(StoragePermutations, BaseTestCase): def create_storage(self): return ops.storage.SQLiteStorage(':memory:') - def test_permissions(self): + def test_permissions_new(self): with tempfile.TemporaryDirectory() as temp_dir: - filename = os.path.join(temp_dir, "framework.data") + filename = os.path.join(temp_dir, ".unit-state.db") storage = ops.storage.SQLiteStorage(filename) self.assertEqual(stat.S_IMODE(os.stat(filename).st_mode), stat.S_IRUSR | stat.S_IWUSR) storage.close() + def test_permissions_existing(self): + with tempfile.TemporaryDirectory() as temp_dir: + filename = os.path.join(temp_dir, ".unit-state.db") + ops.storage.SQLiteStorage(filename).close() + # Set the file to access that will need fixing for user, group, and other. + os.chmod(filename, 0o744) + storage = ops.storage.SQLiteStorage(filename) + self.assertEqual(stat.S_IMODE(os.stat(filename).st_mode), stat.S_IRUSR | stat.S_IWUSR) + storage.close() + + @unittest.mock.patch("os.path.exists") + def test_permissions_race(self, exists: unittest.mock.MagicMock): + exists.return_value = False + with tempfile.TemporaryDirectory() as temp_dir: + filename = os.path.join(temp_dir, ".unit-state.db") + # Create an existing file, but the mock will simulate a race condition saying that it + # does not exist. + open(filename, "w").close() + self.assertRaises(RuntimeError, ops.storage.SQLiteStorage, filename) + + @unittest.mock.patch("os.chmod") + def test_permissions_failure(self, chmod: unittest.mock.MagicMock): + chmod.side_effect = OSError + with tempfile.TemporaryDirectory() as temp_dir: + filename = os.path.join(temp_dir, ".unit-state.db") + open(filename, "w").close() + self.assertRaises(RuntimeError, ops.storage.SQLiteStorage, filename) + def setup_juju_backend(test_case: unittest.TestCase, state_file: pathlib.Path): """Create fake scripts for pretending to be state-set and state-get."""