Netty4 HTTP authn enhancements (#92220) #96703

albertzaharovits · 2023-06-08T13:18:11Z

This is a backport of multiple work items related to authentication enhancements for HTTP,
which were originally merged in the 8.8 - 8.9 releases.
Hence, the HTTP (only the netty4-based implementation (default), not the NIO one) authentication
implementation gets a throughput boost (especially for requests failing authn).

Relates to: ES-6188 #92220 #95112

This introduces a way to validate HTTP headers prior to reading the request body. Co-authored-by: Albert Zaharovits <albert.zaharovits@elastic.co>

Authentication for the HTTP channel is changing to not have access to the contents of the HTTP request body. But auditing of authentication success still requires access to the request body. Consequently, auditing of authentication success is now separated from the authentication logic: auditing is invoked in the SecurityRestFilter#handleRequest, after the AuthenticationService has done its job earlier in the handling flow for the HTTP request.

…lastic#94959) In preparation for when the audit trail will not be able to use the request interface to extract the remote endpoint address of rest requests, this PR makes the LoggingAuditTrail to look into the thread context for the remote address, and the SecurityRestFilter to populate as such the thread context before invoking the authentication.

albertzaharovits · 2023-06-09T15:00:22Z

@elasticmachine run elasticsearch-ci/part-2

…uests (elastic#94990) AuthN and Audit are NOT going to have access to the Rest interface anymore, because the AuthenticationService is going to be invoked before the RestRequest is built. This refactoring makes the AuthenticationService and the AuditTrail take as parameters HTTP-related request interfaces.

…xt supplier (elastic#95040) When `Security` authenticates a request (AuthenticationService#authenticate) it mostly looks in the thread context for all the request details that it needs. Consequently, there's code that populates the thread context before invoking authentication. This PR brings all the code that's responsible for populating the said thread context, as a parameter to the `Security` plugin. This is required in the follow-up that is going to involve setting up the thread context (given the aforementioned parameter), and running the authentication under that context, before the request is dispatched (rather than "while" it's being dispatched, currently).

…95189) This ensures that Warning and X-elastic-product response headers are only removed in case of 401 or 403 HTTP error response codes. Previously, the response headers were also removed in non 401 and 403 conditions, in case of esoteric errors during authN, but this behavior displaced the response-filtering logic to an uncomfortable code site from a reusability pov.

…ic#95498) This ensures that the validator (of http requests over the netty channel) is unable to alter the ES context of netty worker threads. The netty worker threads are oblivious to the ES context, but they eventually call into the ES code, at which point any tampering of the context during the validation is visible and potentially dangerous.

Hooks "REST" authN, as a "validator", into the new netty channel interceptor for http headers.

This address HTTP OPTIONS requests following the authentication refactoring in elastic#95112. Relates elastic#95112

Instead of not authN and letting them through, this PR rejects OPTIONS requests with a body (400). Relates elastic#95112

This PR tests that malformed HTTP requests that fail at the decoding stage don't go through validation and are further dispatched as bad requests. Related: elastic#95112

Skips SecurityNetty4HttpServerTransportTests testMalformedRequestDispatchedNoAuthn under turkish-like locales because the asserted messages are internally processed with String#toUpper(). This results in different error messages under such locales, and it's easier to skip the test entirely than to assert the different messages. Closes: elastic#95983

Fixes SecurityNetty4HttpServerTransportTests testAuthnContextWrapping The mocking removed code that called release on request objects. Closes elastic#95846

In patological cases it might be possible that validation in invoked on requests that are not primed correctly (wrapped with HttpHeadersAuthenticatorUtils#wrapAsMessageWithAuthenticationContext) during HTTP decoding. This test asserts that such occurences are a case of 500 error.

Large requests, that exceed the http.max_content_length, are never dispatched, even if the HTTP header validation fails and the request should otherwise be dispatched as a "bad" one.

albertzaharovits · 2023-06-19T11:10:33Z

server/src/main/java/org/elasticsearch/ElasticsearchException.java

+            org.elasticsearch.http.HttpHeadersValidationException.class,
+            org.elasticsearch.http.HttpHeadersValidationException::new,
+            162,
+            Version.V_7_17_11


Reminder to fix this with the version that this PR actually lands in.
Also, I need to figure out what this (transport) version needs to be since the HttpHeadersValidationException was also merged into 8.9 (but with a different id).

I've pushed 140f323 to make the HttpHeadersValidationException not extend ElasticsearchException such that it doesn't have to be registered for serialization. This is possible because HttpHeadersValidationExceptions are never serialized across nodes (they are exclusively used to "communicate" between the validating channel handler and the RestController).

(I turns out backporting new ElasticsearchException types to 7.17 is not easily done).

albertzaharovits · 2023-06-20T11:15:20Z

I've pushed 74bced2 to reinstate the tests for authn failure in the SecurityRestFilter.
The changes merged into main (8.8-8.9) completely removed authn (except secondary authn) in the SecurityRestFilter, and consequently the tests changed as well, but in this backport the SecurityRestFilter still handles authn for NIO request types, so tests for authn failures at the SecurityRestFilter layer are still in order.

jakelandis

@albertzaharovits - nice job on this back port ! I have spent time reviewing commit by commit (thanks for keeping that as an option) paying special attention to anything specific to 7.x. While reviewing the tests, i mostly looked at changes to existing tests that could indicate behavior changes (didn't find any) and didn't pay much attention to net-new tests (I assume those generally copy pasted). I then diffed many of the key files to their 8.x counterpart looking for any differences not explained by unrelated commits. I only found a couple minor questions... please let me know if there are specific areas that may not be covered by my approach (or anything you just a second set of eyes on).

jakelandis · 2023-08-16T15:46:11Z

...in/security/src/test/java/org/elasticsearch/xpack/security/rest/SecurityRestFilterTests.java

-        filter = new SecurityRestFilter(licenseState, threadContext, authcService, secondaryAuthenticator, restHandler, false);
+        }).when(auditTrail).authenticationSuccess(any(RestRequest.class));
+        TestUtils.UpdatableLicenseState licenseState = new TestUtils.UpdatableLicenseState();
+        licenseState.update(License.OperationMode.GOLD, true, null);


why do we need to update the licenseState ?

Yeah, TestUtils.UpdatableLicenseState licenseState = new TestUtils.UpdatableLicenseState() gives you a null license.

jakelandis · 2023-08-16T18:23:30Z

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

+                ActionListener.wrap(ignored -> listener.onResponse(null), listener::onFailure)
+            );
+        } else {
+            listener.onResponse(null);


nit: can you assert the expected request type and comment that authentication for non-nio requests is handled elsewhere. (i totally read this wrong the first time)

Had to implement some convoluted get-arounds to address this, because besides Netty4HttpRequests, in tests, there are also FakeHttpRequests inbound.

jakelandis · 2023-08-16T19:31:33Z

...tty4/src/main/java/org/elasticsearch/http/netty4/internal/HttpHeadersAuthenticatorUtils.java

+    }
+
+    private static HttpHeadersWithAuthenticationContext unwrapAuthenticatedHeaders(org.elasticsearch.http.HttpRequest request) {
+        if (request instanceof HttpPipelinedRequest) {


Can you help me understand why we don't need to do this in 8.x ? https://github.com/elastic/elasticsearch/blob/main/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/internal/HttpHeadersAuthenticatorUtils.java#L110

In 7.17 there is an extra netty pipeline handler

elasticsearch/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java

Line 336 in fe18a67

ch.pipeline().addLast("pipelining", new Netty4HttpPipeliningHandler(logger, transport.pipeliningMaxEvents));

that assigns the received id tag to requests, by wrapping them in this HttpPipelinedRequest type

elasticsearch/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java

Line 47 in fe18a67

HttpPipelinedRequest pipelinedRequest = aggregator.read(((Netty4HttpRequest) msg));

.
In main, there's no wrapping. The pipelining (HTTP proto) and request dispatch happen in the same channel handler

elasticsearch/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java

Line 114 in 991b06e

netty4HttpRequest = new Netty4HttpRequest(readSequence++, fullHttpRequest, nonError);

.

Hence the code in the HttpHeadersAuthenticatorUtils has to peel away one extra layer in 7.17 to get to the type that contains the authentication.

albertzaharovits · 2023-08-23T13:05:13Z

please let me know if there are specific areas that may not be covered by my approach (or anything you just a second set of eyes on).

There's nothing that I can think of.

jakelandis · 2023-08-23T15:50:26Z

Thanks for the follow ups... LGTM

albertzaharovits added backport v7.17.11 labels Jun 8, 2023

albertzaharovits self-assigned this Jun 8, 2023

Add mechanism for early http header validation (elastic#92220)

1009ac5

This introduces a way to validate HTTP headers prior to reading the request body. Co-authored-by: Albert Zaharovits <albert.zaharovits@elastic.co>

albertzaharovits force-pushed the backport-7.17-DOS715 branch from e20b604 to 1009ac5 Compare June 8, 2023 14:15

albertzaharovits added 2 commits June 8, 2023 19:43

albertzaharovits force-pushed the backport-7.17-DOS715 branch 5 times, most recently from 8e41efc to 0aa3df6 Compare June 12, 2023 18:45

albertzaharovits added 2 commits June 13, 2023 09:45

albertzaharovits force-pushed the backport-7.17-DOS715 branch from 0aa3df6 to 7c956e9 Compare June 13, 2023 06:46

albertzaharovits added 2 commits June 13, 2023 13:39

albertzaharovits force-pushed the backport-7.17-DOS715 branch 6 times, most recently from b1a1499 to f61af24 Compare June 15, 2023 11:57

Security authn via netty channel validator (elastic#95112)

f409613

Hooks "REST" authN, as a "validator", into the new netty channel interceptor for http headers.

albertzaharovits force-pushed the backport-7.17-DOS715 branch from 7dc2d2a to 454af7e Compare June 15, 2023 15:47

albertzaharovits added 3 commits June 15, 2023 19:48

Still authn NIO requests in the SecurityRestFilter

8d6f2d5

Handle Unauthenticated OPTIONS requests (elastic#96061)

250dcf8

This address HTTP OPTIONS requests following the authentication refactoring in elastic#95112. Relates elastic#95112

Reject OPTIONS requests with a body (elastic#96357)

f76d8a0

Instead of not authN and letting them through, this PR rejects OPTIONS requests with a body (400). Relates elastic#95112

albertzaharovits force-pushed the backport-7.17-DOS715 branch from 454af7e to f76d8a0 Compare June 15, 2023 16:48

albertzaharovits added 5 commits June 19, 2023 10:09

Test that malformed HTTP request is not validated (elastic#95886)

928d910

This PR tests that malformed HTTP requests that fail at the decoding stage don't go through validation and are further dispatched as bad requests. Related: elastic#95112

Fix SecurityNetty4HttpServerTransportTests (elastic#95913)

578a06a

Fixes SecurityNetty4HttpServerTransportTests testAuthnContextWrapping The mocking removed code that called release on request objects. Closes elastic#95846

Test large request is never dispatched (elastic#95962)

c616d6b

Large requests, that exceed the http.max_content_length, are never dispatched, even if the HTTP header validation fails and the request should otherwise be dispatched as a "bad" one.

albertzaharovits commented Jun 19, 2023

View reviewed changes

Make the HttpHeadersValidationException a RuntimeException

140f323

albertzaharovits added :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) :Distributed Coordination/Network Http and internode communication implementations >enhancement labels Jun 20, 2023

albertzaharovits requested a review from jakelandis June 20, 2023 08:09

Reinstate authn failure for SecurityRestFilterTests

74bced2

albertzaharovits changed the title ~~Backport 715 (#92220)~~ Netty4 HTTP authn enhancements (#92220) Jun 20, 2023

albertzaharovits marked this pull request as ready for review June 20, 2023 11:16

joegallo added v7.17.12 and removed v7.17.11 labels Jun 23, 2023

rjernst added v7.17.13 and removed v7.17.12 labels Jul 21, 2023

jakelandis approved these changes Aug 16, 2023

View reviewed changes

albertzaharovits added 4 commits August 23, 2023 13:17

Merge branch '7.17' into backport-7.17-DOS715

aeb8443

Nit test

b0c7b65

Nit: assert request type in SecurityRestFilter

70c8225

Fix assert request type fallout

0a6e189

albertzaharovits merged commit 0df52c8 into elastic:7.17 Aug 23, 2023

albertzaharovits deleted the backport-7.17-DOS715 branch August 23, 2023 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Netty4 HTTP authn enhancements (#92220) #96703

Netty4 HTTP authn enhancements (#92220) #96703

albertzaharovits commented Jun 8, 2023 •

edited

Loading

albertzaharovits commented Jun 9, 2023

albertzaharovits Jun 19, 2023

albertzaharovits Jun 20, 2023

albertzaharovits commented Jun 20, 2023

jakelandis left a comment

jakelandis Aug 16, 2023

albertzaharovits Aug 23, 2023

jakelandis Aug 16, 2023

albertzaharovits Aug 23, 2023

jakelandis Aug 16, 2023

albertzaharovits Aug 23, 2023

albertzaharovits commented Aug 23, 2023

jakelandis commented Aug 23, 2023

Netty4 HTTP authn enhancements (#92220) #96703

Netty4 HTTP authn enhancements (#92220) #96703

Conversation

albertzaharovits commented Jun 8, 2023 • edited Loading

albertzaharovits commented Jun 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits commented Jun 20, 2023

jakelandis 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

albertzaharovits commented Aug 23, 2023

jakelandis commented Aug 23, 2023

albertzaharovits commented Jun 8, 2023 •

edited

Loading