sds: secret discovery service. by PiotrSikora · Pull Request #180 · envoyproxy/data-plane-api

PiotrSikora · 2017-09-27T10:52:07Z

No description provided.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2017-09-27T10:55:36Z

    // to know how to speak to the management server. These cluster definitions
    // may not use EDS (i.e. they should be static IP or DNS-based).
    repeated Cluster clusters = 2;
+    repeated Secret secrets = 3;


@htuch Thoughts on placement of repeated fields?

message StaticResources { repeated Listener listeners = 1; repeated Cluster clusters = 2; repeated Secret secrets = 3; } message Secret { TlsCertificate tls_certificate = 1; }

vs

message StaticResources { repeated Listener listeners = 1; repeated Cluster clusters = 2; Secret secrets = 3; } message Secret { repeated TlsCertificate tls_certificates = 1; }

vs

message StaticResources { repeated Listener listeners = 1; repeated Cluster clusters = 2; repeated Secret secrets = 3; } message Secret { repeated TlsCertificate tls_certificates = 1; }

Note that Secret will contain more than just TlsCertificate objects in the future.

Option 1 is cleaner, since when we have a DiscoveryResponse, it already is a repeated list of resources, with the caveat below about the recursive definition.

htuch · 2017-09-27T10:58:10Z

+  // via KDS. When name is specified, but kds_config is empty, the certificate will be loaded
+  // from static resources [V2-API-DIFF].
+  string name = 6;
+  ConfigSource kds_config = 7;


There's a recursive relation here, where a TlsCertificate can point to another one delivered by KDS and then the fetched Secret can point to another ConfigSource etc. Is the intention to support this kind of chaining? Could we instead use a flatter structure, based on oneof, to allow folks to say "this cert must be either on the filesystem, inline, or providing by KDS"?

The intention was to be able to update inlined TLS certificates via KDS in order to rotate them, but there is indeed a KDS recursion when doing that... oneof would indeed be cleaner, but it wouldn't work with the mentioned use case... Good catch, let me think about it.

There is something similar here with not allowing the bootstrap clusters to be an EDS cluster. (It makes no sense to have the EDS cluster use EDS). We could potentially just block this in code like we do there.

Yeah, looking at EDS, it has the same problem, so I'm not sure if it's worth diverging from it.

I don't have strong opinion here (but slight preference to keep it in line with EDS), so I'll leave it as-is, unless @htuch objects.

mattklein123 · 2017-09-27T17:24:33Z

+
+import "google/api/annotations.proto";
+
+service KeyDiscoveryService{


Should we just name this SecretDiscoveryService? We have SDS back now because we moved SDS -> EDS.

Seems reasonable, as long as we never require a ServiceDiscoverySecret ;)

I'm fine with either... But it might be a bit confusing, considering that we just removed old SDS.

To clarify - I need one or both of you to make a ruling on KDS vs SDS.

Let's do SDS.

mattklein123 · 2017-09-27T17:25:54Z

+  // via KDS. When name is specified, but kds_config is empty, the certificate will be loaded
+  // from static resources [V2-API-DIFF].
+  string name = 6;
+  ConfigSource kds_config = 7;


There is something similar here with not allowing the bootstrap clusters to be an EDS cluster. (It makes no sense to have the EDS cluster use EDS). We could potentially just block this in code like we do there.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2017-09-27T23:07:46Z

 message AggregatedConfigSource {
 }

+message KdsSecretConfig {


Note: this landed in base.proto and not in kds.proto, because there was a cycle between tls_context.proto and kds.proto.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

mattklein123 · 2017-09-28T02:38:32Z

+}
+
+message Secret {
+  TlsCertificate tls_certificate = 1;


Should we make this a oneof now since we might add other types of secrets in the future? Not sure if the caller would need to specify the type?

Also, doesn't this need to include name so that Envoy can match up the response secret with the requested secret?

htuch · 2017-09-28T02:46:21Z

  repeated DataSource signed_certificate_timestamp = 5;
+
+  // SDS configuration which allows to fetch and/or reload this TLS certificates.
+  SdsSecretConfig sds_secret_config = 6;


I'm wondering if we can just take existing sites where we have TlsCertificate and make them a oneof with TlsCertificate as exists today and SdsSecretConfig? Not sure if the rules for wire compatibility allow this if we preserve the existing field numbering. Can you look into this and see if it's possible? If so, we could avoid this weirdness (or make use of oneof internally within TlsCertificate to at least make clear these are mutually exclusive).

Unfortunately, we can't, since there is no such thing as repeated oneof and existing code uses repeated TlsCertificate.

If we want to add oneof in order to enforce mutually exclusive behavior, then we would need to add something like this:

message TlsCertificateSource { oneof source { TlsCertificate tls_certificate = 1; SdsSecretConfig sds_secret_config = 2; } } repeated TlsCertificateSource tls_certificate_sources = 6;

and deprecate existing repeated TlsCertificate tls_certificates (or use it as a fallback).

Alternative and backward-compatible way of adding SDS would be to add repeated SdsSecretConfig sds_secret_config along existing repeated TlsCertificate tls_certificates to the CommonTlsContext message, since in theory there can be a mix of static and SDS-provided certificates anyway, but that's a bit less clean API than the oneof approach.

Thoughts?

I'm fine with having two repeated fields, one for static and the other for dynamic resources. This is similar to what we do in bootstrap. I don't think anyone is relying on the TlsCertificate today that we know of, but there's folks who are building management servers around v2, so it might be bad form to break them given these APIs are nominally frozen, so the TlsCertificateSource option probably isn't optimal form that perspective.

htuch · 2017-09-28T02:48:28Z

 message AggregatedConfigSource {
 }

+message SdsSecretConfig {


The EDS/RDS config protos live in rds.proto/eds.proto. Could we put this in sds.proto for consistency?

Per previous comment, which disappeared during rebranding, it's added here, otherwise there is dependency cycle between sds.proto and tls_context.proto.

Perhaps we should pull tls_context.proto into sds.proto?

Ping. Should I merge tls_context.proto into sds.proto or are you OK with leaving this in base.proto?

htuch

The other alternative would be to put SdsSecreteConfig in tls_context.proto to break the cycle. That seems less disruptive, but I'm good with it all being moved into sds.proto, there is precedent for grouping messages related to an xDS API in the service.proto they are most closely related to.

htuch · 2017-10-03T01:58:58Z

  repeated DataSource signed_certificate_timestamp = 5;
+
+  // SDS configuration which allows to fetch and/or reload this TLS certificates.
+  SdsSecretConfig sds_secret_config = 6;


I'm fine with having two repeated fields, one for static and the other for dynamic resources. This is similar to what we do in bootstrap. I don't think anyone is relying on the TlsCertificate today that we know of, but there's folks who are building management servers around v2, so it might be bad form to break them given these APIs are nominally frozen, so the TlsCertificateSource option probably isn't optimal form that perspective.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

mattklein123

LGTM

kds: key discovery service.

f2337b4

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora commented Sep 27, 2017

View reviewed changes

htuch reviewed Sep 27, 2017

View reviewed changes

PiotrSikora mentioned this pull request Sep 27, 2017

Add configuration for TLS session ticket encryption key, to allow #178

Merged

mattklein123 reviewed Sep 27, 2017

View reviewed changes

review: add KdsSecretConfig.

8cf70cf

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora commented Sep 27, 2017

View reviewed changes

review: KDS is now SDS.

de3db12

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora changed the title ~~[DO NOT MERGE] kds: key discovery service.~~ sds: secret discovery service. Sep 28, 2017

mattklein123 reviewed Sep 28, 2017

View reviewed changes

htuch reviewed Sep 28, 2017

View reviewed changes

htuch reviewed Oct 3, 2017

View reviewed changes

PiotrSikora added 4 commits October 3, 2017 04:10

review: move SdsSecretConfig out of TlsCertificate.

253bf85

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: add name and type to the Secret.

44831c2

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: merge tls_context.proto into sds.proto.

00a0904

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: add comment.

90afc07

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

mattklein123 approved these changes Oct 3, 2017

View reviewed changes

htuch approved these changes Oct 3, 2017

View reviewed changes

htuch merged commit daec566 into envoyproxy:master Oct 3, 2017


		import "google/api/annotations.proto";

		service KeyDiscoveryService{

Conversation

PiotrSikora commented Sep 27, 2017

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!

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!

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!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants