Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(http): fix HTTP protocol errors following non-successful requests #4125

Merged
merged 23 commits into from Jan 19, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
97c98bb
reject HTTP requests after full body is read
ideoma Jan 12, 2024
e3ef98e
fix tests, send 404 when HTTP method does not match the processor
ideoma Jan 12, 2024
36deaf5
better http reject messages
ideoma Jan 12, 2024
db0d1a1
clear header parser content
ideoma Jan 12, 2024
3afcebd
move some ILP Http test to run with fuzz tests by renaming
ideoma Jan 12, 2024
6224418
fix rejection with slow senders
ideoma Jan 12, 2024
fbda574
Merge branch 'master' into fix-http-reject-protocol-violation
ideoma Jan 12, 2024
319d946
fix java8 compilation
ideoma Jan 12, 2024
d783f59
make test more stable
ideoma Jan 15, 2024
f677c16
method refactoring to make sure we do not mix "content-type" and inap…
bluestreak01 Jan 15, 2024
37dff26
Merge branch 'master' into fix-http-reject-protocol-violation
bluestreak01 Jan 15, 2024
6bfec72
fix client initiated disconnects on windows resulting to null pointer…
ideoma Jan 15, 2024
bd63658
fix race from disconnect and retry to clear resources
ideoma Jan 16, 2024
766ca66
Revert "fix race from disconnect and retry to clear resources"
ideoma Jan 16, 2024
86fc689
Revert "fix client initiated disconnects on windows resulting to null…
ideoma Jan 16, 2024
b74c4ba
fix double http context read scheduling on rejects
ideoma Jan 17, 2024
eccdd76
refactor to hide dispatcher from IO Context implementation to avoid d…
ideoma Jan 17, 2024
3156c12
Merge remote-tracking branch 'origin/master' into fix-http-reject-pro…
ideoma Jan 17, 2024
6d9b3cf
fix test
ideoma Jan 17, 2024
5592155
restore error check in handleClientRecv on header parse
ideoma Jan 17, 2024
0fe3e9f
Merge remote-tracking branch 'origin/master' into fix-http-reject-pro…
bluestreak01 Jan 18, 2024
1f0f190
inlined single-use functions
bluestreak01 Jan 18, 2024
f70c020
Merge branch 'master' into fix-http-reject-protocol-violation
bluestreak01 Jan 19, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -487,7 +487,7 @@ private void reloadFromTablesFile(
tableNameToTableTokenMap.put(tableName, token);
if (!Chars.startsWith(token.getDirName(), token.getTableName())) {
// This table is renamed, log system to real table name mapping
LOG.info().$("table dir name does not match logical name [table=").utf8(tableName).$(", dirName=").utf8(dirName).I$();
LOG.debug().$("table dir name does not match logical name [table=").utf8(tableName).$(", dirName=").utf8(dirName).I$();
}
dirNameToTableTokenMap.put(token.getDirName(), ReverseTableMapItem.of(token));
}
Expand Down
138 changes: 97 additions & 41 deletions core/src/main/java/io/questdb/cutlass/http/HttpConnectionContext.java
Expand Up @@ -65,6 +65,7 @@ public class HttpConnectionContext extends IOContext<HttpConnectionContext> impl
private final MultipartParserState multipartParserState = new MultipartParserState();
private final NetworkFacade nf;
private final int recvBufferSize;
private final RejectProcessor rejectProcessor = new RejectProcessor();
private final HttpResponseSink responseSink;
private final RetryAttemptAttributes retryAttemptAttributes = new RetryAttemptAttributes();
private final RescheduleContext retryRescheduleContext = retry -> {
Expand Down Expand Up @@ -302,12 +303,17 @@ public HttpConnectionContext of(int fd, @NotNull IODispatcher<HttpConnectionCont
return this;
}

public boolean rejectRequest(int code, CharSequence userMessage, CharSequence cookieName, CharSequence cookieValue) throws PeerDisconnectedException, PeerIsSlowToReadException {
reset();
public HttpRequestProcessor rejectRequest(int code, CharSequence userMessage) {
return rejectRequest(code, userMessage, null, null);
}

public HttpRequestProcessor rejectRequest(int code, CharSequence userMessage, CharSequence cookieName, CharSequence cookieValue) {
LOG.error().$(userMessage).$(" [code=").$(code).I$();
simpleResponse().sendStatusWithCookie(code, userMessage, cookieName, cookieValue);
dispatcher.registerChannel(this, IOOperation.READ);
return false;
rejectProcessor.rejectCode = code;
rejectProcessor.rejectMessage = userMessage;
rejectProcessor.rejectCookieName = cookieName;
bluestreak01 marked this conversation as resolved.
Show resolved Hide resolved
rejectProcessor.rejectCookieValue = cookieValue;
return rejectProcessor;
}

public void reset() {
Expand All @@ -331,6 +337,7 @@ public void reset() {
this.totalReceived = 0;
this.chunkedContentParser.clear();
this.recvPos = recvBuffer;
this.rejectProcessor.clear();
clearSuspendEvent();
}

Expand Down Expand Up @@ -579,7 +586,7 @@ private boolean consumeMultipart(
) throws PeerDisconnectedException, PeerIsSlowToReadException, ServerDisconnectException, QueryPausedException {
if (newRequest) {
if (!headerParser.hasBoundary()) {
return rejectRequest("Bad request. Form data in multipart POST expected.");
return failRequest("Bad request. Form data in multipart POST expected.");
}
processor.onHeadersReady(this);
multipartContentParser.of(headerParser.getBoundary());
Expand Down Expand Up @@ -757,6 +764,11 @@ private void failProcessor(HttpRequestProcessor processor, HttpException e, int
}
}

private boolean failRequest(String errorMessage) throws ServerDisconnectException {
LOG.error().$(errorMessage).$(". Disconnecting [fd=").$(getFd()).I$();
throw new ServerDisconnectException();
}

private HttpRequestProcessor getHttpRequestProcessor(HttpRequestProcessorSelector selector) {
HttpRequestProcessor processor;
final Utf8Sequence url = headerParser.getUrl();
Expand Down Expand Up @@ -805,7 +817,7 @@ private boolean handleClientRecv(HttpRequestProcessorSelector selector, Reschedu
if (url == null) {
throw HttpException.instance("missing URL");
}
HttpRequestProcessor processor = getHttpRequestProcessor(selector);
HttpRequestProcessor processor = isRequestBeingRejected() ? rejectProcessor : getHttpRequestProcessor(selector);
int contentLength = headerParser.getContentLength();
final boolean chunked = Utf8s.equalsNcAscii("chunked", headerParser.getHeader(HEADER_TRANSFER_ENCODING));
final boolean multipartRequest = Utf8s.equalsNcAscii("multipart/form-data", headerParser.getContentType())
Expand All @@ -817,40 +829,48 @@ private boolean handleClientRecv(HttpRequestProcessorSelector selector, Reschedu
}

try {
if (newRequest && processor.requiresAuthentication() && !configureSecurityContext()) {
return rejectUnauthenticatedRequest();
}
if (newRequest) {
if (processor.requiresAuthentication() && !configureSecurityContext()) {
processor = rejectUnauthorized(null);
}

if (newRequest && configuration.areCookiesEnabled()) {
if (!processor.processCookies(this, securityContext)) {
return false;
if (configuration.areCookiesEnabled()) {
if (!processor.processCookies(this, securityContext)) {
processor = rejectProcessor;
}
}
}

try {
securityContext.checkEntityEnabled();
} catch (CairoException e) {
return rejectForbiddenRequest(e.getFlyweightMessage());
try {
securityContext.checkEntityEnabled();
} catch (CairoException e) {
processor = rejectForbiddenRequest(e.getFlyweightMessage());
}
}

if (chunked) {
if (multipartProcessor) {
busyRecv = consumeChunked(processor, headerEnd, read, newRequest);
} else {
if (!multipartProcessor) {
// bad request - regular request for processor that expects multipart
busyRecv = rejectRequest("Bad request. Chunked requests are not supported by the handler.");
processor = rejectRequest(HTTP_NOT_FOUND, "method (chunked POST) not supported");
}
busyRecv = consumeChunked(processor, headerEnd, read, newRequest);
} else if (multipartRequest) {
if (!multipartProcessor) {
// bad request - multipart request for processor that doesn't expect multipart
processor = rejectRequest(HTTP_NOT_FOUND, "method (multipart POST) not supported");
}
} else if (multipartRequest && !multipartProcessor) {
// bad request - multipart request for processor that doesn't expect multipart
busyRecv = rejectRequest("Bad request. Non-multipart GET expected.");
} else if (multipartProcessor && multipartRequest) {
busyRecv = consumeMultipart(socket, processor, headerEnd, read, newRequest, rescheduleContext);
} else if (contentLength > -1 && multipartProcessor) {
} else if (contentLength > -1) {
if (!multipartProcessor) {
processor = rejectRequest(HTTP_NOT_FOUND, "method (POST) not supported");
}
busyRecv = consumeContent(contentLength, socket, processor, headerEnd, read, newRequest);
} else if (multipartProcessor) {
// bad request - regular request for processor that expects multipart
busyRecv = rejectRequest("Bad request. Multipart POST expected.");
} else {
if (multipartProcessor) {
// bad request - regular request for processor that expects multipart
rejectRequest(HTTP_BAD_REQUEST, "Bad request. Multipart POST expected.");
processor = rejectProcessor;
}

// Do not expect any more bytes to be sent to us before
// we respond back to client. We will disconnect the client when
// they abuse protocol. In addition, we will not call processor
Expand Down Expand Up @@ -914,6 +934,10 @@ private boolean handleClientRecv(HttpRequestProcessorSelector selector, Reschedu
return busyRecv;
}

private boolean isRequestBeingRejected() {
return rejectProcessor.rejectCode != 0;
}

private boolean handleClientSend() {
if (resumeProcessor != null) {
try {
Expand Down Expand Up @@ -968,20 +992,12 @@ private boolean parseMultipartResult(
return false;
}

private boolean rejectForbiddenRequest(CharSequence userMessage) throws PeerDisconnectedException, PeerIsSlowToReadException {
private HttpRequestProcessor rejectForbiddenRequest(CharSequence userMessage) throws PeerDisconnectedException, PeerIsSlowToReadException {
return rejectRequest(HTTP_FORBIDDEN, userMessage, null, null);
}

private boolean rejectRequest(CharSequence userMessage) throws PeerDisconnectedException, PeerIsSlowToReadException {
return rejectRequest(HTTP_NOT_FOUND, userMessage, null, null);
}

private boolean rejectUnauthenticatedRequest() throws PeerDisconnectedException, PeerIsSlowToReadException {
reset();
LOG.error().$("rejecting unauthenticated request [fd=").$(getFd()).I$();
simpleResponse().sendStatusWithHeader(HTTP_UNAUTHORIZED, "WWW-Authenticate: Basic realm=\"questdb\", charset=\"UTF-8\"");
dispatcher.registerChannel(this, IOOperation.READ);
return false;
private HttpRequestProcessor rejectUnauthorized(CharSequence userMessage) throws PeerDisconnectedException, PeerIsSlowToReadException {
return rejectRequest(HTTP_UNAUTHORIZED, userMessage, null, null);
}

private void shiftReceiveBufferUnprocessedBytes(long start, int receivedBytes) {
Expand All @@ -990,4 +1006,44 @@ private void shiftReceiveBufferUnprocessedBytes(long start, int receivedBytes) {
Vect.memcpy(recvBuffer, start, receivedBytes);
LOG.debug().$("peer is slow, waiting for bigger part to parse [multipart]").$();
}

private class RejectProcessor implements HttpRequestProcessor, HttpMultipartContentListener {
private int rejectCode;
private CharSequence rejectCookieName = null;
private CharSequence rejectCookieValue = null;
private CharSequence rejectMessage = null;

public void clear() {
rejectCode = 0;
rejectCookieName = null;
rejectCookieValue = null;
rejectMessage = null;
}

@Override
public void onChunk(long lo, long hi) {
}

@Override
public void onPartBegin(HttpRequestHeader partHeader) {
}

@Override
public void onPartEnd() {
}

@Override
public void onRequestComplete(
HttpConnectionContext context
) throws PeerDisconnectedException, PeerIsSlowToReadException {
if (rejectCode == HTTP_UNAUTHORIZED) {
// Special case, include WWW-Authenticate header
context.simpleResponse().sendStatusWithHeader(HTTP_UNAUTHORIZED, "WWW-Authenticate: Basic realm=\"questdb\", charset=\"UTF-8\"");
} else {
simpleResponse().sendStatusWithCookie(rejectCode, rejectMessage, rejectCookieName, rejectCookieValue);
}
reset();
dispatcher.registerChannel(HttpConnectionContext.this, IOOperation.READ);
}
}
}
Expand Up @@ -104,6 +104,7 @@ public void clear() {
this.isStatusCode = true;
this.isStatusText = true;
this.needProtocol = true;
this.contentLength = -1;
// do not clear the pool
// this.pool.clear();
}
Expand Down
Expand Up @@ -595,8 +595,6 @@ public String status(CharSequence httpProtocolVersion, int code, CharSequence co
} else {
putAscii("Content-Length: ").put(contentLength).putEOL();
}
}
if (contentType != null) {
putAscii("Content-Type: ").put(contentType).putEOL();
}

Expand Down Expand Up @@ -774,7 +772,7 @@ public void sendStatus(int code, CharSequence message, CharSequence header, Char

public void sendStatus(int code) throws PeerDisconnectedException, PeerIsSlowToReadException {
buffer.clearAndPrepareToWriteToBuffer();
headerImpl.status(httpVersion, code, CONTENT_TYPE_HTML, -2L);
headerImpl.status(httpVersion, code, null, -2L);
bluestreak01 marked this conversation as resolved.
Show resolved Hide resolved
prepareHeaderSink();
flushSingle();
}
Expand Down