irmen / Pyro5

Pyro 5 - Python remote objects
https://pyro5.readthedocs.io
MIT License
305 stars 36 forks source link

re-introduce pickle serializer and only_exposed=False #16

Closed gst closed 4 years ago

gst commented 4 years ago

Hi Irmen,

I have a diff/patch (not that big) in my hand that allows me to use pyro5 exactly as I am using pyro4. That is totally transparently from client(s) to server(s) side (and vice-versa). Also allowing any python object to be passed along the wire (thx to pickle and yes I know about its security impact, this is for controlled/restricted servers&clients).

Would you be open to such a change in Pyro5 ?

The diff/patch runs ok the current entire test suite. Both of Pyro5 and of one of my project which is using Pyro4 actually ; but I'd like to eventually update to Pyro5 thus. Hence this issue..

Basically I've reintroduced the PickleSerializer and had to make a few changes here and there to make all the test suites to pass..

irmen commented 4 years ago

I appreciate the effort and am glad that you got it to work, but no - I don't want to reintroduce pickle into Pyro5. I left it out for a reason; I want Pyro5 to be a secure mechanism where it is not possible at all to configure it in a way that enables (obvious) security problems.

What is acceptable though is to put it on a separate branch in the repository, or perhaps if you just put it up as a fork of Pyro5 that is clearly labeled as such that it includes pickle and you have to deal with its security implications yadda yadda yadda

gst commented 4 years ago

Thanks for quick answer which I was unfortunately expecting.

louwers commented 4 years ago

We plan to use Pyro4 for development (not for production), specifically to load classes that take a long time to initialize in memory. So in a completely controlled environment. It would be useful if you could put your work in a seperate branch @gst so that this setup will continue to work in the future.

gst commented 4 years ago

https://github.com/gst/Pyro5/tree/allow_false_only_exposed

this is all I needed in my own project.

mortlind commented 4 years ago

@gst I very much appreciate your effort. I hope that there will be a packaged version of Pyro5 available, with pip and in Debian, allowing the use of pickle as serializer. Though I understand the security issues, the flexibility it offers in data modelling is hard to over estimate. Keep up the good work!

ematvey commented 3 years ago

@irmen I appreciate your effort in keeping things secure but would ask to please reconsider this position. I would imagine a lot of use cases where security is handled at a different layer. Mine is running Pyro-based system inside a private network (k8s).

irmen commented 3 years ago

Why do you really require pickle as a transport? Would it perhaps work to pickle your request/response objects explicitly and send over the pickled bytes as a blob? (i.e. taking the pickling out of Pyro itself)?

Why do you require the convenience of not having to @expose your server classes (there are alternatives)?

mortlind commented 3 years ago

Would it perhaps work to pickle your request/response objects explicitly and send over the pickled bytes as a blob? (i.e. taking the pickling out of Pyro itself)?

Would that solve the security problem? It would at least be ugly to have extra, explicit marshalling and de-marshalling operation at either end in the code ...

ematvey commented 3 years ago

Why do you really require pickle as a transport? Would it perhaps work to pickle your request/response objects explicitly and send over the pickled bytes as a blob? (i.e. taking the pickling out of Pyro itself)?

Why do you require the convenience of not having to @expose your server classes (there are alternatives)?

We have a large existing service that serves machine learning model inference by accepting numpy arrays and returning multiple custom types (typing.NamedTuple) which also contain numpy arrays. Until recently it existed as a library but growing complexity forced us to split the pipeline up into separate Docker containers. Clients requests handling as well as communication between containers all now work via Pyro4 with pickle/dill serializer. All of the clients and API are internal, everything works on a single Docker host, and so far I probably would not use Pyro4 for the external API outside of a private network. I realize that you don't recommend Pyro4 for passing large binary arrays, however for our case it works very well.

irmen commented 3 years ago

@ematvey thanks for explaining a bit about your use case. I'm glad you chose Pyro and that it actually performs well enough with the binary data. @mortlind well, in the sense that Pyro itself is no longer responsible for it. Yes it's ugly. I'm just exploring options.

Another option is ofcourse to stick to Pyro4 -- it has not become broken or something all of a sudden ! :-)

What do you see as compelling reasons to upgrade to Pyro5? (assume that the pickle issue is non-existant)

mortlind commented 3 years ago

@irmen I like the restructuring. But otherwise there is currently no big reason for me to move to Pyro5. Pyro4 works, and it works very well. Thanks for that!

mortlind commented 1 year ago

@gst Would you consider keeping your fork of Pyro5 up to date? I would then happily switch to that from Pyro4. Perhaps you could even consider publishing a "Pyro5Unsafe" package to PyPI? I would be more than happy to help, if this will be a burden to you.

gst commented 1 year ago

@mortlind not sure what you refer to as to keep up2date, you mean the branch I've does not apply anymore to latest master ?

but anyway: I do not use anymore this/pyro for any of the project I work on for quite some time.. so not sure. would depends the amount of effort I guess ;)

mortlind commented 1 year ago

Hi @gst,

Thanks for a quick reply!

In your fork, about the allow_false_only_exposed-branch it is stated that "This branch is 1 commit ahead, 165 commits behind irmen:master." Those "165 commits behind" is what I mean by not up to date.

Since you are not using Pyro any longer, I will not burden you with my request for maintenance. I will try to make my own fork, and maintain an unsafe version of Pyro5.

Good luck with the other projects!

gst commented 1 year ago

I synced my master branch with upstream and create a draft PR : https://github.com/gst/Pyro5/pull/2

but there are effectively few conflicts as I was expecting after this long time. Normally I think it's easy to handle them though.

mortlind commented 1 year ago

OK. I can try and have a look at it. Would you still be interested in participating in maintaining this branch, and possibly release a PyPI version?