Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
23 changes: 22 additions & 1 deletion envoy/config/filter/http/ext_authz/v2/ext_authz.proto
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import "validate/validate.proto";

// [#not-implemented-hide:]
// External Authorization filter calls out to an external service over the
// gRPC Authorization API defined by
// gRPC or HTTP Authorization API defined by
// :ref:`external_auth <envoy_api_msg_auth.CheckRequest>`.
// A failed check will cause this filter to return 403 Forbidden.
message ExtAuthz {
Expand All @@ -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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@gsagula gsagula Mar 20, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.


// The external authorization HTTP service configuration. Only applicable when `use_http_service`
// is true.
message HttpService {

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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


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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be a google.protobuf.Duration.


// 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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@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).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?


// Sets an optional prefix to the value of request header `path` in the authorization request.
string path_prefix = 4;
}
}
18 changes: 18 additions & 0 deletions envoy/service/auth/v2/external_auth.proto
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@gsagula gsagula Mar 20, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

// 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@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.

message HttpResponse {
// Http status code.
int32 status_code = 1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

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


// Http entity headers.
map<string, string> headers = 2;

// Http entity body.
string body = 3;
}
}