[1/n] Add `id` column on tokens and sessions by david-crespo · Pull Request #8137 · oxidecomputer/omicron

david-crespo · 2025-05-12T19:08:26Z

Closes #8139. Token IDs are used in #8227 and #8231 for token CRUD.

WIP. Everything compiles and existing tests work, but we are not yet testing retrieval by ID and we need to think about authz checks on the datastore functions for retrieving tokens and sessions by token string.

This PR would be a lot shorter if not for the fact that the token column was the primary key on both tables, so adding the id requires a bunch of migration steps to change the primary key. There were also a lot of changes to code and tests around making things ID-centric instead of token-centric.

SQL migrations for adding id col and changing primary key to that
Add TypedUuid to session and token DB models
Update authz_resource! and lookup_resource! calls for new primary key
Update Session trait methods to use id instead of token
Update app and datastore session and token methods to use ID instead of token
Update two distinct but nearly identical FakeSession implementations (one for unit tests, one for integration tests) to a) track sessions in a Vec instead of a HashMap with token keys
Figure out authz situation in new fetch session/token by token string methods

nexus/db-lookup/src/lookup.rs

david-crespo · 2025-05-13T21:38:31Z

nexus/auth/src/authz/api_resources.rs

-    primary_key = String, // token
+    primary_key = { uuid_kind = AccessTokenKind },
    roles_allowed = false,
    polar_snippet = FleetChild,


I'm realizing FleetChild will probably have to change.

nexus/db-queries/src/db/datastore/console_session.rs

hawkw

Overall, this makes sense to me. I had some minor nitpicks and style suggestions.

nexus/auth/src/authn/external/session_cookie.rs

nexus/tests/integration_tests/authn_http.rs

schema/crdb/dbinit.sql

schema/crdb/token-and-session-ids/up01.sql

schema/crdb/token-and-session-ids/up02.sql

david-crespo · 2025-06-02T21:35:02Z

@hawkw I addressed all your feedback (I think) in f3bd37a

hawkw

Lovely, thanks for addressing my suggestions!

Closes #2302. Built on top of #8137, but doesn't really rely on much in there. Implementing [RFD 570](https://rfd.shared.oxide.computer/rfd/0570). - [x] Add `silo_settings` table with nullable device token expiration (seconds) column. Null setting means no max - Also populate this table with null maxes for all existing silos - [x] Follow behavior of quotas table: create silo settings entry on silo create, delete it on silo delete - [x] View and update settings endpoints at `/v1/settings` with all necessary plumbing - Following RFD 570, these are silo-scoped endpoints, only accessible to users inside the silo, as opposed to, for example, fleet operators being able to set it for any silo - [x] Users don't set token expiration directly yet — at token create time, if the current silo has its max set, the token gets an expiration timestamp based on that max. Otherwise the token does not expire - [x] Test that trying to set a negative or 0 TTL 400s - [x] Fix authz tests and verify endpoint tests - [x] Bikeshed endpoint paths, operation IDs, property names, etc

Built on top of #8137 and #8214. This is only for a user to list and delete their own tokens. It doesn't quite match RFD 570, which says `/v1/device-tokens` instead of `/v1/me/access-tokens`, but it feels good under `/v1/me`, and after trying to make the UI too, I think "access tokens" is much more intuitive. If I stick with this, I will update RFD 570 to match. ~~I'm not sure about the path `/v1/device-tokens` — in the API we call them `Device Access Tokens`. I think `/v1/access-tokens` might be more intuitive because the `device` is sort of an implementation detail, it refers to the OAuth device auth flow, which we are using. In practice, the user just gets a token with the CLI and pastes a code into the web UI and they don't have to think too much about it, so exposing that detail in the name might not be worth it.~~ Went with `/v1/me/access-tokens`. - [x] Basic token list and delete - [x] Basic integration tests - [x] Finalize endpoint paths - [x] Figure out authz story - Went with restricting datastore functions to current actor for now

Closes #8147. Built on #8137, #8214, and #8227. This is pretty straightforward, I think. The user gives us a TTL in seconds at token request time. We store it on the request row. When they come back in the later step to confirm the code and generate the token, we retrieve the TTL, validate that it is less than the silo max (if one is set), and we use it to generate the `time_expires` timestamp, which cannot be changed later. One slightly surprising bit is that we can't validate the TTL against the silo max at initial request time because we don't have any idea what silo the user is associated with until the confirm step. So probably want to make sure we are handling TTL validation errors nicely in the web console, because I think that's where they will show up.

Built on top of #8231. Fix for an issue with the internals of session expiration discussed in #8137 (comment). When I added ID primary keys to sessions and tokens, I changed the session delete method on the Session trait to operate by ID as well, and that meant a more complex authz situation compared to deleting by token. A bearer token is its own authz, at least according to the implicit policy we decided on. When I changed it to delete by ID, I made the delete function (now removed in this PR) use the user ID from the `opctx` to filter tokens to ensure that users could only delete their own sessions. The problem is that the `opctx` used in that case is the external authenticator op context, which has its own special actor that is distinct from the user in question. This means we would never actually delete a token this way because the query would always come up empty. This is not actually a problem from a correctness point of view because on subsequent requests we always check the expiration time on the session we pull out of the DB anyway. But it does mean that we probably end up with a lot more sessions piling up in the DB than we otherwise would. Not a big deal either way, but I feel more comfortable having it keep working the way it has worked for several years. It's also nice that we are deleting some app code here and replacing it with a test for a behavior that was previously untested. (I added the test in the first commit and confirmed that it fails without the changes from the second commit.)

david-crespo commented May 12, 2025

View reviewed changes

nexus/db-lookup/src/lookup.rs Show resolved Hide resolved

david-crespo force-pushed the token-session-ids branch from 48c4c68 to 7f377d8 Compare May 12, 2025 19:21

david-crespo commented May 13, 2025

View reviewed changes

david-crespo marked this pull request as ready for review May 20, 2025 15:27

david-crespo commented May 20, 2025

View reviewed changes

nexus/db-queries/src/db/datastore/console_session.rs Show resolved Hide resolved

david-crespo force-pushed the token-session-ids branch from ce269ce to 6fbe688 Compare May 23, 2025 04:32

david-crespo mentioned this pull request May 24, 2025

[2/n] Silo-level max device token TTL #8214

Merged

7 tasks

david-crespo changed the title ~~Add id column on tokens and sessions~~ [1/n] Add id column on tokens and sessions May 24, 2025

This was referenced May 27, 2025

[3/n] /v1/me/access-tokens list and delete #8227

Merged

[4/n] Let user request a specific TTL for a token #8231

Merged

david-crespo force-pushed the token-session-ids branch from 6fbe688 to a4d0d74 Compare May 30, 2025 16:33

david-crespo added 11 commits May 30, 2025 18:59

add IDs to tokens and sessions

fe237ec

use direct datastore method for session lookup by token

5f6c956

use TypedUuid for session and token IDs

abbe611

datastore method for access token lookup by token

d15117c

update primary key in schema.rs, fix lookups to use typed uuids

532ad47

update session methods to operator on ID instead of token

db83d55

make both FakeSession things work the same

bf5804d

migration before/after tests

f359c8b

do the right authz thing for lookups by token

f9f9d8f

tweak logout logic and comment better

424598b

on second thought, add hard delete by token

6f16cfe

david-crespo force-pushed the token-session-ids branch from a4d0d74 to 6f16cfe Compare May 31, 2025 00:00

hawkw self-requested a review June 2, 2025 17:27

hawkw reviewed Jun 2, 2025

View reviewed changes

eliza feedback

f3bd37a

hawkw approved these changes Jun 2, 2025

View reviewed changes

simplify migrations

ed2a9b2

david-crespo merged commit c0f8d23 into main Jun 3, 2025
16 checks passed

david-crespo deleted the token-session-ids branch June 3, 2025 16:04

david-crespo mentioned this pull request Jun 3, 2025

[5/n] Expire sessions by token again #8260

Merged

david-crespo mentioned this pull request Jun 24, 2025

Include tracking metadata with internal storage of user credentials #7818

Closed

This was referenced Aug 8, 2025

Audit log MVP #7339

Merged

Audit log: log ID of token or session used for authn #8813

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1/n] Add `id` column on tokens and sessions#8137

[1/n] Add `id` column on tokens and sessions#8137
david-crespo merged 13 commits intomainfrom
token-session-ids

david-crespo commented May 12, 2025 •

edited

Loading

Uh oh!

Uh oh!

david-crespo May 13, 2025 •

edited

Loading

Uh oh!

Uh oh!

hawkw left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

david-crespo commented Jun 2, 2025

Uh oh!

hawkw left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

david-crespo commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

david-crespo May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

david-crespo commented Jun 2, 2025

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

david-crespo commented May 12, 2025 •

edited

Loading

david-crespo May 13, 2025 •

edited

Loading