fix(auth): GC stale FactorLookup when metadata does not authorize - #238
fix(auth): GC stale FactorLookup when metadata does not authorize#238orekav wants to merge 12 commits into
Conversation
Distinguish own factor-id retries from concurrent same-identity winners on ambiguous S3 puts, add exact-match key-only upgrades with ambiguous reconcile, and expose consistent DynamoDB factor lookup after races.
Trim redundant commentary and correct resolve_put_object_outcome docs so Inserted requires a matching factor id for every ambiguous put error.
Keep simple classify_put_object_error (NotInserted vs Unknown) and stop re-reading metadata after 412/timeout/5xx. Ambiguous failures keep the FactorLookup; orphans can be GC'd at auth time.
On authentication, if Dynamo maps a factor to a backup but S3 metadata is missing or does not contain that factor, delete the lookup row (best-effort) before returning BackupMissing / UnauthorizedFactor.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9db8e8fbc1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Deletes a `FactorLookup` row that pointed at a backup where the factor is no longer present | ||
| /// (or the backup is gone). Best-effort: auth still fails; delete errors are logged only. | ||
| async fn gc_stale_factor_lookup(&self, scope: FactorScope, factor: &FactorToLookup) { | ||
| match self.factor_lookup.delete(scope, factor).await { |
There was a problem hiding this comment.
The cleanup assumes that missing or unauthorized metadata proves the lookup is stale, but the write flows publish lookup rows first: for example, src/routes/create_backup.rs:164-180 inserts both mappings before creating the S3 object, and the add-factor flows use the same ordering. Because authentication does not share the create/add lock, a concurrent request can observe the new lookup during that window, reach this unconditional delete, and then have the metadata write succeed, leaving a valid backup or factor permanently untraceable. Coordinate cleanup with these writers or distinguish pending lookup rows before deleting them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
a simple lock could resolve this
There was a problem hiding this comment.
Implemented a factor-scoped Redis mutate lock (Paolo’s suggestion):
create_backup/add_factor/add_sync_factorholdfactor_lookup_mutate:{scope}#{factor_pk}from beforeFactorLookupinsert until after the S3 metadata write (and any rollback).- Auth stale-delete acquires the same lock; if it’s held (or Redis fails), we skip the delete and leave the row for a later attempt.
Pushed in the latest commit on this PR.
Stop early only after seeing lengths 70/71/72, not after any three lengths (which could include rare 69 and miss 71).
|
|
||
| /// Deletes a `FactorLookup` row that pointed at a backup where the factor is no longer present | ||
| /// (or the backup is gone). Best-effort: auth still fails; delete errors are logged only. | ||
| async fn gc_stale_factor_lookup(&self, scope: FactorScope, factor: &FactorToLookup) { |
There was a problem hiding this comment.
dumb question what does GC stand for?
There was a problem hiding this comment.
Garbage collection — cleaning up a stale Dynamo index row once we know S3 metadata no longer authorizes that factor.
Renamed to delete_stale_factor_lookup in 70e38bf so it’s clearer.
GC meant garbage-collect; prefer delete_stale_factor_lookup.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70e38bf8b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Deletes a `FactorLookup` row that pointed at a backup where the factor is no longer present | ||
| /// (or the backup is gone). Best-effort: auth still fails; delete errors are logged only. | ||
| async fn delete_stale_factor_lookup(&self, scope: FactorScope, factor: &FactorToLookup) { | ||
| match self.factor_lookup.delete(scope, factor).await { |
There was a problem hiding this comment.
When a stale lookup for a factor is removed and the factor is registered to another backup after this request's lookup() but before this call, the unconditional key-only delete removes the newly inserted mapping rather than the stale mapping that produced not_verified_backup_id. This is especially possible because authentication uses an eventually consistent read; the new registration can complete successfully while its factor becomes permanently untraceable. Pass the observed backup ID into this helper and make the DynamoDB deletion conditional on BackupId still matching it.
Useful? React with 👍 / 👎.
Writers hold a factor-scoped Redis lock from lookup insert through the metadata put. Auth stale-delete skips when that lock is held so it cannot remove an in-flight mapping before S3 catches up.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c77cdfc04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| }; | ||
|
|
||
| match self.factor_lookup.delete(scope, factor).await { |
There was a problem hiding this comment.
When delete_factor removes the old mapping and the same factor is registered to another backup after this request's initial lookup, acquiring the new per-factor lock only waits for that registration to finish; this key-only delete then removes the replacement mapping. Fresh evidence in this revision is that the helper still calls delete(scope, factor) without passing the observed not_verified_backup_id, even though writers now release this same lock after committing new metadata. Make the DynamoDB deletion conditional on BackupId still matching the ID observed by authentication.
Useful? React with 👍 / 👎.
| factor_lookup_mutate_lock_id(FactorScope::Sync, &sync_factor_to_lookup), | ||
| Some(FACTOR_LOOKUP_MUTATE_LOCK_TTL_SECS), | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
When this factor's mutate lock is briefly held by stale cleanup or another writer, this ? returns Locked after use_sync_factor_token has atomically marked the one-time token as used. This path never calls unuse_sync_factor_token, so retrying gets AlreadyUsed even though no lookup or metadata was written; acquire the lock before consuming the token or restore the token on acquisition failure.
Useful? React with 👍 / 👎.
| let _ = main_factor_lock.release().await; | ||
| let _ = sync_factor_lock.release().await; |
There was a problem hiding this comment.
On a failed create, releasing these locks before deleting the inserted rows leaves the rollback outside the coordination protocol. A concurrent authentication can delete the stale row, another create or add-factor operation can then register the same factor to a valid backup, and the original request's subsequent unconditional rollback deletes that new mapping; release each mutate lock only after its corresponding failure cleanup completes.
Useful? React with 👍 / 👎.
| .try_acquire_lock_guard( | ||
| FACTOR_LOOKUP_MUTATE_LOCK_PREFIX, | ||
| factor_lookup_mutate_lock_id(scope, factor), | ||
| Some(FACTOR_LOOKUP_MUTATE_LOCK_TTL_SECS), | ||
| ) |
There was a problem hiding this comment.
When authentication reads a newly inserted DynamoDB lookup while its writer still holds the lock, the S3 read can report missing or unauthorized metadata; if the writer commits and releases before this helper runs, the helper acquires the now-free lock and deletes the valid mapping. Fresh evidence in this revision is that lock acquisition occurs only after the caller has made its stale-metadata decision, so checking whether the lock is currently held does not order that metadata read against the write. Acquire the lock before inspecting metadata or re-read the lookup and metadata under the lock before deleting.
Useful? React with 👍 / 👎.
2 participants
Summary
FactorLookuprow but S3 metadata is missing or does not contain that factor, delete the lookup (best-effort) before returningBackupMissing/UnauthorizedFactor.Stack
Base: #231 (
uaf/1-factor-write-reconcile)Test plan