Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
e95cafb
api: add 'redacted' option for protobuf messages, and redact SSL certs
Dec 11, 2019
794aa85
fix missing import in v3alpha cert.proto
Dec 11, 2019
4948c4b
redact specific string fields instead of clearing entire messages
Dec 11, 2019
927d875
use udpa.annotations.sensitive and redact non-string fields
Dec 13, 2019
848b869
oops
Dec 13, 2019
cf6ebc2
whitelist sensitive annotation in protoxform
Dec 13, 2019
2f261f6
reify is a word
Dec 13, 2019
186a8b3
testing and bulletproofing
Dec 13, 2019
bd56dba
recognize UDPA TypedStruct
Dec 16, 2019
782d092
Merge branch 'master' into redacted_protobuf
Dec 17, 2019
0f493fa
introduce a test proto for better coverage, and add a bit of user doc…
Dec 17, 2019
8013dbe
Merge branch 'master' into redacted_protobuf
Dec 19, 2019
3552086
lizan is right
Dec 19, 2019
b4c9bf9
apply to TlsSessionTicketKeys and address differences in expected output
Dec 19, 2019
af5a7df
special case handling for DataSource
Dec 20, 2019
32673ff
use fallthru macro
Dec 20, 2019
09d70fc
Merge branch 'master' into redacted_protobuf
Jan 2, 2020
11f9d43
update generated_api_shadow
Jan 2, 2020
2bf220f
Merge branch 'master' into redacted_protobuf
Jan 3, 2020
f6e49c6
fix format
Jan 3, 2020
d9e836c
don't worry about v3alpha yet
Jan 3, 2020
5dc3c06
split tests up for clarity
Jan 3, 2020
29a6172
Merge branch 'master' into redacted_protobuf
Jan 6, 2020
403fdd2
Merge branch 'master' into redacted_protobuf
Jan 9, 2020
208e9b4
remove special-case handling for DataSource and replace with better h…
Jan 9, 2020
77f295b
use pure reflection rather than dynamic_cast
Jan 10, 2020
2488863
Merge branch 'master' into redacted_protobuf
Jan 10, 2020
769e817
update spelling dict and whitelist protobuf/utility.cc for SerializeA…
Jan 10, 2020
891da34
"reified" is a word
Jan 10, 2020
e996e29
Merge branch 'master' into redacted_protobuf
Jan 13, 2020
b690640
htuch suggested reflection improvements
Jan 13, 2020
e77cb8f
htuch feedback
Jan 13, 2020
7a6b20c
Merge branch 'master' into redacted_protobuf
Jan 14, 2020
f8167a0
fix bug: empty opaque types are legal
Jan 14, 2020
5f1211e
kick ci
Jan 14, 2020
358bac9
Merge branch 'master' into redacted_protobuf
Jan 15, 2020
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
4 changes: 2 additions & 2 deletions api/bazel/repository_locations.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ PROMETHEUS_SHA = "783bdaf8ee0464b35ec0c8704871e1e72afa0005c3f3587f65d9d6694bf391

KAFKA_SOURCE_SHA = "ae7a1696c0a0302b43c5b21e515c37e6ecd365941f68a510a7e442eebddf39a1" # 2.2.0-rc2

UDPA_GIT_SHA = "d1f2ba7f5ba62c55b7466409e7f972c93e957d2b" # Dec 6, 2019
UDPA_SHA256 = "0271fb8ad2ec9ade21e4c7737dd128d2a8d8edebe911b777e2fc2585414aa045"
UDPA_GIT_SHA = "89624d5efc546eac78cf2784bed4dc36a09d5772" # Dec 12, 2019
UDPA_SHA256 = "4ec5716adc13459bb23ee11b2d842fb4da7ae10add113f4e2d9eed1659c34b44"

ZIPKINAPI_RELEASE = "0.2.2" # Aug 23, 2019
ZIPKINAPI_SHA256 = "688c4fe170821dd589f36ec45aaadc03a618a40283bc1f97da8fa11686fc816b"
Expand Down
5 changes: 4 additions & 1 deletion api/envoy/api/v2/auth/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,8 @@ load("@envoy_api//bazel:api_build_system.bzl", "api_proto_package")
licenses(["notice"]) # Apache 2

api_proto_package(
deps = ["//envoy/api/v2/core:pkg"],
deps = [
"//envoy/api/v2/core:pkg",
"@com_github_cncf_udpa//udpa/annotations:pkg",
],
)
6 changes: 4 additions & 2 deletions api/envoy/api/v2/auth/cert.proto
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import "google/protobuf/any.proto";
import "google/protobuf/struct.proto";
import "google/protobuf/wrappers.proto";

import "udpa/annotations/sensitive.proto";

import "validate/validate.proto";

// [#protodoc-title: Common TLS configuration]
Expand Down Expand Up @@ -123,7 +125,7 @@ message TlsCertificate {
core.DataSource certificate_chain = 1;

// The TLS private key.
core.DataSource private_key = 2;
core.DataSource private_key = 2 [(udpa.annotations.sensitive) = true];

// BoringSSL private key method provider. This is an alternative to :ref:`private_key
// <envoy_api_field_auth.TlsCertificate.private_key>` field. This can't be
Expand All @@ -136,7 +138,7 @@ message TlsCertificate {

// The password to decrypt the TLS private key. If this field is not set, it is assumed that the
// TLS private key is not password encrypted.
core.DataSource password = 3;
core.DataSource password = 3 [(udpa.annotations.sensitive) = true];

// [#not-implemented-hide:]
core.DataSource ocsp_staple = 4;
Expand Down
5 changes: 3 additions & 2 deletions api/envoy/api/v3alpha/auth/cert.proto
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import "google/protobuf/any.proto";
import "google/protobuf/struct.proto";
import "google/protobuf/wrappers.proto";

import "udpa/annotations/sensitive.proto";
import "udpa/annotations/versioning.proto";

import "validate/validate.proto";
Expand Down Expand Up @@ -134,7 +135,7 @@ message TlsCertificate {
core.DataSource certificate_chain = 1;

// The TLS private key.
core.DataSource private_key = 2;
core.DataSource private_key = 2 [(udpa.annotations.sensitive) = true];

// BoringSSL private key method provider. This is an alternative to :ref:`private_key
// <envoy_api_field_api.v3alpha.auth.TlsCertificate.private_key>` field. This can't be
Expand All @@ -147,7 +148,7 @@ message TlsCertificate {

// The password to decrypt the TLS private key. If this field is not set, it is assumed that the
// TLS private key is not password encrypted.
core.DataSource password = 3;
core.DataSource password = 3 [(udpa.annotations.sensitive) = true];

// [#not-implemented-hide:]
core.DataSource ocsp_staple = 4;
Expand Down
8 changes: 8 additions & 0 deletions docs/root/operations/admin.rst
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,14 @@ modify different aspects of the server:
messages. See the :ref:`response definition <envoy_api_msg_admin.v2alpha.ConfigDump>` for more
information.

.. warning::
Configuration may include :ref:`TLS certificates <envoy_api_msg_auth.TlsCertificate>`. Before
dumping the configuration, Envoy will attempt to redact the ``private_key`` and ``password``
fields from any certificates it finds. This relies on the configuration being a strongly-typed
protobuf message. If your Envoy configuration uses deprecated ``config`` fields (of type
``google.protobuf.Struct``), please update to the recommended ``typed_config`` fields (of type
``google.protobuf.Any``) to ensure sensitive data is redacted properly.

.. warning::
The underlying proto is marked v2alpha and hence its contents, including the JSON representation,
are not guaranteed to be stable.
Expand Down
2 changes: 2 additions & 0 deletions source/common/protobuf/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ envoy_cc_library(
"//source/common/common:assert_lib",
"//source/common/common:hash_lib",
"//source/common/common:utility_lib",
"@com_github_cncf_udpa//udpa/annotations:pkg_cc_proto",
"@com_github_cncf_udpa//udpa/type/v1:pkg_cc_proto",
"@envoy_api//envoy/type:pkg_cc_proto",
],
)
110 changes: 110 additions & 0 deletions source/common/protobuf/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
#include "common/protobuf/protobuf.h"

#include "absl/strings/match.h"
#include "udpa/annotations/sensitive.pb.h"
#include "udpa/type/v1/typed_struct.pb.h"
#include "yaml-cpp/yaml.h"

namespace Envoy {
Expand Down Expand Up @@ -506,6 +508,114 @@ std::string MessageUtil::CodeEnumToString(ProtobufUtil::error::Code code) {
}
}

namespace {

std::unique_ptr<Protobuf::Message> typeUrlToNewMessage(Protobuf::MessageFactory& message_factory,
absl::string_view type_url) {
const absl::string_view type_name = TypeUtil::typeUrlToDescriptorFullName(type_url);
const Protobuf::Descriptor* descriptor =
Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName(
static_cast<std::string>(type_name));
Comment thread
mergeconflict marked this conversation as resolved.
Outdated
return std::unique_ptr<Protobuf::Message>(
descriptor == nullptr ? nullptr : message_factory.GetPrototype(descriptor)->New());
}

// Recursive helper method for MessageUtil::redact() below. Note that we have to keep track of
// whether an ancestor was marked as `sensitive`, not just the field, because of cases like
// `TlsContext::private_key`, which is of type `core.DataSource` rather than `string`.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm do we want to redact if the DataSource is pointing to a file rather than inline string/bytes?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach doesn't give us any way to differentiate, unfortunately.

void redactInPlace(Protobuf::Message* message, bool ancestor_is_sensitive) {
// If the message is an `Any`, we have to first unpack it to its original type to redact it...
auto* any = dynamic_cast<ProtobufWkt::Any*>(message);
if (any != nullptr) {
Comment thread
mergeconflict marked this conversation as resolved.
Outdated
Protobuf::DynamicMessageFactory message_factory;
auto typed_message = typeUrlToNewMessage(message_factory, any->type_url());
if (typed_message == nullptr) {
// If the type URL doesn't correspond to a known proto, give up redacting and treat this
// message the same as a `ProtobufWkt::Struct`. See the documented limitation on
// `MessageUtil::redact()` for more context.
ENVOY_LOG_MISC(warn, "Could not redact ProtobufWkt::Any with unknown type URL {}",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there tests that check for this condition (ideally doing an EXPECT_THROW_WITH_MESSAGE on this message)?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are tests for this condition, although not using EXPECT_THROW_WITH_MESSAGE. We don't throw here, we fall back to treating the input as a struct.

any->type_url());
return;
}

any->UnpackTo(typed_message.get());
redactInPlace(typed_message.get(), ancestor_is_sensitive);
any->PackFrom(*typed_message);
return;
}

// If the message is a `TypedStruct`, it also contains a `type_url` and can be handled the same
// way as `Any` above.
auto* typed_struct = dynamic_cast<udpa::type::v1::TypedStruct*>(message);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how safe this is in general. You can have a TypeStruct message created by a DynamicMessageFactory, which isn't C++ dynamic castable. We are doing this trick increasingly in the VersionConverter (I have two active PRs that do this). I'm wondering if we should check the proto descriptor's full_name here, and use reflection to extract out the fields.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, they are of type google::protobuf::DynamicMessage. I will update.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, this was a really important catch. PTAL, thanks!

if (typed_struct != nullptr) {
Protobuf::DynamicMessageFactory message_factory;
auto typed_message = typeUrlToNewMessage(message_factory, typed_struct->type_url());
if (typed_message == nullptr) {
// If the type URL doesn't correspond to a known proto, give up redacting and treat this
// message the same as a `ProtobufWkt::Struct`. See the documented limitation on
// `MessageUtil::redact()` for more context.
ENVOY_LOG_MISC(warn, "Could not redact udpa::type::v1::TypedStruct with unknown type URL {}",
Comment thread
mergeconflict marked this conversation as resolved.
Outdated
typed_struct->type_url());
return;
}

MessageUtil::jsonConvert(typed_struct->value(), ProtobufMessage::getNullValidationVisitor(),
*typed_message);
redactInPlace(typed_message.get(), ancestor_is_sensitive);
MessageUtil::jsonConvert(*typed_message, *(typed_struct->mutable_value()));
return;
}

// Otherwise, use reflection to traverse all populated fields of this message...
Comment thread
mergeconflict marked this conversation as resolved.
Outdated
const auto* reflection = message->GetReflection();
std::vector<const Protobuf::FieldDescriptor*> field_descriptors;
reflection->ListFields(*message, &field_descriptors);
Comment thread
mergeconflict marked this conversation as resolved.
Outdated
for (const auto* field_descriptor : field_descriptors) {
// Redact if this field or any of its ancestors have the `sensitive` option set.
const bool sensitive = ancestor_is_sensitive ||
field_descriptor->options().GetExtension(udpa::annotations::sensitive);

if (field_descriptor->type() == Protobuf::FieldDescriptor::TYPE_MESSAGE) {
// Recursive case: traverse message fields.
if (field_descriptor->is_repeated()) {
const int field_size = reflection->FieldSize(*message, field_descriptor);
for (int i = 0; i < field_size; ++i) {
redactInPlace(reflection->MutableRepeatedMessage(message, field_descriptor, i),
sensitive);
}
} else {
redactInPlace(reflection->MutableMessage(message, field_descriptor), sensitive);
}
} else if (sensitive) {
// Base case: replace strings with "[redacted]" and clear all others.
if (field_descriptor->type() == Protobuf::FieldDescriptor::TYPE_STRING) {
if (field_descriptor->is_repeated()) {
const int field_size = reflection->FieldSize(*message, field_descriptor);
for (int i = 0; i < field_size; ++i) {
reflection->SetRepeatedString(message, field_descriptor, i, "[redacted]");
}
} else {
reflection->SetString(message, field_descriptor, "[redacted]");
}
} else {
reflection->ClearField(message, field_descriptor);
}
}
}
}

} // namespace

std::unique_ptr<Protobuf::Message> MessageUtil::redact(const Protobuf::Message& message) {
// 1. Clone the original message. New() will return an empty message of the correct dynamic type.
Comment thread
mergeconflict marked this conversation as resolved.
Outdated
std::unique_ptr<Protobuf::Message> redacted(message.New());
redacted->MergeFrom(message);

// 2. Redact the cloned message.
redactInPlace(redacted.get(), /* ancestor_is_sensitive = */ false);
return redacted;
}

ProtobufWkt::Value ValueUtil::loadFromYaml(const std::string& yaml) {
try {
return parseYamlNode(YAML::Load(yaml));
Expand Down
22 changes: 22 additions & 0 deletions source/common/protobuf/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,28 @@ class MessageUtil {
* @param code the protobuf error code
*/
static std::string CodeEnumToString(ProtobufUtil::error::Code code);

/**
* Makes a copy of a source message with all sensitive data (that is, fields annotated as
* `udpa.annotations.sensitive`) redacted for display. String-typed fields annotated as
* `sensitive` will be replaced with the string "[redacted]", primitive-typed fields (including
* enums) will be cleared, and message-typed fields will be traversed recursively to redact
* their contents.
*
* LIMITATION: This works properly for strongly-typed messages, as well as for messages packed in
* a `ProtobufWkt::Any` with a `type_url` corresponding to a proto that was compiled into the
* Envoy binary. However it does not work for messages encoded as `ProtobufWkt::Struct`, since
* structs are missing the "sensitive" annotations that this function expects. Similarly, it fails
* for messages encoded as `ProtobufWkt::Any` with a `type_url` that isn't registered with the
* binary. If you're working with struct-typed messages, including those that might be hiding
* within strongly-typed messages, please reify them to strongly-typed messages using
* `MessageUtil::jsonConvert()` before calling `MessageUtil::redact()`.
*
* @param message original message to copy from.
* @return cloned message of the same dynamic type as the original, with all sensitive data
* redacted.
*/
static std::unique_ptr<Protobuf::Message> redact(const Protobuf::Message& message);
};

class ValueUtil {
Expand Down
3 changes: 2 additions & 1 deletion source/server/http/admin.cc
Original file line number Diff line number Diff line change
Expand Up @@ -541,9 +541,10 @@ Http::Code AdminImpl::handlerConfigDump(absl::string_view, Http::HeaderMap& resp
auto& any_message = *(dump.add_configs());
any_message.PackFrom(*message);
}
const auto redacted = MessageUtil::redact(dump);
Comment thread
mergeconflict marked this conversation as resolved.
Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Given the limitations that are now spelled out around Struct, how comfortable are we using this for config dump in admin endpoint when we have the TlsCertificate inside an opaque TransportSocket (which is now the recommended way to do it, explicit TLS contexts are deprecated)? Do we need some comments here or user facing documentation on the limits of redaction.

Ultimately I'm wondering how we're going to document the effective result of this process in version history in a way that end users can grok and get predictable behavior from.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is tough. On the one hand, usage of Struct is deprecated in v2 (including TransportSocket.config), and we should be safer in v3. On the other hand, even in v3 we're still not totally safe with this approach: in the case you described at some point earlier, where some extension is dynamically loaded, it's possible that the extension's type urls won't be registered in the descriptor pool.

I think I'd like to move forward with this anyways, just so we can have something in place, but I definitely agree it's dissatisfying having this limitation.

Comment thread
mergeconflict marked this conversation as resolved.
Outdated

response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Json);
response.add(MessageUtil::getJsonStringFromMessage(dump, true)); // pretty-print
response.add(MessageUtil::getJsonStringFromMessage(*redacted, true)); // pretty-print

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel we also want this in various logs, e.g. trace level xDS logs, where this has come up previously as an issue.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, definitely. I'm thinking to just target this one spot in this PR, as a proof of concept, and then track down others (e.g. #4757) as a follow-up.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'm good with keeping the issue open for follow-up.

return Http::Code::OK;
}

Expand Down
1 change: 1 addition & 0 deletions test/common/protobuf/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ envoy_cc_test(
"//test/mocks/protobuf:protobuf_mocks",
"//test/mocks/server:server_mocks",
"//test/proto:deprecated_proto_cc_proto",
"//test/proto:sensitive_proto_cc_proto",
"//test/test_common:environment_lib",
"//test/test_common:logging_lib",
"//test/test_common:utility_lib",
Expand Down
Loading