api: enhance v3 CommonTlsContext for agentless support by sanjaypujare · Pull Request #11061 · envoyproxy/envoy

sanjaypujare · 2020-05-05T04:42:29Z

Signed-off-by: Sanjay Pujare sanjaypujare@users.noreply.github.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message: api: enhance v3 CommonTlsContext for agentless support
Additional Description: add protos/fields needed to support agentless (without SDS) secret access
Risk Level: low
Testing: pre-submit and do_ci fix format checks
Docs Changes: none
Release Notes: -NA-
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

@htuch @costinm @markdroth PR for agentless secrets as discussed.

repokitteh-read-only · 2020-05-05T04:42:34Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/.

🐱

Caused by: #11061 was opened by sanjaypujare.

see: more, trace.

htuch

Thanks, some comments to get started.
/wait

htuch · 2020-05-05T14:06:49Z

When https://github.com/envoyproxy/envoy/pull/10908/files#r420136445 lands, we should switch to this here. This should happen before this PR merges.

Okay, will use TypedExtensionConfig instead of Any once that one is in.

ggreenway · 2020-05-05T16:07:41Z

Can you please elaborate on the description of this change? It sounds like there was a previous conversation related to this, but for people (such as myself) that weren't part of the conversation, it isn't clear what the purpose of this change is.

sanjaypujare · 2020-05-05T16:24:56Z

Can you please elaborate on the description of this change? It sounds like there was a previous conversation related to this, but for people (such as myself) that weren't part of the conversation, it isn't clear what the purpose of this change is.

Sure. gRPC recently added xDS support for load balancing and other routing features. This enables users to create proxyless service meshes. However for transport security (TLS/mTLS etc) the xDS protocol assumes the presence of SDS servers (for example Istio Agent or Node agent). To make the solution agentless as well as proxyless we are considering an SDS-server-less approach where we bypass the SDS server and have a CA-client in gRPC directly to talk to a service mesh CA to acquire certificates. This PR is related to that. Hope that answers your question.

ggreenway · 2020-05-05T16:38:12Z

Thank you, that's very helpful context.

So is this logically similar to talking to an SDS server, but with a different, previously existing/implemented protocol?

sanjaypujare · 2020-05-05T17:18:08Z

Thank you, that's very helpful context.

So is this logically similar to talking to an SDS server, but with a different, previously existing/implemented protocol?

In case of SDS, the SDS server creates the CSR and returns cert + private key to the client (Envoy) . With the CA client approach, the client (Envoy) creates the CSR so private key is not shared with anybody.

muxi · 2020-05-05T17:50:36Z

Should this be inside oneof validation_context_type and be mutually exclusive with SdsSecretConfig validation_context_sds_secret_config = 7;?

Good catch. Yes it should be moved under oneof validation_context_type. Once moved there it will be mutually exclusive with all the other members in there not just validation_context_sds_secret_config. I assume there is no backward compatibility issue with adding a new member to a "oneof"

Cool. And maybe the one above (CAClientPluginConfig tls_certificate_ca_client_plugin = 9) also needs to be mutually exclusive with repeated SdsSecretConfig tls_certificate_sds_secret_configs = 6. I'm not very sure though since tls_certificate_sds_secret_configs is a repeated field. Maybe Envoy meant to support fetching tls certificate from multiple sources; in that case the current way is probably ok as it just adds one more source to the list.

The main problem here is backward compatibility: moving an existing field (tls_certificate_sds_secret_configs) to oneof creates backward compatibility issues so we decided against it. However this might be a good candidate for the field_migrate annotations. @htuch do you agree?

Just use the oneof promotion annotation for that, document in the interim.

easwars · 2020-05-05T21:40:16Z

Does it make sense to make the different fields for getting the tls_certificates also a oneof? If so, could we mark them with appropriate annotation to promote to oneof as well.

Yes, I think I will go ahead and just do that for all oneof candidates.

Thinking more about it, I see that they already had oneof validation_context_type to select only one among the 3 existing options (before I added the 4th one). But they didn't do the same for tls_certificates and tls_certificate_sds_secret_configs which could have been put in oneof.

@htuch could you throw some light on this? Should my field_migrate.oneof_promotion annotation include all 3 (tls_certificates, tls_certificate_sds_secret_configs and tls_certificate_ca_client_plugin) or just the last 2 or you think oneof is not warranted here? And the reason for that?

markdroth · 2020-05-05T22:49:56Z

I think this PR is premature. I don't think we have enough consensus yet on exactly how this should work. I think we need to finalize our design before we add anything to the xDS API.

costinm · 2020-05-06T15:26:27Z

One of the problems with SDS was the verbose config - if you have 5000 services you'll need to duplicate that message for each cluster and listener. It would be ideal to not repeat it :-)

There are discussions about better naming and referencing of resources - could this be expressed as just a reference (the name of the provider) ? For most servers there will be a single 'plugin config' - maybe few, but not one per cluster and listener.

That's a good point and I have thought about it too but it's not easy to do it in a stateless way. If you have any ideas that do not impose any architectural requirements, let me know.

What do others think about creating a new resource type just for this? That way the config is available as a resource and its name can be referenced here. I would prefer not to do it in this PR.

Yeah, let's punt this to another PR. Ideally we motivate any additional API decomposition with concrete use cases.

htuch · 2020-05-06T15:27:07Z

Agreed with @markdroth; can we have a public design document describing how this will work? Thanks.

sanjaypujare · 2020-05-06T16:59:00Z

Agreed with @markdroth; can we have a public design document describing how this will work? Thanks.

Sure, I'll create a design doc and attach to this PR. Hopefully that will help us reach broad consensus.

mattklein123 · 2020-05-06T17:23:10Z

FWIW I'm definitely in favor of doing this change. I have been thinking about it for a while. It will make it much easier to natively implement ACME (#96) as an extension. We need to think about async reload support for these extensions also, so there is some plumbing overlap with SDS.

sanjaypujare · 2020-05-06T17:50:38Z

FWIW I'm definitely in favor of doing this change. I have been thinking about it for a while. It will make it much easier to natively implement ACME (#96) as an extension. We need to think about async reload support for these extensions also, so there is some plumbing overlap with SDS.

@mattklein123 thanks. Can you explain what you mean by "async reload" support and "plumbing overlap with SDS" in this context? We have used "async reload" at the transport/handshake level where a cert is acquired dynamically and asynchronously for a new (m)TLS connection. But this is implied in xDS via the *TlsContext fields for clusters/listeners and that semantic is not changed by this proposal.

mattklein123 · 2020-05-06T18:04:39Z

Can you explain what you mean by "async reload" support and "plumbing overlap with SDS" in this context?

Sorry I just meant that we need to make sure that the Envoy side implementation supports async fetch from the extension. This would allow certificates to be fetched over the network, refreshed, etc., much like how SDS works.

Optimally, SDS would be implemented in a future version as an extension, but I think that is out of scope for this set of changes.

sanjaypujare · 2020-05-06T21:04:59Z

Is there a preferred format for a design doc? Is Google doc okay?

sanjaypujare · 2020-05-07T17:48:52Z

The requested design doc is here https://docs.google.com/document/d/1A1_QVCrfwgkFY2YxNqbVe_eKt54kkJ6-iQq5EA_QaNc/edit?usp=sharing

ggreenway · 2020-05-07T18:49:18Z

Please make the doc world-readable.

sanjaypujare · 2020-05-07T20:11:26Z

Please make the doc world-readable.

Just did. I had to make a copy to do that. I edited my comment to include the new link.

mattklein123 · 2020-05-07T22:46:01Z

@sanjaypujare sorry can you please make it world commentable?

sanjaypujare · 2020-05-07T22:49:02Z

@sanjaypujare sorry can you please make it world commentable?

Done.

ggreenway · 2020-05-08T18:08:43Z

/wait

sanjaypujare · 2020-05-08T18:58:33Z

/wait

I notice this added the "waiting" label - curious what it signifies.

ggreenway · 2020-05-08T19:33:56Z

/wait

I notice this added the "waiting" label - curious what it signifies.

It allows filtering PRs by label. PRs with the waiting label are waiting for something and so they don't need immediate review.

Pushing a commit automatically clears the label.

In this case, since the current work is happening in the linked design doc, I added the waiting label here.

sanjaypujare · 2020-05-11T06:39:49Z

@htuch and others: I have addressed your comments in the design doc as well as in the PR.

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

sanjaypujare · 2020-05-19T22:14:57Z

And I think this PR should be ready to merge? It looks like there is more interest in the edge case so I am looking into it and may add a para or 2 in the design doc.

Yes seems fine to move forward and merge. Can you merge master to fix CI and check for merge conflicts?

@mattklein123 addressed the pending comments and rebased to master. Hopefully all okay now.

mattklein123

Thanks, LGTM with small PGV comments. @htuch @markdroth any further comments?

/wait

mattklein123 · 2020-05-20T16:07:50Z

+  message CertificateProvider {
+    // opaque name used to specify certificate instances or types. For example, "ROOTCA" to specify
+    // a root-certificate (validation context) or "TLS" to specify a new tls-certificate.
+    string name = 1;


Should this be required by validation?

yes it should be required. Adding that.

mattklein123 · 2020-05-20T16:08:10Z

+    // to maintain a single certificate-provider instance. The sharing can happen, for
+    // example, among multiple clusters or between the tls_certificate and validation_context
+    // certificate providers of a cluster.
+    oneof config {


Can you add a required validation to the oneof?

markdroth · 2020-05-20T16:40:43Z

I'm still a little nervous that we haven't fleshed out the details for ACME, but if Matt is confident enough that this API will work for that when the time comes, then I'm happy with it as well.

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

mattklein123 · 2020-05-20T22:45:17Z

Can you check CI?

/wait

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

htuch

LGTM modulo two tiny docs comments.

htuch · 2020-05-21T02:12:51Z

-    SdsSecretConfig validation_context_sds_secret_config = 2
-        [(validate.rules).message = {required: true}];
+    // Config for fetching validation context via SDS API. Note SDS API allows certificates to be
+    // fetched/refreshed over the network "asynchronously" with respect to the TLS handshake.


Nit: why quotes around "asynchronously"? This seems to imply it's not really async?

removed quotes

htuch · 2020-05-21T02:14:03Z

+      (udpa.annotations.field_migrate).oneof_promotion = "dynamic_validation_context"
+    ];
+
+    // Certificate provider for fetching validation context


For documentation purposes, best to spell out the mutex relationship between the SDS and certificate provider. The oneof_promotion annotations are removed during doc generation.

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

sanjaypujare · 2020-05-21T03:50:12Z

Can you check CI?

/wait

fixed the issue. Hope it can be merged now

htuch

LGTM

This is a follow up from envoyproxy#11061 which added support for sending arbitrary certificateProvider plugin configuration. Signed-off-by: Easwar Swaminathan <easwars@google.com>

repokitteh-read-only Bot added the api label May 5, 2020

alyssawilk assigned ggreenway May 5, 2020

htuch suggested changes May 5, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label May 5, 2020

repokitteh-read-only Bot removed the waiting label May 5, 2020

muxi reviewed May 5, 2020

View reviewed changes

easwars reviewed May 5, 2020

View reviewed changes

costinm reviewed May 6, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label May 8, 2020

sanjaypujare added 9 commits May 19, 2020 15:03

api: address review comments - 2nd set

097d099

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

api: fix field numbering

ed9d51f

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

api: add field_migrate.oneof_promotion annotation

afffb45

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

api: migrate from google.protobuf.Any to the new TypedExtensionConfig

90c2424

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

api: rename CAClientPluginConfig to CertificateProvider

6bc63f8

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

api: fix typo in comment

cd60ee9

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

api: clarify comments based on feedback

2d93cfe

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

api: fix typo

d09a366

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

api: add oneof to support a future named resource

71b3508

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

sanjaypujare force-pushed the add-agentless-fields branch from 27dc9dd to 71b3508 Compare May 19, 2020 22:12

mattklein123 requested changes May 20, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label May 20, 2020

api: add the required validate rules

94ce3fb

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

repokitteh-read-only Bot removed the waiting label May 20, 2020

repokitteh-read-only Bot added the waiting label May 20, 2020

api: single file change for correct validate.rules

f3be260

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

repokitteh-read-only Bot removed the waiting label May 21, 2020

htuch reviewed May 21, 2020

View reviewed changes

api: address the minor comment related issues

8e09f75

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

mattklein123 approved these changes May 21, 2020

View reviewed changes

repokitteh-read-only Bot removed the api label May 21, 2020

htuch approved these changes May 21, 2020

View reviewed changes

mattklein123 merged commit 639c226 into envoyproxy:master May 21, 2020

markdroth mentioned this pull request Jul 22, 2020

api: Add CertificateProviderInstance to CommonTlsContext. #12237

Merged

markdroth mentioned this pull request Jan 4, 2022

Implement CertificateProvider mechanism #19308

Closed

Conversation

sanjaypujare commented May 5, 2020

Uh oh!

repokitteh-read-only Bot commented May 5, 2020

Uh oh!

htuch left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ggreenway commented May 5, 2020

Uh oh!

sanjaypujare commented May 5, 2020

Uh oh!

ggreenway commented May 5, 2020

Uh oh!

sanjaypujare commented May 5, 2020

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!

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!

Uh oh!

markdroth commented May 5, 2020

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 commented May 6, 2020

Uh oh!

sanjaypujare commented May 6, 2020

Uh oh!

mattklein123 commented May 6, 2020

Uh oh!

sanjaypujare commented May 6, 2020

Uh oh!

mattklein123 commented May 6, 2020

Uh oh!

sanjaypujare commented May 6, 2020

Uh oh!

sanjaypujare commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggreenway commented May 7, 2020

Uh oh!

sanjaypujare commented May 7, 2020

Uh oh!

mattklein123 commented May 7, 2020

Uh oh!

sanjaypujare commented May 7, 2020

Uh oh!

ggreenway commented May 8, 2020

Uh oh!

sanjaypujare commented May 8, 2020

Uh oh!

ggreenway commented May 8, 2020

sanjaypujare commented May 7, 2020 •

edited

Loading