Skip to content

Add TLS support for TCP sockets #211

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

Merged
merged 6 commits into from
Jan 29, 2020

Conversation

moisesguimaraes
Copy link
Contributor

@moisesguimaraes moisesguimaraes commented Dec 9, 2019

This PR is still a work in progress. I would like to add tests to it before merging.


This change is Reviewable

@jaysonsantos
Copy link
Owner

Hey @moisesguimaraes sorry for the delay.
First of all, thanks for the PR!
I was looking around and could not see a standard API on memcached libraries, didn't I look right or none of them implement SSL?
I would like to always maintain the same API like other libraries e.g. pylibmc, but if none of them implement this I guess it would be ok to use tls_context.

@moisesguimaraes
Copy link
Contributor Author

Hi @jaysonsantos. So far there is no support at all for TLS in python memcached clients. You can follow our research here: https://etherpad.openstack.org/p/oslo-cache-tls-support-worksheet

I think working with the context will give more flexibility on which TLS implementation or which TLS options are set in place. It is basically decoupling it and handling it as a dependency injection. Otherwise we would have to have params for keys, certificates, allowed TLS version, allowed cipher suites, and many more TLS options.

@@ -30,14 +30,16 @@ def __init__(self, servers=('127.0.0.1:11211',), username=None,
socket_timeout=SOCKET_TIMEOUT,
pickle_protocol=0,
pickler=pickle.Pickler,
unpickler=pickle.Unpickler):
unpickler=pickle.Unpickler,
tls_context=None):
Copy link
Owner

@jaysonsantos jaysonsantos Dec 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there, could you also add some info about in on the __doc__?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'm on vacation now and will be back on this task in the first week of January.

@jaysonsantos
Copy link
Owner

Hey @moisesguimaraes I've just rebased your branch with master to see how it goes on the tests

@jaysonsantos
Copy link
Owner

The tests are being skipped on travis, is there any way to make them work there? https://travis-ci.org/jaysonsantos/python-binary-memcached/jobs/642982975?utm_medium=notification&utm_source=github_status

@moisesguimaraes
Copy link
Contributor Author

Tests are skipped when the Memcached server itself was not built with TLS support. That will take a while, but my team is working on pushing TLS enablement upstream. A way to make it work would be to recompile memcached with --enable-tls.

In order to test TLS support we need a couple of keys and certs.
TLS tests requires a TLS enabled memcached server. In order to get
one, you must compile memcached with `--enable-tls`.
You can get a TLS enabled memcached by compiling it with --enable-tls.
@jaysonsantos jaysonsantos merged commit 3e930ce into jaysonsantos:master Jan 29, 2020
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