[WIP] feat: Ability to limit request size and connection count using HTTPServerConfigurationHandler. #222
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| /* | ||
| * Copyright IBM Corporation 2019 | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| import Foundation | ||
|
|
||
| public struct HTTPServerConfiguration { | ||
| /// Defines the maximum size of an incoming request, in bytes. If requests are received that are larger | ||
| /// than this limit, they will be rejected and the connection will be closed. | ||
|
|
||
| /// A value of `nil` means no limit. | ||
| public let requestSizeLimit: Int? | ||
|
|
||
| /// Defines the maximum number of concurrent connections that a server should accept. Clients attempting | ||
| /// to connect when this limit has been reached will be rejected. | ||
| /// A value of `nil` means no limit. | ||
| public let connectionLimit: Int? | ||
|
|
||
| /// A default limit of 1mb on the size of requests that a server should accept | ||
| /// A default limit of 10,000 on the number of concurrent connections that a server should accept. | ||
| public static var `default` = HTTPServerConfiguration(requestSizeLimit: 1048576, connectionLimit: 10000) | ||
|
|
||
|
|
||
| /// Create an `HTTPServerConfiguration` to determine the behaviour of a `Server`. | ||
| /// | ||
| /// - parameter requestSizeLimit: The maximum size of an incoming request. Defaults to `IncomingSocketOptions.defaultRequestSizeLimit`. | ||
| /// - parameter connectionLimit: The maximum number of concurrent connections. Defaults to `IncomingSocketOptions.defaultConnectionLimit`. | ||
|
|
||
| public init(requestSizeLimit: Int?, connectionLimit: Int?) | ||
| { | ||
| self.requestSizeLimit = requestSizeLimit | ||
| self.connectionLimit = connectionLimit | ||
| } | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| import NIO | ||
| import NIOHTTP1 | ||
| import NIOWebSocket | ||
| import LoggerAPI | ||
| import Foundation | ||
| import Dispatch | ||
| import NIOConcurrencyHelpers | ||
|
|
||
| internal class HTTPServerConfigurationHandler: ChannelDuplexHandler { | ||
| // The HTTPServer instance on which this handler is installed | ||
| var server: HTTPServer | ||
| let requestSizeLimit: Int? | ||
| let connectionLimit: Int? | ||
| var requestSize: Int = 0 | ||
| var connectionCount = 0 | ||
| typealias InboundIn = ByteBuffer | ||
| typealias OutboundIn = ByteBuffer | ||
| typealias InboundOut = ByteBuffer | ||
| typealias OutboundOut = ByteBuffer | ||
|
|
||
| public init(for server: HTTPServer) { | ||
| self.server = server | ||
| // if let requestSizeLimit = server.serverConfig.requestSizeLimit { | ||
| // self.requestSizeLimit = requestSizeLimit | ||
| // } | ||
| // if let connectionLimit = server.serverConfig.connectionLimit { | ||
| // self.connectionLimit = connectionLimit | ||
| // } | ||
| self.requestSizeLimit = server.serverConfig.requestSizeLimit ?? nil | ||
| self.connectionLimit = server.serverConfig.connectionLimit ?? nil | ||
| } | ||
| public func channelRead(context: ChannelHandlerContext, data: NIOAny) { | ||
| let data = self.unwrapInboundIn(data) | ||
| requestSize = requestSize + data.readableBytes | ||
| if let requestSizeLimit1 = requestSizeLimit { | ||
| if requestSize > requestSizeLimit1 { | ||
| let statusDescription = HTTP.statusCodes[HTTPStatusCode.requestTooLong.rawValue] ?? "" | ||
| var discriptionBuffer = ByteBufferAllocator().buffer(capacity: requestSizeLimit!) | ||
| discriptionBuffer.writeString(statusDescription) | ||
| context.writeAndFlush(NIOAny(discriptionBuffer),promise: nil) | ||
| requestSize = 0 | ||
| context.close(mode: .all, promise: nil) | ||
| return | ||
| } | ||
| } | ||
| context.fireChannelRead(wrapInboundOut(data)) | ||
| } | ||
|
|
||
| public func triggerUserOutboundEvent(context: ChannelHandlerContext, event: Any, promise: EventLoopPromise<Void>?) { | ||
| requestSize = event as? Int ?? 0 | ||
|
There was a problem hiding this comment. @RudraniW there are a number of issues here:
But really you're making your life unnecessarily hard. You should be doing the request body size limiting after the HTTP parser. Then the whole limiting is pretty straightforward. As an example, see the handler below: final class HTTPRequestBodySizeLimiter: ChannelInboundHandler {
typealias InboundIn = HTTPServerRequestPart
typealias InboundOut = HTTPServerRequestPart
private var bytesReceivedSoFar: Int
private let maximumAllowedBytes: Int
init() {
self.bytesReceivedSoFar = 0
self.maximumAllowedBytes = 1024 * 1024
}
func channelRead(context: ChannelHandlerContext, data: NIOAny) {
switch self.unwrapInboundIn(data) {
case .head:
// at this point in time we must have received 0 bytes
assert(self.bytesReceivedSoFar == 0)
case .body(let newBytes):
guard self.bytesReceivedSoFar + newBytes.readableBytes <= self.maximumAllowedBytes else {
// for this example, let's just close.
context.close(promise: nil)
return
}
self.bytesReceivedSoFar += newBytes.readableBytes
case .end:
// let's reset the bytes received so far
self.bytesReceivedSoFar = 0
}
// let's forward whatever we have received
context.fireChannelRead(data)
}
}The way you would integrate that is Does this make sense? There was a problem hiding this comment. @weissi The objective here was to prevent oversized requests from exhausting the memory on the server. The reason I suggested the approach of limiting before HTTP header parsing is that a misbehaving client could (deliberately or otherwise) send arbitrary amounts of data as HTTP headers. I believe that many HTTP servers have a separate limit on the size of the HTTP headers (Apache for example), but Kitura does not (at least, not via Kitura-net). Does Swift-NIO provide a way to limit the size of the headers it accepts? If so, then imposing separate limits on headers and body would be a superior approach. There was a problem hiding this comment. @djones6 SwiftNIO does have a non-configurable 80kiB limit on the total size of all headers. If you can find anything where SwiftNIO buffers indefinite amounts of anything in memory, please contact us and we will treat it like a security issue. There's no need to limit anything in front of NIO's HTTP parser or any other parsers. To make it easy for your own parsers not to grow infinitely in memory, NIO's There was a problem hiding this comment. @RudraniW I think the solution here should probably be two limits: the request header size and the body size (we'd need to implement this in Kitura-net, but that should be a relatively straightforward change on top of the existing prototype). There was a problem hiding this comment. @djones6 you mean two limits for Kitura-net or Kitura-NIO? Because I think for Kitura-NIO you only need to limit what you buffer (the body). Pretty much every web browser/server has a limit and 80 kiB seems large enough for everybody & small enough that it doesn't matter (we took the same value as NodeJS/nginx because it's in http_parser). It doesn't make sense to limit the header block even further because 80 kiB is smaller than most kernel's receive buffers. So even if you limit it in user-space it's in memory already anyway and there's nothing you can do about it. For Kitura-net I'd suggest also just picking an arbitrary value of something like 80kiB, don't think it needs to be configurable tbh. There was a problem hiding this comment. @djones6 btw, there are other things in HTTP/1 that need limiting: the URL and the status description. In NIO we count that towards the header size. So we treat all of those the same: or if we parse responses there can also be So more precisely, NIO's limit is everything until we see There was a problem hiding this comment. I think it'll make sense then for Kitura-net to match NIO's 80kb default for headers. That's also how I would implement it for Kitura-net, which uses CHTTPParser, so we'd impose a header read limit up to the point where the parser indicates that headers are complete (method, path, headers), after which we'd impose the body limit on anything after that. There was a problem hiding this comment. @djones6 cool, if you use |
||
| } | ||
| public func channelActive(context: ChannelHandlerContext) { | ||
| _ = server.connectionCount.add(1) | ||
|
There was a problem hiding this comment. do you want to restrict "connections" or "currently active requests"? With HTTP/1.1 keep-alive those two are different. You're implementing limiting "connections", is that what you want? There was a problem hiding this comment. Thanks for your feedback @weissi. For now I am implementing limiting "connections". But may be in future I will look into limiting "currently active requests". There was a problem hiding this comment. The decision to limit the number of connections here was largely based on what could be easily implemented in Kitura-net. Limiting active requests is more attractive, but might take some extra work to implement on -net (when waking up a reader thread, we'd need to check if we were allowed to process another request, and have a way to defer the read(s) until later). Though this doesn't necessarily preclude us from implementing it in Kitura-NIO - the configuration would need to be consistent across the two, but we could mark it as unsupported on -net. |
||
| if let connectionLimit1 = connectionLimit { | ||
| if server.connectionCount.load() > connectionLimit1{ | ||
| let statusDescription = HTTP.statusCodes[HTTPStatusCode.serviceUnavailable.rawValue] ?? "" | ||
| var statusBuffer = ByteBufferAllocator().buffer(capacity: 1024) | ||
| statusBuffer.writeString(statusDescription) | ||
| context.writeAndFlush(NIOAny(statusBuffer),promise: nil) | ||
| _ = server.connectionCount.sub(1) | ||
| context.close(mode: .all, promise: nil) | ||
| return | ||
| } | ||
| } | ||
| context.fireChannelActive() | ||
| } | ||
|
|
||
| func channelInactive(context: ChannelHandlerContext) { | ||
| _ = server.connectionCount.sub(1) | ||
| context.fireChannelInactive() | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import NIO | ||
| import NIOHTTP1 | ||
| import NIOWebSocket | ||
| import LoggerAPI | ||
| import Foundation | ||
| import Dispatch | ||
| import XCTest | ||
| @testable import KituraNet | ||
| internal class HTTPConfigTestsResponseHandler: ChannelInboundHandler { | ||
| typealias InboundIn = ByteBuffer | ||
| let expectedSubstring: String | ||
| let expectation: XCTestExpectation | ||
| public init(expectation: XCTestExpectation, expectedSubstring: String ){ | ||
| self.expectedSubstring = expectedSubstring | ||
| self.expectation = expectation | ||
| } | ||
|
|
||
| public func channelRead(context: ChannelHandlerContext, data: NIOAny) { | ||
| let response = self.unwrapInboundIn(data) | ||
| XCTAssert(response.getString(at: 0, length: response.readableBytes)?.starts(with: expectedSubstring) ?? false) | ||
| expectation.fulfill() | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can just drop the
return channel.eventLoop.makeSucceededFuture(()) } .flatMap {