auth: periodically clean up expired sessions in the web_sessions system table

[dhartunian]

Is your feature request related to a problem? Please describe.
In the long run, the web_sessions table will currently continue to grow and accumulate expired sessions indefinitely.

Describe the solution you'd like
The table should either delete a user's old sessions upon creating a new session for them, or periodically delete expired sessions from the table to keep it from growing indefinitely.

Describe alternatives you've considered
N/A

Additional context
N/A

[blathers-crl]

Hi @dhartunian, I've guessed the C-ategory of your issue and suitably labeled it. Please re-label if inaccurate.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[knz]

I have investigated this a little bit.

• We want auto-expiration for the following conditions:

  • expiresAt is older than a configurable interval (new cluster setting server.web_session.purge_ttl), default 1 hour.
  • revokedAt is older than the same interval.
  • lastUsedAt is older than another configurable interval (new cluster setting server.web_session.auto_logout_timeout), with a default of 1 week.

• We should also be careful to not require a full table scans for these purges. Assuming an index on each of these columns, we should use 3 different DELETE statements, not just 1 with a combined condition. It's also probably necessary to spell out the index name explicitly e.g. DELETE FROM system.web_sessions@"web_sessions_expiresAt_idx" .... An EXPLAIN statement should confirm that the index is used.

• In reference to the index assumption above: we already have SQL indexes on expiresAt, but not on the two other columns. Therefore, we should add additional indexes.

• Even with 3 separate DELETE statements, we should minimize the amount of back-and-forth between the authentication layer and SQL. Therefore, we should use a single multi-CTE statement (e.g. WITH a AS (DELETE ...), b AS (DELETE ...), ...)

• It may also be worth bunching these DELETEs together with the existing INSERT statements to ensure that the inserts never catch up with deletes, e.g. WITH a AS (DELETE) INSERT ..., WITH a AS (DELETE) UPDATE.

So here's a possible strategy:

1. add the 2 cluster settings.
2. implement a new unit test TestPurgeSession in server/authentication_test.go that does the following:
   a. creates a test server.
   b. customizes the cluster settings to a lower value than defaults.
   c. inserts some seemingly-old entries in system.web_sessions using explicit SQL inserts. The age should be chosen so that it's older than the configured setting values, but younger than what would be expired assuming defaults. (This is important to check that the settings are effective.)
   d. calls ( *authenticationServer) newAuthSession()
   e. verifies that newAuthSession() has deleted the test entries created at the 3nd step.

The code of TestCreateSession can be used for inspiration.
Of course at this point the new test is going to fail because the logic is not yet implemented.

Then:

3. create a new SQL migration to add the missing indexes on revokedAt and lastUsedAt. May need to use the new "long-running migrations" mechanism.
4. modify ( *authenticationServer) newAuthSession() to add the DELETE statements as CTEs for the existing INSERT statement.
5. confirm that the new test passes.

[cameronnunez]

• lastUsedAt is older than another configurable interval (new cluster setting server.web_session.auto_logout_timeout), with a default of 1 week.

@knz To be clear, the server.web_session_timeout cluster setting is used to mark an entry as expired (by default 1 week after being created), while the server.web_session.auto_logout_timeout proposed cluster setting is used to delete entries that have not be used for at least the length of this duration, correct?

[knz]

the server.web_session_timeout cluster setting is used to mark an entry as expired (by default 1 week after being created)

yes. You can think about "expiration timeout" as "after this time, the user cannot login any more"

the server.web_session.auto_logout_timeout proposed cluster setting is used to delete entries that have not be used for at least the length of this duration, correct?

yes. with emphasis on "have not been used".
You can think about "auto logout timeout" as "after this time, the user has not actually logged in any more" (assuming they could).