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

Added SASL authentication support, for Memcached ver. 2.2.0 and above #36

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kehati
Copy link

@kehati kehati commented Jun 18, 2014

Hi,
I added SASL auth support, for Memcached ver. 2.2.0 and above.
This also solves issue #32.
Thanks!

@tollmanz
Copy link
Owner

Hi @kehati!

Thanks for your work here!

I've been a little skeptical of SASL connections to Memcached. Can you tell me a little bit more about why this is needed?

Also, it's looking like this patch breaks the unit tests pretty badly. Any thoughts?

@kehati
Copy link
Author

kehati commented Jun 19, 2014

Hi @tollmanz,

The memcached binary protocol supports SASL authentication mechanism, providing a security layer to the connection. It's part of the binary protocol spec.

As the PECL Memcached extension supports SASL authentication since version 2.2.0 (note the requirements: http://il1.php.net/manual/en/memcached.requirements.php), many users would want to implement secured connection to their memcached server using the SASL authentication mechanism and I think the library should support it.

As the SASL mechanism requires the use of the binary protocol, I set the Memcached client to use it with:
$this->m->setOption(Memcached::OPT_BINARY_PROTOCOL, true);

after looking into it, i think that almost all errors in the unit tests, as seen in the Travis CI, are caused by the test assertion trying to compare the key and the value previously set to the key and value fetched from the server, using the binary protocol which is fetching the CAS check as well (as part of the spec). For example:

MemcachedUnitTests::test_get_delayed_by_key_returns_correct_values
Failed asserting that Array &0 (
'key' => 'wptests_:default:0.708750001403074436-sharp'
'value' => 10
* 'cas' => 133.0*
) is identical to Array &0 (
'key' => 'wptests_:default:0.708750001403074436-sharp'
'value' => 10
).

I think that if the unit tests will compare the keys and values only, they should succeed.

@tollmanz
Copy link
Owner

tollmanz commented Jul 6, 2014

Hi @kehati!

I'm still trying to comprehend if this is something that should be merged into this library, but would be interested in getting this in a state that we can test a bit more thoroughly. Here are some things that I think we need to consider:

  • I'd like to keep this library dependent upon only a single global variable. Perhaps we can move $memcached_username and $memcached_password to the $memcached_servers global variable.
  • It's really important the unit tests are passing before this can be considered for inclusion. Ideally, we need to run the same unit tests that we currently have using a SASL connection instead of the direct connection. I think the way forward would be to tinker with the setup routines. Note that I refactored the tests over the weekend, so you would want to rebase those into your fork before starting new work on this.

I've been reading up on this a bit to get a sense of the pros and cons of SASL. Do you have any information about performance impacts? I'm concerned that the latency in negotiating the secure connection could be a major performance issue. Do you know of any SaaS's out there that support the SASL memcached connections? I'd like to test against something like that.

Thanks again for your work on this!

@tollmanz
Copy link
Owner

tollmanz commented Jul 6, 2014

I just noticed that you are with RedisLabs. I was looking at the service earlier as I was trying to answer the following question:

Do you know of any SaaS's out there that support the SASL memcached connections? I'd like to test against something like that.

In looking through the service, I'm not seeing mention of SASL. Is this something you support?

@tollmanz
Copy link
Owner

tollmanz commented Jul 8, 2014

Regarding the config, I'd like to see this integrated in the global config discussed in #40.

@kehati
Copy link
Author

kehati commented Jul 8, 2014

Hey @tollmanz ,

  1. Regarding global variables- the idea sounds good to me but I'm not sure where and how would you like to define or configure them. I must admit I'm no WP (nor PHP) expert at all.
  2. Regarding SASL perfomance- as the new fix uses the binary protocol (which the SASL mechanism requires), the performance will of course be better than the text protocol. The negotiation for secured connection using the SASL mechanism occurres once, when the connection is initiated.
    Our Memcached Cloud service supports SASL mechanism of course, and the configuration is pretty easy and straightforward. You are more than welcome to sign up at Redis Labs and create a free 25MB bucket for your testings. Let me know how I can help you with this.

Thanks!

@tollmanz
Copy link
Owner

Regarding global variables- the idea sounds good to me but I'm not sure where and how would you like to define or configure them. I must admit I'm no WP (nor PHP) expert at all.

No worries! I have a game plan for the this, which will be tracked in #40.

Regarding SASL perfomance- as the new fix uses the binary protocol (which the SASL mechanism requires), the performance will of course be better than the text protocol.

Given that you work as a Memcached cloud company, do you have any statistics on this performance? I'm really curious about it. I've only found a few reports of this and would love to have some more solid data.

Our Memcached Cloud service supports SASL mechanism of course, and the configuration is pretty easy and straightforward. You are more than welcome to sign up at Redis Labs and create a free 25MB bucket for your testings. Let me know how I can help you with this.

Good to know! I'll be checking into this soon.

I did some testing and got all but 4 tests to pass with the binary protocol, which I think is the first hurdle to getting SASL support. Only the CAS tests are failing. Hopefully that won't be too hard to figure out. I'll likely be re-writing how it is implemented to make sure that everything functions as usual, but there is an option to move to the binary protocol and SASL integration.

@shmuel-krakower
Copy link

@tollmanz - does that mean that currently this plugin don't support the binary protocol?

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.

3 participants