Repository navigation
fix(storage-mongodb): store each access key ID once - #823
LeeroyHannigan wants to merge 8 commits into
Conversation
The MongoDB catalog had no unique index on access_keys.access_key_id, so every import stored a document, also under an ID that another document held. A credential lookup read the first matching document its query met, and a delete removed one document, so a key imported more than once still authenticated after its delete. PostgreSQL, SQLite and Cassandra key the table by the ID and answer such an import with 409. Migration 6, unique_access_key_ids, creates the unique index on MongoDB and does not apply to the other backends. One aggregation scans the collection in natural order and groups the documents by ID. Where one user holds every document of an ID, the body keeps the copy a lookup reads, logs and deletes the others, and records a note with their _id values. To find that copy it reads the record IDs of the ID's copies and sends explain for the lookup's query with that ID, because a partial index can give one ID another plan than the next. When the plan is a forward collection scan, the body keeps the copy with the lowest record ID, which that scan reads first. With any other plan it sends the lookup's own query. A copy deleted or given another ID since the read is neither deleted nor named in the note. detect stops the run with exit 5, and nothing is written, when the documents of an ID belong to more than one user, when two or more documents have no ID or share one that is not a string, or when an ID is an array. It also stops the run for an index that would make the build fail (access_key_id_1 with other options, or the unique index under another name) and for the unique index when it is hidden, which a lookup does not use. Each item names the documents or the index and the mongosh command that fixes it. The body reads again before it writes and refuses the same way. The build commits once a majority of the data-bearing voting members have built the index. Once the index is built, the read looks for array IDs alone, so a reconverge does not group the collection. listIndexes with includeIndexBuildInfo tells a build that is still running from a built index, and while one runs the read groups every document. A generated ID that another key holds now answers an internal error, as on PostgreSQL and SQLite, instead of 404 "User not found". The MongoDB migration tests expect migration 6. Tests on released catalogs cover the body: a golden test, which also checks with the profiler that the body sent no lookup query of its own, refusals of copies and of indexes, a table of index variants measured against MongoDB's own createIndexes, an operator's compound index, a partial index that answers the lookup for some IDs only, query settings that make the lookup scan backward (MongoDB 8 only), the read of array IDs beside the index, the body's own refusal, and a read whose $group spills to disk. Unit tests of the body cover Decimal128 key directions, entries of listIndexes for a running build, the explained command, the explain replies that report a forward scan, and the copy such a scan reads. New pytest cases check, on every backend, that an import of an existing ID answers 409 and keeps the first secret, and that a deleted key stops authenticating. The PostgreSQL CLI migration tests read the not applicable lines of migrate --list. docs/schema/mongodb.txt has the new index, and the upgrade manual describes the migration, its notes, its size limit, the steps that remove extra copies by hand on a large collection, and what to do when it stops.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| /// one, which the build creates beside it. This migration's index, hidden, | ||
| /// is refused too: MongoDB 8 answers the build with IndexOptionsConflict, | ||
| /// and MongoDB 7 answers that the index exists. | ||
| fn conflict(spec: &Document) -> Option<Conflict> { |
There was a problem hiding this comment.
I'm trying to understand the problem this is solving. It sounds like we're trying to accommodate operators putting their own indexes on access_keys, and working around them if they conflict with the index we want to add. Is that correct? If so, I'd seriously question whether we should be that accommodating. At the most, we should tell users that if they modify the schema we create, it's at their own risk and may prevent future updates, etc., from completing successfully. I don't think we should open the Pandora's box of letting users tweak up ExtendDB-owned tables. Apologies if I'm misreading the intent here.
There was a problem hiding this comment.
Partly. When an index would make the build fail, the migration stops before it writes anything and names the index to drop. It accepts one index that it did not create.
That one is the index the upgrade manual tells an operator to build by hand. The runner gives each request 10 s, and on a local mongo:7 replica set the grouping read passed that limit at 2 million keys. For a collection that large, the manual gives the mongosh commands to delete the extra copies and to create the index with createIndex({ access_key_id: 1 }, { unique: true }). MongoDB names that index access_key_id_1, so migration 6 treats it as its own and reads only the documents whose ID is an array.
conflict() covers every other index that makes the build fail: a unique index on access_key_id under another name, a different index named access_key_id_1, and ours while it is hidden. With the refusal, the preview and migrate --yes both stop before they delete a copy. Without it, run deletes any extra copies, then createIndexes fails with MongoDB's own error, and the next run completes once the operator drops the index. The deletes are the same either way. The one exception is our index hidden on MongoDB 7, where the build succeeds and the index stays hidden.
I agree that we should not support changes to ExtendDB's collections. If you would rather not carry the refusals, I will remove Clash, Conflict, conflict() and their tests, keep the check for the hand-built index, and add a sentence to the admin guide that a change to the catalog's collections or indexes can make a later migration fail. Do you want me to make that change?
One other part reads an operator's index, and I would keep it. When one user holds an ID more than once, the body uses the lookup's query plan to find the copy that the lookup reads, because an index on access_key_id can change which copy that is. The body keeps that copy and deletes the others, so the secret that authenticates today still authenticates after the migration.
There was a problem hiding this comment.
Okay ... I'm not going to dig in on this, but I do think it'd be wise to advise users that changing the schema of ExtendDB-owned tables is not supported, and may cause migration and other failures. We will create a great deal of complexity for ourselves otherwise. Also left a note requesting a pass through the "advice" messages to make them simpler and clearer for human consumption.
There was a problem hiding this comment.
I made the change in 99953b1. Migration 6 no longer refuses an index that an operator added to access_keys. Clash, Conflict, conflict() and their tests are gone. So are the Decimal128 handling of index keys and options and the live test of 14 index variants, which only those indexes needed.
The [storage] section of the admin guide now says that ExtendDB owns the collections, tables and indexes of its catalog and data databases. A change to them is not supported and can make a later migration fail. The guide's advice for slow PostgreSQL queries no longer tells operators to add indexes.
Two parts still read an index that the migration did not create:
- The unique
access_key_id_1that the upgrade manual tells the operator of a large collection to build counts as the migration's own index. - When one user holds an ID more than once, the body asks for the lookup's query plan and keeps the copy that the lookup reads.
When an added index makes the build fail, migrate --yes now exits 1 with MongoDB's error. On 7.0.45 and 8.0.34, a non-unique access_key_id_1 gives IndexKeySpecsConflict, and the unique index under another name gives IndexOptionsConflict. After the operator drops the index, the next run applies migration 6. The upgrade manual says this in one paragraph.
Main now has its own migrations 6 and 7, from #671 and #819, so this one becomes migration 8 when I merge main.
jcshepherd
left a comment
jcshepherd
left a comment
There was a problem hiding this comment.
Questions about whether we're accommodating operator-owned changes to ExtendDB-owned schema, plus a nit.
Migration 6 no longer refuses an index that an operator added to access_keys. Before this change, detect stopped the run with exit 5 when access_keys had one of these indexes: - an access_key_id_1 other than the index the migration creates - the unique index on access_key_id under another name - the unique access_key_id_1 when hidden Each refusal printed the mongosh command that drops or unhides the index. ExtendDB owns the collections, tables and indexes of its catalog and data databases, and a change to them is not supported. The migration does not refuse such a change. This change deletes Clash, Conflict, conflict() and the tests of the index refusals. It also deletes the Decimal128 handling of key directions and index options. The only index that the migration reads as its own is the one that the upgrade manual's command builds, which has an Int32 direction. Beside an index whose key direction is a Decimal128, the read now groups the whole collection, which gives the same groups and is slower. true_value() needs no Decimal128 case: on 7.0.45 the server lists unique and sparse as booleans, whatever number created them. This change also deletes the live test that applied migration 6 beside 14 index variants. Each variant ran on a catalog that holds one key, so the migration had nothing to delete. The test checked only that the index build succeeds with that index in place. Ten of the variants were indexes that an operator adds under another name. ExtendDB does not support such indexes. The rest of the migration behaves as before. The read groups only the documents with an array ID when access_key_id_1 is built with the key and options of the manual's command. The same applies when that index is hidden. detect refuses an ID that more than one user holds, and an array ID. It also refuses two or more documents that have no ID or share an ID that is not a string. run refuses copies written after detect. When one user holds an ID more than once, the body sends explain for the lookup's query and keeps the copy that the lookup reads. On mongo:7 (server 7.0.45) and mongo:8.0 (server 8.0.34) replica sets, a non-unique access_key_id_1 stops migrate --yes with exit 1 and IndexKeySpecsConflict. The run deletes the extra copies before the build fails, and only its warnings name them. A unique index on access_key_id under another name stops migrate --yes with exit 1 and IndexOptionsConflict. After the operator drops either index, migrate --yes applies migration 6. With a hidden access_key_id_1, migrate --yes on 7.0.45 applies migration 6 and the index stays hidden. On 8.0.34 it exits 1 with IndexOptionsConflict, and it applies migration 6 after the index is unhidden. The [storage] section of the admin guide's configuration reference now says that ExtendDB owns these collections, tables and indexes. It adds that a change to them is not supported and can make a later migration fail. The guide's advice for slow PostgreSQL queries no longer tells the operator to add indexes. The upgrade manual no longer lists the index refusals. It names the two added indexes that made the build fail. It tells the operator to keep the output of a failed run, whose warnings are the only record of the copies that run deleted. A new unit test checks which forms of access_key_id_1 the migration reads as its own. Signed-off-by: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com>
| owner's with {delete} in mongosh" | ||
| ) | ||
| } | ||
| (Bson::Null, None) => format!( |
There was a problem hiding this comment.
Can we make a pass on these "advice" snippets, and apply STE to them to make them more immediately intelligible to a user. E.g., "{n} documents in {db}.{COLLECTION} have no access key ID. No login uses these documents, so deleting them is safe. Keep at most one such document and delete the others. To delete them, run this in mongosh: {delete}. Affected documents: {copies}". Problem, reason, action, plain language.
There was a problem hiding this comment.
I rewrote migration 6's refusals in f693e0b, so each one states the problem, then the reason, then the action, in short sentences. The list of documents and the mongosh delete command come last, so an operator can copy the command on its own. The warning before each delete and the ledger note follow the same order.
Once you're happy with the content, I'll merge main in and move this migration to 10.
…on 6's advice Migration 6's refusals now state the problem, then the reason, then the action, in short sentences. Each refusal then lists the documents. It ends with the mongosh command that deletes one of them, so an operator can copy the command on its own. The warning before each delete and the ledger note follow the same order and use the same terms. Every count, name, _id and command stays in the text. The refusal test and the golden file assert the new wording of the forms that they cover. Signed-off-by: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com>
jcshepherd
left a comment
jcshepherd
left a comment
There was a problem hiding this comment.
Changes look good: thanks. Please sync from main, clear the CodeQL call-outs (both in test code), and you are good to go.
Main added migrations 6 to 9, so unique_access_key_ids is now migration 10. Each backend's migration table keeps main's arms and adds this migration after them. The MongoDB migration tests keep main's checks against the latest migration. Their lists of applied steps and ledger rows now end with migration 10. The expected catalog adds the unique access_key_id_1 index to main's definitions. The CONTRIBUTING example becomes migration 11. The setup manuals show the catalog at migration 10. Signed-off-by: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com>
Main took migrations 6 to 9, so the MongoDB body moves to m0010_unique_access_key_ids.rs. Its fingerprint, its warning and its golden file now use the number 10. The tests find the migration's ledger row by its id and compare the status with the latest migration. The tests that stop before this migration now migrate through migration 9. The upgrade manual, the admin guide and a code comment name migration 10. Signed-off-by: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com>
…text The two assertions that check each refusal of migration 10 printed the refusal text when they failed, and that text names the fixture's account. Each expected refusal now carries a short label, and a failure prints only that label. The assertions check the same refusals as before. Signed-off-by: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com>
…e has The merge of main added MigrationId to the file's own imports. Two test functions and the run_body helper still imported it a second time. Signed-off-by: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com>
Signed-off-by: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com>
|
@jcshepherd I merged main in, and the branch is at cc67a04.
All 58 checks pass on cc67a04, and CodeQL reports both alerts as fixed. |
What
Migration 10,
unique_access_key_ids, gives the MongoDB catalog a unique index onaccess_keys.access_key_id. It does not apply to PostgreSQL, SQLite or Cassandra, which key that table by the ID.Before it builds the index, the migration groups the documents of
access_keysby ID:verifyprints.explainfor the lookup's query with that ID. On a forward collection scan it keeps the copy with the lowest record ID. With any other plan, for example through an operator's index onaccess_key_id, it sends the lookup's own query.mongoshcommand that deletes or corrects them. The cases are:listIndexeswithincludeIndexBuildInfotells a build that is still running from a built index.The migration does not refuse an index that an operator added to
access_keys. ExtendDB owns the collections, tables and indexes of its catalog and data databases, and it does not support a change to them. When an added index makes the build fail,migrate --yesexits 1 with the server's error. The migration reads one index that it did not create as its own. That index is the uniqueaccess_key_id_1that the upgrade manual tells the operator of a large collection to build.On MongoDB,
create_access_keynow answers an internal error when a generated ID is already stored, as PostgreSQL and SQLite do. Before, it answered 404 "User not found".The upgrade manual describes the migration, its notes and the cases where it stops. It also gives its size limit and a
mongoshprocedure for a large collection. It names the added indexes that made the build fail. The[storage]section of the admin guide says that ExtendDB owns the collections, tables and indexes of its catalog and data databases. It does not support a change to them, and such a change can make a later migration fail. The guide's advice for slow PostgreSQL queries no longer tells the operator to add indexes.docs/schema/mongodb.txthas the new index, and the docs that name the latest migration now name 10.Why
The MongoDB catalog had no unique index on
access_key_id. Each import stored a document, also under an ID that another document held. A credential lookup read the first of those documents that its query plan met. A delete of the key deleted one of them. So, a key that a user imported twice still authenticated after its delete, and the user's key list still showed it. An import also succeeded for an ID that another user held, and lookups then read either user's document. PostgreSQL, SQLite and Cassandra key the table by the ID, and they answer such an import with 409.Testing done
At d266558, with main merged at adbe434, on toolchain 1.97.0:
cargo fmt -- --checkandcargo clippy --all-targets -- -D warningsof the five storage crates are clean.After that, 47677c1 deletes three imports that the test file already has, and cc67a04 merges main at e681b8b. That merge changes no file of the storage crates. At cc67a04,
cargo +1.97.0 clippy -p extenddb-storage-mongodb --all-targets --features extenddb-storage-mongodb/test-hooks -- -D warningsis clean.At 99953b1:
migrations_fresh,migrations_guards,migrations_holders,migrations_locks,migrations_releasedandmigrations_spill. On 8.0.34,migrations_released,migrations_freshandmigrations_spillpass.migrations_releasedadds 7 tests of the migration:detect.migrations_spillchecks a read whose$groupspills to disk.listIndexesentries of a running build, the explained command, and the copy that a forward scan reads. The crate's 173 unit tests pass.access_key_id_1,migrate --yesdeleted the extra copy and then exited 1 with IndexKeySpecsConflict. Beside a unique index onaccess_key_idunder another name, it exited 1 with IndexOptionsConflict. With either index dropped, the nextmigrate --yesapplied the migration.migrate --yesapplied the migration and the index stayed hidden. On 8.0.34 it exited 1 with IndexOptionsConflict until the index was unhidden.cargo +1.97.0 clippy -p extenddb-storage-mongodb --all-targets --all-features -- -D warnings,cargo fmt --all -- --checkandcargo +1.88.0 checkof theextenddb-storage-mongodbpackage are clean.At 57ccdc7. The changes since then delete code that none of these runs reach:
access_key_id_1. While it held the build, the preview,verifyandmigrate --yesexited 5 and named an ID of two users.mongo:7replica set with 1,000,000 keys and 1,000 IDs stored twice,migrate --yestook 35 s. Each ID kept the copy its lookup read. With 2,000,000 keys the preview stopped after the 10 s request limit. After the manual'smongoshprocedure, the preview took 0.6 s andmigrate --yestook 1.9 s.TestAccessKeyIdIsStoredOnceintests/test_auth_permissions.pypasses on MongoDB. Earlier on this branch it passed on PostgreSQL and SQLite, and it failed on MongoDB with amainbuild. The PostgreSQL CLI migration tests also passed earlier on this branch. The files these tests cover did not change after those runs.Checklist
cargo test --workspace)cargo fmt --check)cargo clippy -- -W clippy::pedantic)Storagetrait, auth model, on-diskformat, or public CLI surface, an RFC has been accepted or is linked
below. Otherwise, an ADR captures the decision (link below).
ADR / RFC: ADR-0023, catalog migrations. This change adds migration 10 under it.
Breaking changes
migrate --yescan stop with exit 5 until an operator deletes or corrects documents. The output and the upgrade manual give themongoshcommands.mongoshfirst.Merging with main
Main has its own migrations 6 to 9:
backup_catalog_index,stream_pending,restore_progressandwarm_throughput. With main merged, this migration is migration 10. The runs at 99953b1 and 57ccdc7 tested it under the number 6. The example of a new migration in CONTRIBUTING.md now uses 11.By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache License 2.0 and I agree to the Developer Certificate of
Origin (DCO). See CONTRIBUTING.md for details.