api: Add CertificateProviderInstance to CommonTlsContext. #12237

markdroth · 2020-07-22T23:12:57Z

Signed-off-by: Mark D. Roth roth@google.com

Commit Message: api: Add CertificateProviderInstance to CommonTlsContext.
Additional Description: This is similar to the CertificateProvider API added in #11061, but it allows the actual plugin instances to be configured on the client side.
Risk Level: Low
Testing: N/A
Docs Changes: Included in PR
Release Notes: N/A

CC @htuch
@jiangtaoli2016 @yihuazhang
@jaychenatr @karthikbox
@costinm @louiscryan
@easwars @sanjaypujare @ejona86 @dfawley @yashykt @srini100

Signed-off-by: Mark D. Roth <roth@google.com>

repokitteh-read-only · 2020-07-22T23:13:02Z

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

🐱

Caused by: #12237 was opened by markdroth.

see: more, trace.

costinm · 2020-07-22T23:49:16Z

api/envoy/extensions/transport_sockets/tls/v3/tls.proto

+    string plugin_instance_name = 1;
+
+    // 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.


"TLS" may not be a good example - "example.com" ?

sanjaypujare · 2020-07-23T15:34:56Z

api/envoy/extensions/transport_sockets/tls/v3/tls.proto

+    string plugin_instance_name = 1;
+
+    // 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.


to specify a new tls-certificate.

Why "new"? If the same plugin_instance_name and certificate_name combination is used for a different cluster (or such) will the provider provide the same certificate or has to mint a new one?

Would be good to mention this is optional depending on the plugin_instance i.e. some plugins may require it and others may ignore it.

I copied this text from what you added in #11061. :) But I've changed the wording.

sanjaypujare · 2020-07-23T15:37:10Z

api/envoy/extensions/transport_sockets/tls/v3/tls.proto

@@ -144,6 +158,12 @@ message CommonTlsContext {
    // [#not-implemented-hide:]
    CertificateProvider validation_context_certificate_provider = 3
        [(udpa.annotations.field_migrate).oneof_promotion = "dynamic_validation_context"];
+
+    // Certificate provider instance for fetching validation context - only to be used when
+    // validation_context_sds_secret_config is not used.


"and validation_context_certificate_provider is not used"?

sanjaypujare · 2020-07-23T15:44:39Z

api/envoy/extensions/transport_sockets/tls/v3/tls.proto

+    // bootstrap file) to correspond to a plugin instance (i.e., the same data in the typed_config
+    // field that would be sent in the CertificateProvider message if the config was sent by the
+    // control plane). If not present, defaults to "default".
+    string plugin_instance_name = 1;


Previously someone commented on the use of the word "plugin" in this context - IIRC the preferred word is "extension". Will need to be changed in the name of the field and the comments. But I am okay with this word.

sanjaypujare

LGTM but left some suggestions for the content in the comments.

Signed-off-by: Mark D. Roth <roth@google.com>

jiangtaoli2016 · 2020-07-23T17:13:46Z

api/envoy/extensions/transport_sockets/tls/v3/tls.proto

@@ -123,6 +123,26 @@ message CommonTlsContext {
    }
  }

+  // Similar to CertificateProvider above, but allows the provider instances to be configured on
+  // the client side instead of being sent from the control plane.
+  message CertificateProviderInstance {


Since CertificateProvider has not been implemented with [#not-implemented-hide:]
Why not replace/merge CertificateProvider with CertificateProviderInstance?

If someone uses old CertificateProvider, it infers that there is only one CertificateProvider. CertificateProvider is a special case of CertificateProviderInstance with the default instance.

@jiangtaoli2016 we cannot replace CertificateProvider since that option (of supplying the config from the control plane) should be supported.

We discussed merging CertificateProviderInstance into CertificateProvider but decided in favor of keeping them separate for a clean-looking API.

If someone uses old CertificateProvider, it infers that there is only one CertificateProvider.

Not true. Treat them as separate entities in their own "space". The control plane can still send multiple CertificateProvider configs each with distinct plugin-name and/or config.

CertificateProvider is a special case of CertificateProviderInstance with the default instance.

Actually CertificateProviderInstance can be considered as a special case of CertificateProvider where the control plane tells the plugin to pick its config locally from a bootstrap file. But all of this has been discussed and we have decided to keep them separate.

I think you're misunderstanding how the CertificateProvider field is intended to work. That field does not imply that there is only one provider instance; the cardinality is exactly the same in both CertificateProvider and CertificateProviderInstance.

In both cases, the API can specify a different provider instance for each of the identity and root certificates, which means that there can be either one or two provider instances per CommonTlsContext. And each Listener (for incoming connections) and Cluster (for outgoing connections) can specify a different CommonTlsContext, and there is no limit to the number of Listeners or Clusters used by a given client. So there is no limit to the number of provider instances that could be used by a given client (although in practice, I would not expect the number to be large).

In both cases, the client will maintain a global map of currently instantiated provider instances. However, the map is structured a little bit differently between the two cases:

In the CertificateProviderInstance case, the set of instances in the map is statically configured in the bootstrap file, and the map is keyed by the instance name assigned in the bootstrap file. That instance name is sent by the control plane to tell the client which of its configured instances to use.

In the CertificateProvider case, the set of instances in the map will change dynamically based on the set of CertificateProvider configurations sent by the control plane. The map is keyed by the provider (implementation) name and config in the CertificateProvider typed_config field. Therefore, if two different CertificateProviders use the same typed_config, they will wind up using the same instance.

The reason we are not replacing the CertificateProvider field is that we think there may be cases in the future where we will want to send the provider config from the control plane. We're not planning to implement support for the CerttificateProvider field in gRPC until/unless we need to (and we currently don't have anyone signed up to implement either of these fields in Envoy), but there's no point in removing it if we think we may want it later.

I meant merge two as follows (which seems cleaner).

message CommonTlsContext { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.auth.CommonTlsContext"; // Config for Certificate provider to get certificates. This provider should allow certificates to be // fetched/refreshed over the network asynchronously with respect to the TLS handshake. 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 certificate_name = 1 [(validate.rules).string = {min_bytes: 1}]; // Provider specific config. oneof config { option (validate.required) = true; // provider config config.core.v3.TypedExtensionConfig provider_config = 2; // provider name string provider_name = 3; } }

I agree we need to support the control plane to supply which certificate provider instance to use. The difference is the certificate provider config is from local config or supplied by the control plane. Either way, The control plane needs to tell which cert provider to use.

If you already decided, it is fine. I won't hold you back.

Sanjay actually suggested the same thing. I'm the one who has been advocating for keeping them separate.

I think they are actually sufficiently different intents that they should be expressed separately in the API. For example, a client implementation that supports both mechanisms is likely to have a separate global map for each of the two mechanisms rather than sharing a single map.

That having been said, if the consensus from everyone else is to combine them in the API, I can probably live with that.

As I said earlier, Sanjay originally suggested the same thing you did.

I'd like feedback from the Envoy folks on this.

Yes, my idea was to have a single one as described in #12237 (comment) .

@markdroth the question to me is "who will implement these?". If gRPC plans on doing both, then let's keep both, if we're only likely to see the bootstrap indirect in the near term, let's go with that for now.

Yes the plan is to keep and implement both (although not at the same time necessarily). I don't think we should take out anything. Also it will be good to get input from @arjayara01 .

@htuch I think it's very likely that we'll still want CertificateProvider in the long term, but we're going to wait for a concrete use-case before implementing it. My inclination is to keep it for now, so that we don't need to re-add it later.

Given that the fields are already here, I assume we can't delete them anyway, right? I mean, in practice, I doubt anyone is using them yet, but technically it seems like we'd be breaking the API guarantee if we did.

htuch · 2020-07-23T23:42:33Z

api/envoy/extensions/transport_sockets/tls/v3/tls.proto

+    //
+    // Instance names should generally be defined not in terms of the underlying provider
+    // implementation (e.g., "file_watcher") but rather in terms of the function of the
+    // certificates (e.g., "foo_deployment_identity").


This is a bit weird in xDS. Generally, when we name things, we put them in some kind of reverse DNS namespace, e.g. envoy.foo_provider or com.acme.bar_provider.

I think what you're trying to do here is create some rendezvous with the bootstrap, so this is pretty different. If that's the case, why imply any policy here at all?

Yeah, this is different from those other cases. I think reverse-DNS might make sense for the plugin names themselves, but not for the plugin instances; those are just deployment-specific logical names in the bootstrap file. The distinction I'm trying to draw here is between those two things, since there was some confusion in our meeting between the plugin implementation name and the plugin instance name.

htuch

Can you provide a [#not-implemented-hide:] bootstrap implementation to demonstrate this use in Envoy? We probably need to have a tracking issue and idea of how this will eventually land. LGTM otherwise as discussed at the meeting.

Signed-off-by: Mark D. Roth <roth@google.com>

markdroth · 2020-07-24T18:34:41Z

@htuch I've added the global map to the bootstrap file, as requested.

htuch

LGTM, will defer to @envoyproxy/api-shepherds for non-Google review and merge.

…#12237) Signed-off-by: Mark D. Roth <roth@google.com> Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

…#12237) Signed-off-by: Mark D. Roth <roth@google.com>

…#12237) Signed-off-by: Mark D. Roth <roth@google.com> Signed-off-by: chaoqinli <chaoqinli@google.com>

api: Add CertificateProviderInstance to CommonTlsContext.

6b966b6

Signed-off-by: Mark D. Roth <roth@google.com>

repokitteh-read-only bot added the api label Jul 22, 2020

costinm reviewed Jul 22, 2020

View reviewed changes

costinm mentioned this pull request Jul 23, 2020

move gateway certs specified in server TLS settings to SDS istio/istio#25786

Merged

sanjaypujare reviewed Jul 23, 2020

View reviewed changes

sanjaypujare previously approved these changes Jul 23, 2020

View reviewed changes

update comments

6d24ea0

Signed-off-by: Mark D. Roth <roth@google.com>

markdroth dismissed sanjaypujare’s stale review via 6d24ea0 July 23, 2020 16:09

jiangtaoli2016 reviewed Jul 23, 2020

View reviewed changes

htuch reviewed Jul 23, 2020

View reviewed changes

Add global CertificateProvider instance map in bootstrap file.

d6b1b67

Signed-off-by: Mark D. Roth <roth@google.com>

htuch reviewed Jul 26, 2020

View reviewed changes

alyssawilk assigned lizan Jul 27, 2020

mattklein123 approved these changes Jul 27, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Jul 27, 2020

mattklein123 merged commit 79d7d4e into envoyproxy:master Jul 27, 2020

markdroth deleted the certificate_provider_instance branch July 28, 2020 20:51

KBaichoo pushed a commit to KBaichoo/envoy that referenced this pull request Jul 30, 2020

api: Add CertificateProviderInstance to CommonTlsContext. (envoyproxy…

8315923

…#12237) Signed-off-by: Mark D. Roth <roth@google.com> Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

chaoqin-li1123 pushed a commit to chaoqin-li1123/envoy that referenced this pull request Aug 7, 2020

api: Add CertificateProviderInstance to CommonTlsContext. (envoyproxy…

24396b1

…#12237) Signed-off-by: Mark D. Roth <roth@google.com>

chaoqin-li1123 pushed a commit to chaoqin-li1123/envoy that referenced this pull request Aug 7, 2020

api: Add CertificateProviderInstance to CommonTlsContext. (envoyproxy…

21ecbe7

…#12237) Signed-off-by: Mark D. Roth <roth@google.com> Signed-off-by: chaoqinli <chaoqinli@google.com>

markdroth mentioned this pull request Jan 4, 2022

Implement CertificateProvider mechanism #19308

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: Add CertificateProviderInstance to CommonTlsContext. #12237

api: Add CertificateProviderInstance to CommonTlsContext. #12237

markdroth commented Jul 22, 2020

repokitteh-read-only bot commented Jul 22, 2020

costinm Jul 22, 2020

markdroth Jul 23, 2020

sanjaypujare Jul 23, 2020

sanjaypujare Jul 23, 2020

markdroth Jul 23, 2020

sanjaypujare Jul 23, 2020

markdroth Jul 23, 2020

sanjaypujare Jul 23, 2020

markdroth Jul 23, 2020

sanjaypujare left a comment

jiangtaoli2016 Jul 23, 2020

sanjaypujare Jul 23, 2020

markdroth Jul 23, 2020

jiangtaoli2016 Jul 23, 2020

markdroth Jul 23, 2020

markdroth Jul 23, 2020

sanjaypujare Jul 23, 2020

htuch Jul 23, 2020

sanjaypujare Jul 24, 2020

markdroth Jul 24, 2020

htuch Jul 23, 2020

markdroth Jul 24, 2020

htuch left a comment

markdroth commented Jul 24, 2020

htuch left a comment

api: Add CertificateProviderInstance to CommonTlsContext. #12237

api: Add CertificateProviderInstance to CommonTlsContext. #12237

Conversation

markdroth commented Jul 22, 2020

repokitteh-read-only bot commented Jul 22, 2020

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

Choose a reason for hiding this comment

sanjaypujare left a 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

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

htuch left a comment

Choose a reason for hiding this comment

markdroth commented Jul 24, 2020

htuch left a comment

Choose a reason for hiding this comment