-
Notifications
You must be signed in to change notification settings - Fork 36
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
Comments
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 |
Thanks for quick answer which I was unfortunately expecting. |
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. |
https://github.com/gst/Pyro5/tree/allow_false_only_exposed this is all I needed in my own project. |
@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! |
@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). |
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 |
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 ... |
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. |
@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. 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) |
@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! |
@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. |
@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 ;) |
Hi @gst, Thanks for a quick reply! In your fork, about the 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! |
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. |
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? |
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..
The text was updated successfully, but these errors were encountered: