Skip to content

Commit 0e95bb4

Browse files
albertzaharovitsalbertzaharovits
authored
Reject OPTIONS requests with a body (#96357)
Instead of not authN and letting them through, this PR rejects OPTIONS requests with a body (400). Relates #95112
1 parent 7eaa868 commit 0e95bb4

File tree

2 files changed

+241
-21
lines changed

2 files changed

+241
-21
lines changed

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

Lines changed: 74 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,14 @@
77
package org.elasticsearch.xpack.security;
88

99
import io.netty.channel.Channel;
10+
import io.netty.handler.codec.http.HttpMethod;
11+
import io.netty.handler.codec.http.HttpUtil;
1012

1113
import org.apache.logging.log4j.LogManager;
1214
import org.apache.logging.log4j.Logger;
1315
import org.apache.lucene.util.SetOnce;
1416
import org.elasticsearch.ElasticsearchSecurityException;
17+
import org.elasticsearch.ElasticsearchStatusException;
1518
import org.elasticsearch.TransportVersion;
1619
import org.elasticsearch.action.ActionListener;
1720
import org.elasticsearch.action.ActionRequest;
@@ -82,6 +85,7 @@
8285
import org.elasticsearch.rest.RestHandler;
8386
import org.elasticsearch.rest.RestHeaderDefinition;
8487
import org.elasticsearch.rest.RestRequest;
88+
import org.elasticsearch.rest.RestStatus;
8589
import org.elasticsearch.script.ScriptService;
8690
import org.elasticsearch.search.internal.ShardSearchRequest;
8791
import org.elasticsearch.threadpool.ExecutorBuilder;
@@ -1656,26 +1660,6 @@ public boolean test(String profile, InetSocketAddress peerAddress) {
16561660
}
16571661
final AuthenticationService authenticationService = this.authcService.get();
16581662
final ThreadContext threadContext = this.threadContext.get();
1659-
final HttpValidator httpValidator = (httpRequest, channel, listener) -> {
1660-
HttpPreRequest httpPreRequest = HttpHeadersAuthenticatorUtils.asHttpPreRequest(httpRequest);
1661-
// step 1: Populate the thread context with credentials and any other HTTP request header values (eg run-as) that the
1662-
// authentication process looks for while doing its duty.
1663-
perRequestThreadContext.accept(httpPreRequest, threadContext);
1664-
populateClientCertificate.accept(channel, threadContext);
1665-
RemoteHostHeader.process(channel, threadContext);
1666-
// step 2: Run authentication on the now properly prepared thread-context.
1667-
// This inspects and modifies the thread context.
1668-
if (httpPreRequest.method() != RestRequest.Method.OPTIONS) {
1669-
authenticationService.authenticate(
1670-
httpPreRequest,
1671-
ActionListener.wrap(ignored -> listener.onResponse(null), listener::onFailure)
1672-
);
1673-
} else {
1674-
// allow for unauthenticated OPTIONS request
1675-
// this includes CORS preflight, and regular OPTIONS that return permitted methods for a given path
1676-
listener.onResponse(null);
1677-
}
1678-
};
16791663
return getHttpServerTransportWithHeadersValidator(
16801664
settings,
16811665
networkService,
@@ -1687,12 +1671,81 @@ public boolean test(String profile, InetSocketAddress peerAddress) {
16871671
tracer,
16881672
new TLSConfig(sslConfiguration, sslService::createSSLEngine),
16891673
acceptPredicate,
1690-
httpValidator
1674+
(httpRequest, channel, listener) -> {
1675+
HttpPreRequest httpPreRequest = HttpHeadersAuthenticatorUtils.asHttpPreRequest(httpRequest);
1676+
// step 1: Populate the thread context with credentials and any other HTTP request header values (eg run-as) that the
1677+
// authentication process looks for while doing its duty.
1678+
perRequestThreadContext.accept(httpPreRequest, threadContext);
1679+
populateClientCertificate.accept(channel, threadContext);
1680+
RemoteHostHeader.process(channel, threadContext);
1681+
// step 2: Run authentication on the now properly prepared thread-context.
1682+
// This inspects and modifies the thread context.
1683+
authenticationService.authenticate(
1684+
httpPreRequest,
1685+
ActionListener.wrap(ignored -> listener.onResponse(null), listener::onFailure)
1686+
);
1687+
},
1688+
(httpRequest, channel, listener) -> {
1689+
// allow unauthenticated OPTIONS request through
1690+
// this includes CORS preflight, and regular OPTIONS that return permitted methods for a given path
1691+
// But still populate the thread context with the usual request headers (as for any other request that is dispatched)
1692+
HttpPreRequest httpPreRequest = HttpHeadersAuthenticatorUtils.asHttpPreRequest(httpRequest);
1693+
perRequestThreadContext.accept(httpPreRequest, threadContext);
1694+
populateClientCertificate.accept(channel, threadContext);
1695+
RemoteHostHeader.process(channel, threadContext);
1696+
listener.onResponse(null);
1697+
}
16911698
);
16921699
});
16931700
return httpTransports;
16941701
}
16951702

1703+
// "public" so it can be used in tests
1704+
public static Netty4HttpServerTransport getHttpServerTransportWithHeadersValidator(
1705+
Settings settings,
1706+
NetworkService networkService,
1707+
ThreadPool threadPool,
1708+
NamedXContentRegistry xContentRegistry,
1709+
HttpServerTransport.Dispatcher dispatcher,
1710+
ClusterSettings clusterSettings,
1711+
SharedGroupFactory sharedGroupFactory,
1712+
Tracer tracer,
1713+
TLSConfig tlsConfig,
1714+
@Nullable AcceptChannelHandler.AcceptPredicate acceptPredicate,
1715+
HttpValidator httpValidator,
1716+
HttpValidator httpOptionsValidator
1717+
) {
1718+
return getHttpServerTransportWithHeadersValidator(
1719+
settings,
1720+
networkService,
1721+
threadPool,
1722+
xContentRegistry,
1723+
dispatcher,
1724+
clusterSettings,
1725+
sharedGroupFactory,
1726+
tracer,
1727+
tlsConfig,
1728+
acceptPredicate,
1729+
(httpRequest, channel, listener) -> {
1730+
if (httpRequest.method() == HttpMethod.OPTIONS) {
1731+
if (HttpUtil.getContentLength(httpRequest, -1L) > 1 || HttpUtil.isTransferEncodingChunked(httpRequest)) {
1732+
// OPTIONS requests with a body are not supported
1733+
listener.onFailure(
1734+
new ElasticsearchStatusException(
1735+
"OPTIONS requests with a payload body are not supported",
1736+
RestStatus.BAD_REQUEST
1737+
)
1738+
);
1739+
} else {
1740+
httpOptionsValidator.validate(httpRequest, channel, listener);
1741+
}
1742+
} else {
1743+
httpValidator.validate(httpRequest, channel, listener);
1744+
}
1745+
}
1746+
);
1747+
}
1748+
16961749
// "public" so it can be used in tests
16971750
public static Netty4HttpServerTransport getHttpServerTransportWithHeadersValidator(
16981751
Settings settings,

‎x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/netty4/SecurityNetty4HttpServerTransportTests.java‎

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import org.elasticsearch.rest.RestChannel;
4444
import org.elasticsearch.rest.RestRequest;
4545
import org.elasticsearch.rest.RestResponse;
46+
import org.elasticsearch.rest.RestStatus;
4647
import org.elasticsearch.test.rest.FakeRestRequest;
4748
import org.elasticsearch.threadpool.TestThreadPool;
4849
import org.elasticsearch.threadpool.ThreadPool;
@@ -681,4 +682,170 @@ public void dispatchBadRequest(final RestChannel channel, final ThreadContext th
681682
testThreadPool.shutdownNow();
682683
}
683684
}
685+
686+
public void testOptionsRequestsFailWith400AndNoAuthn() throws Exception {
687+
final Settings settings = Settings.builder().put(env.settings()).build();
688+
AtomicReference<Throwable> badRequestCauseReference = new AtomicReference<>();
689+
final HttpServerTransport.Dispatcher dispatcher = new HttpServerTransport.Dispatcher() {
690+
@Override
691+
public void dispatchRequest(final RestRequest request, final RestChannel channel, final ThreadContext threadContext) {
692+
logger.error("--> Unexpected dispatched request [" + FakeRestRequest.requestToString(channel.request()) + "]");
693+
throw new AssertionError("Unexpected dispatched request");
694+
}
695+
696+
@Override
697+
public void dispatchBadRequest(final RestChannel channel, final ThreadContext threadContext, final Throwable cause) {
698+
badRequestCauseReference.set(cause);
699+
}
700+
};
701+
final ThreadPool testThreadPool = new TestThreadPool(TEST_MOCK_TRANSPORT_THREAD_PREFIX);
702+
try (
703+
Netty4HttpServerTransport transport = Security.getHttpServerTransportWithHeadersValidator(
704+
settings,
705+
new NetworkService(List.of()),
706+
testThreadPool,
707+
xContentRegistry(),
708+
dispatcher,
709+
randomClusterSettings(),
710+
new SharedGroupFactory(settings),
711+
Tracer.NOOP,
712+
TLSConfig.noTLS(),
713+
null,
714+
(httpPreRequest, channel, listener) -> {
715+
throw new AssertionError("should not be invoked for OPTIONS requests");
716+
},
717+
(httpPreRequest, channel, listener) -> {
718+
throw new AssertionError("should not be invoked for OPTIONS requests with a body");
719+
}
720+
)
721+
) {
722+
final ChannelHandler handler = transport.configureServerChannelHandler();
723+
final EmbeddedChannel ch = new EmbeddedChannel(handler);
724+
// OPTIONS request with fixed length content written in one chunk
725+
{
726+
ByteBuf buf = ch.alloc().buffer();
727+
ByteBufUtil.copy(AsciiString.of("OPTIONS /url/whatever/fixed-length-single-chunk HTTP/1.1"), buf);
728+
buf.writeByte(HttpConstants.LF);
729+
if (randomBoolean()) {
730+
ByteBufUtil.copy(AsciiString.of("Host: localhost"), buf);
731+
buf.writeByte(HttpConstants.LF);
732+
}
733+
if (randomBoolean()) {
734+
ByteBufUtil.copy(AsciiString.of("Accept: */*"), buf);
735+
buf.writeByte(HttpConstants.LF);
736+
}
737+
if (randomBoolean()) {
738+
ByteBufUtil.copy(AsciiString.of("Content-Encoding: gzip"), buf);
739+
buf.writeByte(HttpConstants.LF);
740+
}
741+
if (randomBoolean()) {
742+
ByteBufUtil.copy(
743+
AsciiString.of("Content-Type: " + randomFrom("text/plain; charset=utf-8", "application/json; charset=utf-8")),
744+
buf
745+
);
746+
buf.writeByte(HttpConstants.LF);
747+
}
748+
String content = randomAlphaOfLengthBetween(4, 1024);
749+
// having a "Content-Length" request header is what makes it "fixed length"
750+
ByteBufUtil.copy(AsciiString.of("Content-Length: " + content.length()), buf);
751+
buf.writeByte(HttpConstants.LF);
752+
// end of headers
753+
buf.writeByte(HttpConstants.LF);
754+
ByteBufUtil.copy(AsciiString.of(content), buf);
755+
// write everything in one single chunk
756+
testThreadPool.generic().submit(() -> {
757+
ch.writeInbound(buf);
758+
ch.flushInbound();
759+
}).get();
760+
ch.runPendingTasks();
761+
Throwable badRequestCause = badRequestCauseReference.get();
762+
assertThat(badRequestCause, instanceOf(HttpHeadersValidationException.class));
763+
assertThat(badRequestCause.getCause(), instanceOf(ElasticsearchException.class));
764+
assertThat(((ElasticsearchException) badRequestCause.getCause()).status(), is(RestStatus.BAD_REQUEST));
765+
assertThat(
766+
((ElasticsearchException) badRequestCause.getCause()).getDetailedMessage(),
767+
containsString("OPTIONS requests with a payload body are not supported")
768+
);
769+
}
770+
{
771+
ByteBuf buf = ch.alloc().buffer();
772+
ByteBufUtil.copy(AsciiString.of("OPTIONS /url/whatever/chunked-transfer?encoding HTTP/1.1"), buf);
773+
buf.writeByte(HttpConstants.LF);
774+
if (randomBoolean()) {
775+
ByteBufUtil.copy(AsciiString.of("Host: localhost"), buf);
776+
buf.writeByte(HttpConstants.LF);
777+
}
778+
if (randomBoolean()) {
779+
ByteBufUtil.copy(AsciiString.of("Accept: */*"), buf);
780+
buf.writeByte(HttpConstants.LF);
781+
}
782+
if (randomBoolean()) {
783+
ByteBufUtil.copy(AsciiString.of("Content-Encoding: gzip"), buf);
784+
buf.writeByte(HttpConstants.LF);
785+
}
786+
if (randomBoolean()) {
787+
ByteBufUtil.copy(
788+
AsciiString.of("Content-Type: " + randomFrom("text/plain; charset=utf-8", "application/json; charset=utf-8")),
789+
buf
790+
);
791+
buf.writeByte(HttpConstants.LF);
792+
}
793+
// do not write a "Content-Length" header to make the request "variable length"
794+
if (randomBoolean()) {
795+
ByteBufUtil.copy(AsciiString.of("Transfer-Encoding: " + randomFrom("chunked", "gzip, chunked")), buf);
796+
} else {
797+
ByteBufUtil.copy(AsciiString.of("Transfer-Encoding: chunked"), buf);
798+
}
799+
buf.writeByte(HttpConstants.LF);
800+
buf.writeByte(HttpConstants.LF);
801+
// maybe append some chunks as well
802+
String[] contentParts = randomArray(0, 4, String[]::new, () -> randomAlphaOfLengthBetween(1, 64));
803+
for (String content : contentParts) {
804+
ByteBufUtil.copy(AsciiString.of(Integer.toHexString(content.length())), buf);
805+
buf.writeByte(HttpConstants.CR);
806+
buf.writeByte(HttpConstants.LF);
807+
ByteBufUtil.copy(AsciiString.of(content), buf);
808+
buf.writeByte(HttpConstants.CR);
809+
buf.writeByte(HttpConstants.LF);
810+
}
811+
testThreadPool.generic().submit(() -> {
812+
ch.writeInbound(buf);
813+
ch.flushInbound();
814+
}).get();
815+
// append some more chunks as well
816+
ByteBuf buf2 = ch.alloc().buffer();
817+
contentParts = randomArray(1, 4, String[]::new, () -> randomAlphaOfLengthBetween(1, 64));
818+
for (String content : contentParts) {
819+
ByteBufUtil.copy(AsciiString.of(Integer.toHexString(content.length())), buf2);
820+
buf2.writeByte(HttpConstants.CR);
821+
buf2.writeByte(HttpConstants.LF);
822+
ByteBufUtil.copy(AsciiString.of(content), buf2);
823+
buf2.writeByte(HttpConstants.CR);
824+
buf2.writeByte(HttpConstants.LF);
825+
}
826+
// finish chunked request
827+
ByteBufUtil.copy(AsciiString.of("0"), buf2);
828+
buf2.writeByte(HttpConstants.CR);
829+
buf2.writeByte(HttpConstants.LF);
830+
buf2.writeByte(HttpConstants.CR);
831+
buf2.writeByte(HttpConstants.LF);
832+
testThreadPool.generic().submit(() -> {
833+
ch.writeInbound(buf2);
834+
ch.flushInbound();
835+
}).get();
836+
ch.runPendingTasks();
837+
Throwable badRequestCause = badRequestCauseReference.get();
838+
assertThat(badRequestCause, instanceOf(HttpHeadersValidationException.class));
839+
assertThat(badRequestCause.getCause(), instanceOf(ElasticsearchException.class));
840+
assertThat(((ElasticsearchException) badRequestCause.getCause()).status(), is(RestStatus.BAD_REQUEST));
841+
assertThat(
842+
((ElasticsearchException) badRequestCause.getCause()).getDetailedMessage(),
843+
containsString("OPTIONS requests with a payload body are not supported")
844+
);
845+
}
846+
} finally {
847+
testThreadPool.shutdownNow();
848+
}
849+
}
850+
684851
}

0 commit comments

Comments
 (0)