Fix sslib new versions incompatibility bug (#159)

* Fix sslib new versions incompatibility bug

The incompatibility with newer securesystemslib versions was caused
because of a new breaking change introduced in:
secure-systems-lab/securesystemslib#231

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

* LocalStorage.put use restrict by default

We want to use the "restrict" by default as we are the ones who will
save metadata and target files through LocalStorage.put and we want
to have as least privileges as possible because of security.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

* Isort lint fix

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

* Update securesystemslib to 0.26.0

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

* Make "restrict" true by default for IStorage.put()

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

* Remove StorageBackendInterface dependency

Simplify IStorage by not inheriting StorageBackendInterface anymore.
In the last securesystemslib version, the contributors there added
a lot of functionality inside the StorageBackendInterface which we
don't use and is irrelevant to us.
Additionally, we had at least two cases where our tests failed because
of StorageBackendInterface API changes and we were blocked to use an
older securesystemslib version or update IStorage accordingly.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

* Make IStorage an interface again

By mistake I have made IStorage a regular class as opposed to an
interface which is its purpose.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

* Move write file logic of persist into IStorage.put

Move the logic where we actually save the file content into the
IStorage.put API call as this will help us easily support more
storages within persist() call.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

* Remove unused imports

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Pipfile

@@ -12,7 +12,7 @@ redis = "*"
 tuf = "==2.0.0"
 dynaconf = {extras = ["ini"], version = "*"}
 supervisor = "*"
-securesystemslib = "==0.23.0"
+securesystemslib = "*"
 sqlalchemy = "*"
 psycopg2 = "*"
 alembic = "*"
@@ -63,7 +63,7 @@ pyparsing = "==3.0.9"
 pytz = "==2022.2.1"
 redis = "==4.3.4"
 requests = "==2.28.1"
-securesystemslib = "==0.23.0"
+securesystemslib = "==0.25.0"
 six = "==1.16.0"
 toml = "==0.10.2"
 tomli = "==2.0.1"

repository_service_tuf_worker/interfaces.py

@@ -16,14 +16,9 @@
 
 from abc import ABC, abstractmethod
 from dataclasses import dataclass
-from io import TextIOBase
-from typing import Any, Dict, List
+from typing import Any, Dict, List, Optional
 
-from tuf.api.metadata import (  # type: ignore
-    Metadata,
-    StorageBackendInterface,
-    T,
-)
+from tuf.api.metadata import Metadata, T
 
 
 @dataclass
@@ -65,7 +60,7 @@ def put(self, file_object: str, filename: str) -> None:
         pass  # pragma: no cover
 
 
-class IStorage(StorageBackendInterface):
+class IStorage(ABC):
     @classmethod
     @abstractmethod
     def configure(cls, settings: Any):
@@ -90,8 +85,13 @@ def get(self, rolename: str, version: int) -> "Metadata[T]":
         raise NotImplementedError  # pragma: no cover
 
     @abstractmethod
-    def put(self, file_object: TextIOBase, filename: str) -> None:
+    def put(
+        self,
+        file_data: bytes,
+        filename: str,
+        restrict: Optional[bool] = True,
+    ) -> None:
         """
-        Stores file object with a specific filename.
+        Stores file bytes within a file with a specific filename.
         """
         raise NotImplementedError  # pragma: no cover

repository_service_tuf_worker/repository.py

@@ -29,16 +29,10 @@
 from securesystemslib.signer import SSlibSigner  # type: ignore
 from tuf.api.metadata import (  # noqa
     SPECIFICATION_VERSION,
-    TOP_LEVEL_ROLE_NAMES,
-    DelegatedRole,
-    Delegations,
-    Key,
     Metadata,
     MetaFile,
-    Role,
     Root,
     Snapshot,
-    SuccinctRoles,
     TargetFile,
     Targets,
     Timestamp,
@@ -244,7 +238,8 @@ def _persist(self, role: Metadata, role_name: str) -> str:
             if filename[0].isdigit() is False:
                 filename = f"{role.signed.version}.{filename}"
 
-        role.to_file(filename, JSONSerializer(), self._storage_backend)
+        bytes_data = role.to_bytes(JSONSerializer())
+        self._storage_backend.put(bytes_data, filename)
 
         return filename
 

repository_service_tuf_worker/services/storage/local.py

@@ -4,10 +4,10 @@
 
 import glob
 import os
-import shutil
+import stat
 from contextlib import contextmanager
-from io import BufferedReader, TextIOBase
-from typing import List
+from io import BufferedReader
+from typing import List, Optional
 
 from securesystemslib.exceptions import StorageError  # noqa
 
@@ -69,18 +69,35 @@ def get(self, role, version=None) -> BufferedReader:
             if file_object is not None:
                 file_object.close()
 
-    def put(self, file_object: TextIOBase, filename: str) -> None:
+    def put(
+        self,
+        file_data: bytes,
+        filename: str,
+        restrict: Optional[bool] = True,
+    ) -> None:
         """
         Writes passed file object to configured TUF repo path using the passed
         filename.
         """
-        file_path = os.path.join(self._path, filename)
-        if not file_object.closed:
-            file_object.seek(0)
+        filename = os.path.join(self._path, filename)
+
+        if restrict:
+            # On UNIX-based systems restricted files are created with read and
+            # write permissions for the user only (octal value 0o600).
+            fd = os.open(
+                filename, os.O_WRONLY | os.O_CREAT, stat.S_IRUSR | stat.S_IWUSR
+            )
+        else:
+            # Non-restricted files use the default 'mode' argument of os.open()
+            # granting read, write, and execute for all users (mode 0o777).
+            # NOTE: mode may be modified by the user's file mode creation mask
+            # (umask) or on Windows limited to the smaller set of OS supported
+            # permisssions.
+            fd = os.open(filename, os.O_WRONLY | os.O_CREAT)
 
         try:
-            with open(file_path, "wb") as destination_file:
-                shutil.copyfileobj(file_object, destination_file)
+            with os.fdopen(fd, "wb") as destination_file:
+                destination_file.write(file_data)
                 destination_file.flush()
                 os.fsync(destination_file.fileno())
         except OSError:

requirements-dev.txt

@@ -59,7 +59,7 @@ pytz==2022.7.1
 pyyaml==6.0 ; python_version >= '3.6'
 redis==4.4.2
 requests==2.28.2 ; python_version >= '3.7' and python_version < '4'
-securesystemslib==0.23.0
+securesystemslib==0.26.0
 setuptools==66.1.1 ; python_version >= '3.7'
 six==1.16.0 ; python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'
 snowballstemmer==2.2.0
@@ -89,4 +89,4 @@ mako==1.2.4 ; python_version >= '3.7'
 psycopg2==2.9.5
 pydantic==1.10.4
 sqlalchemy==1.4.46
-supervisor==4.2.5
+supervisor==4.2.5

requirements.txt

@@ -27,7 +27,7 @@ pynacl==1.5.0
 pytz==2022.7.1
 redis==4.4.2
 requests==2.28.2 ; python_version >= '3.7' and python_version < '4'
-securesystemslib==0.23.0
+securesystemslib==0.26.0
 setuptools==66.1.1 ; python_version >= '3.7'
 six==1.16.0 ; python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'
 sqlalchemy==1.4.46
@@ -37,4 +37,4 @@ typing-extensions==4.4.0 ; python_version >= '3.7'
 urllib3==1.26.14 ; python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3, 3.4, 3.5'
 vine==5.0.0 ; python_version >= '3.6'
 watchdog==2.2.1
-wcwidth==0.2.6
+wcwidth==0.2.6

tests/unit/tuf_repository_service_worker/services/storage/test_local.py

@@ -2,6 +2,8 @@
 #
 # SPDX-License-Identifier: MIT
 
+import os
+
 import pretend
 import pytest
 
@@ -170,12 +172,18 @@ def test_get_OSError(self, monkeypatch):
         ]
         assert local.glob.glob.calls == [pretend.call("/path/2.root.json")]
 
-    def test_put(self, monkeypatch):
-        service = local.LocalStorage("/path")
+    def _put_setup(self, fake_destination_file, restrict=True):
+        """Setup helper for all required functions in LocalStorage.put()"""
 
-        local.shutil = pretend.stub(
-            copyfileobj=pretend.call_recorder(lambda *a: None)
-        )
+        class FakeDestinationFile:
+            def __init__(self):
+                pass
+
+            def __enter__(self):
+                return fake_destination_file
+
+            def __exit__(self, type, value, traceback):
+                pass
 
         local.os = pretend.stub(
             path=pretend.stub(
@@ -184,40 +192,68 @@ def test_put(self, monkeypatch):
                 )
             ),
             fsync=pretend.call_recorder(lambda *a: None),
+            O_WRONLY=0,
+            O_CREAT=0,
+            open=pretend.call_recorder(lambda *a: 0),
+            fdopen=pretend.call_recorder(lambda *a: FakeDestinationFile()),
         )
 
-        fake_file_object = pretend.stub(
-            closed=False,
-            seek=pretend.call_recorder(lambda *a: None),
-        )
+        if restrict:
+            local.stat = pretend.stub(S_IRUSR=0, S_IWUSR=0)
+
+    def test_put(self):
+        service = local.LocalStorage("/path")
 
         fake_destination_file = pretend.stub(
+            write=pretend.call_recorder(lambda *a: None),
             flush=pretend.call_recorder(lambda: None),
             fileno=pretend.call_recorder(lambda: "fileno"),
         )
 
-        class FakeDestinationFile:
-            def __init__(self, file, mode):
-                return None
+        fake_bytes = b"data"
 
-            def __enter__(self):
-                return fake_destination_file
+        self._put_setup(fake_destination_file)
+        result = service.put(fake_bytes, "3.bin-e.json")
 
-            def __exit__(self, type, value, traceback):
-                pass
+        assert result is None
+        assert local.os.path.join.calls == [
+            pretend.call(service._path, "3.bin-e.json"),
+        ]
+        expected_file_path = os.path.join(service._path, "3.bin-e.json")
+        assert local.os.open.calls == [pretend.call(expected_file_path, 0, 0)]
+        assert local.os.fdopen.calls == [pretend.call(0, "wb")]
+        assert fake_destination_file.write.calls == [pretend.call(fake_bytes)]
+        assert fake_destination_file.flush.calls == [pretend.call()]
+        assert fake_destination_file.fileno.calls == [pretend.call()]
+        assert local.os.fsync.calls == [pretend.call("fileno")]
+
+    def test_put_without_restrict(self):
+        service = local.LocalStorage("/path")
+
+        fake_destination_file = pretend.stub(
+            write=pretend.call_recorder(lambda *a: None),
+            flush=pretend.call_recorder(lambda: None),
+            fileno=pretend.call_recorder(lambda: "fileno"),
+        )
 
-        monkeypatch.setitem(local.__builtins__, "open", FakeDestinationFile)
+        fake_bytes = b"data"
 
-        result = service.put(fake_file_object, "3.bin-e.json")
+        self._put_setup(fake_destination_file, False)
+        result = service.put(fake_bytes, "3.bin-e.json", False)
 
         assert result is None
-        assert fake_file_object.seek.calls == [pretend.call(0)]
         assert local.os.path.join.calls == [
             pretend.call(service._path, "3.bin-e.json"),
         ]
+        expected_file_path = os.path.join(service._path, "3.bin-e.json")
+        assert local.os.open.calls == [pretend.call(expected_file_path, 0)]
+        assert local.os.fdopen.calls == [pretend.call(0, "wb")]
+        assert fake_destination_file.write.calls == [pretend.call(fake_bytes)]
+        assert fake_destination_file.flush.calls == [pretend.call()]
+        assert fake_destination_file.fileno.calls == [pretend.call()]
         assert local.os.fsync.calls == [pretend.call("fileno")]
 
-    def test_put_OSError(self, monkeypatch):
+    def test_put_OSError(self):
         service = local.LocalStorage("/path")
 
         local.os = pretend.stub(
@@ -226,23 +262,18 @@ def test_put_OSError(self, monkeypatch):
                     lambda *a, **kw: "/path/3.bin-e.json"
                 )
             ),
+            open=pretend.raiser(PermissionError("don't want this message")),
+            O_WRONLY=0,
+            O_CREAT=0,
         )
 
-        fake_file_object = pretend.stub(
-            closed=False,
-            seek=pretend.call_recorder(lambda *a: None),
-        )
-
-        monkeypatch.setitem(
-            local.__builtins__,
-            "open",
-            pretend.raiser(PermissionError("don't want this message.")),
-        )
+        local.stat = pretend.stub(S_IRUSR=0, S_IWUSR=0)
+        fake_bytes = b"data"
 
-        with pytest.raises(local.StorageError) as err:
-            service.put(fake_file_object, "3.bin-e.json")
+        with pytest.raises(OSError) as err:
+            service.put(fake_bytes, "3.bin-e.json")
 
-        assert "Can't write role file '3.bin-e.json'" in str(err)
+        assert "don't want this message" in str(err)
         assert local.os.path.join.calls == [
             pretend.call(service._path, "3.bin-e.json"),
         ]