[performance] Apply minimal auth to the search route#257497
[performance] Apply minimal auth to the search route#257497drewdaemon merged 18 commits intoelastic:mainfrom
Conversation
| const user = getCurrentUser(request); | ||
| if (user?.roles.length === 0) { | ||
| this.logger.warn( | ||
| `A user authenticated with the "${user.authentication_realm.name}" (${user.authentication_realm.type}) realm doesn't have any roles and isn't authorized to perform request.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
This log statement was creating a server error in some cases:
Error: Property "authentication_realm" is not available for minimally authenticated users.
There was a problem hiding this comment.
Thanks!
Context: "With minimal auth, we won’t always have roles available, so this message will become more and more misleading when debugging issues."
| * to reduce number of saved object updates and reduce a chance of running over update retries limit | ||
| * @internal | ||
| */ | ||
| public trackId = async ( |
There was a problem hiding this comment.
trackId is called from the now-minimally-authenticated search route, so it needs to avoid accessing authentication_realm
| * request can continue rather than restart. | ||
| * @internal | ||
| */ | ||
| public getId = async ( |
There was a problem hiding this comment.
getId is called from the now-minimally-authenticated search route, so it needs to avoid accessing authentication_realm
There was a problem hiding this comment.
does it mean that it's possible to get search sessions from other users?
There was a problem hiding this comment.
getId is called when restoring a search, so technically it's possible for them to get a search ID from another search session (in the rare case that they have the same username but different realms) but it shouldn't matter all that much because they still won't have access to the results from that ID.
There was a problem hiding this comment.
yeah or your could add a search to another user's session with trackId. But the same data access controls ultimately apply
AlexGPlay
left a comment
There was a problem hiding this comment.
took me a bit to understand but i think now i get it, but let me go over it just in case:
- the search route now has minimal auth which means we only care about the credentials being there and the full auth is deferred to elastic
- this means we now don't have
authentication_realmpresent so we can't run those checks getIdandtrackIdare called from this route so this two can't run the ownership checks now - this last part was the one that i didn't get, but if i'm not mistaken this 2 functions are only used from that route so it makes sense now
i'm happy to give my approval as i think it makes sense, not sure if you prefer to have it also from Lukas
jeramysoucy
left a comment
There was a problem hiding this comment.
Kibana Security changed LGTM!
| const user = getCurrentUser(request); | ||
| if (user?.roles.length === 0) { | ||
| this.logger.warn( | ||
| `A user authenticated with the "${user.authentication_realm.name}" (${user.authentication_realm.type}) realm doesn't have any roles and isn't authorized to perform request.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Thanks!
Context: "With minimal auth, we won’t always have roles available, so this message will become more and more misleading when debugging issues."
walterra
left a comment
There was a problem hiding this comment.
datavis owned changes LGTM, just added some nit comments/considerations.
| }, | ||
| authc: { | ||
| enabled: 'minimal', | ||
| reason: `This route is optimized for performant retrieval of data from Elasticsearch.`, |
There was a problem hiding this comment.
Absolute nit: This string should be find with regular single quotes '.
|
|
||
| const session = await this.get(deps, user, sessionId); | ||
| // Skip user check for getId - called from minimal auth search route | ||
| const session = await this.get(deps, user, sessionId, true); |
There was a problem hiding this comment.
This looks fine for now, just a thought about possible future route that would use full auth also calling getId/trackId, they would silently skip the user check too. Is it worth hardening this down a bit more to avoid this slipping through in a future update?
| queue[0].user, | ||
| sessionId, | ||
| { idMapping: batchedIdMapping }, | ||
| true // Skip user check for trackId - called from minimal auth search route |
There was a problem hiding this comment.
This looks fine for now, just a thought about possible future route that would use full auth also calling getId/trackId, they would silently skip the user check too. Is it worth hardening this down a bit more to avoid this slipping through in a future update?
There was a problem hiding this comment.
This is unlikely to happen, but I still think it's good feedback. I'll look at turning off this check from route instead of by default.
| }); | ||
|
|
||
| it('works with minimal auth user (no authentication_realm access)', async () => { | ||
| const minimalAuthUser = new Proxy( |
There was a problem hiding this comment.
This could be moved to a shared factory at the top of the file to avoid code duplication.
| }); | ||
|
|
||
| it('works with minimal auth user (no authentication_realm access)', async () => { | ||
| const minimalAuthUser = new Proxy( |
There was a problem hiding this comment.
This could be moved to a shared factory at the top of the file to avoid code duplication.
Your understanding is correct!
I'd like a review from both of you. I changed some things in d3ea63a, feel free to re-check. |
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
lukasolson
left a comment
There was a problem hiding this comment.
LGTM, just a couple of small comments below
| searchId: string, | ||
| options: ISearchOptions | ||
| options: ISearchOptions, | ||
| skipRealmCheck: boolean = false |
There was a problem hiding this comment.
Since trackId is always called with skipRealmCheck: true does it make sense to even have this parameter?
There was a problem hiding this comment.
lol, well I got the opposite feedback above #257497 (comment)
| searchRequest: IKibanaSearchRequest, | ||
| { sessionId, isStored, isRestore, requestHash }: ISearchOptions | ||
| { sessionId, isStored, isRestore, requestHash }: ISearchOptions, | ||
| skipRealmCheck: boolean = false |
There was a problem hiding this comment.
Same question as above, isn't this always called with true now?
| * request can continue rather than restart. | ||
| * @internal | ||
| */ | ||
| public getId = async ( |
There was a problem hiding this comment.
getId is called when restoring a search, so technically it's possible for them to get a search ID from another search session (in the rare case that they have the same username but different realms) but it shouldn't matter all that much because they still won't have access to the results from that ID.
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Public APIs missing comments
History
|
* commit '11ed3645c5ededae2a6e29f2a79b31f52208b441': (157 commits) remove sync register uiAction methods (elastic#254590) [performance] Apply minimal auth to the search route (elastic#257497) [ES|QL] Reports correctly the controls server side errors (elastic#263020) [SecuritySolution][Navigation] Enable classic nav updates (elastic#262358) [Inference] Use pretty name and logo on feature settings page (elastic#262531) [Security Solution] fix AT-AB cypress test (elastic#262991) [SigEvents] Seed sigevents env script (elastic#261172) Adjust conditions for validating no refetch for expanded row (elastic#262978) [Agent Builder] update copy for the announcement modal (elastic#263034) [Search] Hide index management links for users without privileges (elastic#262627) Simplify OAS schema for GET `/api/spaces/space` query params (elastic#260831) Fix fleet output OAS regressions: SSL type explosion and Kafka union wrappers (elastic#260842) [Dashboards in chat] fix agent confusing the axes in a horizontal chat (elastic#263064) [One Workflow] Add alert state checkbox UI for workflow connector (elastic#259770) [One Workflow] Deprecate legacy Cases step types in workflow authoring (elastic#262070) skip failing test suite (elastic#248090) fix flaky test: MonitorDetails filter apply button not enabled (elastic#260788) fix: propagate AbortSignal to executeAsReasoningAgent for task cancellation (elastic#262811) [Security Solution][Alert KPI] Fix white space bug in alert KPIs (elastic#260803) [Streams] Move helpers and format_size_unit to utils folder (elastic#262550) ... # Conflicts: # x-pack/platform/plugins/shared/dashboard_agent/public/attachment_types/canvas_integration/dashboard_canvas_content.test.tsx # x-pack/platform/plugins/shared/dashboard_agent/public/attachment_types/canvas_integration/dashboard_canvas_content.tsx # x-pack/platform/plugins/shared/dashboard_agent/public/attachment_types/canvas_integration/use_register_canvas_action_buttons.ts # x-pack/platform/plugins/shared/dashboard_agent/public/attachment_types/index.test.tsx # x-pack/platform/plugins/shared/dashboard_agent/public/attachment_types/index.tsx
Summary
Part of #84923
Enable minimal authentication for the search route!