Skip to content
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

Jump to bottom

api: enhance v3 CommonTlsContext for agentless support #11061

Merged
merged 14 commits into from
May 21, 2020
Merged

api: enhance v3 CommonTlsContext for agentless support #11061

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
f74cb76
api: enhance v3 CommonTlsContext for agentless support
sanjaypujare May 5, 2020
5e8f97b
api: address review comments
sanjaypujare May 5, 2020
097d099
api: address review comments - 2nd set
sanjaypujare May 5, 2020
ed9d51f
api: fix field numbering
sanjaypujare May 5, 2020
afffb45
api: add field_migrate.oneof_promotion annotation
sanjaypujare May 8, 2020
90c2424
api: migrate from google.protobuf.Any to the new TypedExtensionConfig
sanjaypujare May 11, 2020
6bc63f8
api: rename CAClientPluginConfig to CertificateProvider
sanjaypujare May 13, 2020
cd60ee9
api: fix typo in comment
sanjaypujare May 13, 2020
2d93cfe
api: clarify comments based on feedback
sanjaypujare May 14, 2020
d09a366
api: fix typo
sanjaypujare May 14, 2020
71b3508
api: add oneof to support a future named resource
sanjaypujare May 19, 2020
94ce3fb
api: add the required validate rules
sanjaypujare May 20, 2020
f3be260
api: single file change for correct validate.rules
sanjaypujare May 21, 2020
8e09f75
api: address the minor comment related issues
sanjaypujare May 21, 2020
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
51 changes: 45 additions & 6 deletions api/envoy/extensions/transport_sockets/tls/v3/tls.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,15 @@ syntax = "proto3";

package envoy.extensions.transport_sockets.tls.v3;

import "envoy/config/core/v3/extension.proto";
import "envoy/extensions/transport_sockets/tls/v3/common.proto";
import "envoy/extensions/transport_sockets/tls/v3/secret.proto";

import "google/protobuf/any.proto";
import "google/protobuf/duration.proto";
import "google/protobuf/wrappers.proto";

import "udpa/annotations/migrate.proto";
import "udpa/annotations/status.proto";
import "udpa/annotations/versioning.proto";
import "validate/validate.proto";
Expand Down Expand Up @@ -96,10 +99,28 @@ message DownstreamTlsContext {
}

// TLS context shared by both client and server TLS contexts.
// [#next-free-field: 9]
// [#next-free-field: 11]
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 name = 1;
Copy link
Member

Choose a reason for hiding this comment

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

Should this be required by validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it should be required. Adding that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


// Provider specific config.
// Note: an implementation is expected to dedup multiple instances of the same config
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to provide hooks here to allow the config to be fetched as a separate xDS resource type? I am thinking that if there are a large number of Cluster and Listener resources that contain the same CertificateProvider, if we want to change the CertificateProvider, it would be nice to just send a single update for the CertificateProvider instead of sending an update for every single Cluster and Listener that is affected. But I'm also not sure how common this use-case might be.

I'd be interested in feedback from @mattklein123 and @htuch on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think @htuch answered it here #11061 (comment)

Copy link
Contributor

@markdroth markdroth May 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, didn't see that earlier thread. Sorry for the duplication.

I am fine with actually adding this in a separate PR, but I do think it's worth considering how we would structure the API to make that work. It would be nice to set this up in a way that doesn't add unnecessary roadblocks to making that change later.

For example, let's say that this PR gets merged as-is right now. If we later wanted to add support for fetching these configs as a separate resource type, what would we need to change? It seems like we'd need the ability to specify a ConfigSource and a resource name to request. And presumably, we'd want to be able to specify either an inline config or an external xDS resource to use, so there'd have to be a oneof. So we'd probably want something like this here:

// Describes a resource to fetch via Certificate Provider Discovery Service (CPDS).
message CPDS {
  string name = 1;  // Resource name.
  ConfigSource config_source = 2;
}

oneof config {
  config.core.v3.TypedExtensionConfig typed_config = 2;
  CPDS cpds = 3;
}

Does that look right? Would we be okay handling this with a oneof_promotion annotation, or should we preemptively add the oneof in this PR, even if it currently contains only one option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, generally right but your typed_config is of config.core.v3.TypedExtensionConfig type and I was thinking instead:

oneof config {
  CertificateProvider typed_config = 2;
  CPDS cpds = 3;
}

The CPDS.name is the unique resource name that returns the CertificateProvider resource and the CertificateProvider.name is then used as I have described above. If everyone (other than me:-)) feels that name is not at all needed then I'll get rid of CertificateProvider struct and just use config.core.v3.TypedExtensionConfig instead.

Regarding oneof, it is best to add the oneof now (instead of oneof_promotion) if there is the issue of backward compatibility (which I think there is) even if we don't define CPDS yet. The oneof_promotion creates oneof for v4alpha but we may end up making this change before we move from v3 to v4alpha.

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 the underlying question here is whether the resource returned by CDPS would be the whole CertificateProvider message or only the typed config for the plugin instance. The main difference is whether the name of the certificate being requested lives here where CDPS would be called or in the CDPS resource. If it is in the CDPS resource, then we will need a separate CDPS resource for every named certificate we will use, even if the entire rest of the config is the same. That seems inefficient.

Let's say that you have two CertificiateProvider entries in a Cluster. They both have the same config, but they differ by the name of the certificate that we are asking for: one sets the name to "ROOTCA" and the other sets it to "TLS". I think you would want a single CDPS resource for this plugin, and the places where we refer to this plugin would specify which certificate name to request from the plugin. The advantage of this approach becomes even more clear in a situation where different clusters use different certificates, but they all come from the same plugin instance.

Note that this is actually a question about what the plugin API is going to look like. Will a plugin instance allow registering a watcher for a particular name (e.g., void StartWatch(std::string name, Watcher* watcher);), or will it assume that all watchers are for the same name (e.g., void StartWatch(Watcher* watcher);)? If the name of the certificate to request is a first-class citizen in this API, then I think it should be the former.

I think this also relates to the question of requiring plugin implementations to internally share instances. When we were discussing (offline from this PR) the question of how this would work in gRPC, we said that we wanted to be able to reuse plugin instances in the framework, so that individual plugin implementations don't each need to do this. I suspect that Envoy will want that same consideration, but @htuch and @mattklein123 can weigh in on this.

I think my main point here is that we need to flesh out some of the details of how this is going to be implemented in both gRPC and Envoy in order to know how to structure this API.

Copy link
Member

Choose a reason for hiding this comment

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

I'm happy to defer to all of you on what you think is best, but if we think that we want to add referential behavior soon(ish) I would absolutely preemptively add a oneof now even if it only has a single value currently. We do this all over the place to future proof similar situations.

I suspect that Envoy will want that same consideration, but @htuch and @mattklein123 can weigh in on this.

This part is an implementation detail obviously, but yes, if possible we would de-dup plugins probably based on config via the singleton manager. We also use this pattern elsewhere. (@htuch does not like this from an API perspective, but it's a way of de-duping without a generic or API specific reference system.)

Copy link
Member

Choose a reason for hiding this comment

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

Since we've adopted this as a pattern in a few places now (dedup in the client based on config), I think it's totally reasonable to make this standard for v3 at least. If we find that there are some exceptions that this pattern of dedup really causes confusion for or is not effective in practice, we can change as we move towards v4.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, if we're comfortable de-duping based on the config, then the way this is now is fine. Let's just add a oneof around the typed_config field, so that we have the option of adding CPDS later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sanjay, we still need to add the oneof here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's in my TODO list

// 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 {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a required validation to the oneof?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// This config could be supplied inline or it could be a named xDS resource (in future).
config.core.v3.TypedExtensionConfig typed_config = 2;
}
}

message CombinedCertificateValidationContext {
option (udpa.annotations.versioning).previous_message_type =
"envoy.api.v2.auth.CommonTlsContext.CombinedCertificateValidationContext";
Expand All @@ -108,9 +129,17 @@ message CommonTlsContext {
CertificateValidationContext default_validation_context = 1
[(validate.rules).message = {required: true}];

// Config for fetching validation context via SDS API.
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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed quotes

SdsSecretConfig validation_context_sds_secret_config = 2 [
(validate.rules).message = {required: true},
(udpa.annotations.field_migrate).oneof_promotion = "dynamic_validation_context"
];

// Certificate provider for fetching validation context
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// [#not-implemented-hide:]
CertificateProvider validation_context_certificate_provider = 3
[(udpa.annotations.field_migrate).oneof_promotion = "dynamic_validation_context"];
}

reserved 5;
Expand All @@ -126,15 +155,21 @@ message CommonTlsContext {
// used for clients that support ECDSA.
repeated TlsCertificate tls_certificates = 2;

// Configs for fetching TLS certificates via SDS API.
// Configs for fetching TLS certificates via SDS API. Note SDS API allows certificates to be
// fetched/refreshed over the network "asynchronously" with respect to the TLS handshake.
repeated SdsSecretConfig tls_certificate_sds_secret_configs = 6
[(validate.rules).repeated = {max_items: 1}];

// Certificate provider for fetching TLS certificates.
// [#not-implemented-hide:]
CertificateProvider tls_certificate_certificate_provider = 9;

oneof validation_context_type {
// How to validate peer certificates.
CertificateValidationContext validation_context = 3;

// Config for fetching validation context via SDS API.
// 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.
SdsSecretConfig validation_context_sds_secret_config = 7;

// Combined certificate validation context holds a default CertificateValidationContext
Expand All @@ -145,6 +180,10 @@ message CommonTlsContext {
// CertificateValidationContext, and concatenates repeated fields to default
// CertificateValidationContext, and logical OR is applied to boolean fields.
CombinedCertificateValidationContext combined_validation_context = 8;

// Certificate provider for fetching validation context.
// [#not-implemented-hide:]
CertificateProvider validation_context_certificate_provider = 10;
}

// Supplies the list of ALPN protocols that the listener should expose. In
Expand Down
52 changes: 46 additions & 6 deletions api/envoy/extensions/transport_sockets/tls/v4alpha/tls.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ syntax = "proto3";

package envoy.extensions.transport_sockets.tls.v4alpha;

import "envoy/config/core/v4alpha/extension.proto";
import "envoy/extensions/transport_sockets/tls/v4alpha/common.proto";
import "envoy/extensions/transport_sockets/tls/v4alpha/secret.proto";

import "google/protobuf/any.proto";
import "google/protobuf/duration.proto";
import "google/protobuf/wrappers.proto";

Expand Down Expand Up @@ -96,11 +98,32 @@ message DownstreamTlsContext {
}

// TLS context shared by both client and server TLS contexts.
// [#next-free-field: 9]
// [#next-free-field: 11]
message CommonTlsContext {
option (udpa.annotations.versioning).previous_message_type =
"envoy.extensions.transport_sockets.tls.v3.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 {
option (udpa.annotations.versioning).previous_message_type =
"envoy.extensions.transport_sockets.tls.v3.CommonTlsContext.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;

// Provider specific config.
// Note: an implementation is expected to dedup multiple instances of the same config
// 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 {
// This config could be supplied inline or it could be a named xDS resource (in future).
config.core.v4alpha.TypedExtensionConfig typed_config = 2;
}
}

message CombinedCertificateValidationContext {
option (udpa.annotations.versioning).previous_message_type =
"envoy.extensions.transport_sockets.tls.v3.CommonTlsContext."
Expand All @@ -110,9 +133,16 @@ message CommonTlsContext {
CertificateValidationContext default_validation_context = 1
[(validate.rules).message = {required: true}];

// Config for fetching validation context via SDS API.
SdsSecretConfig validation_context_sds_secret_config = 2
[(validate.rules).message = {required: true}];
oneof dynamic_validation_context {
// 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.
SdsSecretConfig validation_context_sds_secret_config = 2
[(validate.rules).message = {required: true}];

// Certificate provider for fetching validation context
// [#not-implemented-hide:]
CertificateProvider validation_context_certificate_provider = 3;
}
}

reserved 5;
Expand All @@ -128,15 +158,21 @@ message CommonTlsContext {
// used for clients that support ECDSA.
repeated TlsCertificate tls_certificates = 2;

// Configs for fetching TLS certificates via SDS API.
// Configs for fetching TLS certificates via SDS API. Note SDS API allows certificates to be
// fetched/refreshed over the network "asynchronously" with respect to the TLS handshake.
repeated SdsSecretConfig tls_certificate_sds_secret_configs = 6
[(validate.rules).repeated = {max_items: 1}];

// Certificate provider for fetching TLS certificates.
// [#not-implemented-hide:]
CertificateProvider tls_certificate_certificate_provider = 9;

oneof validation_context_type {
// How to validate peer certificates.
CertificateValidationContext validation_context = 3;

// Config for fetching validation context via SDS API.
// 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.
SdsSecretConfig validation_context_sds_secret_config = 7;

// Combined certificate validation context holds a default CertificateValidationContext
Expand All @@ -147,6 +183,10 @@ message CommonTlsContext {
// CertificateValidationContext, and concatenates repeated fields to default
// CertificateValidationContext, and logical OR is applied to boolean fields.
CombinedCertificateValidationContext combined_validation_context = 8;

// Certificate provider for fetching validation context.
// [#not-implemented-hide:]
CertificateProvider validation_context_certificate_provider = 10;
}

// Supplies the list of ALPN protocols that the listener should expose. In
Expand Down
51 changes: 45 additions & 6 deletions generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/tls.proto
View file
Open in desktop

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

Loading