Repository: fsspec/filesystem_spec
Pull request #1625 (2024-06-14 11:26:31)

Add option to skip password when serializing filesystem
Currently, `AbstractFileSystem.to_dict` (and by extension `AbstractFileSystem.to_json`) store all arguments used to construct the object. However, there are some cases where the user may provide a password, such as in `FTPFileSystem`, `SMBFileSystem`, and `WebHDFS`. This may cause security issues, especially considering that the password is stored in plain text.

~~This PR disables the aforementioned behaviour by default. Users now have to pass `include_password=True` to the relevant methods if they wish to store the password anyway.~~ After some discussion, this feature is now opt-in. I have added a warning to the documentation to remind users to be careful about this.

martindurant: I should start by saying that including passwords directly in code/scripts is not generally a good idea. Many or the backends provide alternative ways to get credentials at runtime, from certain files, environment variables or other means.

Am I right in thinking that you are simply looking for dictionary keys that happen to be called "password"? That would not include other possibly sensitive values like keys and tokens, but I don't suppose there's any way to know whether some value is sensitive or not.

I am mildly against removing any values by default, as it would break the principal function of dict/json serialization, to faithfully recreate the original instance. How would a user re-apply the password when they need it, or how would they know in the first place that it was missing?

However, it might be suitable to provide a "safe ser" mode, but perhaps each backend implementation would declare which instance kwargs are to be considered sensitive?
DarkLight1337: > Am I right in thinking that you are simply looking for dictionary keys that happen to be called "password"? That would not include other possibly sensitive values like keys and tokens, but I don't suppose there's any way to know whether some value is sensitive or not.

Yes, it's that straightforward.

> but perhaps each backend implementation would declare which instance kwargs are to be considered sensitive? 

This would be a more comprehensive way to filter out sensitive data, but would require more time to implement. I think we should release a simple/partial patch first.

> I am mildly against removing any values by default, as it would break the principal function of dict/json serialization, to faithfully recreate the original instance. How would a user re-apply the password when they need it, or how would they know in the first place that it was missing?

It should not take much digging into an authentication error to realize that the password is missing. Do many people expect that serialization includes sensitive data by default? The requirement of passing `include_password=True` explicitly would get people to think twice about security.

martindurant: Let's make removing "password" opt-in for now and rettain the same default behaviour for now. 

> It should not take much digging into an authentication error to realize that the password is missing. 

Unfortunately, you commonly get a "permission error" or something more arcane without further details. Most security systems will not tell you specifically that a password is missing. Of course, the FS repr doesn't include whether a password is set or not. I don't know, but you may even get an attribute-error for some backends if you simply don't set the value (should it be None by default, or ""?).

> Do many people expect that serialization includes sensitive data by default?

It depends! If the purpose is to pass instances within a secured network, then yes. Pickle may well do that in a worker cluster, after all. Where instances need to be (re)created from a set of arguments plus additional information from the user or environment, you may well need a helper like Intake to encode this intent.

Patch:
diff --git a/fsspec/implementations/dirfs.py b/fsspec/implementations/dirfs.py
index 1b311ab..04f7479 100644
--- a/fsspec/implementations/dirfs.py
+++ b/fsspec/implementations/dirfs.py
@@ -56,6 +56,8 @@ class DirFileSystem(AsyncFileSystem):
             if not path:
                 return self.path
             return self.fs.sep.join((self.path, self._strip_protocol(path)))
+        if isinstance(path, dict):
+            return {self._join(_path): value for _path, value in path.items()}
         return [self._join(_path) for _path in path]
 
     def _relpath(self, path):
diff --git a/fsspec/json.py b/fsspec/json.py
index 3d1e473..81d1f3a 100644
--- a/fsspec/json.py
+++ b/fsspec/json.py
@@ -1,16 +1,18 @@
 import json
 from contextlib import suppress
 from pathlib import PurePath
-from typing import Any, Callable, Dict, List, Optional, Tuple
+from typing import Any, Callable, ClassVar, Dict, List, Optional, Tuple
 
 from .registry import _import_class, get_filesystem_class
 from .spec import AbstractFileSystem
 
 
 class FilesystemJSONEncoder(json.JSONEncoder):
+    include_password: ClassVar[bool] = True
+
     def default(self, o: Any) -> Any:
         if isinstance(o, AbstractFileSystem):
-            return o.to_dict()
+            return o.to_dict(include_password=self.include_password)
         if isinstance(o, PurePath):
             cls = type(o)
             return {"cls": f"{cls.__module__}.{cls.__name__}", "str": str(o)}
diff --git a/fsspec/spec.py b/fsspec/spec.py
index 951e9a2..edc7abf 100644
--- a/fsspec/spec.py
+++ b/fsspec/spec.py
@@ -1386,20 +1386,38 @@ class AbstractFileSystem(metaclass=_Cached):
                 length = size - offset
             return read_block(f, offset, length, delimiter)
 
-    def to_json(self) -> str:
+    def to_json(self, *, include_password: bool = True) -> str:
         """
         JSON representation of this filesystem instance.
 
+        Parameters
+        ----------
+        include_password: bool, default True
+            Whether to include the password (if any) in the output.
+
         Returns
         -------
         JSON string with keys ``cls`` (the python location of this class),
         protocol (text name of this class's protocol, first one in case of
         multiple), ``args`` (positional args, usually empty), and all other
         keyword arguments as their own keys.
+
+        Warnings
+        --------
+        Serialized filesystems may contain sensitive information which have been
+        passed to the constructor, such as passwords and tokens. Make sure you
+        store and send them in a secure environment!
         """
         from .json import FilesystemJSONEncoder
 
-        return json.dumps(self, cls=FilesystemJSONEncoder)
+        return json.dumps(
+            self,
+            cls=type(
+                "_FilesystemJSONEncoder",
+                (FilesystemJSONEncoder,),
+                {"include_password": include_password},
+            ),
+        )
 
     @staticmethod
     def from_json(blob: str) -> AbstractFileSystem:
@@ -1426,25 +1444,40 @@ class AbstractFileSystem(metaclass=_Cached):
 
         return json.loads(blob, cls=FilesystemJSONDecoder)
 
-    def to_dict(self) -> Dict[str, Any]:
+    def to_dict(self, *, include_password: bool = True) -> Dict[str, Any]:
         """
         JSON-serializable dictionary representation of this filesystem instance.
 
+        Parameters
+        ----------
+        include_password: bool, default True
+            Whether to include the password (if any) in the output.
+
         Returns
         -------
         Dictionary with keys ``cls`` (the python location of this class),
         protocol (text name of this class's protocol, first one in case of
         multiple), ``args`` (positional args, usually empty), and all other
         keyword arguments as their own keys.
+
+        Warnings
+        --------
+        Serialized filesystems may contain sensitive information which have been
+        passed to the constructor, such as passwords and tokens. Make sure you
+        store and send them in a secure environment!
         """
         cls = type(self)
         proto = self.protocol
 
+        storage_options = dict(self.storage_options)
+        if not include_password:
+            storage_options.pop("password", None)
+
         return dict(
             cls=f"{cls.__module__}:{cls.__name__}",
             protocol=proto[0] if isinstance(proto, (tuple, list)) else proto,
             args=self.storage_args,
-            **self.storage_options,
+            **storage_options,
         )
 
     @staticmethod


Test patch:
diff --git a/fsspec/implementations/tests/test_dirfs.py b/fsspec/implementations/tests/test_dirfs.py
index 300bdd8..c04ba66 100644
--- a/fsspec/implementations/tests/test_dirfs.py
+++ b/fsspec/implementations/tests/test_dirfs.py
@@ -162,6 +162,11 @@ def test_pipe(dirfs):
     dirfs.fs.pipe.assert_called_once_with(f"{PATH}/file", *ARGS, **KWARGS)
 
 
+def test_pipe_dict(dirfs):
+    dirfs.pipe({"file": b"foo"}, *ARGS, **KWARGS)
+    dirfs.fs.pipe.assert_called_once_with({f"{PATH}/file": b"foo"}, *ARGS, **KWARGS)
+
+
 @pytest.mark.asyncio
 async def test_async_pipe_file(adirfs):
     await adirfs._pipe_file("file", *ARGS, **KWARGS)
diff --git a/fsspec/tests/test_spec.py b/fsspec/tests/test_spec.py
index 32db78c..2bb948b 100644
--- a/fsspec/tests/test_spec.py
+++ b/fsspec/tests/test_spec.py
@@ -912,6 +912,20 @@ def test_dict_idempotent():
     assert DummyTestFS.from_dict(outa) is a
 
 
+def test_serialize_no_password():
+    fs = DummyTestFS(1, password="admin")
+
+    assert "password" not in fs.to_json(include_password=False)
+    assert "password" not in fs.to_dict(include_password=False)
+
+
+def test_serialize_with_password():
+    fs = DummyTestFS(1, password="admin")
+
+    assert "password" in fs.to_json(include_password=True)
+    assert "password" in fs.to_dict(include_password=True)
+
+
 def test_from_dict_valid():
     fs = DummyTestFS.from_dict({"cls": "fsspec.tests.test_spec.DummyTestFS"})
     assert isinstance(fs, DummyTestFS)