Add generic transport security context by lizan · Pull Request #189 · envoyproxy/data-plane-api

lizan · 2017-10-11T21:37:36Z

Signed-off-by: Lizan Zhou zlizan@google.com

Signed-off-by: Lizan Zhou <zlizan@google.com>

lizan · 2017-10-11T21:37:54Z

PiotrSikora · 2017-10-11T23:59:54Z

api/sds.proto

+import "api/transport_security.proto";

 import "google/api/annotations.proto";
 import "google/protobuf/wrappers.proto";


I think google/protobuf/wrappers.proto can be removed now.

PiotrSikora · 2017-10-12T00:01:50Z

api/transport_security.proto

+  ConfigSource sds_config = 2;
+}
+
+message TransportSecurityContext {


Nit: feel free to add this before SdsSecretConfig, so it's next to other context definition.

PiotrSikora · 2017-10-12T00:02:40Z

api/transport_security.proto

+
+  // Implementation specific configuration which depends on the implementation being instantiated.
+  // See the supported transport security implementations for further documentation.
+  google.protobuf.Any config = 2;


As discussed in person, I'm fine with Any, but we use Struct in other places for opaque configs.

cc @htuch @mattklein123

Yes I am following discussion at envoyproxy/envoy#1680 . For this I feel Any is better since there won't be v1 compatibility issue with JSON.

My POC work actually does parsing Any from JSON without problem.

I honestly don't fully grok all the history on Struct vs. Any so will defer to @htuch on this.

I followed up at envoyproxy/envoy#168, but I think that if this is another opaque config that isn't going to always have the proto descriptor available for the inner messages at data-plane-api, then we should at least make the option of Struct, and TBH, I would just defer the Any handling until #1680 gets done as an orthogonal piece of work.

Followed up on envoyproxy/envoy#1680 too.

Also note a Struct can be wrapped into Any. So it seems like for new opaque configs like this PR, can be defined with Any only, to avoid complexities and confusions between Struct and Any, while we provide both options, and gains from typed config.

I rather not have inconsistency in how opaque configs are provided. Either we resolve envoyproxy/envoy#1680 and apply the solution across the API surface, or we stick with Struct here for now IMHO.

OK, let's make this just an Any for now, but please document that the dynamically typed Struct equivalent for any proto that is supported by the Any will also be accepted as a value, and Envoy will do the work of attempting to parse this under the expected type for the transport security in question.

I think we still need to sort out whether this is a transport security feature or not. Based on the implementation it seems like a full listener/connection swap? (Any/Struct is fine with me).

Sorry for the delay, yes I'm still figuring out the split between transport security or full connection impl. Yet busy with other stuff...

The implementation what I want to hook up (TSI) is just shuffling bytes, so it does't have to be a full listener/connection, though the existing BoringSSL does operate on file descriptors directly.

PiotrSikora · 2017-10-12T00:06:13Z

api/lds.proto

-  DownstreamTlsContext tls_context = 2;
+
+  oneof transport_security {
+    DownstreamTlsContext tls_context = 2;


While you're here, can you add comment here, similar to the one for UpstreamTlsContext?

Signed-off-by: Lizan Zhou <zlizan@google.com>

PiotrSikora

LGTM, pending Any vs Struct decision from @htuch and @mattklein123.

mattklein123 · 2017-10-12T02:23:03Z

api/transport_security.proto

@@ -0,0 +1,151 @@
+syntax = "proto3";


this is all just moves, right? did anything actually change in here beyond the addition of the opaque config message?

Right, just moved all contexts out of sds.proto to here, and added TransportSecurityContext.

mattklein123 · 2017-10-12T02:24:27Z

Seems reasonable to me. @lizan can you add a small amount of detail in terms of how this will work? Where will the dividing line be? Will these be new listeners and new connection impls? Or will this all be baked into SslConnectionImpl somehow?

lizan · 2017-10-12T03:13:14Z

@mattklein123 The plan is to add a factory via registry, which can instantiate listeners and connection impls. I'll make a POC branch in my envoy repo to show how this works.

mattklein123 · 2017-10-12T03:19:22Z

If that's the case, I'm not sure this is really related to security at all? It's basically about plugging in a new connection level stack (which is fine). Can we rename appropriately? Someone could come along and plug in QUIC or any other proprietary thing that can shuffle bytes around.

lizan · 2017-10-12T03:38:40Z

Hmm, I see your point. Let me play around this a bit more, may be next week, I'm busy with other stuff the rest of this week.

The internal PoC did have another listener / connection impls (they also inherits ConnectionImpl) just because Ssl::ConnectionImpl is tightly coupled with ConnectionImpl. I'll see if there are chances that they can be decoupled.

I'm not sure QUIC will fall into this or not (even we make it pluggable connection level stack). Will need to sync with @alyssawilk, since the new impls is still following TCP semantics.

mattklein123 · 2017-10-12T03:41:56Z

cc @jpinner-lyft ^

htuch · 2017-10-29T17:28:13Z

@lizan where are we with this PR? Do you have an update or should we close out for now?

lizan · 2017-10-30T06:03:49Z

Closing this, I'll make another PR based on conversation in envoyproxy/envoy#1924

Add generic transport security context

86093f7

Signed-off-by: Lizan Zhou <zlizan@google.com>

PiotrSikora reviewed Oct 12, 2017

View reviewed changes

Address comments

6f8185d

Signed-off-by: Lizan Zhou <zlizan@google.com>

PiotrSikora approved these changes Oct 12, 2017

View reviewed changes

mattklein123 reviewed Oct 12, 2017

View reviewed changes

lizan mentioned this pull request Oct 12, 2017

Use typed protobuf messages for configs of builtin filters. envoyproxy/envoy#1680

Closed

lizan mentioned this pull request Oct 23, 2017

[WIP] RFC: Pluggable transport security envoyproxy/envoy#1924

Closed

4 tasks

lizan closed this Oct 30, 2017

lizan mentioned this pull request Oct 31, 2017

Add transport socket config #218

Merged

Conversation

lizan commented Oct 11, 2017

Uh oh!

lizan commented Oct 11, 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!

htuch Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

PiotrSikora 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!

mattklein123 commented Oct 12, 2017

Uh oh!

lizan commented Oct 12, 2017

Uh oh!

mattklein123 commented Oct 12, 2017

Uh oh!

lizan commented Oct 12, 2017

Uh oh!

mattklein123 commented Oct 12, 2017

Uh oh!

htuch commented Oct 29, 2017

Uh oh!

lizan commented Oct 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

htuch Oct 12, 2017 •

edited

Loading

htuch Oct 12, 2017 •

edited

Loading