Ensure http channel validator does not alter ES thread context#95498
Conversation
|
Pinging @elastic/es-distributed (Team:Distributed) |
| } | ||
|
|
||
| private void forwardFullRequest(ChannelHandlerContext ctx) { | ||
| Transports.assertDefaultThreadContext(threadContext); |
There was a problem hiding this comment.
++ on the asserts here (and anywhere for the thread context - it is very helpful). however, could be outside the context of this PR but it would be great if only had 1 definition of what default thread context means.
org.elasticsearch.transport.Transports#assertDefaultThreadContext defines it as
assert threadContext.getRequestHeadersOnly().isEmpty()
|| REQUEST_HEADERS_ALLOWED_ON_DEFAULT_THREAD_CONTEXT.containsAll(threadContext.getRequestHeadersOnly().keySet())
: "expected empty context but was " + threadContext.getRequestHeadersOnly() + " on " + Thread.currentThread().getName();and org.elasticsearch.common.util.concurrent.ThreadContext#isDefaultContext defines it as
return threadLocal.get() == DEFAULT_CONTEXT;I understand they are slightly different, but ideally they would not be and we would only have a single definition. Not needed to address on this PR , just wanted to get your thoughts if it is possible to collapse these 2 into 1.
There was a problem hiding this comment.
ideally they would not be and we would only have a single definition.
++ Looked into this briefly too. I would've expected threadLocal.get() == DEFAULT_CONTEXT to be used everywhere inplace of threadContext.getRequestHeadersOnly().isEmpty() ..... I tried it and it failed intermitently on one of the license IT, where the thread context contained the license expiration warning response header. I think that response headers might be the reason for the different ways to check the default context....
I haven't investigated further... it might be a bug with response headers leaking....
I'll make a note to follow-up on this when I'll feel out the woods...
|
Thank you for the quick review, Jake! |
|
@elasticsearchmachine update branch |
…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.
This PR 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.