zhebrak / raftos

Asynchronous replication framework for distributed Python projects
MIT License
350 stars 58 forks source link

Making encryption optional #12

Open avinash240 opened 7 years ago

avinash240 commented 7 years ago

I'm running in an environment where we run a custom version of openssl. Because of this custom SSL python-cryptography is a nightmare to compile as is. I am only using the leader election portion of the library and don't need the encryption. I'd like to send you a pull request where that is a configurable option, so that python-cryptography becomes an option requirement. It seems pretty straightforward since you've done such a good job with the code, I only see the encrypt and decrypt calls in two seconds of code for the UDP protocol. Any interest in this work?

zhebrak commented 7 years ago

Hey Marlon! Sounds good to me, how about making DummyCryptor in cryptor.py with encrypt/decrypt that just return data as is and an option in configuration which enables it?

avinash240 commented 7 years ago

That works for me as well, as long as I have a switch around the cryptography import I'm happy. We spend the entire day trying to get it to build yesterday. I'll have the pull request in for you today, and you let me know when it's entirely acceptable.

bakwc commented 7 years ago

I am only using the leader election portion of the library

Keep in mind that it's not safe - you can get two leaders at the same time, see #4 for details.

avinash240 commented 7 years ago

@bakwc yes, I appreciate you bringing this up. I'm ok with that edge case for my needs. @zhebrak I create a pull request, please let me know if you need any changes, there are a couple different ways I could have gone, just didn't exactly know what you'd be comfortable with. Please let me know.

zhebrak commented 7 years ago

@avinash240 you can now pass _cryptoenabled=False to use DummyCryptor instead of proper one. You can also force pip to ignore dependencies with --no-deps flag. Please, let me know if it works for you.

avinash240 commented 7 years ago

I tried this pattern in my dev workspace as one of the pull requests I intended on sending you. importing raftos automatically set crypto to use the Cryptor() object which was then attached to the UDPProtocol() object. This was all triggered on import, then the raft.configure() would be called but to no effect on the cryptor object being used on the UDPProtocol, only on where the cryptor.cryptor identifier was now pointing. My point being having a configuration variable didn't seem to have any effect on choice of cryptor modules so why bother having it. I'm ok with the --no-deps flag but I feel, that's going to have limited functionality for a lot of people, as opposed to setting a bracketed extra deps call on setup i.e. pip install raftos[cryptography]

zhebrak commented 7 years ago

You are right, my bad! Fixed it to match serializer behaviour, {'cryptor': raftos.cryptors.DummyCryptor}. Should be working right now.

I don't want to introduce raftos[cryptography] style since some folks (I don't really know if there any though) could already be using pip install raftos mode and expect it to do cryptography by default.

avinash240 commented 7 years ago

Thanks. =) And yes, I agree about the default behavior. My original thought was to come up with pip install raftos[no-crypto] but I don't now how to do that. Any ideas? The --no-deps will work but it's a pretty clunky solution, that I can see creating a problem down the road because now we have to handle raftos dependencies outside of the package.

zhebrak commented 7 years ago

As far as I know, there is no away to ignore particular dependency with pip. Supporting dependencies manually doesn't seem to be right way for sure... I'll think about it, and let me know if you have any ideas.

avinash240 commented 7 years ago

What I came up with looks a lot like what's in pull request

https://github.com/zhebrak/raftos/pull/14/commits/4933e17a27f21a018bae49c9e4d0be13f3c27345

Basically I turn it on if cryptography is available and leave it off if it's not. However, It's not really something that's configurable. I know there is a request for pip to be able to exclude specific dependencies, if that ever gets into pip then I think that code mixed with a document stub of --exclude-dep=cryptography will work. However, that's not currently available so I'm at a loss as well.