Using smb safely in multiuser environments

[bernt-matthias]

I was considering to implement access to smb for the Galaxy project where we use fsspec a lot. Galaxy is a multi user environment where we would safely store the information needed to access a smb share (server, port, username, password, path ...). Then we connect via fsspec.implementations.smb.SMBFileSystem.

By chance I noticed that the underlying smbprotocol uses a connection cache which is shared between all connections in a python process. The cache seems to work on server + port as key (maybe username .. but I'm unsure). Hence to me it seems unsafe to use this in a multi user environment, since users might hijack other users sessions (involuntary or deliberately) since they only need to know the server+port+username.

Would you agree with this? Are there possibilities to fix this, e.g. by disabling the global connection cache (one can pass a dict that would be used) as outlined here. Are there maybe other fsspec implementations using different libraries?

Edit: Meanwhile I found the tests and verified my theory (I think) #2007

[martindurant]

Interesting problem! I only know about the smb connection cache now from having followed your links.

I suppose you can experiment by providing your on cache dict and seeing what gets set there. If your model is correct, then the fsspec implementation certainly could (optionally) supply a cache of its own and provide ways to interact with it. Then, you can modify SMBFileSystem until #2007 does the right thing. (for the sake of style, I would add pytest.raises around the critical lines - the behaviour we want - so that the test fails; this will prevent accidentally merging the PR)

You should also be aware, that fsspec also maintains a cache of filesystem instances, keyed on the kwargs supplied. I don't think this will apply to you, since each instantiation contains the username/password, so there's no way to guess that.

[bernt-matthias]

Interesting problem! I only know about the smb connection cache now from having followed your links.

I accidentally stumbled over some exception message. Was wondering if other libraries used in the project do similar stuff.

I suppose you can experiment by providing your on cache dict and seeing what gets set there. If your model is correct, then the fsspec implementation certainly could (optionally) supply a cache of its own and provide ways to interact with it. Then, you can modify SMBFileSystem until #2007 does the right thing.

True. But even if I create a ton of tests I would have a hard time to convince myself, the maintainers of the Galaxy project, or admins to use/deploy this. The filesystems may contain sensitive data and I would prefer them not to get leaked :)

But I might be wrong. Maybe its sufficient to supply a fresh dict with each operation in fsspec that interacts with the smbclient/protocol and destroy it afterwards?

(for the sake of style, I would add pytest.raises around the critical lines - the behaviour we want - so that the test fails; this will prevent accidentally merging the PR)

Good point.

You should also be aware, that fsspec also maintains a cache of filesystem instances, keyed on the kwargs supplied.

Thanks for the note. Meanwhile I also discovered this and discuss with other devs of the Galaxy project if this might be a problem. Maybe shared credentials might be a problem.

I don't think this will apply to you, since each instantiation contains the username/password, so there's no way to guess that.

Could be a problem if username + keyfilename would be possible.

[martindurant]

No, I have not heard of this kind of problem before.

even if I create a ton of tests I would have a hard time to convince myself, the maintainers of the Galaxy project, or admins to use/deploy this

I meant experiments in code versus the test case you already submitted. We should be able to introspect all the instances created to check for credentials.

Maybe its sufficient to supply a fresh dict with each operation in fsspec that interacts with the smbclient/protocol and destroy it afterwards?

The linked issue suggests this. I assume it comes with some extra per-connection overhead, but that's OK for the sake of security. I don't really know how the runtime within Galaxy works, I'm afraid.

[martindurant]

@bernt-matthias , do you have aplan of what to do here?

[bernt-matthias]

I could imagine to either add a comment to the test that documents that this is the current behavior an/or add pytest.raises as suggested by you. In addition I would suggest to add a big warning to the docs of the module?

Edit: I do not think that we will use the module as long as it uses smbprotocol (even if we would implement a per-connection cache). So I'm reluctant in investing more time here. But I will discuss with other admins/developers.

[martindurant]

Have you tried passing an explicit cache dict to the SMB pool? If that's per instance, and we either make the instance not cacheable (or pass skip_instance_cache=True), that might be all you need.

[bernt-matthias]

I used the opportunity to play around a bit with opencode: bdb0104

Guess that is what could be done from fsspec side. Still I would not trust this, since one needs to be sure that smbclient correctly passes kwargs down to all its functions.

Maybe we should open an issue over there?

[martindurant]

It's certainly annoying to have to pass the argument around all over the place - most implementations have a single call() location to handle this. Nevertheless, I expect it works, though. An upstream issue is fine, but I you should put these changes into a PR, I think. What would it take for you to trust it?