api: add 'redacted' option for protobuf messages, and redact SSL certs by mergeconflict · Pull Request #9315 · envoyproxy/envoy

mergeconflict · 2019-12-11T17:05:04Z

Implement MessageUtil::redact() to redact proto fields with the udpa.annotations.sensitive option set. Apply this to SSL certs in the admin config_dump.

Risk Level: low
Testing: unit tests
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Dan Rosen mergeconflict@google.com

Signed-off-by: Dan Rosen <mergeconflict@google.com>

repokitteh-read-only · 2019-12-11T17:05:10Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9315 was opened by mergeconflict.

see: more, trace.

mergeconflict · 2019-12-11T17:05:22Z

/review @htuch

Signed-off-by: Dan Rosen <mergeconflict@google.com>

api/envoy/protobuf/BUILD

source/common/protobuf/utility.cc

api/envoy/protobuf/descriptor.proto

jmarantz

WDYT of simply populating the cert with a hashed version of the cert? It might be a lot less code. What're the pros & cons?

PiotrSikora

Please take a look at the discussion for when we redacted SDS values in /config_dump:
#7365

api/envoy/api/v2/auth/cert.proto

mergeconflict · 2019-12-11T20:12:33Z

WDYT of simply populating the cert with a hashed version of the cert? It might be a lot less code. What're the pros & cons?

@jmarantz: I think my current approach isn't a tremendous amount of code, and it's general purpose: anything else in the config dump that needs to be redacted can use this mechanism. See e.g. https://github.com/envoyproxy/envoy/blob/master/source/common/secret/secret_manager_impl.cc#L118.

mattklein123 · 2019-12-11T20:37:34Z

Big +1 for implementing this. We need this for tapping and other general data exfiltration issues.

incfly · 2019-12-11T21:51:29Z

/cc @incfly

incfly · 2019-12-11T22:00:21Z

WDYT of simply populating the cert with a hashed version of the cert? It might be a lot less code. What're the pros & cons?

Istio builds some tooling to actually get the cert and parse out the expiration time for troubleshooting.

Only clearing the private key and password is good enough and above tooling would still work.

Signed-off-by: Dan Rosen <mergeconflict@google.com>

htuch

Nice, this is a very clean approach to the problem, happy to see it landing.
/wait

htuch · 2019-12-11T23:28:40Z

api/envoy/protobuf/descriptor.proto

+  // 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


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

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

htuch · 2019-12-11T23:29:21Z

source/server/http/admin.cc


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


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

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.

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

source/server/http/admin.cc

source/common/protobuf/utility.cc

Signed-off-by: Dan Rosen <mergeconflict@google.com>

mergeconflict · 2019-12-13T21:43:58Z

@htuch thanks for your help; I think this is ready for a second look.

mergeconflict · 2019-12-13T23:26:51Z

/retest

repokitteh-read-only · 2019-12-13T23:26:55Z

🙀 Error while processing event:

evaluation error
error: function error error: user: combined status is failure, but no failed builds.

🐱

Caused by: a #9315 (comment) was created by @mergeconflict.

see: more, trace.

Signed-off-by: Dan Rosen <mergeconflict@google.com>

htuch

LGTM modulo two minor comments.
/wait

source/common/protobuf/utility.cc

Signed-off-by: Dan Rosen <mergeconflict@google.com>

htuch

LGTM, needs master merge. Epic work, thanks for this contribution.

Signed-off-by: Dan Rosen <mergeconflict@google.com>

htuch

Thanks!

Signed-off-by: Dan Rosen <mergeconflict@google.com>

mergeconflict · 2020-01-15T02:01:44Z

@htuch merged master

api: add 'redacted' option for protobuf messages, and redact SSL certs

e95cafb

Signed-off-by: Dan Rosen <mergeconflict@google.com>

repokitteh-read-only bot added the api label Dec 11, 2019

repokitteh-read-only bot requested a review from htuch December 11, 2019 17:05

mergeconflict mentioned this pull request Dec 11, 2019

Leak of private key inline_string when specifying multiple tls certificates #4757

Closed

fix missing import in v3alpha cert.proto

794aa85

Signed-off-by: Dan Rosen <mergeconflict@google.com>

mergeconflict commented Dec 11, 2019

View reviewed changes

api/envoy/protobuf/BUILD Show resolved Hide resolved

jmarantz self-assigned this Dec 11, 2019

mattklein123 reviewed Dec 11, 2019

View reviewed changes

source/common/protobuf/utility.cc Outdated Show resolved Hide resolved

mattklein123 reviewed Dec 11, 2019

View reviewed changes

api/envoy/protobuf/descriptor.proto Outdated Show resolved Hide resolved

jmarantz reviewed Dec 11, 2019

View reviewed changes

PiotrSikora reviewed Dec 11, 2019

View reviewed changes

api/envoy/api/v2/auth/cert.proto Outdated Show resolved Hide resolved

redact specific string fields instead of clearing entire messages

4948c4b

Signed-off-by: Dan Rosen <mergeconflict@google.com>

htuch suggested changes Dec 11, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 11, 2019

htuch self-assigned this Dec 11, 2019

use udpa.annotations.sensitive and redact non-string fields

927d875

Signed-off-by: Dan Rosen <mergeconflict@google.com>

repokitteh-read-only bot removed the waiting label Dec 13, 2019

Dan Rosen added 4 commits December 12, 2019 21:32

oops

848b869

Signed-off-by: Dan Rosen <mergeconflict@google.com>

whitelist sensitive annotation in protoxform

cf6ebc2

Signed-off-by: Dan Rosen <mergeconflict@google.com>

reify is a word

2f261f6

Signed-off-by: Dan Rosen <mergeconflict@google.com>

testing and bulletproofing

186a8b3

Signed-off-by: Dan Rosen <mergeconflict@google.com>

Merge branch 'master' into redacted_protobuf

e996e29

Signed-off-by: Dan Rosen <mergeconflict@google.com>

repokitteh-read-only bot removed the waiting label Jan 13, 2020

htuch suggested reflection improvements

b690640

Signed-off-by: Dan Rosen <mergeconflict@google.com>

htuch suggested changes Jan 13, 2020

View reviewed changes

source/common/protobuf/utility.cc Outdated Show resolved Hide resolved

source/common/protobuf/utility.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Jan 13, 2020

htuch feedback

e77cb8f

Signed-off-by: Dan Rosen <mergeconflict@google.com>

repokitteh-read-only bot removed the waiting label Jan 13, 2020

htuch previously approved these changes Jan 14, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Jan 14, 2020

Merge branch 'master' into redacted_protobuf

7a6b20c

Signed-off-by: Dan Rosen <mergeconflict@google.com>

mergeconflict dismissed htuch’s stale review via 7a6b20c January 14, 2020 15:05

repokitteh-read-only bot added the api label Jan 14, 2020

Dan Rosen added 2 commits January 14, 2020 10:49

fix bug: empty opaque types are legal

f8167a0

Signed-off-by: Dan Rosen <mergeconflict@google.com>

kick ci

5f1211e

Signed-off-by: Dan Rosen <mergeconflict@google.com>

htuch previously approved these changes Jan 14, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Jan 14, 2020

Merge branch 'master' into redacted_protobuf

358bac9

Signed-off-by: Dan Rosen <mergeconflict@google.com>

mergeconflict dismissed htuch’s stale review via 358bac9 January 15, 2020 01:59

repokitteh-read-only bot added the api label Jan 15, 2020

htuch approved these changes Jan 15, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Jan 15, 2020

htuch merged commit 7bd0c07 into envoyproxy:master Jan 15, 2020

sthagen mentioned this pull request Jan 15, 2020

api: add 'redacted' option for protobuf messages, and redact SSL cert… sthagen/envoyproxy-envoy#149

Merged

PiotrSikora mentioned this pull request Jan 15, 2020

api: redact more fields. #9692

Merged

PiotrSikora mentioned this pull request Jan 26, 2020

Redact secrets in LDS/CDS config dump. #7873

Closed

incfly mentioned this pull request Feb 19, 2020

Private key [redacted] is base64 encoded in config_dump #10096

Closed

tsaarni mentioned this pull request May 23, 2023

debug: redact sensitive info from debug logs #27579

Closed

tsaarni mentioned this pull request Jun 6, 2023

Design proposal: redacting sensitive user data from debug logs #27820

Closed

Conversation

mergeconflict commented Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Dec 11, 2019

Uh oh!

mergeconflict commented Dec 11, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mergeconflict commented Dec 11, 2019

Uh oh!

mattklein123 commented Dec 11, 2019

Uh oh!

incfly commented Dec 11, 2019

Uh oh!

incfly commented Dec 11, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

mergeconflict Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

htuch Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

mergeconflict Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

htuch Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mergeconflict commented Dec 13, 2019

Uh oh!

mergeconflict commented Dec 13, 2019

Uh oh!

repokitteh-read-only bot commented Dec 13, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

mergeconflict commented Jan 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mergeconflict commented Dec 11, 2019 •

edited

Loading