Add UserLogToken: reversible pseudonyms for user ids in logs - #74609
Add UserLogToken: reversible pseudonyms for user ids in logs#74609alex-m-brown wants to merge 11 commits into
Conversation
Introduce lib/cdo/user_log_token.rb, which mints and resolves "log tokens": reversible pseudonyms of user ids, so raw user ids stay out of our logs and out of third-party systems. Keys come from the new user_log_token_keys config, a JSON object of version => 32+ random bytes (base64). The highest version encrypts; every version is retained so previously written tokens stay readable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…into ai-gateway-observability/user-log-token-lib
cd0b305 to
2baa3a4
Compare
Introduce lib/cdo/user_log_token.rb, which mints and resolves "log tokens": reversible pseudonyms of user ids, so raw user ids stay out of our logs and out of third-party systems. Keys come from the new user_log_token_keys config, a JSON object of version => 32+ random bytes (base64). The highest version encrypts; every version is retained so previously written tokens stay readable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2baa3a4 to
c5dc145
Compare
…/github.com/code-dot-org/code-dot-org into ai-gateway-observability/user-log-token-lib
| @@ -0,0 +1,265 @@ | |||
| # frozen_string_literal: true | |||
There was a problem hiding this comment.
Since our User object belongs to our Rails application, could this be implemented as a User Model Concern in dashboard/app/models/concerns/user?
unlox775-code-dot-org
left a comment
unlox775-code-dot-org
left a comment
There was a problem hiding this comment.
This looks technically solid for what it is trying to do, and I appreciate the care in the implementation. 💪
My main concern is the long-term operational cost: rotation is basically forward-only, and if we want to keep resolving historical tokens, we have to keep old keys (and key-version history) around as long as those logs matter. So this can become a durable key-management burden unless we set very clear retention/decode windows up front.
The bigger point is the one from our existing discussion: this feels like a side effect of the current client-relay approach to AI gateway identity/context. This token design can help with representation, but it doesn’t really change that underlying security concern by itself. If we don't relay through an untrusted client, we don't need to worry about a pseudonym, or this key-rotation tech burden.
So I’m good with this as a primitive, but I’d strongly pair it with:
- explicit key retention/retirement policy, and
- follow-through on the server-to-gateway architecture direction from the other thread.
Totally fair. Our current policy is data retention for 90 days.
Sentry cleanly abides by that policy. Cloudflare is a little hairier. They currently only offer retention policies based on the number of logs, not by their age. To workaround, we could make a cron job that deletes logs out of policy. Until that is implemented, while traffic is still relatively low, we can estimate how many logs 90 days would be (something closer to 2mil by some rough math). That way, we really only need to retain 2 keys at a time if we rotate every 90 days
Understood, but I think this set of PRs that I'm proposing solves a different issue than the ai-gateway issue. Legal has concerns about logging raw user ids in 3rd party observability platforms. While this change was driven by the ai gateway work, it has broader implications that apply to any logging software we use. Tagging in @edcodedotorg regarding the security concerns with gateway itself. |
unlox775-code-dot-org
left a comment
unlox775-code-dot-org
left a comment
There was a problem hiding this comment.
Awesome work Alex! Thank you!
Review asked for this to live with the model it describes rather than in lib/. Cdo::UserLogToken becomes User::LogToken at dashboard/app/models/concerns/user/log_token.rb, and User includes it. The concern adds three ways in, matching how callers actually hold a user: user.log_token(destination:) a loaded record User.log_token_for(user_id, destination:) an id and no record User.resolve_log_token(token, actor_id:, reason:) the audited direction log_token_for exists because the admin lookup page derives from an id it deliberately does not load a row for. The cipher stays on User::LogToken itself rather than moving into class_methods, so User does not gain private class methods named keys, parse, decrypt and audit. Behaviour is unchanged. The test moves with it, from the lib suite to dashboard/test/models/concerns/user/log_token_test.rb, and now runs under Rails, which the lib suite could not do on a machine without a seeded dashboard_test database. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| # Broad on purpose. CDO.user_log_token_keys is a lazy !Secret. The first | ||
| # read can raise an error from Secrets Manager or from JSON. derive must | ||
| # not raise an error for any cause. | ||
| disabled("could not be read: #{exception.class}: #{exception.message}") |
There was a problem hiding this comment.
Wouldn't this potentially cause keys to be exposed in logs?
| end | ||
|
|
||
| private def parse_keys | ||
| raw = CDO.user_log_token_keys |
There was a problem hiding this comment.
Since this is a secret, should we guard it as optional? We need to ensure open source edition of the repo works properly as well. Based on my understanding, this could cause a runtime error if we don't make the secret optional (you can probably test this by excluding the key in locals.yml).
| raise ArgumentError, 'reason is required to resolve a user log token' if blank?(reason) | ||
|
|
||
| result = decrypt(token) | ||
| audit(result: result, actor_id: actor, reason: reason, request_id: request_id) |
There was a problem hiding this comment.
To note, this is technically bypassable in productinon console. Do we want to ensure audit even in production console?
There was a problem hiding this comment.
If the intended usage is ONLY for Sentry, would it make sense for this to live in the Observability engine directly?
4 participants
Adds
lib/cdo/user_log_token.rb, a library for turning a user id into an opaque token that can be written to logs and sent to third parties, and turned back into a user id by someone holding the key.Nothing calls it yet. This PR is the primitive and its configuration only.
Why
Log and telemetry records need to be correlated per user -- to answer "did this one account generate all of these errors" -- but raw user ids should not sit in our logs or in a vendor's data. A one-way hash gives correlation and loses the ability to answer the question in the other direction, which we need for support and abuse work
Construction
AES-256-GCM. The IV is derived by HMAC of the plaintext under a subkey separate from the encryption key, so the same user produces the same token. Both subkeys are HKDF-derived from the configured key; the configured key is never used directly as a cipher key.
The plaintext is the destination and the user id padded to a fixed width. Without the padding, GCM ciphertext length equals plaintext length, so token length would reveal how many digits the user id has -- and since ids are sequential, that is a proxy for account age.
Determinism is required for correlation and necessarily leaks equality: anyone can see that two records belong to the same user.
The destination is inside the encrypted plaintext, not alongside it, so each destination yields an unrelated token for the same user. Today there is one destination,
sentry.Keys and rotation
CDO.user_log_token_keysholds a JSON object of version => 32-or-more random bytes, base64. The highest version encrypts; every version is retained for decryption, and tokens carry their version as av<n>.prefix. A key must outlive the longest retention window of any destination carrying its tokens -- unlike a hash, a key we no longer hold is data we can never read again.derivenever raises on a missing or malformed key. It sits on a per-request path, so a misconfiguration costs a debugging dimension rather than the site, and there is deliberately no fallback to the raw id. A bad configuration warns once and disables tokens.The governed direction
resolverequiresactor_idandreason, and writes the audit record itself rather than leaving that to a controller -- a controller-level audit is bypassed by anyone with a production console.actor_idmust be a user id; anything elseraises rather than silently auditing user 0. Failed resolves are audited too, since a burst of them is a signal. The record deliberately omits the token: the audit is readable by more people than the key is, and a log of token => user id
pairs would be a lookup table for exactly the thing being protected.
Deployment note
user_log_token_keysis a new!Secret. Until it is provisioned per environment,derivereturns nil and logs one warning. Generate a key with:Testing
lib/test/cdo/test_user_log_token.rbcovers determinism, Integer/String id equivalence, destination binding, round-trip, tampering, the audit record, key rotation across versions, both the Hash and String configuration shapes, and each way the key configuration can be unusable.Follow-up
🤖 Generated with Claude Code