Merge pull request #66 from machow/feat-read-insecure-flag

feat!: require user to opt-in to read unsafe pins

Author: machow

docs/getting_started.Rmd

@@ -130,6 +130,9 @@ While we’ll do our best to keep the automatically generated metadata consisten
 ## 🚧 Versioning
 
 
+> ⚠️: Warning the examples in this section use joblib to read and write data. Joblib uses the pickle format, and **pickle files are not secure**. Only read pickle files you trust. In order to read pickle files, set the `allow_pickle_read=True` argument. See: https://docs.python.org/3/library/pickle.html.
+
+
 > ⚠️: versioning is not yet implemented. These docs are copied from R pins.
 
 In many situations it's useful to version pins, so that writing to an existing pin does not replace the existing data, but instead adds a new copy.
@@ -154,9 +157,8 @@ The primary exception is `board_folder()` since that stores data on your compute
 Once you have turned versioning on, every `pin_write()` will create a new version:
 
 ```{python}
-# TODO: can save lists using joblib, but should warn about security
+board2 = board_temp(versioned = True, allow_pickle_read=True)
 
-board2 = board_temp(versioned = True)
 board2.pin_write([1,2,3,4,5], name = "x", type = "joblib", title="TODO")
 board2.pin_write([1,2,3], name = "x", type = "joblib", title="TODO")
 board2.pin_write([1,2], name = "x", type = "joblib", title="TODO")

pins/boards.py

@@ -50,6 +50,7 @@ def __init__(
         fs: IFileSystem,
         versioned=True,
         meta_factory=MetaFactory(),
+        allow_pickle_read: "bool | None" = None,
     ):
         self.board = str(board)
         self.fs = fs
@@ -59,6 +60,7 @@ def __init__(
             raise NotImplementedError()
 
         self.versioned = versioned
+        self.allow_pickle_read = allow_pickle_read
 
     def pin_exists(self, name: str) -> bool:
         """Determine if a pin exists.
@@ -201,7 +203,10 @@ def pin_read(self, name, version: Optional[str] = None, hash: Optional[str] = No
         pin_name = self.path_to_pin(name)
 
         return load_data(
-            meta, self.fs, self.construct_path([pin_name, meta.version.version])
+            meta,
+            self.fs,
+            self.construct_path([pin_name, meta.version.version]),
+            allow_pickle_read=self.allow_pickle_read,
         )
 
     def pin_write(
@@ -603,7 +608,9 @@ def pin_download(self, name, version=None, hash=None) -> Sequence[str]:
         meta = self.pin_meta(name, version)
 
         if isinstance(meta, MetaRaw):
-            return load_data(meta, self.fs, None)
+            return load_data(
+                meta, self.fs, None, allow_pickle_read=self.allow_pickle_read
+            )
 
         raise NotImplementedError("TODO: allow download beyond MetaRaw.")
 

pins/config.py

@@ -2,11 +2,30 @@
 import os
 
 PINS_NAME = "pins-py"
+PINS_ENV_DATA_DIR = "PINS_DATA_DIR"
+PINS_ENV_CACHE_DIR = "PINS_CACHE_DIR"
+PINS_ENV_INSECURE_READ = "PINS_ALLOW_PICKLE_READ"
 
 
 def get_data_dir():
-    return os.environ.get("PINS_DATA_DIR", appdirs.user_data_dir(PINS_NAME))
+    return os.environ.get(PINS_ENV_DATA_DIR, appdirs.user_data_dir(PINS_NAME))
 
 
 def get_cache_dir():
-    return os.environ.get("PINS_CACHE_DIR", appdirs.user_cache_dir(PINS_NAME))
+    return os.environ.get(PINS_ENV_CACHE_DIR, appdirs.user_cache_dir(PINS_NAME))
+
+
+def get_allow_pickle_read(flag):
+    if flag is None:
+        env_var = os.environ.get(PINS_ENV_INSECURE_READ, "0")
+        try:
+            env_int = int(env_var)
+        except ValueError:
+            raise ValueError(
+                f"{PINS_ENV_INSECURE_READ} must be '0' or '1', but was set to "
+                f"{repr(env_var)}."
+            )
+
+        flag = bool(env_int)
+
+    return flag

pins/constructors.py

@@ -21,6 +21,7 @@ def board(
     path: str = "",
     versioned: bool = True,
     cache: "DEFAULT | None" = DEFAULT,
+    allow_pickle_read=None,
     storage_options: "dict | None" = None,
     board_factory: "callable | BaseBoard | None" = None,
 ):
@@ -39,6 +40,14 @@ def board(
         Whether to use a cache. By default, pins attempts to select the right cache
         directory, given your filesystem. If None is passed, then no cache will be
         used. You can set the cache using the PINS_CACHE_DIR environment variable.
+    allow_pickle_read: optional, bool
+        Whether to allow reading pins that use the pickle protocol. Pickles are unsafe,
+        and can execute arbitrary code. Only allow reading pickles if you trust the
+        board to be able to execute python code on your computer.
+
+        You can enable reading pickles by setting this to True, or by setting the
+        environment variable PINS_ALLOW_PICKLE_READ. If both are set, this argument
+        takes precedence.
     storage_options:
         Additional options passed to the underlying filesystem created by
         fsspec.filesystem.
@@ -72,27 +81,50 @@ def board(
 
     # construct board ----
 
+    pickle_kwargs = {"allow_pickle_read": allow_pickle_read}
     # TODO: should use a registry or something
     if protocol == "rsc" and board_factory is None:
-        board = BoardRsConnect(path, fs, versioned)
+        board = BoardRsConnect(path, fs, versioned, **pickle_kwargs)
     elif board_factory is not None:
-        board = board_factory(path, fs, versioned)
+        board = board_factory(path, fs, versioned, **pickle_kwargs)
     else:
-        board = BaseBoard(path, fs, versioned)
+        board = BaseBoard(path, fs, versioned, **pickle_kwargs)
     return board
 
 
 # TODO(#31): change file boards to unversioned once implemented
 
 
-def board_folder(path, versioned=True):
-    return board("file", path, versioned, cache=None)
+def board_folder(path: str, versioned=True, allow_pickle_read=None):
+    """Create a pins board inside a folder.
+
+    Parameters
+    ----------
+    path:
+        The folder that will hold the board.
+    **kwargs:
+        Passed to the pins.board function.
+    """
+
+    return board(
+        "file", path, versioned, cache=None, allow_pickle_read=allow_pickle_read
+    )
 
 
-def board_temp(versioned=True):
+def board_temp(versioned=True, allow_pickle_read=None):
+    """Create a pins board in a temporary directory.
+
+    Parameters
+    ----------
+    **kwargs:
+        Passed to the pins.board function.
+    """
+
     tmp_dir = tempfile.TemporaryDirectory()
 
-    board_obj = board("file", tmp_dir.name, versioned, cache=None)
+    board_obj = board(
+        "file", tmp_dir.name, versioned, cache=None, allow_pickle_read=allow_pickle_read
+    )
 
     # TODO: this is necessary to ensure the temporary directory dir persists.
     # without maintaining a reference to it, it could be deleted after this
@@ -102,26 +134,36 @@ def board_temp(versioned=True):
     return board_obj
 
 
-def board_local(versioned=True):
+def board_local(versioned=True, allow_pickle_read=None):
+    """Create a board in a system data directory.
+
+    Parameters
+    ----------
+    **kwargs:
+        Passed to the pins.board function.
+    """
     path = get_data_dir()
 
-    return board("file", path, versioned, cache=None)
+    return board(
+        "file", path, versioned, cache=None, allow_pickle_read=allow_pickle_read
+    )
 
 
-def board_github(org, repo, path="", versioned=True, cache=DEFAULT):
+def board_github(
+    org, repo, path="", versioned=True, cache=DEFAULT, allow_pickle_read=None
+):
     """Returns a github pin board.
 
     Parameters
     ----------
-    path:
-        TODO
-    versioned:
-        TODO
     org:
         Name of the github org (e.g. user account).
     repo:
         Name of the repo.
-
+    path:
+        A subfolder in the github repo holding the board.
+    **kwargs:
+        Passed to the pins.board function.
 
     Note
     ----
@@ -144,12 +186,26 @@ def board_github(org, repo, path="", versioned=True, cache=DEFAULT):
     """
 
     return board(
-        "github", path, versioned, cache, storage_options={"org": org, "repo": repo}
+        "github",
+        path,
+        versioned,
+        cache,
+        allow_pickle_read=allow_pickle_read,
+        storage_options={"org": org, "repo": repo},
     )
 
 
-def board_urls(path: str, pin_paths: dict, cache=DEFAULT):
-    """
+def board_urls(path: str, pin_paths: dict, cache=DEFAULT, allow_pickle_read=None):
+    """Create a board from individual urls.
+
+    Parameters
+    ----------
+    path:
+        A base url to prefix all individual pin urls with.
+    pin_paths: Mapping
+        A dictionary mapping pin name to pin url .
+    **kwargs:
+        Passed to the pins.board function.
 
     Example
     -------
@@ -175,18 +231,29 @@ def board_urls(path: str, pin_paths: dict, cache=DEFAULT):
     else:
         raise NotImplementedError("Can't currently pass own cache object")
 
-    return BoardManual(path, fs, versioned=True, pin_paths=pin_paths)
+    return BoardManual(
+        path,
+        fs,
+        versioned=True,
+        allow_pickle_read=allow_pickle_read,
+        pin_paths=pin_paths,
+    )
 
 
-def board_rsconnect(versioned=True, server_url=None, api_key=None, cache=DEFAULT):
-    """
+def board_rsconnect(
+    versioned=True, server_url=None, api_key=None, cache=DEFAULT, allow_pickle_read=None
+):
+    """Create a board to read and write pins from an RStudio Connect instance.
 
     Parameters
     ----------
     server_url:
-        TODO
+        Url to the RStudio Connect server.
     api_key:
-        TODO
+        API key for server. If not specified, pins will attempt to read it from
+        CONNECT_API_KEY environment variable.
+    **kwargs:
+        Passed to the pins.board function.
     """
 
     # TODO: api_key can be passed in to underlying RscApi, equiv to R's manual mode
@@ -196,9 +263,18 @@ def board_rsconnect(versioned=True, server_url=None, api_key=None, cache=DEFAULT
         server_url = os.environ.get("CONNECT_SERVER")
 
     kwargs = dict(server_url=server_url, api_key=api_key)
-    return board("rsc", "", versioned, storage_options=kwargs)
+    return board("rsc", "", versioned, cache, allow_pickle_read, storage_options=kwargs)
+
 
+def board_s3(path, versioned=True, cache=DEFAULT, allow_pickle_read=None):
+    """Create a board to read and write pins from an AWS S3 bucket folder.
 
-def board_s3(path, versioned=True):
+    Parameters
+    ----------
+    path:
+        Path of form <bucket_name>/<optional>/<subdirectory>.
+    **kwargs:
+        Passed to the pins.board function.
+    """
     # TODO: user should be able to specify storage options here?
-    return board("s3", path, versioned)
+    return board("s3", path, versioned, cache, allow_pickle_read)

pins/drivers.py

@@ -2,14 +2,23 @@
 
 from pathlib import Path
 
+from .config import get_allow_pickle_read, PINS_ENV_INSECURE_READ
 from .meta import Meta
-
+from .errors import PinsInsecureReadError
 
 # TODO: move IFileSystem out of boards, to fix circular import
 # from .boards import IFileSystem
 
 
-def load_data(meta: Meta, fs, path_to_version: "str | None" = None):
+UNSAFE_TYPES = frozenset(["joblib"])
+
+
+def load_data(
+    meta: Meta,
+    fs,
+    path_to_version: "str | None" = None,
+    allow_pickle_read: "bool | None" = None,
+):
     """Return loaded data, based on meta type.
     Parameters
     ----------
@@ -21,6 +30,15 @@ def load_data(meta: Meta, fs, path_to_version: "str | None" = None):
         A filepath used as the parent directory the data to-be-loaded lives in.
     """
     # TODO: extandable loading with deferred importing
+    if meta.type in UNSAFE_TYPES and not get_allow_pickle_read(allow_pickle_read):
+        raise PinsInsecureReadError(
+            f"Reading pin type {meta.type} involves reading a pickle file, so is NOT secure."
+            f"Set the allow_pickle_read=True when creating the board, or the "
+            f"{PINS_ENV_INSECURE_READ}=1 environment variable.\n"
+            "See:\n"
+            "  * https://docs.python.org/3/library/pickle.html \n"
+            "  * https://scikit-learn.org/stable/modules/model_persistence.html#security-maintainability-limitations"
+        )
 
     # Check that only a single file name was given
     fnames = [meta.file] if isinstance(meta.file, str) else meta.file

pins/errors.py

@@ -4,3 +4,7 @@ class PinsError(Exception):
 
 class PinsVersionError(PinsError):
     pass
+
+
+class PinsInsecureReadError(PinsError):
+    pass