authz_filter: configuration to support Ambassador authorization flow #563

gsagula · 2018-03-19T04:10:32Z

This PR includes the necessary modifications in support of envoyproxy/envoy#2828.

Added additional configuration to ext_authz.proto so that the filter is able to call an HTTP/1.1 authorization service.
In external_auth.proto, added a nested message to CheckResponse that allows the authorization service to pass additional HTTP response attributes back to the authz filter.

Signed-off-by: Gabriel <[email protected]>

…service auth Signed-off-by: Gabriel <[email protected]>

Signed-off-by: Gabriel <[email protected]>

ggreenway · 2018-03-19T17:17:08Z

envoy/config/filter/http/ext_authz/v2/ext_authz.proto

@@ -23,4 +23,25 @@ message ExtAuthz {
  // is NOT denied then traffic will be permitted.
  // Defaults to false.
  bool failure_mode_allow = 2;
+
+  // When this value is set to true, the filter must call an HTTP authorization service only.
+  bool use_http_service = 3;


I think an enum instead of a bool would allow future extensibility

Better IMO would actually be to make this a oneof and just merge HttpService as one of the options inside the oneof. This is in general what we are trying to do elsewhere. This brings the enum and the "only use if enum is set" logic together. If this impacts the already implemented non-HTTP code I would just go ahead and change it now since IMO I still consider this filter under development.

Unless I'm missing something, It seems that the impact on the already implemented non-HTTP code would be the same with either enum or oneof.

The ExtAuthz message would look something like this:

message ExtAuthz { oneof services { envoy.api.v2.core.GrpcService grpc_service = 1; HttpService http_service = 3; } bool failure_mode_allow = 2; }

And the decision on which client to use, somewhere here: https://github.com/envoyproxy/envoy/blob/da0dc5f5dac12651f211e321f9cd7b2c70bedde9/source/server/config/http/ext_authz.cc#L17

The oneof is a simpler way to represent mutually exclusive config when it's more than just a scalar value, i.e. the message. What you have there is basically what it would look like.

htuch · 2018-03-19T22:10:21Z

envoy/config/filter/http/ext_authz/v2/ext_authz.proto

+    string cluster = 1;
+
+    // Sets the time in milliseconds within the service should respond to an authorization request.
+    uint32 timeout_ms = 2;


This should be a google.protobuf.Duration.

htuch · 2018-03-19T22:11:04Z

envoy/config/filter/http/ext_authz/v2/ext_authz.proto

+
+    // Sets a list of authorization response headers that are allowed to be injected in the
+    // downstream client request before dispatching it to the upstream.
+    repeated string allowed_headers = 3;


Why is this necessary? Don't we trust the authz server to do the right thing?

I just copied this from the Datawire's filter. It seems to prevent that all authz headers get copied over to the original request. E.g., content-length.

if (!config_->allowed_headers_.empty()) { // Yup. Let's see if any of them are present. for (const std::string& allowed_header : config_->allowed_headers_) { LowerCaseString key(allowed_header); // OK, do we have this header? const HeaderEntry* hdr = response->headers().get(key); if (hdr) { // Well, the header exists at all. Does it have a value? const HeaderString& value = hdr->value(); if (!value.empty()) { // Not empty! Copy it into our request_headers_. std::string valstr{value.c_str()}; ENVOY_STREAM_LOG(trace, "ExtAuth allowing response header {}: {}", *callbacks_, allowed_header, valstr); addedHeaders = true; request_headers_->addCopy(key, valstr); } } }

https://github.com/datawire/envoy/blob/546f6bbe3b74249d871073ef514cf71144e88dbf/source/common/http/filter/extauth.cc#L215

@kflynn Would you mind to help us with this one? Thanks!

@htuch @mattklein123 Folks we've talked to have been less comfortable with given an auth service carte blanche to change anything it wants going upstream, more comfortable with only allowing it to change known things (cf some of the comments around trust on https://docs.google.com/document/d/1L3aRk9yaT6Lsb4epI6rdjCdStOuwTfhoeSP82zunkGg/edit).

I'm not sure I really buy this. Why would the filter then trust anything the auth service does? Whether the decision, the response, etc.?

@mattklein123 I think there were three underlying concerns:

Bugs in the auth service are always a thing; having some protection in the upstream-modification path is a bit of a safeguard. Same idea as not wanting even your favorite Unix developer run everything as root.

Policy enforcement is sometimes a thing. It may be necessary to be able to tell auditors that only this one specific set of modifications can ever happen, unless the following audited process is followed, etc.

Finally, sure, malicious auth services might someday be a thing somewhere? but they clearly do not need unchecked upstream-header modification to wreak havoc, so I don't consider protecting against that case to be a major driver for this config element.

I agree that it seems a bit arbitrary just policing the headers. If we don't trust the auth service, it can do a lot bad things, e.g. inject content in the reply for the body.

I can see that in some applications, you do want to strip headers unconditionally, when something passes between internal <-> external on edge proxies. We could use a separate filter in the stack for this, would that work in your application? It seems if the auth server can't set that header, backends probably shouldn't be trusted to do it either?

htuch · 2018-03-19T22:14:29Z

envoy/service/auth/v2/external_auth.proto

+  // modified in the original request before it gets sent to the upstream.
+  message HttpResponse {
+    // Http status code.
+    int32 status_code = 1;


You probably want to make this consistent with other uses of status code in the API, e.g.

data-plane-api/envoy/api/v2/route/route.proto

Line 582 in 641920a

uint32 status = 1 [(validate.rules).uint32 = {gte: 100, lt: 600}];

htuch · 2018-03-19T22:15:21Z

envoy/service/auth/v2/external_auth.proto

@@ -26,4 +26,22 @@ message CheckRequest {
 message CheckResponse {
  // Status `OK` allows the request. Any other status indicates the request should be denied.
  google.rpc.Status status = 1;
+
+  // An optional message which contains the attributes of an HTTP response object. This message is


This PR doesn't make clear how exactly this will work. Does the HTTP service send back a proto with this? Or is this an enhancement for the normal gRPC authz service to provide it the same capabilities as the HTTP authz service?

@htuch This should be an enhancement for the standard gRPC authz service to be able to respond to HTTP downstream client. I'm not sure if we need map<string, string> headers though since gRPC metadata could be used to accomplish the same thing. In this case, have a list of headers that are allowed to be copied or to be modified seems reasonable to me.

I think it's definitely the right thing for the gRPC authz service to make it explicit here. Metadata is really about the channel between the gRPC authz server and Envoy, it shouldn't be used to provide additional response information.

saumoh · 2018-03-19T23:35:40Z

envoy/config/filter/http/ext_authz/v2/ext_authz.proto

+  message HttpService {
+
+    // Sets the cluster name which the authorization request must be sent to.
+    string cluster = 1;


you can mandate this be set. see https://github.com/gsagula/data-plane-api/blob/339db0e1225d31e13ea3d35e7a8459abfbda3970/envoy/api/v2/core/grpc_service.proto#L25

Do you think there would ever be a need for a TLS configuration and also could the http service be made a core service (like grpc_service). (Not asking for you to change anything but thinking out loud).

TLS config is configured at the cluster level, so I think this is covered.

saumoh · 2018-03-19T23:57:50Z

envoy/service/auth/v2/external_auth.proto

+  // E.g., when the authorization status is not OK, this message can be used by the authorization
+  // service to the tell the filter to respond with specific HTTP headers, body, and status code.
+  // When the authorization status is OK, this object may include the headers that should be
+  // modified in the original request before it gets sent to the upstream.


Is the desired behavior that the headers specified here would ONLY modify existing headers in the original request? Could it be that the service could add new headers to the request.
Would it make sense for the authz server to send all the headers that should be sent upstream. So if headers is there then those are the only headers that would get forwarded to upstream? Or can the authz server not have enough context to do that.

@saumoh Sorry for these confusing words. Yes, it should allow modifying the existing headers as well as inject new ones. I will rephrase all this comment. Thanks for pointing this out.

saumoh · 2018-03-19T23:59:36Z

envoy/service/auth/v2/external_auth.proto

+  // An optional message which contains the attributes of an HTTP response object. This message is
+  // particularly useful when the authorization service needs to send specific responses to the
+  // downstream client or to modify the request headers before dispatching them to the upstream.
+  // E.g., when the authorization status is not OK, this message can be used by the authorization


Could you clarify the behavior when the response does not include a HttpResponse at all for both OK and not OK cases. I assume that there is no expectation for a HttpResponse to be there and in that case the behavior will not change from what is there today... right?

That's right if no extra attributes are passed from a gRPC authz service to the filter, the behavior should continue the same.

mattklein123

Some quick skim comments. Not a full review.

mattklein123 · 2018-03-20T00:57:34Z

envoy/config/filter/http/ext_authz/v2/ext_authz.proto

@@ -23,4 +23,25 @@ message ExtAuthz {
  // is NOT denied then traffic will be permitted.
  // Defaults to false.
  bool failure_mode_allow = 2;
+
+  // When this value is set to true, the filter must call an HTTP authorization service only.
+  bool use_http_service = 3;


Better IMO would actually be to make this a oneof and just merge HttpService as one of the options inside the oneof. This is in general what we are trying to do elsewhere. This brings the enum and the "only use if enum is set" logic together. If this impacts the already implemented non-HTTP code I would just go ahead and change it now since IMO I still consider this filter under development.

mattklein123 · 2018-03-20T00:58:05Z

envoy/config/filter/http/ext_authz/v2/ext_authz.proto

+  message HttpService {
+
+    // Sets the cluster name which the authorization request must be sent to.
+    string cluster = 1;


TLS config is configured at the cluster level, so I think this is covered.

mattklein123 · 2018-03-20T00:58:17Z

envoy/config/filter/http/ext_authz/v2/ext_authz.proto

+
+    // Sets a list of authorization response headers that are allowed to be injected in the
+    // downstream client request before dispatching it to the upstream.
+    repeated string allowed_headers = 3;


gsagula · 2018-03-20T03:58:09Z

@htuch @saumoh @mattklein123 @ggreenway Thank you for the review. These are all super valuable questions. Give me some time to review the design, and start to answer all the above.

kyessenov · 2018-03-20T22:30:21Z

Per https://github.com/envoyproxy/data-plane-api/issues/458, do you mind moving the config definition to v2alpha, assuming the implementation is not following the API update immediately, or if there is a chance of backwards-incompatible changes.

gsagula · 2018-03-21T06:58:34Z

@kyessenov I will do it.

Signed-off-by: Gabriel <[email protected]>

gsagula · 2018-03-25T17:10:59Z

@htuch @kyessenov @mattklein123 Thanks for the initial review. Please, feel free to take another look whenever you get a chance.

htuch · 2018-03-26T18:17:46Z

envoy/config/filter/http/ext_authz/v2alpha/ext_authz.proto

+// The external authorization HTTP service configuration.
+message HttpService {
+  // Sets the cluster name which the authorization request must be sent to.
+  string cluster = 1;


Wondering if https://github.com/envoyproxy/data-plane-api/blob/master/envoy/api/v2/core/http_uri.proto#L9 would be appropriate here. Leaning towards saying "no", since we are probably not making arbitrary HTTP calls and can statically configure the service, but if we want to leave room for growing to a dynamic auth service resolution later, then HttpUri might be appropriate.

As far as it is compatible with the previous design, which seems that it is, I think HttpUri makes sense. I'm not entirely sure about what this change though.

htuch · 2018-03-26T18:18:08Z

envoy/config/filter/http/ext_authz/v2alpha/ext_authz.proto

+    HttpService http_service = 3;
+  }
+
+  bool failure_mode_allow = 2;


htuch · 2018-03-26T18:19:49Z

envoy/service/auth/v2alpha/attribute_context.proto

+
+// [#proto-status: draft]
+
+package envoy.service.auth.v2alpha;


These are just copies from the existing protos? How come the existing ones aren't being deleted (i.e. this isn't a move)?

Yes, I just copied them. I wasn't 100% sure if I should delete the old ones.

Signed-off-by: Gabriel <[email protected]>

htuch · 2018-03-27T21:07:12Z

LGTM, thanks for the patience during iteration. @saumoh @kyessenov @kflynn I'll merge once you folks sign-off.

mattklein123 · 2018-03-27T21:14:47Z

envoy/config/filter/http/ext_authz/v2alpha/ext_authz.proto

+
+  // Sets the time, in milliseconds, within the service should respond to an authorization
+  // request.
+  google.protobuf.Duration timeout = 2 [(gogoproto.stdduration) = true];


Thoughts on merging this into HttpUri? Everyone needs this.

Good point. I will do it.

mattklein123 · 2018-03-27T21:16:27Z

Also, heads up on this PR: envoyproxy/envoy#2915

saumoh

lgtm.
one minor nit (i think)

saumoh · 2018-03-27T21:15:53Z

envoy/config/filter/http/ext_authz/v2alpha/ext_authz.proto

+    envoy.api.v2.core.GrpcService grpc_service = 1;
+
+    // The external authorization HTTP service configuration.
+    HttpService http_service = 3;


nit: should it be 2.

Is it ok to re-index them? bool failure_mode_allow = 3?

you are right oneof doesn't encapsulate the messages under it. We can leave it as is.

kyessenov · 2018-03-27T21:31:46Z

envoy/config/filter/http/ext_authz/v2alpha/ext_authz.proto

+syntax = "proto3";
+
+package envoy.config.filter.http.ext_authz.v2alpha;
+option go_package = "v2";


can you please change this to v2alpha?

kyessenov · 2018-03-27T21:34:22Z

envoy/service/auth/v2alpha/external_auth.proto

@@ -2,13 +2,14 @@ syntax = "proto3";

 // [#proto-status: draft]

-package envoy.service.auth.v2;
+package envoy.service.auth.v2alpha;
 option go_package = "v2";


same here, v2alpha instead of v2

kyessenov

lgtm, a couple of nits

gsagula · 2018-03-27T22:40:13Z

Thanks, everyone for the reviews! I will do these changes later today.

Signed-off-by: Gabriel <[email protected]>

gsagula · 2018-03-28T07:57:50Z

Please, feel free to take another look. Thanks!

htuch

LGTM, one last question.

htuch · 2018-03-28T19:20:51Z

envoy/api/v2/core/http_uri.proto

@@ -34,4 +38,7 @@ message HttpUri {
    //
    string cluster = 2 [(validate.rules).string.min_bytes = 1];
  }
+
+  // Sets the maximum duration in milliseconds that a response can take to arrive upon request.


What's the default if not specified?

Would be better to make this required and >= 0? It's not clear to me when/how to enforce the default value.

mattklein123

LGTM other than @htuch comment. Thanks!

Signed-off-by: Gabriel <[email protected]>

This PR extends the current Ext_Authz filter to allow optional HTTP attributes being passed from the Authorization service down to client or, to the upstream services. I would like to get some feedback on the changes to the current gRPC async client and filter before moving to implementation of HTTP part of this extension and tests. *issue: #2828 Risk Level: Medium Testing: Manual, unit testing. Docs Changes: envoyproxy/data-plane-api#563 Signed-off-by: Gabriel <[email protected]>

This PR extends the current Ext_Authz filter to allow optional HTTP attributes being passed from the Authorization service down to client or, to the upstream services. I would like to get some feedback on the changes to the current gRPC async client and filter before moving to implementation of HTTP part of this extension and tests. *issue: #2828 Risk Level: Medium Testing: Manual, unit testing. Docs Changes: #563 Signed-off-by: Gabriel <[email protected]> Mirrored from https://github.com/envoyproxy/envoy @ 5244597e93c70b4945c03a9fc55f8924a2da6fbc

Gabriel added 8 commits March 15, 2018 00:36

authz_filter: added http response support

61e546f

Signed-off-by: Gabriel <[email protected]>

authz_filter: fixed proto field id

2c8f23c

Signed-off-by: Gabriel <[email protected]>

authz_filter: fixed typo

9810412

Signed-off-by: Gabriel <[email protected]>

authz_filter: added http status code and improved comments

e6c5342

Signed-off-by: Gabriel <[email protected]>

authz_filter: added configuration message definition to support http …

fdd6594

…service auth Signed-off-by: Gabriel <[email protected]>

authz_filter: modified comments

49dedbd

Signed-off-by: Gabriel <[email protected]>

authz_filter: changed top comment

3128f16

Signed-off-by: Gabriel <[email protected]>

authz_filter: changed http_status to status_code

339db0e

Signed-off-by: Gabriel <[email protected]>

ggreenway reviewed Mar 19, 2018

View reviewed changes

zuercher assigned ggreenway and htuch Mar 19, 2018

htuch suggested changes Mar 19, 2018

View reviewed changes

saumoh reviewed Mar 19, 2018

View reviewed changes

mattklein123 reviewed Mar 20, 2018

View reviewed changes

htuch changed the title ~~authz_filter: configuration to support Embassador authorization flow~~ authz_filter: configuration to support Ambassador authorization flow Mar 20, 2018

Gabriel added 4 commits March 21, 2018 00:16

authz_filter: wip code review changes

f663e11

Signed-off-by: Gabriel <[email protected]>

Merge branch 'master' into generalist_auth_filter

0f55cb6

Signed-off-by: Gabriel <[email protected]>

Merge branch 'master' into generalist_auth_filter

b0c3389

Signed-off-by: Gabriel <[email protected]>

authz_filter: code review changes

79d770a

Signed-off-by: Gabriel <[email protected]>

htuch reviewed Mar 26, 2018

View reviewed changes

Gabriel added 2 commits March 27, 2018 00:57

authz_filter: code review changes

f024a1b

Signed-off-by: Gabriel <[email protected]>

authz_filter: fixed bazel docs

a8db286

Signed-off-by: Gabriel <[email protected]>

mattklein123 reviewed Mar 27, 2018

View reviewed changes

saumoh reviewed Mar 27, 2018

View reviewed changes

kyessenov reviewed Mar 27, 2018

View reviewed changes

authz_filter: code review changes

e16fcdb

Signed-off-by: Gabriel <[email protected]>

htuch reviewed Mar 28, 2018

View reviewed changes

mattklein123 reviewed Mar 28, 2018

View reviewed changes

authz_filter: changed timeout validation

5eceb00

Signed-off-by: Gabriel <[email protected]>

htuch approved these changes Apr 2, 2018

View reviewed changes

htuch merged commit f88047c into envoyproxy:master Apr 2, 2018

gsagula mentioned this pull request Apr 23, 2018

authz_filter: extended ext_authz to support v2alpha api envoyproxy/envoy#3162

Merged


		// [#proto-status: draft]

		package envoy.service.auth.v2alpha;

authz_filter: configuration to support Ambassador authorization flow #563

authz_filter: configuration to support Ambassador authorization flow #563

Conversation

gsagula commented Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsagula Mar 20, 2018 • edited Loading

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

gsagula Mar 20, 2018 • edited Loading

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

mattklein123 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

gsagula commented Mar 20, 2018

kyessenov commented Mar 20, 2018

gsagula commented Mar 21, 2018

gsagula commented Mar 25, 2018

Choose a reason for hiding this comment

gsagula Mar 27, 2018 • edited Loading

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 commented Mar 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Mar 27, 2018

saumoh 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

kyessenov left a comment

Choose a reason for hiding this comment

gsagula commented Mar 27, 2018

gsagula commented Mar 28, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsagula Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

gsagula commented Mar 19, 2018 •

edited

Loading

gsagula Mar 20, 2018 •

edited

Loading

gsagula Mar 20, 2018 •

edited

Loading

gsagula Mar 27, 2018 •

edited

Loading

gsagula Mar 29, 2018 •

edited

Loading