Add a decompression limit #729
Conversation
Motivation: Compression can be an attack vector for maclicious users. Specifically, bad actors may send 'zip-bombs' which have very high compression ratios which may consume a large amount of memory when decompressed. Modifications: - Intoruce a `DecompressionLimit` which has two options, an `absolute` (an absolute size limit for a decompressed message), and `ratio` (based on the compression ratio of the message). - Add decompression limits to server and client compression configuration - Explicitly make compression `enabled` or `disabled` Result: Easier to protect against zip-bombs
There was a problem hiding this comment.
Looks good overall.
However, I am starting to get a bit concerned about the amount of changes our recent PRs required throughout the entire codebase.
Part of this might be technically unrelated refactorings that could just fit well with the PR itself, of course. But in some cases, the amount of changes needed for a particular addition simply feels surprising. That amount of "code churn" also makes it a bit harder to recognize the "essence" of the PR in a few cases. To counter that, though, I have been trying to put that "churn" aside and focus on the parts of the PR where actual stuff happens.
Also, I am starting to feel that I "lose track" of the overall codebase and the ability to fully evaluate changes (which could also explain the recent decline in comments from me). I guess that can't be fully avoided if one mostly reviews the code without actually working inside the codebase, though.
| public init( | ||
| forRequests outbound: CompressionAlgorithm?, | ||
| acceptableForResponses inbound: [CompressionAlgorithm] = CompressionAlgorithm.all | ||
| acceptableForResponses inbound: [CompressionAlgorithm] = CompressionAlgorithm.all, | ||
| decompressionLimit: DecompressionLimit |
There was a problem hiding this comment.
Should be provide a "reasonable" default here?
Sources/GRPC/GRPCError.swift
Outdated
| @@ -118,6 +118,23 @@ public enum GRPCError { | |||
| } | |||
| } | |||
|
|
|||
| /// The decompression limit was exceeded while decompressing a message. | |||
| public struct DecompressionLimitExceeded: GRPCErrorProtocol { | |||
| var compressedSize: Int | |||
There was a problem hiding this comment.
public let? I guess we could allow this to be part of the public API.
| case supported(CompressionAlgorithm) | ||
| enum ValidationResult { | ||
| /// The requested compression is supported. | ||
| case supported(CompressionAlgorithm, DecompressionLimit, acceptEncoding: [String]) |
There was a problem hiding this comment.
how about naming all these associated values? :-) (Up to you, given that the types are already very telling.)
| @@ -172,4 +174,77 @@ class MessageCompressionTests: GRPCTestCase { | |||
|
|
|||
| self.wait(for: [response, trailers, status], timeout: self.defaultTimeout) | |||
| } | |||
|
|
|||
| func testDecompressionLimitIsRespectedByServerForUnaryCall() throws { | |||
| try self.setupServer(encoding: .enabled(.init(decompressionLimit: .absolute(1)))) | |||
There was a problem hiding this comment.
Should we also test a ratio limit?
There was a problem hiding this comment.
Okay, I guess that is covered by ZlibTests...
There's definitely been some of that. I'll try to avoid that in the future to make the PRs smaller.
I noticed this too; but I think the changes in functionality have been fairly substantial (compression, support for non-protobuf payloads and support for different channels). We didn't plan on doing these at all (apart from compression) so I'm not too worried about how large they were.
Totally fair; we need to find another core dev :) |
Motivation:
Compression can be an attack vector for maclicious users. Specifically,
bad actors may send 'zip-bombs' which have very high compression
ratios which may consume a large amount of memory when decompressed.
Modifications:
DecompressionLimitwhich has two options, anabsolute(an absolute size limit for a decompressed message), and
ratio(based on the compression ratio of the message).
configuration
enabledordisabledResult:
Easier to protect against zip-bombs