Refactor request marking for serverless and operator modes #110370
Conversation
n1v0lg
added
>refactoring
:Security/Security
n1v0lg
changed the title
path_restricted to restrict_for_serverlesspath_restricted to use_serverless_partial_api_restrictions
|
Pinging @elastic/es-security (Team:Security) |
n1v0lg
changed the title
path_restricted to use_serverless_partial_api_restrictions
n1v0lg
changed the title
server/src/main/java/org/elasticsearch/cluster/metadata/DataStreamLifecycle.java
Show resolved
Hide resolved
| @@ -85,8 +85,10 @@ private static String decodeQueryStringParam(final String s) { | |||
| } | |||
|
|
|||
| private static void addParam(Map<String, String> params, String name, String value) { | |||
| if (PATH_RESTRICTED.equalsIgnoreCase(name)) { | |||
| throw new IllegalArgumentException("parameter [" + PATH_RESTRICTED + "] is reserved and may not set"); | |||
| for (var reservedParameter : INTERNAL_MARKER_REQUEST_PARAMETERS) { | |||
There was a problem hiding this comment.
nit: technically, we don't need to use case-insensitive checking (arguably we shouldn't be since http params are case intensive). If we don't case insensitive checking, the request with these paramas (say all uppercase) would still fail due to the consumed params check. I do slightly prefer the case-insenstive check since it prevents someone from accidentally mirroring these names with slightly different case. However, this code is called for every request and could save a couple clock cycles (I think) and a couple allocations (I think) if we ignored case and just did a set.contains. Micro optimization for sure, but would make for a simple micro benchmark. super nitpick, nothing wrong as-is, so feel free to ignore this comment.
There was a problem hiding this comment.
I agree with the potential optimization but would prefer to keep this refactor "focused" -- moving away from equalsIgnoreCase is a functional change so I'd rather follow up with a separate PR
| ALWAYS_SUPPORTED, | ||
| // these internal parameters cannot be set by end-users, but are used by Elasticsearch internally. | ||
| // they must be accepted by all handlers | ||
| RestRequest.INTERNAL_MARKER_REQUEST_PARAMETERS, |
There was a problem hiding this comment.
Is this necessary because we are marking the request earlier in the process ?
There was a problem hiding this comment.
we need this because we now set the parameter for actions for which we did not set it before (we didn't set the parameter for internal actions previously, now we do). A couple of these actions (e.g., RestDeleteSnapshotAction) override allSupportedParameters which in turn means that we run a check against all request parameters and reject "unsupported" ones. The default (for virtually all other actions) is to return null which means no check, so we simply didn't exercise this code path before.
...n/security/src/main/java/org/elasticsearch/xpack/security/operator/OperatorOnlyRegistry.java
Outdated
Show resolved
Hide resolved
…lastic#110370)" This reverts commit b7b1872.
…10370) The internal REST parameter `path_restricted` was introduced to signal to APIs that a request should be subject to partial restrictions in Serverless mode. The semantics of that parameter have since changed, subtly: in the initial iteration, only requests that corresponded to APIs with partial restrictions were marked. Since then, we have shifted to mark _all_ requests to public APIs by non-operator users, and relying on downstream API handlers to apply partial restrictions (if they had any). This PR replaces the parameter with two new parameters: `serverless_request` and `operator_request`. These parameters are set independently of each other: * `serverless_request` is set for all requests made in serverless mode (regardless of whether the APIs are public or internal) * `operator_request` is set for equests made by operators, both in serverless and in stateful mode This gives REST handlers and other downstream code more flexibility in toggling behavior and decouples the concept of serverless mode from operator mode, which are technically two orthogonal things. Relates: ES-8753
…10370) The internal REST parameter `path_restricted` was introduced to signal to APIs that a request should be subject to partial restrictions in Serverless mode. The semantics of that parameter have since changed, subtly: in the initial iteration, only requests that corresponded to APIs with partial restrictions were marked. Since then, we have shifted to mark _all_ requests to public APIs by non-operator users, and relying on downstream API handlers to apply partial restrictions (if they had any). This PR replaces the parameter with two new parameters: `serverless_request` and `operator_request`. These parameters are set independently of each other: * `serverless_request` is set for all requests made in serverless mode (regardless of whether the APIs are public or internal) * `operator_request` is set for equests made by operators, both in serverless and in stateful mode This gives REST handlers and other downstream code more flexibility in toggling behavior and decouples the concept of serverless mode from operator mode, which are technically two orthogonal things. Relates: ES-8753
The internal REST parameter
path_restrictedwas introduced to signal to APIs that a request should be subject to partial restrictions in Serverless mode.The semantics of that parameter have since changed, subtly: in the initial iteration, only requests that corresponded to APIs with partial restrictions were marked. Since then, we have shifted to mark all requests to public APIs by non-operator users, and relying on downstream API handlers to apply partial restrictions (if they had any).
This PR replaces the parameter with two new parameters:
serverless_requestandoperator_request. These parameters are set independently of each other:serverless_requestis set for all requests made in serverless mode (regardless of whether the APIs are public or internal)operator_requestis set for equests made by operators, both in serverless and in stateful modeThis gives REST handlers and other downstream code more flexibility in toggling behavior and decouples the concept of serverless mode from operator mode, which are technically two orthogonal things.
Relates: ES-8753