explicitly record sasl auth states

It was previously possible to bypass authentication due to implicit state management. Now we explicitly consider ourselves unauthenticated on any new connections and authentication attempts. bug316 Signed-off-by: Dustin Sallings <dustin@spy.net>
memcached · Dec 20, 2013 · 87c1cf0 · zhuyong96 · Jan 13, 2014 · 87c1cf0
1 parent c3d22fe
commit 87c1cf0
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 8 deletions.
diff --git a/memcached.c b/memcached.c
@@ -457,6 +457,7 @@ conn *conn_new(const int sfd, enum conn_states init_state,
     c->iovused = 0;
     c->msgcurr = 0;
     c->msgused = 0;
+    c->authenticated = false;
 
     c->write_and_go = init_state;
     c->write_and_free = 0;
@@ -1637,6 +1638,8 @@ static void init_sasl_conn(conn *c) {
     if (!settings.sasl)
         return;
 
+    c->authenticated = false;
+
     if (!c->sasl_conn) {
         int result=sasl_server_new("memcached",
                                    NULL,
@@ -1771,6 +1774,7 @@ static void process_bin_complete_sasl_auth(conn *c) {
 
     switch(result) {
     case SASL_OK:
+        c->authenticated = true;
         write_bin_response(c, "Authenticated", 0, 0, strlen("Authenticated"));
         pthread_mutex_lock(&c->thread->stats.mutex);
         c->thread->stats.auth_cmds++;
@@ -1807,11 +1811,7 @@ static bool authenticated(conn *c) {
         rv = true;
         break;
     default:
-        if (c->sasl_conn) {
-            const void *uname = NULL;
-            sasl_getprop(c->sasl_conn, SASL_USERNAME, &uname);
-            rv = uname != NULL;
-        }
+        rv = c->authenticated;
     }
 
     if (settings.verbose > 1) {

diff --git a/memcached.h b/memcached.h
@@ -376,6 +376,7 @@ typedef struct conn conn;
 struct conn {
     int    sfd;
     sasl_conn_t *sasl_conn;
+    bool authenticated;
     enum conn_states  state;
     enum bin_substates substate;
     struct event event;

diff --git a/t/binary-sasl.t b/t/binary-sasl.t
@@ -13,7 +13,7 @@ use Test::More;
 
 if (supports_sasl()) {
     if ($ENV{'RUN_SASL_TESTS'}) {
-        plan tests => 25;
+        plan tests => 33;
     } else {
         plan skip_all => 'Skipping SASL tests';
         exit 0;
@@ -229,6 +229,38 @@ $check->('x','somevalue');
 }
 $empty->('x');
 
+{
+    my $mc = MC::Client->new;
+
+    # Attempt bad authentication.
+    is ($mc->authenticate('testuser', 'wrongpassword'), 0x20, "bad auth");
+
+    # This should fail because $mc is not authenticated
+    my ($status, $val)= $mc->set('x', "somevalue");
+    ok($status, "this fails to authenticate");
+    cmp_ok($status,'==',ERR_AUTH_ERROR, "error code matches");
+}
+$empty->('x', 'somevalue');
+
+{
+    my $mc = MC::Client->new;
+
+    # Attempt bad authentication.
+    is ($mc->authenticate('testuser', 'wrongpassword'), 0x20, "bad auth");
+
+    # Mix an authenticated connection and an unauthenticated connection to
+    # confirm c->authenticated is not shared among connections
+    my $mc2 = MC::Client->new;
+    is ($mc2->authenticate('testuser', 'testpass'), 0, "authenticated");
+    my ($status, $val)= $mc2->set('x', "somevalue");
+    ok(! $status);
+
+    # This should fail because $mc is not authenticated
+    ($status, $val)= $mc->set('x', "somevalue");
+    ok($status, "this fails to authenticate");
+    cmp_ok($status,'==',ERR_AUTH_ERROR, "error code matches");
+}
+
 # check the SASL stats, make sure they track things correctly
 # note: the enabled or not is presence checked in stats.t
 
@@ -241,8 +273,8 @@ $empty->('x');
 
 {
     my %stats = $mc->stats('');
-    is ($stats{'auth_cmds'}, 2, "auth commands counted");
-    is ($stats{'auth_errors'}, 1, "auth errors correct");
+    is ($stats{'auth_cmds'}, 5, "auth commands counted");
+    is ($stats{'auth_errors'}, 3, "auth errors correct");
 }