Session is not removed from the ClusteredSessionStore, if the session is destroyed

[fbuetler]

Version

4.5.8

Context

We are using the SessionHandler together with an AuthenticationHandler.

Expected

If destroy() is called on a Session, we'd expect a session to be deleted from the SessionStore and further requests with this session are required to be re-authenticated.

Actual

In case of a LocalSessionStore, it is conforming with our expectation. However, in case of ClusteredSessionStore, the user-agent is able to do further requests without any re-authentication.

We assume the reason behind this, is that the state of the session is not synchronized with the session store. Only the session id, timeout, lastAccessed, version and data are synchronized, but the state further includes destroyed, renewed, oldId and crc.

  @Override
  public void writeToBuffer(Buffer buff) {
    byte[] bytes = id().getBytes(UTF8);
    buff.appendInt(bytes.length).appendBytes(bytes);
    buff.appendLong(timeout());
    buff.appendLong(lastAccessed());
    buff.appendInt(version());
    writeDataToBuffer(buff);
  }

Source

The functionality to remove the session from the session store in the SessionHandler upon a request with a destroyed session, is therefore not triggered. Therefore, a destroyed session can still be used (as similarly pointed out earlier #329 (comment)).

Our proposed fix is to include the session state in the synchronization.

If the proposed fix is accepted, I volunteer to create a pull request.

[tsegismont]

Thanks for the report @fbuetler

We have io.vertx.ext.web.tests.handler.SessionHandlerTestBase#testDestroySession which is tested with clustered session store as well (in Vert.x Web and Vert.x Infinispan).

Any idea about why this test doesn't capture the problem?

[fbuetler]

Thank you for the rapid response and for pointing me to the test case.

The ClusteredSessionHandlerTest, and with that the SessionHandlerTestBase, is using the FakeClusterManager. The FakeClusterManager is using a LocalAsyncMap as its SharedDataImpl.getClusterWideMap implementation, just like the LocalSessionStore is using a LocalMapImpl.

In the end, both LocalMapImpl and LocalAsyncMapImpl are using a ConcurrentMap as its underlying implementation.
Therefore, the problematic code in SharedDataSessionImpl is not used. It is first used deep down in the vertx-hazelcast package.

[fbuetler]

I apologize, I overlooked, that you also mentioned vertx-infinispan.

AFAICT, to hit the problematic code in SharedDataSessionImpl in the testDestroyClusteredSession test case, one would need to sync an object that is not Serializable. Interestingly, only NodeInfo and String (session ID) are observed there (checked with a debugger). Moreover, only the remove method calls toCachedObject, hence the session is never actually stored in the infinispan. Why this is the case, I currently don't know.

[fbuetler]

The reason for the session never actually being stored in infinispan, is because a session is only flushed at the end of a request being processed. As we create and destroy the session in one request, it is never actually added to the session store, but only removed, as observed in the previous comment.

[fbuetler]

Maybe something like this would be more appropriate:

@Test
public void testDestroySession() throws Exception {
	// given
	router.route().handler(SessionHandler.create(store));
	final AtomicReference<String> sid = new AtomicReference<>();
	final AtomicInteger requestCount = new AtomicInteger();
	router.route().handler(rc -> {
		Session sess = rc.session();
		assertNotNull(sess);
		assertTrue(System.currentTimeMillis() - sess.lastAccessed() < 500);
		assertNotNull(sess.id());
		switch (requestCount.get()) {
		case 0:
			sid.set(sess.id());
			sess.put("foo", "bar");
			break;
		case 1:
			assertEquals(sid.get(), sess.id());
			// Destroy session in a follow-up request, so that it has been flushed to
			// the session store
			sess.destroy();
			break;
		case 2:
			assertFalse(sid.get().equals(sess.id())); // New session
			assertNull(sess.get("foo"));
			sid.set(sess.id());
			sess.destroy(); // Destroy session in the same request as it is created
			break;
		}
		requestCount.incrementAndGet();
		rc.response().end();
	});

	// when
	final AtomicReference<String> sessionCookie = new AtomicReference<>();
	testRequest(HttpMethod.GET, "/", null, resp -> {
		assertTrue(resp.headers().getAll("set-cookie").size() == 1); // expect new session cookie only
		String setCookie = resp.headers().get("set-cookie");
		assertNotNull(setCookie);
		sessionCookie.set(setCookie.split(";")[0]);
	}, 200, "OK", null);
	testRequest(HttpMethod.GET, "/", req -> req.putHeader("cookie", sessionCookie.get()), resp -> {
		assertTrue(resp.headers().getAll("set-cookie").size() == 1); // expect expired session cookie only
	}, 200, "OK", null);
	testRequest(HttpMethod.GET, "/", req -> req.putHeader("cookie", sessionCookie.get()), null, 200, "OK", null);

	// then
	Thread.sleep(500); // Needed because session.destroy is async
	CountDownLatch latch = new CountDownLatch(1);
	store.get(sid.get(), onSuccess(res -> {
		assertNull(res);
		latch.countDown();
	}));
	awaitLatch(latch);
}

What do you think @tsegismont ?

(This does not test original issue, that we reported, but makes sure the session is actually flushed to the session store)

[fbuetler]

Further, I am looking into a way to use a LocalAsyncMap in the FakeClusterManager, that is actually serializing and deserializing the value it is storing.

[fbuetler]

I created a test case that replicates our problem. It is failing in vertx-infinispan and succeeds with this fix.

We think the main issue is that session.destroy() does not necessarily flush the session destruction to infinispan i.e., remove the session. In our scenario, a client has a session with the server. A third party would like to destroy the session of the client (think of OIDC back-channel logout). Simply loading the session from the session store and calling destroy() on it won't remove it from the session store, as this logic happens with the session of the routing context, i.e., not the client session.

In this scenario, an out-of-band session.destroy() (i.e. not the session associated with the routing context) should use sessionStore.remove(id) instead of sessionStore.get(id).onSuccess(s -> s.destroy()).

In general, we think the SessionHandler along with its session state handling is only working correctly, if the client owning the session is doing the request.

    @Test
    public void testOutOfBandDestroyClusteredSession() throws Exception {
        // given
        router.route().handler(SessionHandler.create(store));
        final AtomicReference<String> sid = new AtomicReference<>();
        final AtomicInteger requestCount = new AtomicInteger();
        final CountDownLatch latch = new CountDownLatch(1);

        router.route().handler(rc -> {
            Session sess = rc.session();
            assertNotNull(sess);
            // assertTrue(System.currentTimeMillis() - sess.lastAccessed() < 500);
            assertNotNull(sess.id());
            switch (requestCount.get()) {
                case 0:
                    sid.set(sess.id());
                    break;
                case 1:
                    // out of band session.destroy
                    store.get(sid.get(), onSuccess(s -> {
                        assertNotNull(s);
                        s.destroy();
                        store.put(s, onSuccess(v -> latch.countDown())); // manually flush the session
                    }));
                    break;
                case 2:
                    // then
                    assertTrue(sess.isDestroyed());
                    assertTrue(String.format("should have new session id: %s, actual %s", sid.get(), sess.id()),
                            sid.get().equals(sess.id())); // Still the old session
                    sess.data(); // Session is lazily regenerated on data access
                    assertTrue(String.format("should have new session id: %s, actual %s", sid.get(), sess.id()),
                            !sid.get().equals(sess.id())); // New session
                    break;
            }
            requestCount.incrementAndGet();
            rc.response().end();
        });

        // when
        final AtomicReference<String> sessionCookie = new AtomicReference<>();
        testRequest(HttpMethod.GET, "/", null, resp -> {
            assertTrue(resp.headers().getAll("set-cookie").size() == 1); // expect new session cookie only
            String setCookie = resp.headers().get("set-cookie");
            assertNotNull(setCookie);
            sessionCookie.set(setCookie.split(";")[0]);
        }, 200, "OK", null);
        Thread.sleep(500); // Needed because session.flush is async

        testRequest(HttpMethod.GET, "/", null, null, 200, "OK", null);
        awaitLatch(latch);

        testRequest(HttpMethod.GET, "/", req -> req.putHeader("cookie", sessionCookie.get()), null, 200, "OK", null);
    }

[fbuetler]

The more I think about it, the less I think the whole session state (like regenerated, oldId and crc) should be synced in the cluster store. The SessionHandler is only capable to handle local session state.

One could argue that the destroyed session state could be synced to simplify out-of-band session destruction.

Conclusion: To implement out-of-band session destruction, one has to use sessionStore.delete(id) instead of sessionStore.get(id).onSuccess(s -> s.destroy()), as session.destroy() only works for the "normal" path in the SessionHandler.

Nevertheless, these test cases still do not cover serialization/deserialization of objects stored in the ClusteredSessionStore:

We have io.vertx.ext.web.tests.handler.SessionHandlerTestBase#testDestroySession which is tested with clustered session store as well (in Vert.x Web and Vert.x Infinispan).

[tsegismont]

Thanks for sharing your findings @fbuetler, this is of great value

We're focused on getting Vert.x 5 out and I can't look at them right now, I'll come back to you as soon as I can