Skip to content

Warn user of using mTLS PERMISSIVE mode and suggest to upgrade to STRICT mode #2114

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

Merged
wenchenglu merged 3 commits into from
Feb 12, 2019
Merged

Warn user of using mTLS PERMISSIVE mode and suggest to upgrade to STRICT mode #2114

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f259047
Warn user of using mTLS PERMISSIVE mode and suggest to upgrade to STR…
yangminzhu Feb 11, 2019
c418d49
fix format
yangminzhu Feb 11, 2019
3581a84
check in constructor
yangminzhu Feb 12, 2019
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
21 changes: 20 additions & 1 deletion src/envoy/http/authn/http_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,26 @@ namespace Istio {
namespace AuthN {

AuthenticationFilter::AuthenticationFilter(const FilterConfig& filter_config)
: filter_config_(filter_config) {}
: filter_config_(filter_config) {
for (const auto& method : filter_config.policy().peers()) {
Copy link

@duderino duderino Feb 12, 2019

Choose a reason for hiding this comment

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

can we wrap this entire loop with something that logs every 1000 calls?

Copy link
Contributor Author

@yangminzhu yangminzhu Feb 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This is already logging at low frequency. Most users only have 1 method in peers TLS related setting. Also note per @PiotrSikora's comment, I have moved this from the data path to the control path, it should only log when a new authN policy is first initialized and won't trigger for any requests. Is this enough to leave it as-is?

Copy link
Contributor

@PiotrSikora PiotrSikora Feb 12, 2019

Choose a reason for hiding this comment

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

Oh, I didn't notice that you moved this to control path. This should be fine, then. Thanks!

Copy link

@duderino duderino Feb 13, 2019

Choose a reason for hiding this comment

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

yeah, SGTM to me too

Copy link

@duderino duderino Feb 21, 2019

Choose a reason for hiding this comment

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

And apparently it wasn't: istio/istio#11925

Copy link
Contributor Author

@yangminzhu yangminzhu Feb 21, 2019

Choose a reason for hiding this comment

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

Yeah, it turns out Envoy actually allocate the filter for every request, so putting it in the constructor doesn't make it on the data path. I moved it to the filter factory createFilterFactory() (#2125) and confirmed it's only called when a new config is received by Envoy, not on every request.

switch (method.params_case()) {
case iaapi::PeerAuthenticationMethod::ParamsCase::kMtls:
if (method.mtls().mode() == iaapi::MutualTls_Mode_PERMISSIVE) {
ENVOY_LOG(
warn,
"mTLS PERMISSIVE mode is used, connection can be either "
"plaintext or TLS, and client cert can be omitted. "
"Please consider to upgrade to mTLS STRICT mode for more secure "
"configuration that only allows TLS connection with client cert. "
"See https://istio.io/docs/tasks/security/mtls-migration/");
return;
}
break;
default:
break;
}
}
}

AuthenticationFilter::~AuthenticationFilter() {}

Expand Down