Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
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",
"//envoy/protobuf:pkg",
],
)
5 changes: 3 additions & 2 deletions api/envoy/api/v2/auth/cert.proto
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ option java_multiple_files = true;

import "envoy/api/v2/core/base.proto";
import "envoy/api/v2/core/config_source.proto";
import "envoy/protobuf/descriptor.proto";

import "google/protobuf/any.proto";
import "google/protobuf/struct.proto";
Expand Down Expand Up @@ -123,7 +124,7 @@ message TlsCertificate {
core.DataSource certificate_chain = 1;

// The TLS private key.
core.DataSource private_key = 2;
core.DataSource private_key = 2 [(envoy.protobuf.redacted) = 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 +137,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 [(envoy.protobuf.redacted) = true];

// [#not-implemented-hide:]
core.DataSource ocsp_staple = 4;
Expand Down
1 change: 1 addition & 0 deletions api/envoy/api/v3alpha/auth/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ api_proto_package(
deps = [
"//envoy/api/v2/auth:pkg",
"//envoy/api/v3alpha/core:pkg",
"//envoy/protobuf:pkg",
"@com_github_cncf_udpa//udpa/annotations:pkg",
],
)
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 @@ -8,6 +8,7 @@ option java_multiple_files = true;

import "envoy/api/v3alpha/core/base.proto";
import "envoy/api/v3alpha/core/config_source.proto";
import "envoy/protobuf/descriptor.proto";

import "google/protobuf/any.proto";
import "google/protobuf/struct.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 [(envoy.protobuf.redacted) = 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 [(envoy.protobuf.redacted) = true];

// [#not-implemented-hide:]
core.DataSource ocsp_staple = 4;
Expand Down
7 changes: 7 additions & 0 deletions api/envoy/protobuf/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# DO NOT EDIT. This file is generated by tools/proto_sync.py.
Comment thread
mergeconflict marked this conversation as resolved.

load("@envoy_api//bazel:api_build_system.bzl", "api_proto_package")

licenses(["notice"]) # Apache 2

api_proto_package()
18 changes: 18 additions & 0 deletions api/envoy/protobuf/descriptor.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
syntax = "proto3";

package envoy.protobuf;
Comment thread
mergeconflict marked this conversation as resolved.
Outdated

option java_outer_classname = "DescriptorProto";
option java_multiple_files = true;
option java_package = "io.envoyproxy.envoy.protobuf";

import "google/protobuf/descriptor.proto";

extend google.protobuf.FieldOptions {
// This option indicates that a field contains personally-identifying or otherwise sensitive data,
// such as a private key or a password. It is used by `MessageUtil::redact` to determine which
// fields need to be sanitized. Please note that this has no effect on standard Protobuf functions
// such as `TextFormat::PrintToString`; you must explicitly call `MessageUtil::redact` wherever

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.

Can we add check_format support to ban all the likely candidates (e.g. DebugString, PrintToString) and force safe uses?

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.

Let's chat about that tomorrow; I'm not sure how big a hammer we want to use, if any.

// you need to sanitize sensitive data.
bool redacted = 50000;
}
1 change: 1 addition & 0 deletions source/common/protobuf/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ envoy_cc_library(
"//source/common/common:assert_lib",
"//source/common/common:hash_lib",
"//source/common/common:utility_lib",
"@envoy_api//envoy/protobuf:pkg_cc_proto",
"@envoy_api//envoy/type:pkg_cc_proto",
],
)
74 changes: 74 additions & 0 deletions source/common/protobuf/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include <limits>
#include <numeric>

#include "envoy/protobuf/descriptor.pb.h"
#include "envoy/protobuf/message_validator.h"

#include "common/common/assert.h"
Expand Down Expand Up @@ -505,6 +506,79 @@ std::string MessageUtil::CodeEnumToString(ProtobufUtil::error::Code code) {
}
}

namespace {

// Recursive helper method for MessageUtil::redact() below.
void redactInPlace(Protobuf::Message* message, bool ancestor_is_redacted) {
// If the message is an `Any`, we can't use any type-based introspection. Instead, we have to
// 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
// 1. Extract the type name from the URL. This is guaranteed to be the portion of the URL
// following the last '/' character.
const std::string& type_url = any->type_url();
const std::size_t last_slash = type_url.find_last_of('/');
const std::string type_name = type_url.substr(last_slash + 1);
const auto* descriptor =
Comment thread
mergeconflict marked this conversation as resolved.
Outdated
Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName(type_name);

// 2. Instantiate a mutable message of the correct dynamic type, and unpack the `Any` into it.
Protobuf::DynamicMessageFactory dmf;
std::unique_ptr<Protobuf::Message> typed_message(dmf.GetPrototype(descriptor)->New());
any->UnpackTo(typed_message.get());

// 3. Redact the typed message and repack it into the original `Any`.
redactInPlace(typed_message.get(), ancestor_is_redacted);
any->PackFrom(*typed_message);

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 `redacted` option set.
Comment thread
mergeconflict marked this conversation as resolved.
Outdated
const bool redacted =
ancestor_is_redacted || field_descriptor->options().GetExtension(envoy::protobuf::redacted);

if (redacted && field_descriptor->type() == Protobuf::FieldDescriptor::TYPE_STRING) {
// Base case: replace strings with "[redacted]".
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 if (field_descriptor->type() == Protobuf::FieldDescriptor::TYPE_MESSAGE) {
Comment thread
mergeconflict marked this conversation as resolved.
Outdated
// 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), redacted);
}
} else {
redactInPlace(reflection->MutableMessage(message, field_descriptor), redacted);
}
}
}
}

} // 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_redacted = */ false);
return redacted;
}

ProtobufWkt::Value ValueUtil::loadFromYaml(const std::string& yaml) {
try {
return parseYamlNode(YAML::Load(yaml));
Expand Down
12 changes: 12 additions & 0 deletions source/common/protobuf/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,18 @@ 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 `redacted` data sanitized. Specifically, any
* string-typed fields that are annotated as `redacted`, or that are contained directly or
* indirectly within a message-typed field that is annotated as `redacted`, will have their
* values replaced with the string "[redacted]" in the result.
* See also envoy.protobuf.redacted.
* @param message original message to copy from.
* @return cloned message of the same dynamic type as the original, with all `redacted` data
* sanitized.
*/
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 @@ -539,9 +539,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