diff --git a/source/server/admin/config_dump_handler.cc b/source/server/admin/config_dump_handler.cc index 69c685d8a6939..4714729d3071c 100644 --- a/source/server/admin/config_dump_handler.cc +++ b/source/server/admin/config_dump_handler.cc @@ -12,6 +12,21 @@ namespace Envoy { namespace Server { namespace { + +// Validates that `field_mask` is valid for `message` and applies `TrimMessage`. +// Necessary because TrimMessage crashes if `field_mask` is invalid. +// Returns `true` on success. +bool checkFieldMaskAndTrimMessage(const Protobuf::FieldMask& field_mask, + Protobuf::Message& message) { + for (const auto& path : field_mask.paths()) { + if (!ProtobufUtil::FieldMaskUtil::GetFieldDescriptors(message.GetDescriptor(), path, nullptr)) { + return false; + } + } + ProtobufUtil::FieldMaskUtil::TrimMessage(field_mask, &message); + return true; +} + // Apply a field mask to a resource message. A simple field mask might look // like "cluster.name,cluster.alt_stat_name,last_updated" for a StaticCluster // resource. Unfortunately, since the "cluster" field is Any and the in-built @@ -31,7 +46,10 @@ namespace { // this to allow arbitrary indexing through Any fields. This is pretty // complicated, we would need to build a FieldMask tree similar to how the C++ // Protobuf library does this internally. -void trimResourceMessage(const Protobuf::FieldMask& field_mask, Protobuf::Message& message) { +/** + * @return true on success, false if `field_mask` is invalid. + */ +bool trimResourceMessage(const Protobuf::FieldMask& field_mask, Protobuf::Message& message) { const Protobuf::Descriptor* descriptor = message.GetDescriptor(); const Protobuf::Reflection* reflection = message.GetReflection(); // Figure out which paths cover Any fields. For each field, gather the paths to @@ -87,13 +105,15 @@ void trimResourceMessage(const Protobuf::FieldMask& field_mask, Protobuf::Messag inner_message.reset(dmf.GetPrototype(inner_descriptor)->New()); MessageUtil::unpackTo(any_message, *inner_message); // Trim message. - ProtobufUtil::FieldMaskUtil::TrimMessage(inner_field_mask, inner_message.get()); + if (!checkFieldMaskAndTrimMessage(inner_field_mask, *inner_message)) { + return false; + } // Pack it back into the Any resource. any_message.PackFrom(*inner_message); reflection->MutableMessage(&message, any_field)->CopyFrom(any_message); } } - ProtobufUtil::FieldMaskUtil::TrimMessage(outer_field_mask, &message); + return checkFieldMaskAndTrimMessage(outer_field_mask, message); } // Helper method to get the resource parameter. @@ -126,14 +146,18 @@ Http::Code ConfigDumpHandler::handlerConfigDump(absl::string_view url, envoy::admin::v3::ConfigDump dump; + absl::optional> err; if (resource.has_value()) { - auto err = addResourceToDump(dump, mask, resource.value(), include_eds); - if (err.has_value()) { - response.add(err.value().second); - return err.value().first; - } + err = addResourceToDump(dump, mask, resource.value(), include_eds); } else { - addAllConfigToDump(dump, mask, include_eds); + err = addAllConfigToDump(dump, mask, include_eds); + } + if (err.has_value()) { + response_headers.addReference(Http::Headers::get().XContentTypeOptions, + Http::Headers::get().XContentTypeOptionValues.Nosniff); + response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Text); + response.add(err.value().second); + return err.value().first; } MessageUtil::redact(dump); @@ -176,7 +200,11 @@ ConfigDumpHandler::addResourceToDump(envoy::admin::v3::ConfigDump& dump, if (mask.has_value()) { Protobuf::FieldMask field_mask; ProtobufUtil::FieldMaskUtil::FromString(mask.value(), &field_mask); - trimResourceMessage(field_mask, msg); + if (!trimResourceMessage(field_mask, msg)) { + return absl::optional>{std::make_pair( + Http::Code::BadRequest, absl::StrCat("FieldMask ", field_mask.DebugString(), + " could not be successfully used."))}; + } } auto* config = dump.add_configs(); config->PackFrom(msg); @@ -191,9 +219,10 @@ ConfigDumpHandler::addResourceToDump(envoy::admin::v3::ConfigDump& dump, std::make_pair(Http::Code::NotFound, fmt::format("{} not found in config dump", resource))}; } -void ConfigDumpHandler::addAllConfigToDump(envoy::admin::v3::ConfigDump& dump, - const absl::optional& mask, - bool include_eds) const { +absl::optional> +ConfigDumpHandler::addAllConfigToDump(envoy::admin::v3::ConfigDump& dump, + const absl::optional& mask, + bool include_eds) const { Envoy::Server::ConfigTracker::CbsMap callbacks_map = config_tracker_.getCallbacksMap(); if (include_eds) { // TODO(mattklein123): Add ability to see warming clusters in admin output. @@ -213,12 +242,17 @@ void ConfigDumpHandler::addAllConfigToDump(envoy::admin::v3::ConfigDump& dump, ProtobufUtil::FieldMaskUtil::FromString(mask.value(), &field_mask); // We don't use trimMessage() above here since masks don't support // indexing through repeated fields. - ProtobufUtil::FieldMaskUtil::TrimMessage(field_mask, message.get()); + if (!checkFieldMaskAndTrimMessage(field_mask, *message)) { + return absl::optional>{std::make_pair( + Http::Code::BadRequest, absl::StrCat("FieldMask ", field_mask.DebugString(), + " could not be successfully used."))}; + } } auto* config = dump.add_configs(); config->PackFrom(*message); } + return absl::nullopt; } ProtobufTypes::MessagePtr ConfigDumpHandler::dumpEndpointConfigs() const { diff --git a/source/server/admin/config_dump_handler.h b/source/server/admin/config_dump_handler.h index f1531ddee04a1..a5f024f288d6e 100644 --- a/source/server/admin/config_dump_handler.h +++ b/source/server/admin/config_dump_handler.h @@ -27,8 +27,9 @@ class ConfigDumpHandler : public HandlerContextBase { Buffer::Instance& response, AdminStream&) const; private: - void addAllConfigToDump(envoy::admin::v3::ConfigDump& dump, - const absl::optional& mask, bool include_eds) const; + absl::optional> + addAllConfigToDump(envoy::admin::v3::ConfigDump& dump, const absl::optional& mask, + bool include_eds) const; /** * Add the config matching the passed resource to the passed config dump. * @return absl::nullopt on success, else the Http::Code and an error message that should be added diff --git a/test/server/admin/config_dump_handler_test.cc b/test/server/admin/config_dump_handler_test.cc index 6075dffa4a673..482d7f2b38091 100644 --- a/test/server/admin/config_dump_handler_test.cc +++ b/test/server/admin/config_dump_handler_test.cc @@ -564,24 +564,16 @@ TEST_P(AdminInstanceTest, ConfigDumpFiltersByResourceAndMask) { EXPECT_EQ(expected_json, output); } -// Test that no fields are present in the JSON output if there is no intersection between the fields +// Test that BadRequest is returned if there is no intersection between the fields // of the config dump and the fields present in the mask query parameter. TEST_P(AdminInstanceTest, ConfigDumpNonExistentMask) { Buffer::OwnedImpl response; Http::TestResponseHeaderMapImpl header_map; auto clusters = admin_.getConfigTracker().add("clusters", testDumpClustersConfig); - const std::string expected_json = R"EOF({ - "configs": [ - { - "@type": "type.googleapis.com/envoy.admin.v3.ClustersConfigDump.StaticCluster" - } - ] -} -)EOF"; - EXPECT_EQ(Http::Code::OK, + EXPECT_EQ(Http::Code::BadRequest, getCallback("/config_dump?resource=static_clusters&mask=bad", header_map, response)); std::string output = response.toString(); - EXPECT_EQ(expected_json, output); + EXPECT_THAT(output, testing::HasSubstr("could not be successfully used")); } // Test that a 404 Not found is returned if a non-existent resource is passed in as the @@ -611,5 +603,55 @@ TEST_P(AdminInstanceTest, ConfigDumpResourceNotRepeated) { getCallback("/config_dump?resource=version_info", header_map, response)); } +TEST_P(AdminInstanceTest, InvalidFieldMaskWithResourceDoesNotCrash) { + Buffer::OwnedImpl response; + Http::TestResponseHeaderMapImpl header_map; + auto clusters = admin_.getConfigTracker().add("clusters", [] { + auto msg = std::make_unique(); + auto* static_cluster = msg->add_static_clusters(); + envoy::config::cluster::v3::Cluster inner_cluster; + inner_cluster.add_transport_socket_matches()->set_name("match1"); + inner_cluster.add_transport_socket_matches()->set_name("match2"); + static_cluster->mutable_cluster()->PackFrom(inner_cluster); + return msg; + }); + + // `transport_socket_matches` is a repeated field, and cannot be indexed through in a FieldMask. + EXPECT_EQ(Http::Code::BadRequest, + getCallback( + "/config_dump?resource=static_clusters&mask=cluster.transport_socket_matches.name", + header_map, response)); + EXPECT_EQ("FieldMask paths: \"cluster.transport_socket_matches.name\"\n could not be " + "successfully used.", + response.toString()); + EXPECT_EQ(header_map.ContentType()->value().getStringView(), + Http::Headers::get().ContentTypeValues.Text); + EXPECT_EQ(header_map.get(Http::Headers::get().XContentTypeOptions)[0]->value(), + Http::Headers::get().XContentTypeOptionValues.Nosniff); +} + +TEST_P(AdminInstanceTest, InvalidFieldMaskWithoutResourceDoesNotCrash) { + Buffer::OwnedImpl response; + Http::TestResponseHeaderMapImpl header_map; + auto bootstrap = admin_.getConfigTracker().add("bootstrap", [] { + auto msg = std::make_unique(); + auto* bootstrap = msg->mutable_bootstrap(); + bootstrap->mutable_node()->add_extensions()->set_name("ext1"); + bootstrap->mutable_node()->add_extensions()->set_name("ext2"); + return msg; + }); + + // `extensions` is a repeated field, and cannot be indexed through in a FieldMask. + EXPECT_EQ(Http::Code::BadRequest, + getCallback("/config_dump?mask=bootstrap.node.extensions.name", header_map, response)); + EXPECT_EQ("FieldMask paths: \"bootstrap.node.extensions.name\"\n could not be " + "successfully used.", + response.toString()); + EXPECT_EQ(header_map.ContentType()->value().getStringView(), + Http::Headers::get().ContentTypeValues.Text); + EXPECT_EQ(header_map.get(Http::Headers::get().XContentTypeOptions)[0]->value(), + Http::Headers::get().XContentTypeOptionValues.Nosniff); +} + } // namespace Server } // namespace Envoy