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

Enhanced security model and exposure system #243

Closed
wants to merge 10 commits into from
Closed

Enhanced security model and exposure system #243

wants to merge 10 commits into from

Conversation

seveirein
Copy link
Contributor

@seveirein seveirein commented Dec 6, 2017

So, the prospect of an expose decorator has come up many times in the issue tracker. And it has been shot down many times. The (at least old) rationale for not supporting seems to be:

It doesn't work for all types--classes/properties/classmethods etc.

Well, I've implemented something that does work for all these types:

  • functions
  • normal methods
  • classes
  • classmethods
  • properties (getter/setter/deleter)
  • staticmethods

The patch also allows you to modify the exposure of attributes of classes and instances after they have been created, and control the exposure of classes and their instances separately. It also is the prelude to several other things I am working on (such as the ability to create a serialization policy to automatically control access to objects based on type).

It also adds a heck of a lot more functionality. it was originally intended to be standalone, but I have integrated it into RPyC.

It also addresses Issue #239 . it is not obvious, but there is no good way to protect a __call__ method on an exposed object with the previous protocol. You can now set the protocol flag "allow_unsafe_calls" to False -- but it only works with the new Exposer class.

it is mostly standalone, but requires some very minor tweaks to RPyC to work that should be backwards compatible. This is because it actually does all this by using metaclass magic to create proxied instances and classes -- like a netref, but even more sophisticated. It currently works even for python2 old style classes.

If you are interested, the best way to get started currently is to pull this, then build and read the new Sphinx API docs under security (the module is stored in "rpyc.security" )

I am still testing. I have very good test coverage in some areas, and unit tests have been added. However, I am still working to cover some cases, so the patch is ongoing.

I haven't been testing in Python 2.7 and 3.5, so there may be some bugs still. There may be some issues supporting Python 2.4 and Python 2.5 (but 2.6 should be okay). According to the documentation you still support 2.4 and 2.5 (but it may be out of date).

I'd really like to see you guys support this, or barring that, the minor mods needed to allow it to be a available as a standalone library.

Also factored some functions out of restrictor.py into a new utility.py
file.
* Docs were touched up
* Fixed some place in OLP that took a lock_list and made then take a same
  type of parameter as exposer
* Added support for wrapping of objects that already have _rpyc_getattr
  to nest calls.
* Protected __class__ being overwritten in `RPyC Exposed` objects.
* Fixed issues with test_decorated_custom_service that came to light from
  coverage checking.
@seveirein
Copy link
Contributor Author

So, continuous integration is failing badly because apparently a new directory in the git repository isn't apparently being made for the continuous integration testing. Understandable. as continuous integration needs to be locked down heavily (sandboxed). But frustrating, as I wanted to look at what CI had to say.

@coldfix
Copy link
Contributor

coldfix commented Dec 6, 2017

Hi,

CI fails because you didn't add rpyc.security to packages in setup.py (and therefore setup.py install overlooks the folder).

I regret to warn you that, as I mentioned on your previous PR, a patch of this magnitude has little chance of me merging it, as ~6k lines are extremely heavy to review and judge bugs (even if half of that is in tests), and introduces complexity that makes it harder to contribute for others.

However, I see that you put considerable effort into this and I will first try to get an overview over your work and your thoughts in the next days before a final decision. Should I reject, you may be interested in simply forking off your branch and creating a more security/feature oriented alternative to rpyc (while rpyc stays on the lightweight side).

Best, Thomas

@seveirein
Copy link
Contributor Author

Fair enough. If you want to communicate with me directly at some point in real time to ask questions it can be arranged. There is also no rush to make a decision.

Work on the patch is ongoing, though there will be a 3.5 week break coming up relatively rapidly (Dec 15 - Jan 8th I'll be out of town on two separate trips).

I'm currently trying to get near 100% source code coverage with https://coverage.readthedocs.io/en/coverage-4.4.2/ (you can use it from nose) and the unit tests, and while I'm not there yet (and still discovering bugs as I add coverage), I thought you should be aware of my approach.

Regarding the patch as I see it there are 3 options:

  • Integrate it and make it part of the RPyC project.
  • Keep it as separate optional package, but make minor mods to RPyC to allow it to work. Most notably, the changes to protocol.py. I could submit a PR with just those changes if full integration is rejected..
  • I could fork off a new version of RPyC.

I'd rather do one of the first 2 options.

…Errors to TypeError

rpyc.security is in setup.py

locks.py now joins the 100% source code coverage club.

4 bugs found, some in locks, a few in olps (next coverage target)

Bunch of ValueErrors changed to TypeError as more appropriate (and their tests)
2 of them were brand new/uncovered--caused by recent code changes.
1 worked fine after 2.7.9, but not on earlier version of python2
There is no __func__ in the Python 2.6 data model. The only way left to get the information
is through the descriptor protocol, which is what we now do for Python2.6
@coldfix
Copy link
Contributor

coldfix commented Dec 7, 2017

Hi,

Your PR is currently 6.5k lines, effectively doubling the size of rpyc. This is not going to happen.

If you can submit a pull-request that is short (say, below ~20 lines) and makes sense for rpyc on its own, that allows you to maintain your extensions as a stand-alone library, I may consider merging it.

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

Successfully merging this pull request may close these issues.

2 participants