Repository navigation
SOLR-18010: Fix security.json corruption from parallel edits - #5065
Conversation
In standalone mode, two security edits running at the same time could undo each other or leave security.json as invalid JSON, and Solr then fails to start. The Admin UI does this when creating a role with permissions, since it sends one request per permission all at once. Security edits in standalone mode now run one at a time.
|
Wow... that is a nice fix. I would have assumed a lot more complexity, but I guess that is literally what I wonder if there are other places in standalone that might have similar issues to this one? |
epugh
left a comment
epugh
left a comment
There was a problem hiding this comment.
LGTM. The G in this case isn't "good" it's "GREAT". I love using Java keyword to fix a problem!
|
tests pass locally. |
|
will leave open till monday, no hurry to get this merged. |
|
🤖 AI text below 🤖 (posted on behalf of Nick Shanin) Thanks for the fix. Two things I tested here, offered as options and not as a change request. First, a forced interleave test at the persistConf level: one edit is parked inside the serializer, and both users must survive as one JSON document. The 20-POST test catches the race only by chance. Second, writing the new contents to a temp file and then moving it into place, so a failed write keeps the old file. Your PR still truncates first, so a failure mid-write can leave a short file. I understand you chose not to do the rename here. I have put the version with both elements up as a draft in #5118 so you can see them worked out. Would you like either as a follow-up commit, or would you rather keep this change to the lock? |
|
Thanks! I'd like to keep this PR to the lock. I left out temp-file-and-move on purpose: it loses security.json's 600 permissions, replaces a symlink with a plain file, and fails on a read-only SOLR_HOME or a single-file Docker mount. Truncated writes could be their own JIRA, and an interleave test is a good follow-up. |
|
🤖 AI text below 🤖 (posted on behalf of Nick Shanin) Thanks, that settles it. I will keep #5118 as a draft and, once this merges, rework it as a follow-up for the truncated-write case under its own ticket, with the permissions, symlink and mount behavior spelled out rather than assumed. |
There was a problem hiding this comment.
🟢 Approval recommended
Static review supports the locking approach and regression coverage, with no unresolved findings; tests were not executed.
No open issues 🎉
What changed in this PR
Prevents standalone security configuration corruption by serializing security API requests, while leaving SolrCloud behavior unchanged.
Changes:
- Locks standalone security requests, including reads.
- Adds a 20-request concurrency test that verifies all edits are preserved.
- Documents the fix in the changelog.
| File | Description |
|---|---|
| solr/core/src/test/org/apache/solr/security/BasicAuthStandaloneTest.java | Adds concurrent-edit regression coverage. |
| solr/core/src/java/org/apache/solr/handler/admin/SecurityConfHandlerLocal.java | Serializes requests using a shared lock. |
| changelog/unreleased/SOLR-18010-security-json-corruption.yml | Records the standalone corruption fix. |
Review effort: Balanced
|
thanks for the nudge and it's great to see the colloboartion @nick-boss-tech and @sarkaramrit1993 |
(cherry picked from commit 1afeee2)
Description
SOLR-18010
Creating a role in the Admin UI sends one request per permission, all at once. In standalone mode each request reads, edits and rewrites security.json with nothing stopping two from overlapping, so edits get lost and the file can end up with leftover text after the JSON. After that the security API fails, and Solr won't start after a restart.
Solution
In standalone mode, security API requests now run one at a time, reads included, so they never see a half-written file. SolrCloud already avoids this through ZooKeeper versioned writes.
Tests
New
testConcurrentEditsinBasicAuthStandaloneTestsends 20 edits at once. It fails on main with an HTTP 500 from the broken file.