Fix default tls mode for destination and add 80-443 mapping for ext svc #314

rshriram · 2018-01-19T21:09:06Z

No description provided.

ZackButcher · 2018-01-19T21:10:57Z

routing/v1alpha2/destination_rule.proto

-    DISABLE = 0;
-
+    // Placeholder
+    TLSMODE_INVALID = 0;


I think we should keep DISABLE the default (or change it to PASSTHROUGH). Either way, IMO our default behavior should be to not touch the connection the app set up.

There's also no reason to add an invalid value to our enum when there's a reasonable default value we can use instead.

Right, but that is covered right? I can omit TLSmode completely, in which case, the semantics is keep things as is.. if I specify a TLS mode, I have to pick something or other (certificates, modes, etc..). Disable implies do not setup a TLS context in the upstream cluster.. I felt that it was a bit dangerous to leave as default.

Actually, to that point: is DISABLED even needed? If you're providing certs, you probably don't want TLS disabled. If you're not providing certs, there's no reason for a TLSOptions config at all, right?

Yeah true. I was looking at it from a disable istio_mtls thing.. So that we have the ability to control the mTLS setting on a service by service basis. But may be this should not get into route rule in the first place. Should be kept strictly separate and let admins tackle that.. From that point, i think even specifying Istio as an mTLS option here seems like conflating auth and routing (I don't want to inherit RBAC related issues inside route rule semantics)..

If Istio is turning on mTLS by default with it's own provided certificates, isn't mode: DISABLED needed so the user can turn it off for a specific destination? Just omitting the TLSOptions will leave it as is. I don't see, however, why ISTIO needs to be called out as a specific option. It seems to me that it's the same as MUTUAL, just without user provided certificate. Am I missing something?

You are right in that ISTIO implies mTLS. But I was trying to avoid this implicit inference that if its mTLS it will be istio, unless you specify your own certs. (for what its worth, I could turn off istio auth completely). In that case, having a distinct ISTIO_MTLS is more clearer than simply mTLS. But that may be me reading too much into this. I am open to any option here.

ZackButcher · 2018-01-19T21:11:41Z

routing/v1alpha2/destination_rule.proto

+    // If set to "disable", the proxy will use not setup a TLS connection
+    // to the upstream server.
+    DISABLE = 4;
+};


nit: wrong indentation here

frankbu · 2018-01-19T21:20:59Z

routing/v1alpha2/destination_rule.proto

+    // If set to "istio", the proxy will set up mutual TLS authentication
+    // using Istio CA issued certificates. This is the default when Istio
+    // mTLS authentication is enabled mesh-wide.
+    ISTIO = 1;


Do we need an explicit value for this? Could we instead use MUTUAL and say that this is what happens if clientCertificate and privateKey are not set?

@wattli @wattli

I like the idea of using the enum to distinguish between 'mutual using local mesh identity' and 'mutual using specific key'. At some point we'll likely end up having to support 'mutual with specific mesh identity' when our identity management system allows for identity delegation.

We can just have the enum be "MUTUAL" but then we have to push the other options down into a one-of.

oneof {
ClientCert { client cert & provate key}
Identity "self" | {other user identity that can be asserted}
}

Sorry just saw this message, right, current setup is more extendable, but maybe we can give a more meaningful name. What about

MUTUAL_TLS_ISTIO_CERT
TLS
MUTUAL_TLS_CLIENT_CERT
...

ZackButcher · 2018-01-19T21:15:55Z

routing/v1alpha2/external_service.proto

@@ -54,6 +54,33 @@ option go_package = "istio.io/api/routing/v1alpha2";
 //       tls:
 //         mode: simple # initiates HTTPS when talking to example.com
 //
+// To reduce the configuration burden for the common case where most
+// external services are accessed over port 443, there is an implicit
+// internal mapping from port 80 to port 443. Such that an external service


~~Such that~~ An external service declared without any port is assumed...

ZackButcher · 2018-01-19T21:18:16Z

routing/v1alpha2/external_service.proto

+//
+// The application is expected to access these services over port 80 using
+// plain text http (i.e. http://foo.com, http://scooby.com, etc.). The
+// sidecar would originate TLS connections to the external service over


The sidecar will originate TLS connections to the external service on port 443

Unless we set it otherwise ? Not all https servers are on 443.

ZackButcher · 2018-01-19T21:18:50Z

routing/v1alpha2/external_service.proto

+// The application is expected to access these services over port 80 using
+// plain text http (i.e. http://foo.com, http://scooby.com, etc.). The
+// sidecar would originate TLS connections to the external service over
+// port 443 (i.e. https://foo.com, https://bar.com). Note that currently,


Nit: no comma after currently, I think it reads fine w/o it:

Note that currently this special case functionality is only available for non-wildcard domains.

ZackButcher · 2018-01-19T21:19:14Z

routing/v1alpha2/external_service.proto

+// plain text http (i.e. http://foo.com, http://scooby.com, etc.). The
+// sidecar would originate TLS connections to the external service over
+// port 443 (i.e. https://foo.com, https://bar.com). Note that currently,
+// this special case functionality is available only for non-wildcard


"only available" instead of "available only" reads better IMO.

ZackButcher · 2018-01-19T21:23:13Z

routing/v1alpha2/external_service.proto

@@ -174,7 +201,11 @@ message ExternalService {
  // the specified destination endpoint IP/host.
  repeated string hosts = 1;

-  // REQUIRED. The ports associated with the external service.
+  // The ports associated with the external service. If omitted, service


The ports associated with the external service. If no ports are provided, the external service is assumed to be reachable over HTTPS on port 443. Applications calling the external service over HTTP on port 80 will be automatically forwarded to the external destination over HTTPS on port 443 by the sidecar.

rshriram · 2018-01-19T21:26:00Z

@ijsnellf feels that this is going to lead to trouble. His point of view is that the following is not so terrible after all.

kind: ExternalService
hosts:
- simpleapi.google.com #can't do wildcards if we want port remapping
ports:
- number: 80
   protocol: http
- number: 443
   protocol: https
---
kind: RouteRule
hosts:
- simpleapi.google.com
http:
- match:
   - port:
         number: 80
   route:
   - destination:
          name: simpleapi.google.com
          port: 443
---
kind: DestinationRule
name: simpleapi.google.com
http:
- port: 443
   tls:
      mode: simple
---

If we get the ability to rewrite ports for wildcards as well, we can avoid repetition of rules, by lumping lots of domains (e.g., *.watson.net) under a single combo of ext service, route rule, dest rule as shown above.

costinm · 2018-01-27T02:23:45Z

routing/v1alpha2/destination_rule.proto

@@ -393,19 +393,29 @@ message OutlierDetection {
 message TLSSettings {


One small issue: if destination is *.foo.com, I'm not sure how SNI and SAN will be specified... Maybe indicate in the doc that if SNI or SAN is needed, you need individual hosts, no wildcard ?

Also, once Piotr has the SNI sniffing - we may want to add mode=TRANSPARENT, in which case the client will initiate
a https connection ( all other modes assume client connects to http://foo.com:443 - I assume ? ).

This is covered by the SAN and SNI fields on this object:

api/routing/v1alpha2/destination_rule.proto

Lines 437 to 443 in 911b3fc

// A list of alternate names to verify the subject identity in the

// certificate. If specified, the proxy will verify that the server

// certificate's subject alt name matches one of the specified values.

repeated string subject_alt_names = 5;

// SNI string to present to the server during TLS handshake.

string sni = 6;

We should update the comment on the SNI field to make clear that it must be a a subset of the hosts in the destination.

costinm · 2018-01-27T02:24:52Z

routing/v1alpha2/external_service.proto

@@ -37,7 +37,7 @@ option go_package = "istio.io/api/routing/v1alpha2";
 //       hosts:
 //       - example.com
 //       ports:
-//       - number: 443
+//       - number: 9443


While 9443 is better then 443 - it may be better to avoid the 443 completely, still red flags... 9080 ?

louiscryan · 2018-01-28T21:06:01Z

routing/v1alpha2/destination_rule.proto

+    // configuration fields (clientCertificate, privateKey).
+    MUTUAL = 3;
+
+    // If set to "disable", the proxy will use not setup a TLS connection


doc is unclear. I tend to agree with Zack on PASSTHROUGH

* Basic integration test * Addressed comments

Automatic merge from submit-queue. [DO NOT MERGE] Auto PR to update dependencies of mixerclient This PR will be merged automatically once checks are successful. ```release-note none ```

80-443 default

911b3fc

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jan 19, 2018

rshriram requested review from ZackButcher and frankbu January 19, 2018 21:09

ZackButcher reviewed Jan 19, 2018

View reviewed changes

frankbu reviewed Jan 19, 2018

View reviewed changes

ZackButcher reviewed Jan 19, 2018

View reviewed changes

costinm reviewed Jan 27, 2018

View reviewed changes

louiscryan reviewed Jan 28, 2018

View reviewed changes

rshriram closed this Apr 27, 2018

rkpagadala deleted the exnits branch May 11, 2018 21:59

incfly pushed a commit to incfly/api that referenced this pull request Jun 13, 2018

transcoding: Basic integration test (istio#314)

658c103

* Basic integration test * Addressed comments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix default tls mode for destination and add 80-443 mapping for ext svc #314

Fix default tls mode for destination and add 80-443 mapping for ext svc #314

rshriram commented Jan 19, 2018

ZackButcher Jan 19, 2018

ZackButcher Jan 19, 2018

rshriram Jan 19, 2018

ZackButcher Jan 19, 2018 •

edited

Loading

rshriram Jan 19, 2018

frankbu Jan 19, 2018

rshriram Feb 13, 2018

ZackButcher Jan 19, 2018

frankbu Jan 19, 2018

louiscryan Jan 28, 2018 •

edited

Loading

wattli Feb 23, 2018

ZackButcher Jan 19, 2018

ZackButcher Jan 19, 2018

costinm Jan 27, 2018

ZackButcher Jan 19, 2018

ZackButcher Jan 19, 2018

ZackButcher Jan 19, 2018

rshriram commented Jan 19, 2018

costinm Jan 27, 2018

ZackButcher Jan 30, 2018 •

edited

Loading

costinm Jan 27, 2018

louiscryan Jan 28, 2018

		@@ -393,19 +393,29 @@ message OutlierDetection {
		message TLSSettings {

	// A list of alternate names to verify the subject identity in the
	// certificate. If specified, the proxy will verify that the server
	// certificate's subject alt name matches one of the specified values.
	repeated string subject_alt_names = 5;

	// SNI string to present to the server during TLS handshake.
	string sni = 6;

Fix default tls mode for destination and add 80-443 mapping for ext svc #314

Fix default tls mode for destination and add 80-443 mapping for ext svc #314

Conversation

rshriram commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZackButcher Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louiscryan Jan 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshriram commented Jan 19, 2018

Choose a reason for hiding this comment

ZackButcher Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZackButcher Jan 19, 2018 •

edited

Loading

louiscryan Jan 28, 2018 •

edited

Loading

ZackButcher Jan 30, 2018 •

edited

Loading