-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
http3: respecting header number limits #15970
Changes from all commits
8690f06
5ef16a3
c84e9b1
05ada94
ae6d313
687f2f8
c7d8001
1baddbc
5dff671
73104c8
0a1895e
4dc82ff
485a234
e0aeb76
6525ecf
2d29b48
96ddf39
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,27 @@ | |
namespace Envoy { | ||
namespace Quic { | ||
|
||
// Changes or additions to details should be reflected in | ||
// docs/root/configuration/http/http_conn_man/response_code_details.rst | ||
class Http3ResponseCodeDetailValues { | ||
public: | ||
// Invalid HTTP header field was received and stream is going to be | ||
// closed. | ||
static constexpr absl::string_view invalid_http_header = "http3.invalid_header_field"; | ||
// The size of headers (or trailers) exceeded the configured limits. | ||
static constexpr absl::string_view headers_too_large = "http3.headers_too_large"; | ||
// Envoy was configured to drop requests with header keys beginning with underscores. | ||
static constexpr absl::string_view invalid_underscore = "http3.unexpected_underscore"; | ||
// The peer refused the stream. | ||
static constexpr absl::string_view remote_refused = "http3.remote_refuse"; | ||
// The peer reset the stream. | ||
static constexpr absl::string_view remote_reset = "http3.remote_reset"; | ||
// Too many trailers were sent. | ||
static constexpr absl::string_view too_many_trailers = "http3.too_many_trailers"; | ||
// Too many headers were sent. | ||
static constexpr absl::string_view too_many_headers = "http3.too_many_headers"; | ||
}; | ||
|
||
// TODO(danzh): this is called on each write. Consider to return an address instance on the stack if | ||
// the heap allocation is too expensive. | ||
Network::Address::InstanceConstSharedPtr | ||
|
@@ -48,14 +69,21 @@ class HeaderValidator { | |
|
||
// The returned header map has all keys in lower case. | ||
template <class T> | ||
std::unique_ptr<T> quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_list, | ||
HeaderValidator& validator) { | ||
std::unique_ptr<T> | ||
quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_list, HeaderValidator& validator, | ||
uint32_t max_headers_allowed, absl::string_view& details) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it because QuicHeaderList doesn't expose size() interface that we need max_headers_allowed to be passed into this function? I slightly prefer checking it in QUICHE and have something like OnHeadersTooLarge() or simply add QuicHeaderList::size() instead of iterating over the header list again. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 100% prefer checking in QUICHE and rejecting early, vs rejecting late in the game, but I think this is still better than nothing because it protects Envoy filters against too many headers. What do you think about landing this as-is and moving where we enforce once someone on quic-dev adds the knob we'd need? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SGTM. |
||
auto headers = T::create(); | ||
for (const auto& entry : header_list) { | ||
if (max_headers_allowed == 0) { | ||
details = Http3ResponseCodeDetailValues::too_many_headers; | ||
return nullptr; | ||
} | ||
max_headers_allowed--; | ||
Http::HeaderUtility::HeaderValidationResult result = | ||
validator.validateHeader(entry.first, entry.second); | ||
switch (result) { | ||
case Http::HeaderUtility::HeaderValidationResult::REJECT: | ||
// The validator sets the details to Http3ResponseCodeDetailValues::invalid_underscore | ||
return nullptr; | ||
case Http::HeaderUtility::HeaderValidationResult::DROP: | ||
continue; | ||
|
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.
Same for client stream?