Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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",
],
)
3 changes: 3 additions & 0 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 @@ -119,6 +120,8 @@ message PrivateKeyProvider {

// [#next-free-field: 7]
message TlsCertificate {
option (envoy.protobuf.redacted) = true;
Comment thread
mergeconflict marked this conversation as resolved.
Outdated

// The TLS certificate chain.
core.DataSource certificate_chain = 1;

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",
],
)
2 changes: 2 additions & 0 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 @@ -129,6 +130,7 @@ message PrivateKeyProvider {
// [#next-free-field: 7]
message TlsCertificate {
option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.auth.TlsCertificate";
option (envoy.protobuf.redacted) = true;

// The TLS certificate chain.
core.DataSource certificate_chain = 1;
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()
13 changes: 13 additions & 0 deletions api/envoy/protobuf/descriptor.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
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.MessageOptions {
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",
],
)
72 changes: 72 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,77 @@ std::string MessageUtil::CodeEnumToString(ProtobufUtil::error::Code code) {
}
}

namespace {

// Recursive helper method for MessageUtil::redact() below.
void redactInPlace(Protobuf::Message* message) {
Comment thread
mergeconflict marked this conversation as resolved.
Outdated
// 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());
any->PackFrom(*typed_message);

return;
}

// If the message isn't an `Any`, determine whether we need to redact it by looking for the
// `redacted` option on the message's descriptor.
const auto* descriptor = message->GetDescriptor();
if (descriptor->options().GetExtension(envoy::protobuf::redacted)) {
// If this message should be redacted, clear it and don't recurse any deeper.
message->Clear();
} else {
// Otherwise, use reflection to recurse into all message-typed fields of this message.
const auto* reflection = message->GetReflection();
std::vector<const Protobuf::FieldDescriptor*> field_descriptors;
reflection->ListFields(*message, &field_descriptors);
for (const auto* field_descriptor : field_descriptors) {
// Note for the future: if we ever need to support a "redacted" option at the field level,
// in addition to the existing message-level option, this is where we would check the field
// descriptor's options and clear out individual fields. Currently, we simply don't touch
// any fields that aren't of message type.
if (field_descriptor->type() == Protobuf::FieldDescriptor::TYPE_MESSAGE) {
// Repeated fields need slightly different handling from non-repeated...
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));
}
} else {
redactInPlace(reflection->MutableMessage(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());
return redacted;
}

ProtobufWkt::Value ValueUtil::loadFromYaml(const std::string& yaml) {
try {
return parseYamlNode(YAML::Load(yaml));
Expand Down
8 changes: 8 additions & 0 deletions source/common/protobuf/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,14 @@ 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 cleared. See 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
* cleared.
*/
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