Servlerless API protection with annotations #93607

masseyke · 2023-02-08T22:04:55Z

This PR makes it so that only REST endpoints that are explicitly annotated with a ServerlessScope are visible when running in serverless mode. Each RestHandler can be given an annotation of either Scope.PUBLIC or Scope.INTERNAL. PUBLIC means that it is visible to everyone. INTERNAL means that it is meant to be visible only to the control plane, and requires a X-elastic-internal-origin HTTP header. No annotation means that the RestHandler is not visible at all in Serverless mode.
For now, this functionality is only enabled if the serverless.enabled node setting is set to true (false by default), but will be configured instead with the stateless.enabled node setting in the future. It is a separate setting in this initial commit because in this commit only two endpoints are enabled, which would make running stateless in a meaningful way impossible.
Also note that this PR only annotates / and /favicon.ico. Follow-up PRs will correctly annotate other endpoints.

elasticsearchmachine · 2023-02-08T22:05:19Z

Hi @masseyke, I've created a changelog YAML for you.

…ub.com:masseyke/elasticsearch into feature/servlerless-api-protection-annotations

tvernum

I left a few comments.
Only 1 of them is really meaningful in terms of "what do we learn from the proof of concept", point of view.

tvernum · 2023-02-10T05:25:06Z

server/src/main/java/org/elasticsearch/rest/Access.java

+public enum Access {
+    PUBLIC,
+    INTERNAL,
+    HIDDEN


Without wanting to bikeshed a Proof of Concept, I think we had some ambiguity in meaning for HIDDEN - some people thought it was equivalent to "internal" (that is, hidden but still exists) but here it means "unavailable" (hidden and cannot be reached).

Could we use UNAVAILABLE / DISABLED / BLOCKED instead?

Do we need a name for it here at all? I think we agreed the default state should be blocking handlers, and that simply omitting the annotation would suffice for that? This way we also don't need any gradle magic to ensure all handlers have the annotation.

Yeah getting rid of it makes sense (it's not used in the logic at all). I had thought of it as an explicit marker that someone had already looked at this rest handler and decided that it ought to be hidden (vs one that someone just hadn't looked at yet).

I have a general preference for explict, but I really don't want two ways to say the same thing (no annotation, or an annotation with "UNAVAILABLE"), so I'm good with dropping it from the enum.

tvernum · 2023-02-10T05:48:30Z

server/src/main/java/org/elasticsearch/rest/RestController.java

@@ -779,4 +801,13 @@ private static CircuitBreaker inFlightRequestsBreaker(CircuitBreakerService circ
        // We always obtain a fresh breaker to reflect changes to the breaker configuration.
        return circuitBreakerService.getBreaker(CircuitBreaker.IN_FLIGHT_REQUESTS);
    }
+
+    @AccessLevel(Access.PUBLIC)


My concern with this Annotation based approach is that it only means something if rest.enforce_api_protections is enabled, which it won't be by default.
I think that is going to be confusing to people who read the code without that (relatively esoteric) piece of information.

When we spoke @rjernst suggested we name the annotation @ServerlessAccessLevel or something like that. That goes a long way to solving the confusion, but leaks "serverless" into the code, where it isn't actually needed.
I'd love to find a way to minimize the confusion that doesn't require us to inject "serverless" into the annotation name.

I have not thought of a way around this. Is this a showstopper for doing this in the code (vs an external list)? Regardless of what we do in the code, it's going to be meaningless outside of Serverless.

I'm not that concerned about leaking "serverless". It's a known thing, and it's a conscious choice. This is specifically for serverless, so calling it another name would be more confusing IMO.

tvernum · 2023-02-10T05:50:05Z

.../plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java

@@ -80,6 +80,10 @@ public boolean allowSystemIndexAccessByDefault() {
        return restHandler.allowSystemIndexAccessByDefault();
    }

+    public RestHandler getRootRestHandler() {
+        return restHandler;


I think this should be

Suggested change

return restHandler;

return restHandler.getRootRestHandler();

in case we have Security wrapper around a Deprecation handler.

tvernum · 2023-02-10T05:50:19Z

server/src/main/java/org/elasticsearch/rest/DeprecationRestHandler.java

@@ -95,6 +95,11 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c
        handler.handleRequest(request, channel, client);
    }

+    @Override
+    public RestHandler getRootRestHandler() {
+        return handler;


Suggested change

return handler;

return handler.getRootRestHandler();

rjernst · 2023-02-10T15:05:00Z

server/src/main/java/org/elasticsearch/rest/AccessLevel.java

+import java.lang.annotation.RetentionPolicy;
+
+@Retention(RetentionPolicy.RUNTIME)
+public @interface AccessLevel {


nit: I think we agreed the name should be clearly for serverless, something like ServerlessScope?

masseyke · 2023-02-13T14:56:31Z

@elasticmachine update branch

elasticsearchmachine · 2023-02-14T17:04:14Z

Pinging @elastic/es-data-management (Team:Data Management)

rjernst

Looks pretty good, I have a couple nits

rjernst · 2023-02-16T19:04:57Z

server/src/main/java/org/elasticsearch/action/ActionModule.java

+                    catActions.add((AbstractCatAction) handler);
+                }
+                restController.registerHandler(handler);
+            } // else there's no way this handler can be reached, so no point in keeping it around


What about having a dummy handler that returns a nicer error? That is, an error indicating this api is not available in serverless, instead of just "not found"?

Do we want users to know that it's an API that they cannot access? I had been thinking we'd want to make it look as if the API just doesn't exist.

I wouldn't phrase it as "can't access", but rather it is unavailable in fully managed Elasticsearch. We want it to not exist, but having a more clear error message may help guide users. For example, if a user is looking at documentation for self managed Elasticsearch, and tries to follow an a example, getting a 404 (that they might have typo'd) could be frustrating. If instead they get a message like "this API is not available in fully managed Elasticsearch" it hints to them they need to check what they are doing (looking at self managed docs instead of fully managed docs).

Alright the response now says something like uri [/_cat/health] with method [GET] exists but is not available when running in serverless mode.

rjernst · 2023-02-16T19:11:01Z

server/src/main/java/org/elasticsearch/rest/RestController.java

+            } else if (Scope.PUBLIC.equals(scope) == false) {
+                handleBadRequest(request.uri(), request.method(), responseChannel);
+                return;
+            }


Perhaps my suggestion about a better error message could be handled with a static dummy handler (that has no scope), and then an else case here which returns the nicer error response?

rjernst · 2023-02-16T19:14:56Z

server/src/main/java/org/elasticsearch/rest/action/RestMainAction.java

@@ -24,6 +26,7 @@
 import static org.elasticsearch.rest.RestRequest.Method.GET;
 import static org.elasticsearch.rest.RestRequest.Method.HEAD;

+@ServerlessScope(Scope.PUBLIC)
 public class RestMainAction extends BaseRestHandler {


Are we sure the main action is in scope for serverless? In its current form it exposes a bunch of information that will not make sense in serverless: cluster name, cluster uuid, build information (which includes the self managed version number). Maybe this should be internal to start?

Oh hmm good point. I'll change it to internal.

rjernst · 2023-02-16T19:16:37Z

server/src/test/java/org/elasticsearch/rest/RestControllerTests.java

@@ -953,6 +967,139 @@ public void testRegisterWithReservedPath() {
        }
    }

+    public void testApiProtection() {
+        {


Can these be separate test methods?

… mode

…EST handlers (#94034) #93607 added the ability to run Elasticsearch in "Serverless" mode, where access to REST endpoints could be restricted so that the full Elasticsearch API is not available (since a lot of it does not make sense in Servlerless). By default no endpoints are available, but they can be exposed with `ServerlessScope` annotations. This PR follows up on #93607 by adding PUBLIC and INTERNAL annotations to the rest handlers owned by the Distributed team. There are several rest endpoints still under discussion. This PR does not label those, so they remain unavailable in Serverless mode.

This commit enforces that the "X-elastic-internal-origin" header may only be provided by users who have been marked as "operators". Relates: elastic#93607

…ransform REST handlers elastic#93607 added the ability to run Elasticsearch in "Serverless" mode, where access to REST endpoints could be restricted so that the full Elasticsearch API is not available (since a lot of it does not make sense in Serverless). By default no endpoints are available, but they can be exposed with `ServerlessScope` annotations. This PR follows up by annotating the transform REST handlers.

…handlers (#95253) #93607 added the ability to run Elasticsearch in "Serverless" mode, where access to REST endpoints could be restricted so that the full Elasticsearch API is not available (since a lot of it does not make sense in Serverless). By default no endpoints are available, but they can be exposed with `ServerlessScope` annotations. This PR follows up by annotating the ML REST handlers.

…ransform REST handlers (#95254) #93607 added the ability to run Elasticsearch in "Serverless" mode, where access to REST endpoints could be restricted so that the full Elasticsearch API is not available (since a lot of it does not make sense in Serverless). By default no endpoints are available, but they can be exposed with `ServerlessScope` annotations. This PR follows up by annotating the transform REST handlers.

…ST handlers (#94037) #93607 added the ability to run Elasticsearch in "Serverless" mode, where access to REST endpoints could be restricted so that the full Elasticsearch API is not available (since a lot of it does not make sense in Servlerless). By default no endpoints are available, but they can be exposed with `ServerlessScope` annotations. This PR follows up on #93607 by adding PUBLIC and INTERNAL annotations to the rest handlers owned by the Core Infra team. There are several rest endpoints still under discussion. This PR does not label those, so they remain unavailable in Serverless mode.

A special header "X-elastic-internal-origin" was introduced to help facilitate testing of serverless scopes introduced in #93607. The use of this header has been superseded and as such this special header is no longer needed. This commit removes reference to the header and adjusts the tests accordingly. Internal vs. Public scopes are now controlled by operator privileges when running in serverless mode. This commit also changes the response from 400 to 404 if the requested path is not available. This matches the 404 response code returned by operator privs if internal scopes are not reachable.

This commit changes the method for enabling Serverless API protections (see elastic#93607) from a system property to a runtime property. Within this project there is no external way to configure this property - it must be controlled by a plugin - but the RestController has been changed to dynamically adapt to a change in that property.

This commit changes the method for enabling Serverless API protections (see #93607) from a system property to a runtime property. Within this project there is no external way to configure this property - it must be controlled by a plugin - but the RestController has been changed to dynamically adapt to a change in that property.

Servlerless API protection with annotations

a58cbd5

masseyke added >enhancement :Data Management/Indices APIs APIs to create and manage indices and templates v8.8.0 labels Feb 8, 2023

masseyke added 3 commits February 8, 2023 16:05

Update docs/changelog/93607.yaml

0bc04b2

cleanup

f686fb3

Merge branch 'feature/servlerless-api-protection-annotations' of gith…

045d5a2

…ub.com:masseyke/elasticsearch into feature/servlerless-api-protection-annotations

tvernum reviewed Feb 10, 2023

View reviewed changes

rjernst reviewed Feb 10, 2023

View reviewed changes

masseyke added 4 commits February 10, 2023 13:43

cleanup, unit testing

5160f09

renaming AccessLevel annotation

9392dee

cleanup

765df4f

Moving the serverless setting into Node

c1d0f31

elasticmachine and others added 2 commits February 14, 2023 01:26

Merge branch 'main' into feature/servlerless-api-protection-annotations

771a68f

cleanup

2c4ae9a

masseyke mentioned this pull request Feb 14, 2023

Servlerless API protection #93534

Closed

masseyke added 2 commits February 14, 2023 10:36

using DiscoveryNode.isStateless instead of a custom serverless setting

daa5559

cleanup

85c864f

masseyke marked this pull request as ready for review February 14, 2023 17:04

elasticsearchmachine added the Team:Data Management Meta label for data/management team label Feb 14, 2023

reintroducing serverless.enabled setting

b9ef1bc

masseyke requested a review from rjernst February 16, 2023 14:56

masseyke added 2 commits February 16, 2023 12:17

Merge branch 'main' into feature/servlerless-api-protection-annotations

4211d93

changing serverless config to a system property

2624c48

rjernst requested changes Feb 16, 2023

View reviewed changes

masseyke added 2 commits February 16, 2023 13:35

code review feedback

5b59124

Better error message when an endpoint cannot be reached in serverless…

a030cb6

… mode

lukeelmers mentioned this pull request Feb 22, 2023

[http] Prevent access to internal-only APIs when running in serverless elastic/kibana#151940

Closed

jakelandis mentioned this pull request Apr 13, 2023

Categorize Security APIs #95245

Merged

droberts195 mentioned this pull request Apr 14, 2023

[ML] Adding initial public and internal serverless scopes to ML REST handlers #95253

Merged

droberts195 mentioned this pull request Apr 14, 2023

[TRANSFORM] Adding initial public and internal serverless scopes to transform REST handlers #95254

Merged

lukeelmers mentioned this pull request Apr 20, 2023

[core.elasticsearch] All outbound requests to Elasticsearch need to include x-elastic-internal-origin header elastic/kibana#155450

Closed

3 tasks

pquentin mentioned this pull request Apr 24, 2023

List all Elasticsearch APIs Rally is using elastic/rally#1709

Closed

jakelandis mentioned this pull request Jun 13, 2023

Remove interim handling of serverless rest protections #96814

Merged

tvernum mentioned this pull request Jun 26, 2023

Enable Serverless API protections dynamically #97079

Merged

111andre111 mentioned this pull request Jul 26, 2023

Set new header for diag requests to Kibana on serverless elastic/support-diagnostics#657

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Servlerless API protection with annotations #93607

Servlerless API protection with annotations #93607

masseyke commented Feb 8, 2023 •

edited

Loading

elasticsearchmachine commented Feb 8, 2023

tvernum left a comment

tvernum Feb 10, 2023

rjernst Feb 10, 2023

masseyke Feb 10, 2023

tvernum Feb 13, 2023

tvernum Feb 10, 2023

masseyke Feb 10, 2023

rjernst Feb 10, 2023

tvernum Feb 10, 2023

tvernum Feb 10, 2023

rjernst Feb 10, 2023

masseyke commented Feb 13, 2023

elasticsearchmachine commented Feb 14, 2023

rjernst left a comment

rjernst Feb 16, 2023

masseyke Feb 16, 2023

rjernst Feb 16, 2023

masseyke Feb 16, 2023

rjernst Feb 16, 2023

rjernst Feb 16, 2023

masseyke Feb 16, 2023

masseyke Feb 16, 2023

rjernst Feb 16, 2023

masseyke Feb 16, 2023

Servlerless API protection with annotations #93607

Servlerless API protection with annotations #93607

Conversation

masseyke commented Feb 8, 2023 • edited Loading

elasticsearchmachine commented Feb 8, 2023

tvernum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masseyke commented Feb 13, 2023

elasticsearchmachine commented Feb 14, 2023

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masseyke commented Feb 8, 2023 •

edited

Loading