Skip to content

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

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
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
178 changes: 96 additions & 82 deletions routing/v1alpha2/destination_rule.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 20 additions & 10 deletions routing/v1alpha2/destination_rule.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ message OutlierDetection {
// privateKey: /etc/certs/client_private_key.pem
// caCertificates: /etc/certs/rootcacerts.pem
//
// The following rule configures a client to use TLS when talking to a foreign service whose domain matches *.foo.com.
// The following rule configures a client to use TLS when talking to an external service whose domain matches *.foo.com.
//
// apiVersion: config.istio.io/v1alpha2
// kind: DestinationRule
Expand All @@ -393,19 +393,29 @@ message OutlierDetection {
message TLSSettings {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 ? ).

Copy link
Contributor

@ZackButcher ZackButcher Jan 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

// TLS connection mode
enum TLSmode {
// If set to "disable", the proxy will use not setup a TLS connection to the
// upstream server.
DISABLE = 0;

// Placeholder
TLSMODE_INVALID = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ZackButcher ZackButcher Jan 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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)..

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


// 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@louiscryan louiscryan Jan 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@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}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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
...


// If set to "simple", the proxy will originate a TLS connection to the
// upstream server.
SIMPLE = 1;
// upstream server. This is the default mode for external services with
// missing port specification.
SIMPLE = 2;

// If set to "mutual", the proxy will secure connections to the
// upstream using mutual TLS by presenting client certificates for
// authentication.
MUTUAL = 2;
};
// authentication. The certificates should be specified using the
// configuration fields (clientCertificate, privateKey).
MUTUAL = 3;

// If set to "disable", the proxy will use not setup a TLS connection
Copy link
Contributor

Choose a reason for hiding this comment

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

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

// to the upstream server.
DISABLE = 4;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: wrong indentation here


// REQUIRED: Indicates whether connections to this port should be secured
// using TLS. The value of this field determines how TLS is enforced.
Expand Down
Loading