Add mechanism for early http header validation #92220
Conversation
This commit moves logic related to implementing security features into the core http server implementation.
Tim-Brooks
added
>non-issue
:Distributed Coordination/Network
|
Pinging @elastic/es-distributed (Team:Distributed) |
elasticsearchmachine
added
the
Team:Distributed (Obsolete)
.../transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpHeaderValidator.java
Outdated
Show resolved
Hide resolved
| } else if (state == STATE.FORWARDING_DATA) { | ||
| assert pending.isEmpty(); | ||
| if (httpObject instanceof LastHttpContent) { | ||
| state = STATE.WAITING_TO_START; | ||
| } | ||
| ctx.fireChannelRead(httpObject); |
There was a problem hiding this comment.
Background
Security is used to convey the authentication information (when successful) in thread-local variables that we call the threadContext.
In essence, all the code that's run by a thread with some authentication information in the threadContext is said to run "as" the "subject" from that authentication information.
Today, the SecurityRestFilter sets and clears the threadContext, in a try-block, before and after executing the request handler.
Question
Assuming authentication is successful, and here we're forwarding httpObjects to the next pipeline handler. We need to ensure that the threadContext contains the authentication information, when the complete request gets to the RestController.
Is is correct to set the threadContext to contain the authentication when we forward the LastHttpContent message?
I think the downstream pipeline handler, upon seeing this message will assemble the complete request and forward it further, under the same thread call stack, i.e. not enqueue it, like it does with the other httpObjects and wait for some other event to forward the complete request, in which case the threadContext will not be populated correctly.
There was a problem hiding this comment.
Yeah. We cannot propagate this through thread locals at this point. As additional context switching could happen. If there is information from Authentication that we need to propagate, the Validator will have to return it and we can find some mechanism to shove it in the http request object until "handling" where we can set the thread context.
There was a problem hiding this comment.
we can find some mechanism to shove it in the http request object until "handling" where we can set the thread context.
We discussed to wrap and decorate the HttpRequest that we forward with the authn result. That looks good, but the problem is that the HttpRequest is not available from the FullHttpRequest that the RestController ultimately deals with, so it cannot get hold of the authn result either (in order to set it in the thread context that runs the request handler).
In order to expose the authn result to the FullHttpRequest we'll need to extend the HttpObjectAggregator slightly (more precisely extend #beginAggregation to return an implementation of FullHttpMessage that contains the authn result from the HttpMessage start parameter).
I'm going to try implementing this.
|
@Tim-Brooks I'm going to push to this branch of yours in order to get it merged. Let me know if you prefer I create a branch on my fork, incorporate the changes here, and iterate on that instead. |
|
@Tim-Brooks This is ready for your review. I'll be adding a few more tests soon, but I think this is now looking very polished. |
|
ping @Tim-Brooks |
| } | ||
| }) | ||
| .addLast("aggregator", aggregator); |
There was a problem hiding this comment.
Can we remove this formatting change?
There was a problem hiding this comment.
I've pushed d6d7b87 to stuff some inline comments following an addLast such that spotless doesn't kick in to combine multiple addLast on the same line.
|
LGTM |
This introduces a way to validate HTTP headers prior to reading the request body. Co-authored-by: Albert Zaharovits <[email protected]>
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
Currently we only check the http header objections after the full request has
been read. This commit offers the ability to validate the headers prior to
reading the full message and short circuit as appropriate.