fix: Randomly derive UUIDs from non-conformant session IDs#51571
fix: Randomly derive UUIDs from non-conformant session IDs#51571codingllama merged 7 commits intomasterfrom
Conversation
codingllama
changed the title
github-actions
Bot
added
audit-log
There was a problem hiding this comment.
Wouldn't we rather do a migration (in the rdbms sense) to a different data type? As soon as we start deriving the session_id in a non-reversible way we won't have the option to do a migration in a quick-ish way, and we'd have to go through the whole events table to read the sid from the event data.
espadolini
left a comment
espadolini
left a comment
There was a problem hiding this comment.
I just realized that in a hypothetical future migration where we change data types we will only have to check the rows that have a UUIDv5 in them, which wouldn't be that bad, all things considered.
codingllama
force-pushed
the
codingllama/pgbe-session-id
branch
from
50a672b to
a96f721
Compare
There was a problem hiding this comment.
@espadolini, making this into a thread.
Wouldn't we rather do a migration (in the rdbms sense) to a different data type? As soon as we start deriving the session_id in a non-reversible way we won't have the option to do a migration in a quick-ish way, and we'd have to go through the whole events table to read the sid from the event data.
I'm wary of a system-driven alter table, in a table we potentially don't manage and whose size I can't determine. There's a lot that can go wrong in this kind of migration, starting with it just taking an unexpectedly long time.
We don't seem to ever read this value into the event, which makes the current approach possible.
I did consider adding a new "translation" table of sorts that records the {modified,original} UUIDs, so we could more easily recover the original (without unmarshaling the JSON data). We could pursue that, but I'm not it's necessary at this point.
I just realized that in a hypothetical future migration where we change data types we will only have to check the rows that have a UUIDv5 in them, which wouldn't be that bad, all things considered.
I suppose that's something. 😬
|
Rebased onto master and pushed a few changes I thought I had pushed before (mainly comments). Apologies for the noise, PTAL. |
codingllama
force-pushed
the
codingllama/pgbe-session-id
branch
from
a96f721 to
db14f87
Compare
|
Friendly ping @kopiczko. |
|
Thanks everyone! |
|
@codingllama See the table below for backport results.
|
* Tests event with non-UUID session ID * Randomly derive UUIDs from non-conformant session IDs * nit: Use "" instead of `` * Link godoc references to Log.deriveSessionID * Mention UUIDv5 for derived IDs * Do not log the session ID * Rename test helper, move truncateEvents out of it
* Tests event with non-UUID session ID * Randomly derive UUIDs from non-conformant session IDs * nit: Use "" instead of `` * Link godoc references to Log.deriveSessionID * Mention UUIDv5 for derived IDs * Do not log the session ID * Rename test helper, move truncateEvents out of it
…51645) * Tests event with non-UUID session ID * Randomly derive UUIDs from non-conformant session IDs * nit: Use "" instead of `` * Link godoc references to Log.deriveSessionID * Mention UUIDv5 for derived IDs * Do not log the session ID * Rename test helper, move truncateEvents out of it
…51644) * Tests event with non-UUID session ID * Randomly derive UUIDs from non-conformant session IDs * nit: Use "" instead of `` * Link godoc references to Log.deriveSessionID * Mention UUIDv5 for derived IDs * Do not log the session ID * Rename test helper, move truncateEvents out of it
…onal#51571) * Tests event with non-UUID session ID * Randomly derive UUIDs from non-conformant session IDs * nit: Use "" instead of `` * Link godoc references to Log.deriveSessionID * Mention UUIDv5 for derived IDs * Do not log the session ID * Rename test helper, move truncateEvents out of it
3 participants
Fix an issue where the Postgres event would drop App Access events due to it using a non-UUID session ID.
Instead of changing the App session ID this goes at the core of the problem, which is the Postgres events backend, and makes it resilient to all non-UUID session IDs.
Addressing the issue on App Access itself is possible, which would lower the bits of the App session ID (likely fine). The caveat is that it doesn't stop future mistakes, or addresses any other non-conformant session IDs that may already exist. Tackling the issue on Postgres increases the reliability of the events backend.
The session_id column is used to query events via SearchSessionEvents. SearchSessionEvents is hard-coded to a few event types, which already excludes App events. Nevertheless the PR tests querying events via its private counterpart, searchEvents. The session_id column is never read and returned as part of events, only written and used as a query filter.
Note that the transformation from "sessionID string" to "session_id UUID" is one-way. The original session ID is contained within the event itself and the column can be queried by transforming the original string, which seems adequate for the uses this sees today. If one wanted a direct column to the "unmodified" session_id this would need additional work, but that seems unnecessary for now. The PR is explicitly trying to avoid schema changes, so in this light the solution presented here is ideal.
UUIDs derived from non-standard session IDs use UUIDv5, instead of the usual UUIDv4 from uuid.Parse. That could be used to as a hint to distinguish them from the "original" UUIDs.
#46207
Changelog: Fixes issue where the Postgres backend would drop App Access events