feat(index): authorize a "<project>.entities" index against its base project - #2311
Draft
pirhoo wants to merge 16 commits into
Draft
feat(index): authorize a "<project>.entities" index against its base project#2311pirhoo wants to merge 16 commits into
pirhoo wants to merge 16 commits into
Conversation
…project The search proxy granted an index only on an exact match against a project name, so a user granted "myproject" was refused on "myproject.entities": the name validator rejected the dot, and no project carried that name. Index names now allow a dot between segments but never a leading one, so the elasticsearch system indices stay out of reach, and the grant check maps an index back to its project by stripping a known ".entities" suffix. Suffix, never prefix, or a grant on "myproject" would leak into "myproject-other". Async-search ownership records the base project too, so a poll re-checks the string its submit authorized. Closes #2199
Java's $ in a ^...$ pattern also matches before a final line terminator, so find() let "foo\n" through where the format is meant to be rejected. matches() anchors the whole string with no such gap. Also corrects a backwards comment on the prefix-match assertion, and adds a mixed-list case (a granted index alongside an unrelated one) to the same refusal test.
checkIndices no longer rejects every dot, so it stopped doubling as the de-facto project-name validator that the REST creation paths relied on: a project named "victim.entities" would have passed creation and become readable by every member of project "victim". projectCreate and the create branch of projectUpdate now validate against Project.NAME_PATTERN, same as the existing admin-service and CLI paths. The update-existing branch is untouched, so a legacy project whose name predates the regex can still be updated.
pirhoo
force-pushed
the
refactor/2199-namespaced-index-authorization
branch
from
August 4, 2026 12:18
224120c to
4775ead
Compare
pirhoo
changed the title
pirhoo
marked this pull request as draft
August 4, 2026 12:41
…oc overclaim IndexResource.recordAsyncSearchOwnership re-derived the same comma-split-then-map baseProjects transform that IndexAccessVerifier.isAuthorizedRequest already did on the same string checkPath just split. A submit and the poll re-checking it must stay on the same project set, so a single IndexAccessVerifier.baseProjects(String) replaces both, removing the now-unused Arrays.stream import from IndexResource. Also corrects the checkIndices grammar's javadoc: a leading-dot ban alone does not keep the elasticsearch system indices out of reach, since "_all" and other "_"-leading names still match it. The grant check downstream is what stops those.
isAuthorizedRequest's grant clause is (isMethodGet || isSearchPath || isCountPath || isAsyncSearchPath); every checkPath assertion here used a GET-mocked context, so each was satisfied by the first term alone and never exercised isSearchPath. A regression dropping the OR's tail would have left this class green. contextFor now takes the method explicitly. The positive .entities assertion runs as a POST so isSearchPath is what authorizes it, and a new case covers a POST to a non-search path on an otherwise-granted index still being refused.
…ng it PUT /api/index/:index had no grant check at all: any authenticated user could create (and thereby read) any index by name, including "victim.entities", which our new suffix rule then serves to everyone granted "victim". createIndex now requires ForbiddenException.requireGranted on the checked index's baseProject, the same centralised gate other project-scoped endpoints use. A user still creates their own "<id>-datashare" index (and its .entities companion) at first login; test_put_create_local_index_in_local_mode's LOCAL-mode fixture is updated to grant the index it creates, since LOCAL mode's grants come from the (mocked) project repository, not a bypass.
Both endpoints checked the mode but never a grant, so any authenticated user could close or open any index by name. Worse, "_all" passes the index-name grammar, so POST /api/index/_all/_close could close every index in the cluster, including .kibana and .security. Both now require ForbiddenException.requireGranted on the checked index's baseProject, same as createIndex. "_all" is refused because it is never a granted project.
…order
Project.NAME_PATTERN (^[a-z0-9][a-z0-9-]{1,63}$) rejects far more than a dot:
no underscore, no single-character name, nothing over 64 characters. Enforcing
it here broke names the REST API previously accepted, e.g. "my_project", which
is not this endpoint's contract to enforce.
Both projectCreate and projectUpdate's create branch now reject only a name
containing a dot, the one invariant the .entities suffix rule actually depends
on. projectUpdate checks it before dataDirVerifier, same order as projectCreate,
so a doubly-invalid body gets the same status regardless of verb. The
@apiresponse docs for both endpoints now mention the dot, not just emptiness.
…mma list
All three called baseProject on the whole checked segment, not per index. A
comma-separated list (which checkIndices already accepts, e.g. "bar,foo.entities")
has no project name equal to it, so baseProject("a,b") returned "a,b" unchanged
and requireGranted always refused it: every multi-index _close/_open/createIndex
request 403'd regardless of grants.
datashare-client's snapshot-restore flow joins a snapshot's indices with a comma
and calls closeIndex/openIndex with the joined string, so this broke multi-index
snapshot restore. Now uses baseProjects(checkedIndex).forEach(requireGranted),
checking each index's project individually, same as the search proxy already
does at IndexAccessVerifier.isAuthorizedRequest. "_all" is still refused: it maps
to itself and no project is ever named "_all".
It called no configure(...) and passed only because the static @ClassRule server retained whatever routes/filter an earlier test in the class had left behind (a BasicAuthFilter granting cecile-datashare to cecile, in whichever run). That was harmless before createIndex checked the caller at all; now that the grant is load-bearing, an unrelated change reordering test methods could break this test in a way that looks nothing like its actual cause.
The dot-only guard left a project nameable "_all". Granted to a user, that
name would make isGranted("_all") true, and GET /api/index/search/_all/_search
would read every index in the cluster including .kibana and .security -
also defeating the _close/_open grant check's assumption that "_all" is
never a granted project.
Both projectCreate and projectUpdate's create branch now also reject a
name starting with an underscore, with its own message so a client learns
which rule was violated. Underscores elsewhere in the name, 1-character
names, and long names are still accepted, same as before this change:
this is not a return to the full Project.NAME_PATTERN.
createIndex was the only one of the three grant-gated endpoints in this file with no modeVerifier.checkAllowedMode call, so it stayed reachable in SERVER mode while _close and _open did not. Added the same LOCAL/EMBEDDED gate as the first statement, and the matching @apiresponse 403 doc. datashare-client defines api.createIndex but has no production call site for it, only a unit test of the wrapper, so no client flow depends on SERVER-mode access to this endpoint.
…th ends The reopen after test_close_index_allows_comma_separated_list_of_granted_projects's close assertion was a plain follow-on statement: a failing close assertion would skip it, leaving es.getIndexNames()[1] and [2] closed for the rest of the class, the exact cascade a full-class run already caught once. Wrapped the reopen in a finally so it runs on every path; changes no behavior. test_close_and_open_refuse_comma_separated_list_with_one_ungranted_project put the ungranted index last in both its _close and _open assertions, so a mutation checking only the final index in a comma list would still pass. Added the reversed ordering (ungranted first). New assertion on already-correct behavior, so no red-green cycle: it passed on the first run, as expected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lets a user who can access a project also reach that project's
<project>.entitiesindex through the search proxy. Part of #2194..entitiessuffix before the grant check, exact match on the remaindermatches()rather thanfind()in the index-name check, so a name with a trailing line terminator no longer validates_allclose/opencreateIndex,_close, and_openauthorize each index in a comma-separated list individually, same as the search proxy already did, so a multi-index request (e.g. snapshot restore) still works instead of always 403ingmy_projectthat the REST API previously accepted still work, while a dotted name (the one hazard the suffix rule depends on) is still rejected on bothPOSTandPUTcreate, in the same order_alland inherit access to every index in the clustercreateIndexthe same way_closeand_openalready are, so it's unreachable in SERVER mode.entitiesindex, and is refused another project's, an unknown suffix, a prefix match, and a foreign index piggybacked on a granted oneWhat this PR does not do
Scope boundaries up front, so a reviewer doesn't have to derive them from the diff.
esHead,esOptionsand the_snapshot/*endpoints still reach Elasticsearch with caller input no grant check has seen. Tracked as fix: search proxy reaches Elasticsearch without a grant check on HEAD, OPTIONS and index creation #2312, with the per-endpoint detail.esHead's exposure is unchanged by this PR: it never calledcheckIndices, so a dotted path always reached it.p,p.entitiesrecordsptwice in the async-search ownership list, previously unreachable since the old grammar rejected the dot and the request never validated. Harmless, the ownership check isallMatch, so deduplicating would add a line that changes no behavior. AlsocreateIndexPreflightnow advertises aPUTthat the new mode gate refuses in SERVER mode (cosmetic, thePUT403s as intended)..entitiesconstant stays private toIndexAccessVerifieruntil a second consumer exists.Project.NAME_PATTERN: the name guard rejects a dot and a leading underscore only, soPOST /api/projectstill accepts names the admin service rejects. Aligning the two contracts is a separate backward-compatibility decision.<project>.entitieswould be readable by anyone granted<project>. What keeps that from being self-inflicted is the mode gate oncreateIndexplus the project-name guard; nothing here vouches for indices Datashare did not create.Closes #2199