Authenticate and wrap HTTP request headers with context supplier by albertzaharovits · Pull Request #93656 · elastic/elasticsearch

albertzaharovits · 2023-02-09T15:28:36Z

Builds on top of #92220 .

Adds authentication to HTTP headers, and packs the authenticated context to be used when executing the URL handler.

This commit moves logic related to implementing security features into the core http server implementation.

…heck

albertzaharovits · 2023-02-20T10:07:01Z

server/src/main/java/org/elasticsearch/common/network/NetworkModule.java

        HttpServerTransport.Dispatcher dispatcher,
+        BiConsumer<BasicHttpRequest, ThreadContext> dispatcherContext,


We can choose to merge the dispatcherContext in the HttpServerTransport.Dispatcher interface, if we opt to go with presented approach where the RestController furnishes the thread context supplier, but it's the http server transport that employs it, because the Security plugin can alter the latter but not the former.

albertzaharovits · 2023-02-20T10:15:39Z

server/src/main/java/org/elasticsearch/http/AbstractHttpServerTransport.java

            if (badRequestCause != null) {
                dispatcher.dispatchBadRequest(channel, threadContext, badRequestCause);
            } else {
+                populateRequestThreadContext0(restRequest, channel, threadContext);
                dispatcher.dispatchRequest(restRequest, channel, threadContext);


This is the code that clears and scopes the thread context for a particular http request to be handled.
This is now extended to also populate the said thread context (with selected HTTP header values).

Is this happening before or after authentication ?

In this PR this is happening after authentication (on main it happens before authentication).

The populateRequestThreadContext hook method is used to instante the authentication context, for the authn that happened earlier.

albertzaharovits · 2023-02-20T10:23:55Z

server/src/main/java/org/elasticsearch/http/BasicHttpRequest.java

+import java.util.List;
+import java.util.Map;
+
+public interface BasicHttpRequest {


This interface is the common ground between the two request types, with or without a body.
It is now used in the AuthenticationService as well as the AudittingService.

albertzaharovits · 2023-02-20T10:27:23Z

server/src/main/java/org/elasticsearch/http/BasicHttpRequest.java

+     * @return the {@link RestRequest.Method} used in the REST request
+     * @throws IllegalArgumentException if the HTTP method is invalid
+     */
+    RestRequest.Method method();


Method doesn't have to be a RestRequest inner enum, and can be moved here, but such a refactoring has a huge diff.

albertzaharovits · 2023-02-20T10:53:32Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

+                dispatcherContext.accept(httpRequest, threadContext);
+                populateClientCertificate.accept(channel, threadContext);
+                RemoteHostHeader.process(channel, threadContext);
+            };


The thread context before running the authentication in the channel handler. It reuses that "dispatch" context that's a sort of baseline (contains selected HTTP header values).

albertzaharovits · 2023-02-20T11:13:25Z

...plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/SSLEngineUtils.java

-    public static void extractClientCertificates(Logger logger, ThreadContext threadContext, TcpChannel tcpChannel) {
-        SSLEngine sslEngine = getSSLEngine(tcpChannel);
-        extract(logger, threadContext, sslEngine, tcpChannel);
+    public static void extractClientCertificates(Logger logger, ThreadContext threadContext, Channel channel) {


Extracting the client certificates must work even when a HttpChannel is not available (it only needs the neety channel anyway).

albertzaharovits · 2023-02-20T11:19:02Z

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

-        if (extractClientCertificate) {
-            HttpChannel httpChannel = request.getHttpChannel();
-            SSLEngineUtils.extractClientCertificates(logger, threadContext, httpChannel);
-        }


The Security plugin now incorporates this when it builds the thread context before running the authentication.

albertzaharovits · 2023-02-20T11:25:57Z

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

-                    logger.trace("Found secondary authentication {} in REST request [{}]", secondaryAuthentication, request.uri());
+                    logger.trace("Found secondary authentication {} in REST request [{}]", secondaryAuthentication, httpRequest.uri());
                }
-                RemoteHostHeader.process(request, threadContext);


This has also been moved to the consolidated thread context populator, before running authentication (here it is "after", but moving it "before" allows more audit events to benefit, and aids the transion from RestRequest to BasicHttpRequest in the auditing service).

albertzaharovits · 2023-02-20T11:30:30Z

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

+        handleException(actionType, channel, e);
+    }
+
+    public static void handleAuthenticationFailed(RestChannel channel, Exception e) {


"Exports" the authn failure response building logic to be used in the channel handler.

albertzaharovits · 2023-02-20T11:49:27Z

...plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticatorChain.java

+        // this throws if a new http request looks to be already authenticated
+        if (authentication != null
+            && (context.getRequest() instanceof AuthenticationService.AuditableHttpRequest)
+            && (((AuthenticationService.AuditableHttpRequest) context.getRequest()).request instanceof HttpRequest == false)) {


Right now, HttpRequests are checked for authentication in two places: the original SecurityRestFilter and the new HttpHeadersAuthenticator, so this check here must account for this fact. But it's likely that the final PR drops the SecurityRestFilter.

albertzaharovits · 2023-02-20T11:52:18Z

...gin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java

-            // There should never be an existing audit-id when processing a rest request.
-            this.requestId = AuditUtil.generateRequestId(threadContext);
+            // There might be an existing audit-id (e.g. generated by the rest request) but there might not be (e.g. an internal action)
+            this.requestId = AuditUtil.getOrGenerateRequestId(threadContext);


Using AuditUtil#getOrGenerateRequestId instead of AuditUtil#generateRequestId is just a temporary measure. Ultimately HttpRequests will only be intercepted once so we can fall back to the stricter generate.

albertzaharovits · 2023-02-20T11:57:04Z

...security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java

        }

-        LogEntryBuilder withRestOrigin(RestRequest request) {
+        LogEntryBuilder withRestOrigin(ThreadContext threadContext) {


Some audit events already relied on the remote address to be set in the thread context.
This PR adopts that approach for all the audit events, because I think it's easier to adapt the auditing service to work in the channel handler this way.

albertzaharovits · 2023-02-20T12:17:29Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

+                public void dispatchBadRequest(RestChannel channel, ThreadContext threadContext, Throwable cause) {
+                    Exception authenticationException = HttpHeadersAuthenticator.extractAuthenticationException(
+                        channel.request().getHttpRequest()
+                    );
+                    if (authenticationException != null) {
+                        SecurityRestFilter.handleAuthenticationFailed(channel, authenticationException);
+                    } else {
+                        dispatcher.dispatchBadRequest(channel, threadContext, cause);
+                    }
+                }


Unfortunately the dispatcher needs to have the the "bad request" branch overridden, because of the small differences between the 400 errors and authn failure errors.

jakelandis

I'm still working though this review , but here are a couple early comments. Also, it would be helpful to have some high level explaination of how this works. I think I understand most of it ...but the order of execution between netty functions and what is in thread context is hard to follow.

jakelandis · 2023-02-22T21:36:52Z

server/src/main/java/org/elasticsearch/http/HttpRequest.java

 * server package's rest handling.
 */
-public interface HttpRequest {
+public interface HttpRequest extends BasicHttpRequest {


This relationship is backwards from what I would expect. I would expect an HttpRequestWithoutBody implements HttpRequest (or possibly PartialHttpRequest to contrast with io.netty.handler.codec.http.FullHttpRequest) and the unsuported methods to throw an exception. Then the usages of HttpRequestWithoutBody can be passed around with the explicit expectations that only part of the larger interface is implemented. i.e. don't use anonymous implementations of BasicHttpRequest instead use a concrete HttpRequestWithoutBody.

Is this a general preference of yours or it's just this particular case?

I think the two options are:

an interface extends another and adds more methods; in case the base interface is passed around and the code has some specialized logic it can use instance of checks to downcast

have a single rich interface and throw "not implemented"

FWIW the body needed for auditing authn success (which, again, is one of the things that's not dealt with properly by this PR) so pending other discussions, we might not need a request interface with a request body....

don't use anonymous implementations of BasicHttpRequest instead use a concrete HttpRequestWithoutBody.

That's separate from the interface conversation, I think. There's going to be some light wrapping needed because the request classes that netty uses are not available to the Security plugin module.

Is this a general preference of yours or it's just this particular case?

I think general ? ... basically I think the interface should represent the concept and any sub interfaces should be specializations of that concept. (IMO) Here the concept is a http request and not having access to the body the a specialization. I understand the throwing not implemented from many methods is also it's own issue, so this is more subjective / preference than any rule. However, I do think as-is this has some readability / expectation issues. Maybe just changing the names would help ... as-is there no indication to what the difference between a HttpRequest and BasicHttpRequest means...java doc can help but good names > doc. I think i still prefer what I suggested above ...but having something like FullHttpRequest extends PartialHttpRequest makes more readily apparent what to expect from the contract.

... has some specialized logic it can use instance of checks to downcast

I would strongly advise against a design that necessitates instanceof checks to downcast

jakelandis · 2023-02-22T22:37:45Z

.../transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpHeaderValidator.java

+
+    private final TriConsumer<HttpRequest, Channel, ActionListener<Void>> validator;
+    private ArrayDeque<HttpObject> pending = new ArrayDeque<>(4);
+    private STATE state = STATE.WAITING_TO_START;


does this class need to be threadsafe ? i.e. what is the cardinatlity of this class ? is it one instance per HTTP request, or one instance per channel ...if the later are there some garuntees for how many threads can call the channelRead method ?

It is one instance per channel, given how it is instantiated in Netty4HttpServerTransport, with ch.pipeline().addLast(..., new Validator()).
There's always one thread per channel. This is core to netty's design and cannot be changed (ever).
So, no it's not required to be thread-safe.

To be pedantic, there are multiple channels per thread, but for a given channel there's always the same single thread that calls the channel handlers.

jakelandis · 2023-02-22T22:45:30Z

.../transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpHeaderValidator.java

+
+import java.util.ArrayDeque;
+
+public class Netty4HttpHeaderValidator extends ChannelInboundHandlerAdapter {


Is it possible to get some javadoc for what this is doing ? I get that it is the hook in point to "validate headers" which can be used to perform authenticatation from the headers ...but all the technical details are beyond my understanding and docs for ChannelInboundHandlerAdapter does not help much. Also, I don't think my review for this class will be worth much ...

Of course we'll polish this, with javadoc, comments, etc. It's TimB creation from #92220 . It also does slightly more than what we strictly need (closing a connection in case authn fails), and there is a small thing that I need to clarify myself when we forward the request but drop the contents.

All that's to say is that this needs trimmings, and it will be a separate PR for it to review and merge.

I propose we sync to explain how it works/supposed to work before we get to the polishing stage.

jakelandis · 2023-02-22T23:16:54Z

server/src/main/java/org/elasticsearch/http/AbstractHttpServerTransport.java

            if (badRequestCause != null) {
                dispatcher.dispatchBadRequest(channel, threadContext, badRequestCause);
            } else {
+                populateRequestThreadContext0(restRequest, channel, threadContext);
                dispatcher.dispatchRequest(restRequest, channel, threadContext);


Is this happening before or after authentication ?

jakelandis · 2023-02-22T23:52:45Z

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

@@ -28,50 +34,185 @@
 public interface RestRequestFilter {


why does this class need to change ?

Good question!
Short answer is that this filtering, which is needed for auditing, currently works on RestRequest types, but we now need it to work on HTTP request bodies.

And that's because auditing works with HTTP requests, because authentication now has to work with HTTP requests, and the two are very much entangled.

BUT I have a proposal to decouple auditing for authentication success from the actual authentication (which sounds bad, I admit) and this is the main reason: today, when we only support authentication on the "REST" channel (no transport client), and the authentication happens before the request body is available, and we want to audit the request body, we cannot cleanly couple authentication and auditing.

Basically, it has to do with auditing, which is not properly addressed by this PR anyway (authn success is audited but not the request body), and for which I have a different proposal anyway (that will not require such changes).

jakelandis · 2023-02-23T19:20:51Z

.../transport-netty4/src/main/java/org/elasticsearch/http/netty4/HeaderValidationException.java

+        this.closeChannel = closeChannel;
+    }
+
+    public boolean shouldCloseChannel() {


Is it OK to close the channel for HTTP pipeline requests ? (will that kill the other requests)

We don't need this functionality. We don't have it today on main and it's not something we plan to ship (confirmed with TimB).

It's added by TimB in #92220 , and I deliberately kept changes to the channel handler implementation to a minimum to ease the rewieving of this for him (but in the process also complicating it for you, sorry...).

jakelandis · 2023-02-23T22:09:53Z

...s/transport-netty4/src/main/java/org/elasticsearch/http/netty4/HttpHeadersAuthenticator.java

+        try (ThreadContext.StoredContext ignore = threadContext.stashContext()) {
+            populateThreadContext.apply(requestToAuthenticate, channel, threadContext);
+            authenticate.accept(requestToAuthenticate, ActionListener.wrap(ignored -> {
+                final ThreadContext.StoredContext authenticatedContext = threadContext.newStoredContext();


I think I remember you said that we need to associate this authenticated context with the request since the context will get lost since it may not part of the same call stack... but can you detail abit more why that is ? Isn't both this code (storing the authenticated context) and the code that restores the authenticated context happening on the same eventloop thread ? (also, what thread does the existing (before this PR) authentication happen on -> the eventloop thread?)

Tim-Brooks and others added 20 commits June 1, 2022 19:35

Move security http server logic to http server

0bc08e3

This commit moves logic related to implementing security features into the core http server implementation.

Changes

b5eaf0b

Merge remote-tracking branch 'upstream/main' into http_single_impl

d8b47a5

Cleanup

32a3ae7

Fix

726b43d

Merge remote-tracking branch 'upstream/main' into http_single_impl

67bfdac

INit

7fba280

Changes

5b43b00

Merge remote-tracking branch 'upstream/main' into early_http_header_c…

4bf2730

…heck

tests

2b66570

Merge remote-tracking branch 'upstream/main' into early_http_header_c…

0ed6cf8

…heck

Changes

b9f2792

Merge remote-tracking branch 'upstream/main' into early_http_header_c…

a4f17ff

…heck

All headers validated

827a161

Merge branch 'main' into header-validator-hook-in

705f826

Spotless

ec6b567

Fix Netty4HttpServerTransportTests

3743447

Merge branch 'main' into header-validator-hook-in

fc31cbc

Merge branch 'main' into header-validator-hook-in-channel-context

a5e530b

Merge branch 'main' into header-validator-hook-in-channel-context

85b2f68

albertzaharovits self-assigned this Feb 9, 2023

elasticsearchmachine added the v8.8.0 label Feb 9, 2023

albertzaharovits force-pushed the header-validator-hook-in-channel-context branch from 9eb56cc to 631e507 Compare February 9, 2023 16:19

Expose populating thread context in HTTP Server Transport

d9da947

albertzaharovits force-pushed the header-validator-hook-in-channel-context branch from 631e507 to d9da947 Compare February 10, 2023 11:40

albertzaharovits added 5 commits February 11, 2023 19:58

Rough header interceptor

3b1f48f

BasicHttpRequest

5c6b2dc

Audit get rest remote address from thread context

9f47c33

Authenticate http requests rather than rest requests

e974d3d

Dispatcher context use BasicHttpRequest

599e65b

albertzaharovits commented Feb 20, 2023

View reviewed changes

Revert useless changes to RestRequest.java

fad090d

albertzaharovits commented Feb 20, 2023

View reviewed changes

Remove more useless diff

8090eb8

albertzaharovits commented Feb 20, 2023

View reviewed changes

albertzaharovits changed the title ~~Header validator hook in channel context~~ Authenticate and wrap request headers with context suppier Feb 20, 2023

albertzaharovits changed the title ~~Authenticate and wrap request headers with context suppier~~ Authenticate and wrap request headers with context supplier Feb 20, 2023

albertzaharovits changed the title ~~Authenticate and wrap request headers with context supplier~~ Authenticate and wrap HTTP request headers with context supplier Feb 20, 2023

albertzaharovits requested review from Tim-Brooks and jakelandis February 20, 2023 12:29

jakelandis reviewed Feb 22, 2023

View reviewed changes

Merge branch 'main' into header-validator-hook-in-channel-context

63154f1

jakelandis reviewed Feb 23, 2023

View reviewed changes

albertzaharovits mentioned this pull request Feb 28, 2023

Add mechanism for early http header validation #92220

Merged

gmarouli added v8.9.0 and removed v8.8.0 labels Apr 26, 2023

pugnascotia added v8.10.0 and removed v8.9.0 labels Jun 22, 2023

albertzaharovits closed this Aug 1, 2023

		HttpServerTransport.Dispatcher dispatcher,
		BiConsumer<BasicHttpRequest, ThreadContext> dispatcherContext,


		import java.util.ArrayDeque;

		public class Netty4HttpHeaderValidator extends ChannelInboundHandlerAdapter {

Conversation

albertzaharovits commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelandis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertzaharovits Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelandis Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

albertzaharovits commented Feb 9, 2023 •

edited

Loading

albertzaharovits Feb 24, 2023 •

edited

Loading

jakelandis Feb 23, 2023 •

edited

Loading