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

Envoy ext_proc filter throw exception when received response timeout Duration is too large #27260

Merged
merged 7 commits into from
May 12, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,10 @@ message ExternalProcessor {
// to the processing mode) if the timer expires before a matching response
// is received. There is no timeout when the filter is running in asynchronous
// mode. Default is 200 milliseconds.
google.protobuf.Duration message_timeout = 7;
google.protobuf.Duration message_timeout = 7 [(validate.rules).duration = {
lte {seconds: 3600}
yanjunxiang-google marked this conversation as resolved.
Show resolved Hide resolved
gte {}
yanjunxiang-google marked this conversation as resolved.
Show resolved Hide resolved
}];

// Optional additional prefix to use when emitting statistics. This allows to distinguish
// emitted statistics between configured *ext_proc* filters in an HTTP filter chain.
Expand All @@ -167,7 +170,10 @@ message ExternalProcessor {
// Specify the upper bound of
// :ref:`override_message_timeout <envoy_v3_api_field_service.ext_proc.v3.ProcessingResponse.override_message_timeout>`
// If not specified, by default it is 0, which will effectively disable the ``override_message_timeout`` API.
google.protobuf.Duration max_message_timeout = 10;
google.protobuf.Duration max_message_timeout = 10 [(validate.rules).duration = {
lte {seconds: 3600}
yanjunxiang-google marked this conversation as resolved.
Show resolved Hide resolved
gte {}
}];

// Prevents clearing the route-cache when the
// :ref:`clear_route_cache <envoy_v3_api_field_service.ext_proc.v3.CommonResponse.clear_route_cache>`
Expand Down
41 changes: 32 additions & 9 deletions source/common/protobuf/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -645,34 +645,57 @@ ProtobufWkt::Value ValueUtil::listValue(const std::vector<ProtobufWkt::Value>& v

namespace {

void validateDuration(const ProtobufWkt::Duration& duration, int64_t max_seconds_value) {
void validateDuration(const ProtobufWkt::Duration& duration, int64_t max_seconds_value,
yanjunxiang-google marked this conversation as resolved.
Show resolved Hide resolved
bool throw_exception, bool& error) {
error = false;
if (duration.seconds() < 0 || duration.nanos() < 0) {
throw DurationUtil::OutOfRangeException(
fmt::format("Expected positive duration: {}", duration.DebugString()));
if (throw_exception) {
throw DurationUtil::OutOfRangeException(
fmt::format("Expected positive duration: {}", duration.DebugString()));
} else {
error = true;
return;
}
}
if (duration.nanos() > 999999999 || duration.seconds() > max_seconds_value) {
throw DurationUtil::OutOfRangeException(
fmt::format("Duration out-of-range: {}", duration.DebugString()));
if (throw_exception) {
throw DurationUtil::OutOfRangeException(
fmt::format("Duration out-of-range: {}", duration.DebugString()));
} else {
error = true;
}
}
}

void validateDuration(const ProtobufWkt::Duration& duration) {
validateDuration(duration, Protobuf::util::TimeUtil::kDurationMaxSeconds);
bool error = false;
validateDuration(duration, Protobuf::util::TimeUtil::kDurationMaxSeconds, true, error);
}

void validateDurationAsMilliseconds(const ProtobufWkt::Duration& duration) {
void validateDurationAsMilliseconds(const ProtobufWkt::Duration& duration, bool throw_exception,
bool& error) {
// Apply stricter max boundary to the `seconds` value to avoid overflow.
// Note that protobuf internally converts to nanoseconds.
// The kMaxInt64Nanoseconds = 9223372036, which is about 300 years.
constexpr int64_t kMaxInt64Nanoseconds =
std::numeric_limits<int64_t>::max() / (1000 * 1000 * 1000);
validateDuration(duration, kMaxInt64Nanoseconds);
validateDuration(duration, kMaxInt64Nanoseconds, throw_exception, error);
}

} // namespace

uint64_t DurationUtil::durationToMilliseconds(const ProtobufWkt::Duration& duration) {
validateDurationAsMilliseconds(duration);
bool error = false;
validateDurationAsMilliseconds(duration, true, error);
return Protobuf::util::TimeUtil::DurationToMilliseconds(duration);
}

uint64_t DurationUtil::durationToMillisecondsNoThrow(const ProtobufWkt::Duration& duration,
yanjunxiang-google marked this conversation as resolved.
Show resolved Hide resolved
bool& error) {
validateDurationAsMilliseconds(duration, false, error);
if (error) {
return 0;
}
return Protobuf::util::TimeUtil::DurationToMilliseconds(duration);
}

Expand Down
9 changes: 9 additions & 0 deletions source/common/protobuf/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -683,6 +683,15 @@ class DurationUtil {
*/
static uint64_t durationToMilliseconds(const ProtobufWkt::Duration& duration);

/**
* Same as DurationUtil::durationToMilliseconds but does not throw an exception.
* Instead, it returns error if duration is out-of-range.
* @param duration protobuf.
* @param error bool, set into true in case of duration out-of-range; otherwise false.
* @return duration in milliseconds.
*/
static uint64_t durationToMillisecondsNoThrow(const ProtobufWkt::Duration& duration, bool& error);

/**
* Same as Protobuf::util::TimeUtil::DurationToSeconds but with extra validation logic.
* Specifically, we ensure that the duration is positive.
Expand Down
15 changes: 12 additions & 3 deletions source/extensions/filters/http/ext_proc/ext_proc.cc
Original file line number Diff line number Diff line change
Expand Up @@ -527,12 +527,21 @@ void Filter::sendTrailers(ProcessorState& state, const Http::HeaderMap& trailers
stats_.stream_msgs_sent_.inc();
}

void Filter::onNewTimeout(const uint32_t message_timeout_ms) {
void Filter::onNewTimeout(const ProtobufWkt::Duration& override_message_timeout) {
bool error = false;
uint32_t message_timeout_ms =
DurationUtil::durationToMillisecondsNoThrow(override_message_timeout, error);
yanjunxiang-google marked this conversation as resolved.
Show resolved Hide resolved
if (error) {
ENVOY_LOG(warn, "Ext_proc server new timeout setting is out of duration range. "
"Ignoring the message.");
stats_.override_message_timeout_ignored_.inc();
return;
}
// The new timeout has to be >=1ms and <= max_message_timeout configured in filter.
const uint32_t min_timeout_ms = 1;
const uint32_t max_timeout_ms = config_->maxMessageTimeout();
if (message_timeout_ms < min_timeout_ms || message_timeout_ms > max_timeout_ms) {
ENVOY_LOG(warn, "Ext_proc server new timeout setting is out of range. "
ENVOY_LOG(warn, "Ext_proc server new timeout setting is out of config range. "
"Ignoring the message.");
stats_.override_message_timeout_ignored_.inc();
return;
Expand All @@ -559,7 +568,7 @@ void Filter::onReceiveMessage(std::unique_ptr<ProcessingResponse>&& r) {

// Check whether the server is asking to extend the timer.
if (response->has_override_message_timeout()) {
onNewTimeout(DurationUtil::durationToMilliseconds(response->override_message_timeout()));
onNewTimeout(response->override_message_timeout());
return;
}

Expand Down
2 changes: 1 addition & 1 deletion source/extensions/filters/http/ext_proc/ext_proc.h
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ class Filter : public Logger::Loggable<Logger::Id::ext_proc>,
void onGrpcClose() override;

void onMessageTimeout();
void onNewTimeout(const uint32_t message_timeout_ms);
void onNewTimeout(const ProtobufWkt::Duration& override_message_timeout);

void sendBufferedData(ProcessorState& state, ProcessorState::CallbackState new_state,
bool end_stream) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ class ExtProcIntegrationTest : public HttpIntegrationTest,

// ext_proc server sends back a response to tell Envoy to stop the
// original timer and start a new timer.
void serverSendNewTimeout(const uint32_t timeout_ms) {
void serverSendNewTimeout(const uint64_t timeout_ms) {
ProcessingResponse response;
if (timeout_ms < 1000) {
response.mutable_override_message_timeout()->set_nanos(timeout_ms * 1000000);
Expand All @@ -327,7 +327,7 @@ class ExtProcIntegrationTest : public HttpIntegrationTest,

// The new timeout message is ignored by Envoy due to different reasons, like
// new_timeout setting is out-of-range, or max_message_timeout is not configured.
void newTimeoutWrongConfigTest(const uint32_t timeout_ms) {
void newTimeoutWrongConfigTest(const uint64_t timeout_ms) {
// Set envoy filter timeout to be 200ms.
proto_config_.mutable_message_timeout()->set_nanos(200000000);
// Config max_message_timeout proto to enable the new timeout API.
Expand Down Expand Up @@ -1960,4 +1960,13 @@ TEST_P(ExtProcIntegrationTest, RequestMessageNewTimeoutNegativeTestTimeoutNotAcc
newTimeoutWrongConfigTest(500);
}

// Send the new timeout to be an extremely large number, which is out-of-range of duration.
// Verify the code appropriately handled it.
TEST_P(ExtProcIntegrationTest, RequestMessageNewTimeoutOutOfBounds) {
// Config max_message_timeout proto to 100ms to enable the new timeout API.
max_message_timeout_ms_ = 100;
const uint64_t override_message_timeout = 1000000000000000;
newTimeoutWrongConfigTest(override_message_timeout);
}

} // namespace Envoy

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.