RFC: preventing id collisions and refactoring...thoughts?

[wt]

So, I have been working on banging out some experiments on how to change the code in order to prevent id generation collisions (after discussion on #169). Due to the need for a major version change for any of the real improvements, I think it would be better to engage with this in a more iterative fashion. With regard to changing and maintaining, I have run into the following serious problems with the code:

• unit test coverage is almost non-existent
• the files are just too damn big

So, I think this gives us an opportunity to do some pretty serious refactoring. You can see the very beginnings of where I am heading at here. This branch is currently a PoC and not ready for merging. It's just to give a little structure to this conversation.

The important piece of those changes is that I am introducing testing infrastructure and completely new types called NewSessionStore and NewSession. These will be used to make any incompatible changes with the current SessionStore and Session. I will try to factor as much code out of Session and SessionStore so that it is used by the old and the new versions. Once that is in progress, I am hoping a more concrete plan for making the major version change can be formed.

As it stands, I will probably hide the new stuff behind a feature flag that we can make not default until the major version change. This is not done in the branch, I will probably modify the branch to include a commit adding the feature before adding the types.

The next steps I plan on taking are literally breaking up some of the files (especially tower-session-core/src/session.rs).

I am also pretty uncomfortable with the logic that's currently embedded into the Session impl. I am hoping we can zhuzh that around a bit to make testing easier.

Does anyone, especially @maxcountryman, have any thoughts about this?

Thanks! wt

[maxcountryman]

Thanks for working on this! I appreciate you taking an interest in this tricky problem.

I've looked over the branch you mentioned and here are a few quick thoughts:

At a high level I would prefer we not introduce a new type and instead rip the bandage off. Because we're pre-1.0, we have more leeway here to make breaking changes. I'd also rather introduce breaking changes than introduce a second set of types which need to be maintained and will ultimately need to be removed anyway.

On a more granular level, I'm not sure the OnceCell approach will work and more specifically we will want to ensure that we are not removing the trace logs in the process: the suspicious activity warning is required by OWASP, for instance. Moreover the semantic of Option regarding the record is important and so either it shouldn't be changed or if it is it needs to be done so carefully so as not to change the current semantic.

unit test coverage is almost non-existent

Just to make sure we're on the same page here, we have around 70% coverage, with doctests providing unit-style tests of the session interface (these are run by CI). I'm not opposed to adding more tests, extending tests by adding a test mod, etc but I did want to make sure you had seen those.

Regarding testing, I think it could make sense to introduce tests independently from the interface changes: those we can merge right away and continue to extend as the interface is evolved to address the collision problem.

Please let me know how I can be helpful. 😁

[wt]

I went ahead and submitted a PR for the test I wrote for the expiry time generation. This has nothing to do with the changes of the types for the store (and any related changes).

[wt]

So, I gave some thought to the OnceCell method I started implementing. I do think it can work as long as we say that only a mutable session can have an id changed.

I have an implementation that should allow something like that following for a known new session creation:

let session = Session::new(session_store, expiry); //notice there is no id here

// Takes a &self and causes creating a new session in the store since the id has
// never been set. Also, record attr will be set to include no data in the session.
info!("session: {:}", session.id());

// doesn't cause a load from the store as the record was loaded before 
info!("record: {:?}", session.record());

Interior mutability would be needed to update the record data, like adding new keys for the session data, so you'd probably really want a let mut session in the example above to allow mutating the data in the session.

For a load of an existing session, it would look like this:

let session = Session::new(session_store, expiry);

// tries to load the record and id from the data store. If successful, sets the id
// and record attrs. If not, returns an error to be handled. This could be an error
// indicating a retryable error (e.g., couldn't talk to DB) or a unretryable error
// (e.g., session doesn't exist in store or maybe session is expired in store). Proper
// OWASP logging can happen here as it does today.
try_from_existing_id(&id)?;

// assuming no error from above; this will cause no hit to the store since that
// was already loaded during the try_from_existing_id call above
info!("session: {:}", session.id());

 // again, no hit to the store
info!("record: {:?}", session.record());

It's possible this cannot work, but I am part of the way there at this point. This experiment has not been reflected in the branch linked above. It's not ready for sharing yet.

[maxcountryman]

I did update the interior mutability impl to use a MappedMutexGuard such that we don't need to return Option from get_record anymore, which does improve ergonomics and clarity imo.

If OnceCell could somehow make the interface over Record sync that would be nice but I would think that's not possible so I'm not exactly sure what the benefit of it would be?

[wt]

I'll look into that. I hadn't really settled the type for the record attr yet. I just needed something there. I need to finish the experiment.

[wt]

And FWIW, once cell is probably not the right type for the record attr. It's quite nice for the id attr.

[maxcountryman]

A thought I had here to help ease the transition to a new method could look something like this:

#[async_trait]
pub trait SessionStore: Debug + Send + Sync + 'static {
   /// A method for saving a session in a store.
    async fn create(&self, session_id: &Id) -> Result<()> {
        self.default_create(session_id).await
    }

    #[deprecated(note = "Please implement the `create` method directly. This default implementation will be removed in the future.")]
    async fn default_create(&self, session_id: &Id) -> Result<()> {
        self.save(session_id).await
    }

    /// A method for saving a session in a store.
    async fn save(&self, session_record: &Record) -> Result<()>;

    /// A method for loading a session from a store.
    async fn load(&self, session_id: &Id) -> Result<Option<Record>>;

    /// A method for deleting a session from a store.
    async fn delete(&self, session_id: &Id) -> Result<()>;
}

[maxcountryman]

It's probably easier to look at something a little more concrete, so I fleshed this out a bit.

I have to say, I'm not too unhappy with this: it's a smaller change than I think it first seemed was required and I like the fact the create method is clear about taking exclusive ownership of the record (which also makes its usage cleaner).

[wt]

I'm not crazy about the store handling the conflict resolution. It feels similar to the problem of bufferbloat. I need to think about this.

My initial concerns are that it puts unknown load on the store due to the possibility of the transparent retries. There is probably a small chance of that due to the 2^128 space of the id.

However, hiding that kind of problem from the service creating the new session and delivering it to the user seems bad. I really can't shake the feeling that he user of the session object should be making decision about retry policies. For example, there may be cases where I would never want to retry and would prefer to just return a 5xx error to the client. I would think that wouldn't be most use cases, but that possibility is gone with your design. Also, you design retries as fast as possible. Should be there some kind of exponential backoff?

I'm really worried about something like this at scale that tries to hide such errors. I've been bitten by many systems that try to transparently retry without notifying the upper layers that such a thing is happening. There is almost always some unintentional destructive behavior that eventually emerges. My general inclination is to fail fast and let the upper layers choose how and when to retry.

And to be more concrete, I think the SessionManager layer should be the place where the retry logic lives, and the SessionLayerManager should be where the policy for the logic is configured. And have something like the following when initializing it:

let session_store = BlahSessionStore(...);
SessionManagerLayer::new(session_store)
    .with_same_site(SameSite::Lax)
    .with_retry_policy(RetryPolicy::builder()
        .with_id_collision_retries(3)
        .build())

Honestly, I think that being able to set the policies like that will also make the objects much easier to test since the policy is separated from the logic in a cleaner way.

Having said that, I think that your design will be better than where it stands today.

[wt]

This article seems somehow relevant: https://www.maxcountryman.com/articles/let-it-fail

[maxcountryman]

Thank you for sharing your thoughts and concerns. I appreciate the depth of your analysis and your commitment to improving the project.

The design direction you suggest is indeed one possibility. However, it diverges from the core philosophy of this crate, which leans towards leaving such decisions to the discretion of store implementers. That said, I do see the value in your approach, and I can envision a version of BlahSessionStore that provides the interface you've described:

let session_store = BlahSessionStore(...)
    .with_retry_policy(
        RetryPolicy::builder()
            .with_id_collision_retries(3)
            .build()
    );
let session_layer = SessionManagerLayer::new(session_store)
    .with_same_site(SameSite::Lax);

This approach allows session stores, which are types we control, to offer extended interfaces that cater to specific needs. While defining an interface for store retry policies might be beyond the scope of this crate, it is certainly within the scope of SessionStore implementations. In fact, such opinionated implementations could be published as their own crates, allowing for greater flexibility and customization.

With all this in mind, my plan is to introduce the create method I've outlined on the branch above and move toward cutting a new release to ensure we're setting folks up for mitigation on a reasonable timeline.

Thank you again for reporting this issue and thinking through the design space. It's a tricky balance, especially considering this thing is already out in the wild with active users depending on our API.

[wt]

I like that idea. Allows the stores to implement whatever config they want. That seems like a relatively good idea. If you find there are common configurations, you could always add that to the interface later.

I think this is a good incremental step. Thanks for working with me on this.

[maxcountryman]

Regarding testing, I'm adding some additional service tests as we work on introducing signing and encryption over here: 90eb541

[wt]

Thanks for the pointer. I will check that out.