Deadlock when using WritableSession in middleware layer

[mtorromeo]

Hi,
I am using axum-sessions inside a middleware layer that is checking if the user is logged in and short-circuits the router with a StatusCode::UNAUTHORIZED response if they are not.

If I then try to use a WritableSession in a route that is "behind" this middleware layer I get a deadlock that I am guessing is caused by the internal RWLock that is being acquired while the middleware layer is still holding a reference to the ReadableSession.

For the time being I am going to work-around this issue by bypassing the middleware when I need a WritableSession but is there a way to release the ReadableSession from the middleware after I am done using it so that I can avoid this issue?

Here's a minimal POC:

Cargo.toml

[package]
name = "axum-sessions-deadlock"
version = "0.1.0"
edition = "2021"

[dependencies]
axum = "0.6"
axum-sessions = "0.5"
hyper = "0.14"
rand = "0.8"
tokio = { version = "1", features = ["full"] }
toml = "0.7"
tower-http = "0.4"

main.rs

use axum::http::Request;
use axum::middleware::Next;
use axum::response::Response;
use axum::{routing::get, Router};
use axum_sessions::extractors::{ReadableSession, WritableSession};
use axum_sessions::{async_session::MemoryStore, SessionLayer};
use hyper::StatusCode;
use rand::RngCore;

#[tokio::main]
async fn main() {
    let store = MemoryStore::new();
    let mut secret = [0u8; 128];
    rand::thread_rng().fill_bytes(&mut secret);
    let session_layer = SessionLayer::new(store, &secret).with_secure(false);

    let app = Router::new()
        .route("/", get(deadlock))
        .layer(axum::middleware::from_fn(auth_middleware))
        .layer(session_layer);

    eprintln!("Listening on 127.0.0.1:3000");

    axum::Server::bind(&"127.0.0.1:3000".parse().unwrap())
        .serve(app.into_make_service())
        .await
        .unwrap();
}

pub async fn auth_middleware<T>(
    session: ReadableSession,
    request: Request<T>,
    next: Next<T>,
) -> Result<Response, StatusCode> {
    // e.g: check if user is logged in
    // let login: Option<String> = session.get("login");
    // match login {
    //     None => Err(StatusCode::UNAUTHORIZED),
    //     Some(_) => Ok(next.run(request).await),
    // }

    // assume logged-in
    Ok(next.run(request).await)
}

async fn deadlock(mut session: WritableSession) -> StatusCode {
    // e.g: session.destroy();
    StatusCode::NO_CONTENT
}

Thanks

[maxcountryman]

This is a side effect of our inability to clone the session. There's upstream work that address this, but it hasn't been merged and I don't know when it will be, unfortunately.

[maxcountryman]

Please see #50.

[maxcountryman]

Hi again, I'm also working on a new direction which would address this among other things. Please see #56.

[maxcountryman]

If you're up for trying an alternative, I've released v0.1.0 of tower-sessions.

This new crate no longer uses async-session and does not need to lock the session in the same manner. (It does use a lock, but follows the same pattern of other tower middleware, like tower-cookies, which take a lock over an inner state only when making operations over that state.)

I'd love feedback if you do end up trying it.

[mtorromeo]

Hi, it doesn't seem to support cookie signing, or am I missing something?

Other than that, I've looked at the examples, and it seems like it would be a good alternative.

[maxcountryman]

@mtorromeo it does not use signing, correct. It's technically capable of it (it uses tower-cookies which uses the cookie crate and these both support signing and encryption). But it's not clear what the benefit of signing is with the cookie only holding a UUID v4 as a pointer to the session. UUID v4 is securely generated with getrandom (this must be verified per platform tho) and we could sign this but again I'm not sure of that value in doing that. Let me know your thoughts.

[mtorromeo]

No, I think you are right. If the library is designed to store just the session id, there is limited value in signing the cookie as long as the UUIDv4 are generated correctly.

Signing would just protect against attacks on the UUID when using non-cryptographically secure random number generators (or using them incorrectly).

I'm fine with the current design. Thanks!

[maxcountryman]

Looking at the uuid docs, we could also provide a custom generator to make this guarantee stronger. I'm happy to incorporate this in future iterations of the tower-sessions crate if folks find that valuable.

And thank you for checking it out. I appreciate your feedback! 😁