Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

re-introduce pickle serializer and only_exposed=False #16

Closed
gst opened this issue Nov 10, 2019 · 16 comments
Closed

re-introduce pickle serializer and only_exposed=False #16

gst opened this issue Nov 10, 2019 · 16 comments

Comments

@gst
Copy link

gst commented Nov 10, 2019

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
Copy link
Owner

irmen commented Nov 11, 2019

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
Copy link
Author

gst commented Nov 11, 2019

Thanks for quick answer which I was unfortunately expecting.

@gst gst closed this as completed Nov 11, 2019
@louwers
Copy link

louwers commented Dec 24, 2019

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
Copy link
Author

gst commented Dec 27, 2019

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

this is all I needed in my own project.

@mortlind
Copy link

mortlind commented Jun 16, 2020

@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
Copy link

ematvey commented Oct 5, 2020

@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
Copy link
Owner

irmen commented Oct 13, 2020

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
Copy link

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
Copy link

ematvey commented Oct 13, 2020

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
Copy link
Owner

irmen commented Oct 14, 2020

@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
Copy link

@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
Copy link

mortlind commented Aug 23, 2023

@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
Copy link
Author

gst commented Aug 23, 2023

@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
Copy link

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
Copy link
Author

gst commented Aug 23, 2023

I synced my master branch with upstream and create a draft PR : gst#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
Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants