add upstream info by kyessenov · Pull Request #2434 · istio/proxy

kyessenov · 2019-09-30T20:11:19Z

Signed-off-by: Kuat Yessenov kuat@google.com

Adds:

upstream.mtls a bool indicating whether client-side connection is mTLS
upstream.failure_reason a string recording connection reset log line (usually due to TLS)

/cc @nrjpoddar @douglas-reid

Signed-off-by: Kuat Yessenov <kuat@google.com>

douglas-reid · 2019-09-30T20:19:02Z

can you add detail on how these new attributes impact the calculation of the connection security policy labels, etc.?

nrjpoddar · 2019-09-30T20:20:09Z

Thanks @kyessenov for this PR. Can you add in a comment few examples of how the newly added attributes show up in mixer access logs or telemetry?

nrjpoddar · 2019-09-30T20:21:49Z

include/istio/utils/attribute_names.h

  static const char kConnectionId[];
  // Record TCP connection status: open, continue, close
  static const char kConnectionEvent[];
+  static const char kUpstreamMtls[];


how does work with this attribute kConnectionMtls

connection.mtls means downstream connection mTLS status.
upstream.mtls means upstream connection mTLS status.
Depending on whether the proxy acts like an outbound or an inbound sidecar, you use one or the other.

i see, may be this attribute kConnectionMtls needs to explicitly say that it's related to downstream. It's can be confusing specially after adding this new attribute.

We can't change the meaning of existing attributes. There are UIs that have expectations. We can change the metric, however, in istio config.

kyessenov · 2019-09-30T20:26:45Z

@douglas-reid

We should modify the mtls label to be conditional(context.reporter.kind == "client", upstream.mtls | false, connection.mtls | false).
Failure reason is more useful in access logs, since we don't have a good catalog of errors yet.

nrjpoddar

What are the types of error messages that can be reported via GetUpstreamFailureReason? I'm worried about cardinality here if we add this on a metric specially if it is directly tied to the Envoy message.

One way to deal with it would be to create known types based of the set of error messages we care about/know in proxy repo and then skip the other kinds to decouple label cardinality explosion from Envoy string message directly.

nrjpoddar · 2019-09-30T20:32:09Z

include/istio/utils/attribute_names.h

  static const char kConnectionId[];
  // Record TCP connection status: open, continue, close
  static const char kConnectionEvent[];
+  static const char kUpstreamMtls[];


i see, may be this attribute kConnectionMtls needs to explicitly say that it's related to downstream. It's can be confusing specially after adding this new attribute.

kyessenov · 2019-09-30T20:49:02Z

@nrjpoddar yes, we'll have to update the attribute vocabulary once this gets in. I think this is better than changing the meaning of the attribute. It was always about downstream, just never properly documented.

Upstream failure reason is coming straight from boringSSL. Yes, it should not be used for metrics, unless we can catalog all the error kinds in the library.

nrjpoddar · 2019-09-30T20:57:15Z

Upstream failure reason is coming straight from boringSSL. Yes, it should not be used for metrics, unless we can catalog all the error kinds in the library. -> I can work with you if needed and we can divide and conquer. Getting this information in metrics, even at low resolution where all SSL related errors are reported as boolean is very useful. An alert via metrics + more information in logs/tracing will be an acceptable short time solution for me.

kyessenov · 2019-09-30T21:00:30Z

@nrjpoddar Sure. I still think we need to propagate full-fidelity info from envoy, in case we get our catalog wrong, or there is a need for more details. As a first pass, we should use a CEL expression (upstream.failure_reason.startsWith("blah")) as a rough error type metric.

kyessenov · 2019-10-02T16:49:35Z

Hey @PiotrSikora , as a WG lead, can you review or assign someone to review? We need this for 1.4 in case Mixer v2 slips.

rshriram · 2019-10-02T18:02:36Z

src/istio/control/http/attributes_builder.cc

  builder.FlattenMapOfStringToStruct(report_data->GetDynamicFilterState());
+
+  builder.AddBool(utils::AttributeName::kUpstreamMtls,
+                  report_data->IsUpstreamMutualTLS());


just a nit. instead of having a bool for mutual TLs, why not have an enum of sorts? upstream: plaintext, tls, mutualTLS ? We are coming across a lot of setups where the sidecar has to terminate one way TLS on VMs.

Enums are generally painful to deal with in proto. mTLS is special since that's the end state, so it's needed to track the progress on getting to that state. We can populate other TLS properties later (e.g. version, cipher suite ID, etc).

lizan · 2019-10-02T18:29:48Z

src/envoy/utils/utils.h

                    std::string* trust_domain);

-// Returns true if connection is mutual TLS enabled.
+// Returns true if downstream connection is mutual TLS enabled.


rename this to IsDownstreamMutualTLS?

nrjpoddar · 2019-10-02T18:31:42Z

src/envoy/utils/utils.cc


+bool IsUpstreamMutualTLS(const StreamInfo::StreamInfo& stream_info) {
+  const auto ssl = stream_info.upstreamSslConnection();
+  return ssl != nullptr && ssl->peerCertificatePresented();


I'm a little confused on the usage of ssl->peerCertificatePresented(). If this flag is set to true, does this imply client was using TLS for talking to upstream and got a successful certificate from upstream or it should mean that client is using TLS irrespective of how the negotiation went and what upstream server presented?

Ouch, I think this might be an issue. We should be checking whether a local certificate was presented, not peer.

that's what I thought. I don't see an Envoy API for that. @lizan may be you know better?

We can extend envoy to report whether a local certificate is presented. That might not mean that the server validated it. Not sure how to get that info.

we didn't have this in Envoy API because this has been simply reflecting what in the TLS context config, though we're adding more dynamic nature to the config for incremental adoption etc, so it makes sense to add, thanks for posting envoyproxy/envoy#8464.

nrjpoddar · 2019-10-02T18:40:03Z

@kyessenov in general, from a user point of view I only care about if the connection over which this transaction is happening is plaintext, TLS or mTLS. In all of these states, I care about errors if any occur.

I'm struggling to see how users can get benefit from this low level separation of upstream/downstream without getting more confused.

kyessenov · 2019-10-02T18:43:02Z

@nrjpoddar We cannot conflate the two for gateways. We only have one set of attributes we report from the gateways, and downstream/upstream connections have very different characteristics.

istio-testing · 2019-10-25T20:14:57Z

@kyessenov: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/proxy-presubmit-release.sh	`1939596`	link	`/test proxy-presubmit-release`

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

istio-policy-bot · 2019-11-16T00:02:34Z

🧭 This issue or pull request has been automatically marked as stale because it has not had activity from an Istio team member since 2019-10-02. It will be closed on 2019-12-01 unless an Istio team member takes action. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

istio-policy-bot · 2019-11-17T00:02:26Z

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2019-10-02. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

add upstream tls info

1939596

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov requested a review from a team September 30, 2019 20:11

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Sep 30, 2019

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 30, 2019

nrjpoddar reviewed Sep 30, 2019

View reviewed changes

rshriram reviewed Oct 2, 2019

View reviewed changes

lizan reviewed Oct 2, 2019

View reviewed changes

nrjpoddar reviewed Oct 2, 2019

View reviewed changes

nrjpoddar mentioned this pull request Oct 2, 2019

Istio metrics for TLS (including mTLS) related errors istio/istio#13791

Closed

kyessenov mentioned this pull request Oct 2, 2019

tls: add local certificate presented flag for mTLS detection envoyproxy/envoy#8464

Closed

istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Nov 2, 2019

istio-policy-bot closed this Nov 17, 2019

istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label Nov 17, 2019

Conversation

kyessenov commented Sep 30, 2019

Uh oh!

douglas-reid commented Sep 30, 2019

Uh oh!

nrjpoddar commented Sep 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Sep 30, 2019

Uh oh!

nrjpoddar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Sep 30, 2019

Uh oh!

nrjpoddar commented Sep 30, 2019

Uh oh!

kyessenov commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyessenov commented Oct 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrjpoddar commented Oct 2, 2019

Uh oh!

kyessenov commented Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-testing commented Oct 25, 2019

Uh oh!

istio-policy-bot commented Nov 16, 2019

Uh oh!

istio-policy-bot commented Nov 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

kyessenov Sep 30, 2019 •

edited

Loading

kyessenov commented Sep 30, 2019 •

edited

Loading

kyessenov commented Oct 2, 2019 •

edited

Loading