fix: remove unsafe STFP options for rclone (#964)

Closes #963.

Also, some minor cleanup for the rclone options.

Author: leafty

components/renku_data_services/storage/rclone.py

@@ -11,7 +11,7 @@
 
 from renku_data_services import errors
 from renku_data_services.app_config import logging
-from renku_data_services.storage.rclone_patches import BANNED_STORAGE, apply_patches
+from renku_data_services.storage.rclone_patches import BANNED_SFTP_OPTIONS, BANNED_STORAGE, apply_patches
 
 logger = logging.getLogger(__name__)
 
@@ -345,6 +345,14 @@ def get_option_for_provider(self, name: str, provider: str | None) -> RCloneOpti
 
         return None
 
+    def check_unsafe_option(self, name: str) -> None:
+        """Check that the option is safe."""
+        if self.prefix != "sftp":
+            return None
+        if name in BANNED_SFTP_OPTIONS:
+            raise errors.ValidationError(message=f"The {name} option is not allowed.")
+        return None
+
     def validate_config(
         self, configuration: Union["RCloneConfig", dict[str, Any]], keep_sensitive: bool = False
     ) -> None:
@@ -369,6 +377,8 @@ def validate_config(
             raise errors.ValidationError(message=f"The following fields are required but missing:\n{missing_str}")
 
         for key in keys:
+            self.check_unsafe_option(key)
+
             value = configuration[key]
 
             option: RCloneOption | None = self.get_option_for_provider(key, provider)

components/renku_data_services/storage/rclone_patches.py

@@ -19,7 +19,6 @@
 }
 
 OAUTH_PROVIDERS: Final[set[str]] = {
-    "acd",
     "box",
     "drive",
     "dropbox",
@@ -31,13 +30,20 @@
     "onedrive",
     "pcloud",
     "pikpak",
-    "premiumzeme",
+    "premiumizeme",
     "putio",
     "sharefile",
     "yandex",
     "zoho",
 }
 
+BANNED_SFTP_OPTIONS: Final[set[str]] = {
+    "key_file",  # path to a local file
+    "pubkey_file",  # path to a local file
+    "known_hosts_file",  # path to a local file
+    "ssh",  # arbitrary command to be executed
+}
+
 
 def find_storage(spec: list[dict[str, Any]], prefix: str) -> dict[str, Any]:
     """Find and return the storage schema from the spec.
@@ -241,6 +247,16 @@ def __patch_switchdrive_storage(spec: list[dict[str, Any]]) -> None:
     )
 
 
+def __patch_schema_remove_banned_sftp_options(spec: list[dict[str, Any]]) -> None:
+    """Remove unsafe SFTP options."""
+    sftp = find_storage(spec, "sftp")
+    options = []
+    for option in sftp["Options"]:
+        if option["Name"] not in BANNED_SFTP_OPTIONS:
+            options.append(option)
+    sftp["Options"] = options
+
+
 def apply_patches(spec: list[dict[str, Any]]) -> None:
     """Apply patches to RClone schema."""
     patches = [
@@ -251,6 +267,7 @@ def apply_patches(spec: list[dict[str, Any]]) -> None:
         __patch_schema_remove_oauth_propeties,
         __patch_polybox_storage,
         __patch_switchdrive_storage,
+        __patch_schema_remove_banned_sftp_options,
     ]
 
     for patch in patches:

test/bases/renku_data_services/data_api/__snapshots__/test_storage.ambr

@@ -9029,38 +9029,6 @@
       'description': 'premiumize.me',
       'name': 'premiumizeme',
       'options': list([
-        dict({
-          'advanced': False,
-          'default': '',
-          'default_str': '',
-          'exclusive': False,
-          'help': '''
-            OAuth Client Id.
-            
-            Leave blank normally.
-          ''',
-          'ispassword': False,
-          'name': 'client_id',
-          'required': False,
-          'sensitive': True,
-          'type': 'string',
-        }),
-        dict({
-          'advanced': False,
-          'default': '',
-          'default_str': '',
-          'exclusive': False,
-          'help': '''
-            OAuth Client Secret.
-            
-            Leave blank normally.
-          ''',
-          'ispassword': False,
-          'name': 'client_secret',
-          'required': False,
-          'sensitive': True,
-          'type': 'string',
-        }),
         dict({
           'advanced': True,
           'default': '',
@@ -14699,24 +14667,6 @@
           'sensitive': True,
           'type': 'string',
         }),
-        dict({
-          'advanced': False,
-          'default': '',
-          'default_str': '',
-          'exclusive': False,
-          'help': '''
-            Path to PEM-encoded private key file.
-            
-            Leave blank or set key-use-agent to use ssh-agent.
-            
-            Leading `~` will be expanded in the file name as will environment variables such as `${RCLONE_CONFIG_DIR}`.
-          ''',
-          'ispassword': False,
-          'name': 'key_file',
-          'required': False,
-          'sensitive': False,
-          'type': 'string',
-        }),
         dict({
           'advanced': False,
           'default': '',
@@ -14750,48 +14700,6 @@
           'sensitive': False,
           'type': 'string',
         }),
-        dict({
-          'advanced': False,
-          'default': '',
-          'default_str': '',
-          'exclusive': False,
-          'help': '''
-            Optional path to public key file.
-            
-            Set this if you have a signed certificate you want to use for authentication.
-            
-            Leading `~` will be expanded in the file name as will environment variables such as `${RCLONE_CONFIG_DIR}`.
-          ''',
-          'ispassword': False,
-          'name': 'pubkey_file',
-          'required': False,
-          'sensitive': False,
-          'type': 'string',
-        }),
-        dict({
-          'advanced': True,
-          'default': '',
-          'default_str': '',
-          'examples': list([
-            dict({
-              'help': "Use OpenSSH's known_hosts file.",
-              'value': '~/.ssh/known_hosts',
-            }),
-          ]),
-          'exclusive': False,
-          'help': '''
-            Optional path to known_hosts file.
-            
-            Set this value to enable server host key validation.
-            
-            Leading `~` will be expanded in the file name as will environment variables such as `${RCLONE_CONFIG_DIR}`.
-          ''',
-          'ispassword': False,
-          'name': 'known_hosts_file',
-          'required': False,
-          'sensitive': False,
-          'type': 'string',
-        }),
         dict({
           'advanced': False,
           'default': False,
@@ -15344,46 +15252,6 @@
           'sensitive': False,
           'type': 'SpaceSepList',
         }),
-        dict({
-          'advanced': False,
-          'default': list([
-          ]),
-          'default_str': '',
-          'exclusive': False,
-          'help': '''
-            Path and arguments to external ssh binary.
-            
-            Normally rclone will use its internal ssh library to connect to the
-            SFTP server. However it does not implement all possible ssh options so
-            it may be desirable to use an external ssh binary.
-            
-            Rclone ignores all the internal config if you use this option and
-            expects you to configure the ssh binary with the user/host/port and
-            any other options you need.
-            
-            **Important** The ssh command must log in without asking for a
-            password so needs to be configured with keys or certificates.
-            
-            Rclone will run the command supplied either with the additional
-            arguments "-s sftp" to access the SFTP subsystem or with commands such
-            as "md5sum /path/to/file" appended to read checksums.
-            
-            Any arguments with spaces in should be surrounded by "double quotes".
-            
-            An example setting might be:
-            
-                ssh -o ServerAliveInterval=20 user@example.com
-            
-            Note that when using an external ssh binary rclone makes a new ssh
-            connection for every hash it calculates.
-  
-          ''',
-          'ispassword': False,
-          'name': 'ssh',
-          'required': False,
-          'sensitive': False,
-          'type': 'SpaceSepList',
-        }),
         dict({
           'advanced': True,
           'default': '',

test/bases/renku_data_services/data_api/test_storage.py

@@ -12,7 +12,7 @@
 from renku_data_services.data_api.dependencies import DependencyManager
 from renku_data_services.migrations.core import run_migrations_for_app
 from renku_data_services.storage.rclone import RCloneValidator
-from renku_data_services.storage.rclone_patches import BANNED_STORAGE, OAUTH_PROVIDERS
+from renku_data_services.storage.rclone_patches import BANNED_SFTP_OPTIONS, BANNED_STORAGE, OAUTH_PROVIDERS
 from test.utils import SanicReusableASGITestClient
 
 _valid_storage: dict[str, Any] = {
@@ -249,6 +249,21 @@ async def storage_test_client(
             422,
             "",
         ),
+        (
+            {
+                "project_id": "123456",
+                "name": "mystorage",
+                "configuration": {
+                    "type": "sftp",
+                    "host": "myhost",
+                    "ssh": "ssh",  # passing in banned option
+                },
+                "source_path": "bucket/myfolder",
+                "target_path": "my/target",
+            },
+            422,
+            "",
+        ),
     ],
 )
 @pytest.mark.asyncio
@@ -508,6 +523,39 @@ async def test_storage_patch_unauthorized(storage_test_client, valid_storage_pay
     assert res.status_code == 403, res.text
 
 
+@pytest.mark.asyncio
+async def test_storage_patch_banned_option(storage_test_client, valid_storage_payload) -> None:
+    storage_test_client, _ = storage_test_client
+    # NOTE: The keycloak dummy client used to authorize the storage patch requests only has info
+    # on a user with name Admin Doe, using a different user will fail with a 401 error.
+    access_token = json.dumps({"is_admin": False, "id": "some-id", "full_name": "Admin Doe"})
+    payload = dict(valid_storage_payload)
+    payload["configuration"] = {
+        "type": "sftp",
+        "host": "myhost",
+    }
+    _, res = await storage_test_client.post(
+        "/api/data/storage",
+        headers={"Authorization": f"bearer {access_token}"},
+        data=json.dumps(payload),
+    )
+    assert res.status_code == 201
+    assert res.json["storage"]["storage_type"] == "sftp"
+    storage_id = res.json["storage"]["storage_id"]
+
+    _, res = await storage_test_client.patch(
+        f"/api/data/storage/{storage_id}",
+        headers={"Authorization": f"bearer {access_token}"},
+        data=json.dumps(
+            {
+                "configuration": {"key_file": "my_key"},
+            }
+        ),
+    )
+    assert res.status_code == 422
+    assert "key_file option is not allowed" in res.text
+
+
 @pytest.mark.asyncio
 async def test_storage_obscure(storage_test_client) -> None:
     storage_test_client, _ = storage_test_client
@@ -632,9 +680,20 @@ async def test_storage_schema_patches(storage_test_client, snapshot) -> None:
     oauth_providers = [s for s in schema if s["prefix"] in OAUTH_PROVIDERS]
     assert all(o["name"] != "client_id" and o["name"] != "client_secret" for p in oauth_providers for o in p["options"])
 
+    # check the OAUTH_PROVIDERS list
+    not_exists = set(p for p in OAUTH_PROVIDERS if p not in set(s["prefix"] for s in schema))
+    assert not_exists == set()
+
     # check custom webdav storage is added
     assert any(s["prefix"] == "polybox" for s in schema)
     assert any(s["prefix"] == "switchDrive" for s in schema)
+
+    # check that unsafe SFTP options are removed
+    sftp = next((e for e in schema if e["prefix"] == "sftp"), None)
+    assert sftp
+    assert all(o["name"] not in BANNED_SFTP_OPTIONS for o in sftp["options"])
+
+    # snapshot the schema
     assert schema == snapshot