Skip to content

debug: redact sensitive info from debug logs #27579

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tsaarni wants to merge 4 commits into from
Closed

debug: redact sensitive info from debug logs #27579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4cb131b
debug: redact sensitive info from debug logs
tsaarni May 24, 2023
ebc501a
debug: negated the meaning of feature flag
tsaarni May 26, 2023
db53148
debug: move HeaderMap dumpState to Utilities
tsaarni May 29, 2023
7043b24
debug: added doc for masking debug logs
tsaarni May 29, 2023
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
14 changes: 14 additions & 0 deletions docs/root/configuration/observability/application_logging.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,17 @@ with the following :ref:`command line options <operations_cli>`:
* The ``--log-level`` flag can be set to control the log severity logged to Stackdriver.

`Reference documentation <https://cloud.google.com/run/docs/logging#container-logs>`_ for Stackdriver on GKE.

Redacting sensitive information from debug logs
-----------------------------------------------

Sometimes it may be necessary to enable ``debug`` level logs to troubleshoot issues in production environments.
This introduces a risk of exposing sensitive user information, such as:

* Authorization header values with tokens or passwords.
* Cookie headers with sensitive cookies.
* Query parameters with sensitive information.

By default header values are logged at in clear.
Envoy can be configured to redact the above header values by disabling runtime feature
``envoy.reloadable_features.debug_include_sensitive_data``.
12 changes: 6 additions & 6 deletions source/common/http/header_map_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,13 +441,13 @@ size_t HeaderMapImpl::removePrefix(const LowerCaseString& prefix) {
});
}

// Forward declaration.
namespace Utility {
void dumpHeaderMap(std::ostream& os, const HeaderMap& headers, int indent_level);
}

void HeaderMapImpl::dumpState(std::ostream& os, int indent_level) const {
iterate([&os,
spaces = spacesForLevel(indent_level)](const HeaderEntry& header) -> HeaderMap::Iterate {
os << spaces << "'" << header.key().getStringView() << "', '" << header.value().getStringView()
<< "'\n";
return HeaderMap::Iterate::Continue;
});
Utility::dumpHeaderMap(os, dynamic_cast<const HeaderMap&>(*this), indent_level);
}

HeaderMapImpl::HeaderEntryImpl& HeaderMapImpl::maybeCreateInline(HeaderEntryImpl** entry,
Expand Down
107 changes: 82 additions & 25 deletions source/common/http/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

#include "source/common/buffer/buffer_impl.h"
#include "source/common/common/assert.h"
#include "source/common/common/dump_state_utils.h"
#include "source/common/common/empty_string.h"
#include "source/common/common/enum_to_int.h"
#include "source/common/common/fmt.h"
Expand Down Expand Up @@ -317,37 +318,41 @@ bool maybeAdjustForIpv6(absl::string_view absolute_url, uint64_t& offset, uint64
return true;
}

void forEachCookie(
absl::string_view cookie_header_value,
const std::function<bool(absl::string_view, absl::string_view)>& cookie_consumer) {
// Split the cookie header into individual cookies.
for (const auto& s : StringUtil::splitToken(cookie_header_value, ";")) {
// Find the key part of the cookie (i.e. the name of the cookie).
size_t first_non_space = s.find_first_not_of(' ');
size_t equals_index = s.find('=');
if (equals_index == absl::string_view::npos) {
// The cookie is malformed if it does not have an `=`. Continue
// checking other cookies in this header.
continue;
}
absl::string_view k = s.substr(first_non_space, equals_index - first_non_space);
absl::string_view v = s.substr(equals_index + 1, s.size() - 1);

// Cookie values may be wrapped in double quotes.
// https://tools.ietf.org/html/rfc6265#section-4.1.1
if (v.size() >= 2 && v.back() == '"' && v[0] == '"') {
v = v.substr(1, v.size() - 2);
}

if (!cookie_consumer(k, v)) {
return;
}
}
}

void forEachCookie(
const HeaderMap& headers, const LowerCaseString& cookie_header,
const std::function<bool(absl::string_view, absl::string_view)>& cookie_consumer) {
const Http::HeaderMap::GetResult cookie_headers = headers.get(cookie_header);

for (size_t index = 0; index < cookie_headers.size(); index++) {
auto cookie_header_value = cookie_headers[index]->value().getStringView();

// Split the cookie header into individual cookies.
for (const auto& s : StringUtil::splitToken(cookie_header_value, ";")) {
// Find the key part of the cookie (i.e. the name of the cookie).
size_t first_non_space = s.find_first_not_of(' ');
size_t equals_index = s.find('=');
if (equals_index == absl::string_view::npos) {
// The cookie is malformed if it does not have an `=`. Continue
// checking other cookies in this header.
continue;
}
absl::string_view k = s.substr(first_non_space, equals_index - first_non_space);
absl::string_view v = s.substr(equals_index + 1, s.size() - 1);

// Cookie values may be wrapped in double quotes.
// https://tools.ietf.org/html/rfc6265#section-4.1.1
if (v.size() >= 2 && v.back() == '"' && v[0] == '"') {
v = v.substr(1, v.size() - 2);
}

if (!cookie_consumer(k, v)) {
return;
}
}
forEachCookie(cookie_headers[index]->value().getStringView(), cookie_consumer);
}
}

Expand Down Expand Up @@ -1494,5 +1499,57 @@ bool Utility::isValidRefererValue(absl::string_view value) {
return true;
}

void handlePath(std::ostream& os, const HeaderString& value) {
std::string stripped = Utility::stripQueryString(value);
if (stripped != value.getStringView()) {
os << stripped << "?[redacted]";
} else {
os << value.getStringView();
}
}

void handleCookie(std::ostream& os, const HeaderString& value) {
bool first = true;
forEachCookie(value.getStringView(),
[&os, &first](absl::string_view k, absl::string_view) -> bool {
if (!first) {
os << ";";
}
first = false;
os << k << "="
<< "[redacted]";

// continue iterating until all cookies are processed.
return true;
});
}

void Utility::dumpHeaderMap(std::ostream& os, const HeaderMap& headers, int indent_level) {
if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.debug_include_sensitive_data")) {
headers.iterate([&os, spaces = spacesForLevel(indent_level)](
const HeaderEntry& header) -> HeaderMap::Iterate {
os << spaces << "'" << header.key().getStringView() << "', '"
<< header.value().getStringView() << "'\n";
return HeaderMap::Iterate::Continue;
});
} else {
headers.iterate([&os, spaces = spacesForLevel(indent_level)](
const HeaderEntry& header) -> HeaderMap::Iterate {
os << spaces << "'" << header.key().getStringView() << "', '";
if (header.key() == Http::Headers::get().Path) {
Copy link
Contributor

@yanavlasov yanavlasov May 31, 2023

Choose a reason for hiding this comment

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

This is where the spec would come in handy. Why sanitize just these headers? This change looks to be specific to your requirements. The main concern is that this code would get bigger and bigger as different people would need different headers to be sanitized.

I think the list of headers needs to be determined by configuration, which is how this feature needs to be enabled too. Runtime flag is not a good choice here.

One approach is to define a bootstrap extension for printing header map into the debug log. You can then override it for your product to sanitize the header map the way you want. You can also implement an extension that takes the list of headers that are removed from the log.

Copy link
Member Author

@tsaarni tsaarni May 31, 2023

Choose a reason for hiding this comment

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

This is where the spec would come in handy.

What type of spec could it be?

For masking three headers addressed in this PR currently, there was a need to write three different parsers: redacting query string, redacting cookie values, redacting complete header.

Would the spec then provide means for user to

  1. apply pre-defined / fixed parsers to mask given headers?
  2. add their own "custom masking" extensions written in C++ (like access log formatters)?

To me (1) sounds reasonable, although only "redact complete header" is generic enough to be applied to any random header.

I guess otherwise we end up in (2) allowing user to write application specific logic, to mask application specific headers. It sounds bit complicated and it might be unlikely people would utilize it that much. After all, masking debug log is quite a rare use case.

This change looks to be specific to your requirements.

I thought the proposed headers were the obvious ones, which would apply to most, not only my requirements.

But I agree, any X-MyExampleApp-Token type of header could contain sensitive information as well.

Copy link
Contributor

@yanavlasov yanavlasov Jun 1, 2023

Choose a reason for hiding this comment

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

Just a GitHub Issue with a short description of how a feature is going to be implemented is enough.

The key is flexibility. Envoy achieves this through extensions. I think the first step is create an extension point with API - interface with a method that takes header map as a parameter and returns formatted string. This can then plumbed into the header map.

Then you can build on top of this to add functionality that you need. I.e. list of headers that need to be redacted, boolean flag to redact query, cookies, etc. You can add regex matchers, whatever suits your purpose.

This is one of the possible solutions, other maintains may suggest simpler things. The Issue with the spec would help define the solution that meets community needs.

handlePath(os, header.value());
} else if (header.key() == Http::Headers::get().Cookie) {
handleCookie(os, header.value());
} else if (header.key() == Http::CustomHeaders::get().Authorization) {
os << "[redacted]";
} else {
os << header.value().getStringView();
}
os << "'\n";
return HeaderMap::Iterate::Continue;
});
}
}

} // namespace Http
} // namespace Envoy
8 changes: 8 additions & 0 deletions source/common/http/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -751,6 +751,14 @@ struct RedirectConfig {
std::string newUri(::Envoy::OptRef<const RedirectConfig> redirect_config,
const Http::RequestHeaderMap& headers);

/**
* Helper for printing Http::HeaderMap while redacting sensitive information.
* @param os supplies the ostream to print to.
* @param headers the headers to print.
* @param indent_level indentation level for the output.
*/
void dumpHeaderMap(std::ostream& os, const HeaderMap& headers, int indent_level);

} // namespace Utility
} // namespace Http
} // namespace Envoy
1 change: 1 addition & 0 deletions source/common/runtime/runtime_features.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ RUNTIME_GUARD(envoy_reloadable_features_append_query_parameters_path_rewriter);
RUNTIME_GUARD(envoy_reloadable_features_append_xfh_idempotent);
RUNTIME_GUARD(envoy_reloadable_features_conn_pool_delete_when_idle);
RUNTIME_GUARD(envoy_reloadable_features_count_unused_mapped_pages_as_free);
RUNTIME_GUARD(envoy_reloadable_features_debug_include_sensitive_data);
RUNTIME_GUARD(envoy_reloadable_features_dfp_mixed_scheme);
RUNTIME_GUARD(envoy_reloadable_features_enable_aws_credentials_file);
RUNTIME_GUARD(envoy_reloadable_features_enable_compression_bomb_protection);
Expand Down