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

Making encryption optional #12

Open
avinash240 opened this issue Apr 27, 2017 · 10 comments
Open

Making encryption optional #12

avinash240 opened this issue Apr 27, 2017 · 10 comments

Comments

@avinash240
Copy link

avinash240 commented Apr 27, 2017

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

zhebrak commented Apr 27, 2017

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

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

bakwc commented Apr 27, 2017

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

@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 added a commit that referenced this issue May 2, 2017
@zhebrak
Copy link
Owner

zhebrak commented May 2, 2017

@avinash240 you can now pass crypto_enabled=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
Copy link
Author

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 added a commit that referenced this issue May 2, 2017
@zhebrak
Copy link
Owner

zhebrak commented May 2, 2017

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

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

zhebrak commented May 2, 2017

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

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

4933e17

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.

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

3 participants