Skip to content

[nexus] Store expiration times on session records #8625

@david-crespo

Description

@david-crespo

Currently we store sessions with their created time and last used time,

omicron/schema/crdb/dbinit.sql

Lines 2424 to 2430 in 1c53bcd

CREATE TABLE IF NOT EXISTS omicron.public.console_session (
id UUID PRIMARY KEY,
token STRING(40) NOT NULL,
time_created TIMESTAMPTZ NOT NULL,
time_last_used TIMESTAMPTZ NOT NULL,
silo_user_id UUID NOT NULL
);

and then in the authentication code we calculate whether they are expired at runtime based on these config values.

// if the session has gone unused for longer than idle_timeout, it is
// expired
let now = Utc::now();
if session.time_last_used() + ctx.session_idle_timeout() < now {
let expired_session = ctx.session_expire(token).await;
if expired_session.is_none() {
debug!(log, "failed to expire session")
}
return SchemeResult::Failed(Reason::BadCredentials {
actor,
source: anyhow!(
"session expired due to idle timeout. last used: {}. \
time checked: {}. TTL: {}",
session.time_last_used(),
now,
ctx.session_idle_timeout()
),
});
}
// if the user is still within the idle timeout, but the session has
// existed longer than absolute_timeout, it is expired and we can no
// longer extend the session
if session.time_created() + ctx.session_absolute_timeout() < now {
let expired_session = ctx.session_expire(token).await;
if expired_session.is_none() {
debug!(log, "failed to expire session")
}
return SchemeResult::Failed(Reason::BadCredentials {
actor,
source: anyhow!(
"session expired due to absolute timeout. created: {}. \
last used: {}. time checked: {}. TTL: {}",
session.time_created(),
session.time_last_used(),
now,
ctx.session_absolute_timeout()
),
});
}

This is fine for the authentication code because it runs at top level in Nexus and has access to the config (SessionStore is a trait implemented on the ServerContext struct). However, when we list sessions for a user (added in #8479), that query requires us to filter out expired tokens, and we do not have access to the server context all the way down in the datastore crate, so we have to pass it down from the handler. This works but feels very janky and it also has the dubious consequence that session expiration times for existing sessions (worst of all, ones that have already expired but are still in the database) can be changed simply by changing the Nexus config and restarting Nexus.

It would be more solid to store explicit idle and absolute expiration times on the session record, analogous to the time_expires column on the tokens table. Instead of updating the last use time on every use, we would update the idle expiration time. Period. The absolute expiration time would never change after creation.

There is some question around what happens when we add the ability for the operator to change the idle and absolute expiration time (#5477, #7475). The most plausible approach off the top of my head would be to say expiration times can only get earlier, but never later. If the operator shortens the TTL, we update the times on all sessions that become expired as a result of that change, but we would never lengthen the time for existing sessions if they change the TTL to be longer.

omicron/schema/crdb/dbinit.sql

Lines 2846 to 2855 in 1c53bcd

CREATE TABLE IF NOT EXISTS omicron.public.device_access_token (
id UUID PRIMARY KEY,
token STRING(40) NOT NULL,
client_id UUID NOT NULL,
device_code STRING(40) NOT NULL,
silo_user_id UUID NOT NULL,
time_requested TIMESTAMPTZ NOT NULL,
time_created TIMESTAMPTZ NOT NULL,
time_expires TIMESTAMPTZ
);

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions