From 65ddc86b7e0464fce26a5b2d5d0d2cb0c53cd88f Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 7 May 2021 14:38:31 -0700 Subject: [PATCH 01/37] A41: xDS RBAC Support --- A41-xds-rbac.md | 199 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 199 insertions(+) create mode 100644 A41-xds-rbac.md diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md new file mode 100644 index 000000000..c2ae1619f --- /dev/null +++ b/A41-xds-rbac.md @@ -0,0 +1,199 @@ +A41: xDS RBAC Support +---- +* Author(s): [Eric Anderson](https://github.com/ejona86), [Ashitha + Santhosh](https://github.com/ashithasantosh) (RBAC engine behavior) +* Approver: markdroth +* Status: Draft {Draft, In Review, Ready for Implementation, Implemented} +* Implemented in: +* Last updated: 2021-05-07 +* Discussion at: (filled after thread exists) + +## Abstract + +Support the [xDS RBAC HTTP filter][RBAC filter] for service- and method-scoped +client authorization (authz) on xDS-enabled gRPC servers. + +[RBAC filter]: https://github.com/envoyproxy/envoy/blob/main/api/envoy/extensions/filters/http/rbac/v3/rbac.proto + +## Background + +[A29 xDS-Based Security][A29] introduced the ability to manage xDS-managed mTLS +meshes. As standard for TLS clients, the client verifies the server's identity +is permitted to run the service. This is normally known as "hostname +verification", but in a mesh scenario may be known as "server authorization" as +the server's certificate does not include the hostname but other identity +information. + +While servers authenticated clients (i.e., the client's identity was verified), +A29 did not include mechanism to limit which clients were authorized to access a +server and for which gRPC methods (i.e., whether the client was permitted). + +Envoy defines the [RBAC HTTP filter][RBAC filter] that is well-suited for +limiting which nodes can access which services within a mesh as encouraged by +the principal of least privilege. + +Resources in REST are present in the HTTP path so it would seem RBAC could be +used in non-gRPC environments as a precise service-level resource authz engine. +However, this 1) requires another filter to perform end-user authentication (as +TLS just provides service-level authn) and 2) does not scale as RBAC provides no +mechanism for loading rules on-demand. Since gRPC encodes service-level +resource identifiers in the request message, RBAC does not have access to the +requested resource for gRPC traffic. + +### Related Proposals: + +* [A36: xDS-Enabled Servers][A36] +* [A29: xDS-Based Security for gRPC Clients and Servers][A29] +* [A39: xDS HTTP Filter Support][A39] + +[A36]: https://github.com/grpc/proposal/blob/master/A36-xds-for-servers.md + +[A29]: https://github.com/grpc/proposal/pull/184 +[A39]: https://github.com/grpc/proposal/blob/master/A39-xds-http-filters.md + +## Proposal + +Support the [xDS RBAC HTTP filter][RBAC filter] as a registered filter type on +xDS-enabled gRPC Servers, and support its `RBACPerRoute` configuration override. +This does _not_ include [RBAC network filter][] support nor running the filter +on gRPC clients. xDS v2 support is not necessary. + +Supporting the RBAC HTTP filter on server-side leverages [A36: xDS-Enabled +Servers][A36] and [A39: xDS HTTP Filter Support][A39]. Server-side HTTP filters +are less common than client-side at this point in time, so this may be the first +usage of `RouteConfiguration` on server-side and so likely involves adding more +complete support for A39 by creating+executing filters on server-side and +processing `VirtualHost`s and `RoutMatch`es to determine which configuration +should be provided to each filter. A39 should be consulted for the expected +behavior. + +The core policy matching logic should be split into an "RBAC engine" to allow +reuse with non-xDS environments. However, there is no requirement that the RBAC +engine have a specialized API; it could simply be an interceptor. + +The [RBAC policy][] has many fields. All current (Envoy-implemented) fields will +be considered in gRPC. However, some fields may have pre-determined values or +behavior. At this time, if the `RBAC.action` is `Action.LOG` then the policy +will be completely ignored, as if RBAC was not configurated. CEL fields are not +supported, so if `Policy.condition` or `Policy.checked_condition` is present the +XdsClient must NACK the configuration. + +The following fields of `Permission` may not be obvious how they map to gRPC: + +| Permission Field | gRPC Equivalent | +| ---------------- | --------------- | +| header | Metadata (with caveats) | +| url_path | Fully-qualified RPC method name with leading slash | +| destination_ip | Local address for this connection | +| destination_port | Local port for this connection | +| metadata | Hard-coded as empty; never matches | +| requested_server_name | Hard-coded as empty string | + +Be strongly aware that Envoy Metadata has no relation to gRPC's Metadata. +Because matching supports NOT, the matcher must still be processed even if a +rule contains references to things that don't generally match; it is not trivial +to "optimize out" the never-matching rules. + +The `header` field is not entirely 1:1 with gRPC Metadata. To begin with, gRPC +Metadata isn't 100% consistent cross-language in its handling of hop-to-hop +headers (e.g., `TE`) and pseudo-headers. For this design, `headers` can include +`:method`, `:scheme`, `:authority`, and`:path` matchers and they must match the +values received on-the-wire independent of whether they are stored in Metadata +or in separate APIs. It is unspecified whether hop-to-hop headers are matched. +Multi-valued metadata is represented as the concatenation of the values along +with a `,` (comma, no added spaces) separator, as permitted by HTTP and gRPC. +Binary headers are represented in their base64-encoded form, although we rarely +expect binary header matchers. + +TODO: Clearly define differences compared to client-side. Align with client-side +routing (maybe changing behavior) to allow reusing code, have a strong grip on +client vs server differences, and notice 'gotcha's. Content-Type is probably +not a problem on server-side. + +As documented for `HeaderMatcher`, Envoy aliases `:authority` and `Host` in its +header map implementation, so they should be treated equivalent for the RBAC +matchers; there must be no behavior change depending on which of the two header +names is used in the _RBAC policy_. + +TODO: We've had no reason to care about :scheme in our APIs and so is not +present in all languages. It could be inferred from whether the connection used +security, but that's technically different from :scheme, as if you go through a +TLS-terminating proxy and use plaintext to the backend the :scheme is used to +re-create the original URL which would be 'https', not 'http'. This may be a +problem for Java and Go. We can maybe copy whatever Envoy does for HTTP/1 +traffic. You might think :scheme doesn't matter, and maybe it doesn't, but it +might be used to detect "is the client using TLS". Although note that in that +case such a check is trusting the client to provide the correct value. + +TODO: Need to audit that server implementations check that :method=POST before +accepting the RPC. This was already a requirement but it is an appropriate time +to double-check. + +TODO: Determine how to handle _requests_ (not policies) that have Host header, +since gRPC generally doesn't observe this header yet the RBAC policy could. +Options: deny such requests, move Host header to :authority, drop Host header. +Host and :authority could also disagree, and that needs to be handled. + +TODO: Should MetadataMatcher's NullMatch match? Need to check not present vs +present-but-null behavior. + +Note that `requested_server_name` can match if the matcher accepts empty string. +TODO: does Envoy default to empty string or null? + +The following fields of `Principal` may not be obvious how they map to gRPC: + +| Principal Field | gRPC Equivalent | +| ---------------- | --------------- | +| authenticated.principal_name | The first URI/DNS SAN; same as Envoy | +| source_ip | Peer address for this connection | +| direct_remote_ip | Peer address for this connection | +| remote_ip | Peer address for this connection | +| header | Metadata (with caveats) | +| url_path | Fully-qualified RPC method name with leading slash | +| metadata | Hard-coded as empty; never matches | + +TODO: Need to NACK to be consistent on `remote_ip` `source_ip` if +x-forwarded-for, proxy protocol, etc is configured. + +[RBAC policy]: https://github.com/envoyproxy/envoy/blob/10c17a7cd90b013c38dfbfbf715d3c24fdd0477c/api/envoy/config/rbac/v3/rbac.proto +[RBAC network filter]: https://github.com/envoyproxy/envoy/blob/main/api/envoy/extensions/filters/network/rbac/v3/rbac.proto + +### Temporary environment variable protection + +The environment variable `GRPC_XDS_EXPERIMENTAL_RBAC` will be used to gate +the feature until it is deemed stable for a particular implementation. If unset +or not `true`, all RBAC logic presented in this gRFC will not come into effect. + +## Rationale + +Few design decisions were necessary; RBAC was a pretty strong fit with gRPC's +needs for an initial authorization policy. The name seems like a misnomer as it +lacks a level of indirection (the "role") between principals and permissions as +would be expected from "role based access control", but that is a fair design +decision and does not impact applicability at this time. + +Fuller-fledged authz policy supporting end-user authorization for +application-specific resources would be useful, but needs to be future work as +it has multiple technical challenges including need for delayed authz policy +loading (i.e., delegating to a remote authz server) to be able scale, a way to +extract the application-level resource from the request, and support for +authenticating end users. + +Envoy also defines an [RBAC network filter][]. Such a filter is helpful for TCP +or non-HTTP proxying, but could be used for HTTP traffic. Such a network filter +has the advantage over an HTTP filter as it would 1) have lower ALLOW overhead +as the check is once per connection and 2) reduce the attack surface to +malicious clients. When used with HTTP/2, however, avoiding wildly out-of-date +authz checks would require limiting the lifetime of HTTP/2 connections. In +addition, there is no way to report clear error messages to the client. Since +it is unable to support path-based verification it would likely need to be used +in conjunction with an RBAC HTTP filter for usage in gRPC. It may be useful to +support one day, but would most likely be a more advanced feature. + +## Implementation + +[A description of the steps in the implementation, who will do them, and when. If a particular language is going to get the implementation first, this section should list the proposed order.] + +## Open issues (if applicable) + +[A discussion of issues relating to this proposal for which the author does not know the solution. This section may be omitted if there are none.] From a9f74421bd09a0935880dcb278483a1c9cc05bd6 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Thu, 20 May 2021 15:42:07 -0700 Subject: [PATCH 02/37] Nail down more details for matchers --- A41-xds-rbac.md | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index c2ae1619f..e8f6db317 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -82,14 +82,13 @@ The following fields of `Permission` may not be obvious how they map to gRPC: | Permission Field | gRPC Equivalent | | ---------------- | --------------- | -| header | Metadata (with caveats) | +| header | Metadata (with caveats below) | | url_path | Fully-qualified RPC method name with leading slash | | destination_ip | Local address for this connection | | destination_port | Local port for this connection | | metadata | Hard-coded as empty; never matches | | requested_server_name | Hard-coded as empty string | -Be strongly aware that Envoy Metadata has no relation to gRPC's Metadata. Because matching supports NOT, the matcher must still be processed even if a rule contains references to things that don't generally match; it is not trivial to "optimize out" the never-matching rules. @@ -134,11 +133,11 @@ since gRPC generally doesn't observe this header yet the RBAC policy could. Options: deny such requests, move Host header to :authority, drop Host header. Host and :authority could also disagree, and that needs to be handled. -TODO: Should MetadataMatcher's NullMatch match? Need to check not present vs -present-but-null behavior. +`metadata` will never match as `ValueMatcher` can only match if the value is +present (even `NullMatch`). Be strongly aware that Envoy Metadata has no +relation to gRPC's Metadata. -Note that `requested_server_name` can match if the matcher accepts empty string. -TODO: does Envoy default to empty string or null? +`requested_server_name` can match if the matcher accepts empty string. The following fields of `Principal` may not be obvious how they map to gRPC: @@ -148,9 +147,9 @@ The following fields of `Principal` may not be obvious how they map to gRPC: | source_ip | Peer address for this connection | | direct_remote_ip | Peer address for this connection | | remote_ip | Peer address for this connection | -| header | Metadata (with caveats) | -| url_path | Fully-qualified RPC method name with leading slash | -| metadata | Hard-coded as empty; never matches | +| header | Same as in Permission | +| url_path | Same as in Permission | +| metadata | Same as in Permission | TODO: Need to NACK to be consistent on `remote_ip` `source_ip` if x-forwarded-for, proxy protocol, etc is configured. From 48c3a2c2756c8dcc215e5e377ec1af1609cdbcbf Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Thu, 20 May 2021 15:42:30 -0700 Subject: [PATCH 03/37] Link to hop-by-hop documentation for those unlikely to read the HTTP spec on weekends as a way to relax --- A41-xds-rbac.md | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index e8f6db317..7644b5c54 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -94,16 +94,20 @@ rule contains references to things that don't generally match; it is not trivial to "optimize out" the never-matching rules. The `header` field is not entirely 1:1 with gRPC Metadata. To begin with, gRPC -Metadata isn't 100% consistent cross-language in its handling of hop-to-hop -headers (e.g., `TE`) and pseudo-headers. For this design, `headers` can include -`:method`, `:scheme`, `:authority`, and`:path` matchers and they must match the -values received on-the-wire independent of whether they are stored in Metadata -or in separate APIs. It is unspecified whether hop-to-hop headers are matched. +Metadata isn't 100% consistent cross-language in its handling of [hop-by-hop +headers][] (e.g., `TE`; RFC 2616 has [a convenient list][hop-by-hop header +list]) and pseudo-headers. For this design, `headers` can include `:method`, +`:scheme`, `:authority`, and`:path` matchers and they must match the values +received on-the-wire independent of whether they are stored in Metadata or in +separate APIs. It is unspecified whether hop-by-hop headers are matched. Multi-valued metadata is represented as the concatenation of the values along with a `,` (comma, no added spaces) separator, as permitted by HTTP and gRPC. Binary headers are represented in their base64-encoded form, although we rarely expect binary header matchers. +[hop-by-hop headers]: https://datatracker.ietf.org/doc/html/rfc7230#section-6.1 +[hop-by-hop header list]: https://datatracker.ietf.org/doc/html/rfc2616#section-13.5.1 + TODO: Clearly define differences compared to client-side. Align with client-side routing (maybe changing behavior) to allow reusing code, have a strong grip on client vs server differences, and notice 'gotcha's. Content-Type is probably From 203a54714ec4755504d675aee8ad2c38fbb59245 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 21 May 2021 10:46:47 -0700 Subject: [PATCH 04/37] Define behavior for more headers --- A41-xds-rbac.md | 31 ++++++++----------------------- 1 file changed, 8 insertions(+), 23 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 7644b5c54..8194b0e4e 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -97,41 +97,26 @@ The `header` field is not entirely 1:1 with gRPC Metadata. To begin with, gRPC Metadata isn't 100% consistent cross-language in its handling of [hop-by-hop headers][] (e.g., `TE`; RFC 2616 has [a convenient list][hop-by-hop header list]) and pseudo-headers. For this design, `headers` can include `:method`, -`:scheme`, `:authority`, and`:path` matchers and they must match the values +`:scheme`, `:authority`, and`:path` matchers and they should match the values received on-the-wire independent of whether they are stored in Metadata or in -separate APIs. It is unspecified whether hop-by-hop headers are matched. +separate APIs. `:scheme` is not universally available in gRPC APIs, so it may be +hard-coded to `http` if unavailable. `:method` can be hard-coded to `POST` if +unavailable and a code audit confirms the server denies requests for all other +method types. It is unspecified whether hop-by-hop headers are matched. Multi-valued metadata is represented as the concatenation of the values along with a `,` (comma, no added spaces) separator, as permitted by HTTP and gRPC. -Binary headers are represented in their base64-encoded form, although we rarely -expect binary header matchers. +The Content-Type provided by the client must be used; not a hard-coded value. +(TODO: support binary headers?) Binary headers are represented in their +base64-encoded form, although we rarely expect binary header matchers. [hop-by-hop headers]: https://datatracker.ietf.org/doc/html/rfc7230#section-6.1 [hop-by-hop header list]: https://datatracker.ietf.org/doc/html/rfc2616#section-13.5.1 -TODO: Clearly define differences compared to client-side. Align with client-side -routing (maybe changing behavior) to allow reusing code, have a strong grip on -client vs server differences, and notice 'gotcha's. Content-Type is probably -not a problem on server-side. - As documented for `HeaderMatcher`, Envoy aliases `:authority` and `Host` in its header map implementation, so they should be treated equivalent for the RBAC matchers; there must be no behavior change depending on which of the two header names is used in the _RBAC policy_. -TODO: We've had no reason to care about :scheme in our APIs and so is not -present in all languages. It could be inferred from whether the connection used -security, but that's technically different from :scheme, as if you go through a -TLS-terminating proxy and use plaintext to the backend the :scheme is used to -re-create the original URL which would be 'https', not 'http'. This may be a -problem for Java and Go. We can maybe copy whatever Envoy does for HTTP/1 -traffic. You might think :scheme doesn't matter, and maybe it doesn't, but it -might be used to detect "is the client using TLS". Although note that in that -case such a check is trusting the client to provide the correct value. - -TODO: Need to audit that server implementations check that :method=POST before -accepting the RPC. This was already a requirement but it is an appropriate time -to double-check. - TODO: Determine how to handle _requests_ (not policies) that have Host header, since gRPC generally doesn't observe this header yet the RBAC policy could. Options: deny such requests, move Host header to :authority, drop Host header. From f782dae93227212359f833200a4a149400cd1887 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 21 May 2021 11:00:29 -0700 Subject: [PATCH 05/37] Grammar fixes --- A41-xds-rbac.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 8194b0e4e..57f839251 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -30,7 +30,7 @@ server and for which gRPC methods (i.e., whether the client was permitted). Envoy defines the [RBAC HTTP filter][RBAC filter] that is well-suited for limiting which nodes can access which services within a mesh as encouraged by -the principal of least privilege. +the principle of least privilege. Resources in REST are present in the HTTP path so it would seem RBAC could be used in non-gRPC environments as a precise service-level resource authz engine. @@ -62,7 +62,7 @@ Supporting the RBAC HTTP filter on server-side leverages [A36: xDS-Enabled Servers][A36] and [A39: xDS HTTP Filter Support][A39]. Server-side HTTP filters are less common than client-side at this point in time, so this may be the first usage of `RouteConfiguration` on server-side and so likely involves adding more -complete support for A39 by creating+executing filters on server-side and +complete support for A39 by creating and executing filters on server-side and processing `VirtualHost`s and `RoutMatch`es to determine which configuration should be provided to each filter. A39 should be consulted for the expected behavior. @@ -163,8 +163,8 @@ decision and does not impact applicability at this time. Fuller-fledged authz policy supporting end-user authorization for application-specific resources would be useful, but needs to be future work as it has multiple technical challenges including need for delayed authz policy -loading (i.e., delegating to a remote authz server) to be able scale, a way to -extract the application-level resource from the request, and support for +loading (i.e., delegating to a remote authz server) to be able to scale, a way +to extract the application-level resource from the request, and support for authenticating end users. Envoy also defines an [RBAC network filter][]. Such a filter is helpful for TCP From 2a30ba66e2a0bdaccbae1809142f0fdd3c08eafd Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 21 May 2021 11:09:01 -0700 Subject: [PATCH 06/37] Add small explanation of Envoy metadata --- A41-xds-rbac.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 57f839251..929fc7d56 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -124,7 +124,8 @@ Host and :authority could also disagree, and that needs to be handled. `metadata` will never match as `ValueMatcher` can only match if the value is present (even `NullMatch`). Be strongly aware that Envoy Metadata has no -relation to gRPC's Metadata. +relation to gRPC Metadata. Envoy Metadata is generic state shared between +filters which has no gRPC equivalent. `requested_server_name` can match if the matcher accepts empty string. From 0eb14ca95cab58939a1d15664ef381201f122c2e Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 21 May 2021 11:27:19 -0700 Subject: [PATCH 07/37] Small clarifications --- A41-xds-rbac.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 929fc7d56..912097f66 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -83,7 +83,7 @@ The following fields of `Permission` may not be obvious how they map to gRPC: | Permission Field | gRPC Equivalent | | ---------------- | --------------- | | header | Metadata (with caveats below) | -| url_path | Fully-qualified RPC method name with leading slash | +| url_path | Fully-qualified RPC method name with leading slash. Same as `:path` header | | destination_ip | Local address for this connection | | destination_port | Local port for this connection | | metadata | Hard-coded as empty; never matches | @@ -133,7 +133,7 @@ The following fields of `Principal` may not be obvious how they map to gRPC: | Principal Field | gRPC Equivalent | | ---------------- | --------------- | -| authenticated.principal_name | The first URI/DNS SAN; same as Envoy | +| authenticated.principal_name | The first URI/DNS Subject Alternative Name in the client's certificate; same as Envoy | | source_ip | Peer address for this connection | | direct_remote_ip | Peer address for this connection | | remote_ip | Peer address for this connection | @@ -151,7 +151,7 @@ x-forwarded-for, proxy protocol, etc is configured. The environment variable `GRPC_XDS_EXPERIMENTAL_RBAC` will be used to gate the feature until it is deemed stable for a particular implementation. If unset -or not `true`, all RBAC logic presented in this gRFC will not come into effect. +or not `true`, all logic presented in this gRFC will not come into effect. ## Rationale From bdece2b8e68184541f9a1fee83e48bf086c9a8a1 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Mon, 24 May 2021 13:38:38 -0700 Subject: [PATCH 08/37] Substantially change authenticated.principal_name This now actually matches Envoy. It is unclear where the original definition was derived from. --- A41-xds-rbac.md | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 912097f66..4a3ae0365 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -133,7 +133,7 @@ The following fields of `Principal` may not be obvious how they map to gRPC: | Principal Field | gRPC Equivalent | | ---------------- | --------------- | -| authenticated.principal_name | The first URI/DNS Subject Alternative Name in the client's certificate; same as Envoy | +| authenticated.principal_name | The URI/DNS SAN or Subject; same as Envoy | | source_ip | Peer address for this connection | | direct_remote_ip | Peer address for this connection | | remote_ip | Peer address for this connection | @@ -141,6 +141,19 @@ The following fields of `Principal` may not be obvious how they map to gRPC: | url_path | Same as in Permission | | metadata | Same as in Permission | +The `authenticated.principal_name` will use the same definition as its +`rbac.proto` comment, although it checks multiple values which isn't clear from +the comment. All values being checked are derived from the client's certificate. +The process is: + +1. Check if any SubjectAltName entry with URI type (type 6) matches. If any + entry matches, the `principal_name` is said to match +2. If there is no SAN with URI type, check if any SAN entry with DNS type (type + 2) matches. If any entry matches, the `principal_name` is said to match +3. If there are no SAN with URI or DNS types, check if the Subject's + distinguished name formatted as an RFC 2253 Name matches. If it matches, the + `principal_name` is said to match + TODO: Need to NACK to be consistent on `remote_ip` `source_ip` if x-forwarded-for, proxy protocol, etc is configured. From be394792b4e3aa94c2b99066dc0d550e8a72a303 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Mon, 24 May 2021 13:50:09 -0700 Subject: [PATCH 09/37] TODO grpc-prefixed headers as well --- A41-xds-rbac.md | 1 + 1 file changed, 1 insertion(+) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 4a3ae0365..f9ffa3566 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -108,6 +108,7 @@ with a `,` (comma, no added spaces) separator, as permitted by HTTP and gRPC. The Content-Type provided by the client must be used; not a hard-coded value. (TODO: support binary headers?) Binary headers are represented in their base64-encoded form, although we rarely expect binary header matchers. +(TODO: what about 'grpc-'-prefixed headers?) [hop-by-hop headers]: https://datatracker.ietf.org/doc/html/rfc7230#section-6.1 [hop-by-hop header list]: https://datatracker.ietf.org/doc/html/rfc2616#section-13.5.1 From aa777e8f94142b6d980fb31f6bc278a436ab321f Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Mon, 24 May 2021 13:50:40 -0700 Subject: [PATCH 10/37] Define xff_num_trusted_hops behavior --- A41-xds-rbac.md | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index f9ffa3566..c4f76b1c4 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -67,6 +67,12 @@ processing `VirtualHost`s and `RoutMatch`es to determine which configuration should be provided to each filter. A39 should be consulted for the expected behavior. +New validation should occur for `HttpConnectionManager` to allow equating +`direct_remote_ip` and `remote_ip`. If the implementation does not distinguish +between these fields, then `xff_num_trusted_hops` must be verified to be unset +or zero. If it is any other value, the Listener must be NACKed. (TODO: Do this +only on server-side, or both client and server?) + The core policy matching logic should be split into an "RBAC engine" to allow reuse with non-xDS environments. However, there is no requirement that the RBAC engine have a specialized API; it could simply be an interceptor. @@ -155,11 +161,17 @@ The process is: distinguished name formatted as an RFC 2253 Name matches. If it matches, the `principal_name` is said to match -TODO: Need to NACK to be consistent on `remote_ip` `source_ip` if -x-forwarded-for, proxy protocol, etc is configured. +`source_ip` is the same as `direct_remote_ip` only as long as +[envoy.extensions.filters.listener.proxy_protocol.v3.ProxyProtocol][] is +unsupported. If a future gRFC adds ProxyProtocol support it must also update +`source_ip` and `remote_ip` handling in RBAC. + +`remote_ip` is the same as `direct_remote_ip` only as long as ProxyProtocol and +`xff_num_trusted_hops` are unsupported. [RBAC policy]: https://github.com/envoyproxy/envoy/blob/10c17a7cd90b013c38dfbfbf715d3c24fdd0477c/api/envoy/config/rbac/v3/rbac.proto [RBAC network filter]: https://github.com/envoyproxy/envoy/blob/main/api/envoy/extensions/filters/network/rbac/v3/rbac.proto +[envoy.extensions.filters.listener.proxy_protocol.v3.ProxyProtocol]: https://github.com/envoyproxy/envoy/blob/main/api/envoy/extensions/filters/listener/proxy_protocol/v3/proxy_protocol.proto ### Temporary environment variable protection From f3486d6bad414f3b7df0738b7c46b7fde9ff657a Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Wed, 26 May 2021 15:42:49 -0700 Subject: [PATCH 11/37] Binary headers and supported and grpc- headers are not --- A41-xds-rbac.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index c4f76b1c4..8037a492f 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -81,8 +81,11 @@ The [RBAC policy][] has many fields. All current (Envoy-implemented) fields will be considered in gRPC. However, some fields may have pre-determined values or behavior. At this time, if the `RBAC.action` is `Action.LOG` then the policy will be completely ignored, as if RBAC was not configurated. CEL fields are not -supported, so if `Policy.condition` or `Policy.checked_condition` is present the -XdsClient must NACK the configuration. +supported, so `Policy.condition` and `Policy.checked_condition` must cause a +validation failure if present. It is also a validation failure if Permission or +Principal has a `header` matcher for a `grpc-`-prefixed header name. As +described in A39, validation failures for filter configuration causes the +listener to be NACKed. The following fields of `Permission` may not be obvious how they map to gRPC: @@ -112,9 +115,8 @@ method types. It is unspecified whether hop-by-hop headers are matched. Multi-valued metadata is represented as the concatenation of the values along with a `,` (comma, no added spaces) separator, as permitted by HTTP and gRPC. The Content-Type provided by the client must be used; not a hard-coded value. -(TODO: support binary headers?) Binary headers are represented in their -base64-encoded form, although we rarely expect binary header matchers. -(TODO: what about 'grpc-'-prefixed headers?) +Binary headers are represented in their base64-encoded form, although we rarely +expect binary header matchers other than presence-checking. [hop-by-hop headers]: https://datatracker.ietf.org/doc/html/rfc7230#section-6.1 [hop-by-hop header list]: https://datatracker.ietf.org/doc/html/rfc2616#section-13.5.1 From 88f00d5dbe787c2b830c06212893360213e2fa0f Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Wed, 26 May 2021 15:43:16 -0700 Subject: [PATCH 12/37] Add to TODOs --- A41-xds-rbac.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 8037a492f..6147791d9 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -71,7 +71,9 @@ New validation should occur for `HttpConnectionManager` to allow equating `direct_remote_ip` and `remote_ip`. If the implementation does not distinguish between these fields, then `xff_num_trusted_hops` must be verified to be unset or zero. If it is any other value, the Listener must be NACKed. (TODO: Do this -only on server-side, or both client and server?) +only on server-side, or both client and server? Or just implement the feature?) + +TODO: Support RDS The core policy matching logic should be split into an "RBAC engine" to allow reuse with non-xDS environments. However, there is no requirement that the RBAC From 27ba988b7ef7483a11d337c2d6ee7452b2dc294e Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Mon, 7 Jun 2021 16:17:02 -0700 Subject: [PATCH 13/37] Default authenticated behavior for unset matchers and missing client cert --- A41-xds-rbac.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 6147791d9..6ebbf99c9 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -154,8 +154,9 @@ The following fields of `Principal` may not be obvious how they map to gRPC: The `authenticated.principal_name` will use the same definition as its `rbac.proto` comment, although it checks multiple values which isn't clear from -the comment. All values being checked are derived from the client's certificate. -The process is: +the comment. If `principal_name` is unset, then `Authenticated` is said to match +if the connection uses TLS; a client certificate is not necessary. Other +values being checked are derived from the client's certificate. The process is: 1. Check if any SubjectAltName entry with URI type (type 6) matches. If any entry matches, the `principal_name` is said to match @@ -164,6 +165,8 @@ The process is: 3. If there are no SAN with URI or DNS types, check if the Subject's distinguished name formatted as an RFC 2253 Name matches. If it matches, the `principal_name` is said to match +4. If there is no client certificate (thus no SAN nor Subject), check if `""` + (empty string) matches. If it matches, the `principal_name` is said to match `source_ip` is the same as `direct_remote_ip` only as long as [envoy.extensions.filters.listener.proxy_protocol.v3.ProxyProtocol][] is From 2ab6d4a60ad5f21c528e57970aa8894b1f75376a Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Wed, 9 Jun 2021 14:32:33 -0700 Subject: [PATCH 14/37] Do not match hop-by-hop headers --- A41-xds-rbac.md | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 6ebbf99c9..22f5a82fa 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -104,24 +104,26 @@ Because matching supports NOT, the matcher must still be processed even if a rule contains references to things that don't generally match; it is not trivial to "optimize out" the never-matching rules. -The `header` field is not entirely 1:1 with gRPC Metadata. To begin with, gRPC -Metadata isn't 100% consistent cross-language in its handling of [hop-by-hop -headers][] (e.g., `TE`; RFC 2616 has [a convenient list][hop-by-hop header -list]) and pseudo-headers. For this design, `headers` can include `:method`, -`:scheme`, `:authority`, and`:path` matchers and they should match the values -received on-the-wire independent of whether they are stored in Metadata or in -separate APIs. `:scheme` is not universally available in gRPC APIs, so it may be -hard-coded to `http` if unavailable. `:method` can be hard-coded to `POST` if -unavailable and a code audit confirms the server denies requests for all other -method types. It is unspecified whether hop-by-hop headers are matched. -Multi-valued metadata is represented as the concatenation of the values along -with a `,` (comma, no added spaces) separator, as permitted by HTTP and gRPC. -The Content-Type provided by the client must be used; not a hard-coded value. -Binary headers are represented in their base64-encoded form, although we rarely -expect binary header matchers other than presence-checking. +The `header` field is not entirely 1:1 with gRPC Metadata, in part because which +HTTP headers are present in Metadata is not 100% consistent cross-language. +For this design, `headers` can include `:method`, `:scheme`, `:authority`, +and`:path` matchers and they should match the values received on-the-wire +independent of whether they are stored in Metadata or in separate APIs. +`:scheme` is not universally available in gRPC APIs, so it may be hard-coded to +`http` if unavailable. `:method` can be hard-coded to `POST` if unavailable and +a code audit confirms the server denies requests for all other method types. +Implementations must not match [hop-by-hop headers][]. Since hop-by-hop headers +[are not used in HTTP/2 except for `te: trailers`][rfc7540 connection header], +implementations must ensure they are disallowing the `Connection` header and not +matching the `TE` header. Multi-valued metadata is represented as the +concatenation of the values along with a `,` (comma, no added spaces) separator, +as permitted by HTTP and gRPC. The `Content-Type` provided by the client must be +used; not a hard-coded value. Binary headers are represented in their +base64-encoded form, although we rarely expect binary header matchers other than +presence-checking. [hop-by-hop headers]: https://datatracker.ietf.org/doc/html/rfc7230#section-6.1 -[hop-by-hop header list]: https://datatracker.ietf.org/doc/html/rfc2616#section-13.5.1 +[rfc7540 connection header]: https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2.2 As documented for `HeaderMatcher`, Envoy aliases `:authority` and `Host` in its header map implementation, so they should be treated equivalent for the RBAC From 6228f60486da8a58db5d6a9ca4c459ed1f7b1962 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 11 Jun 2021 08:47:45 -0700 Subject: [PATCH 15/37] Add RouteConfiguration handling to A36-xds-for-servers A36 had "if you don't use RouteConfiguration" language but didn't say what to do if "you do use RouteConfiguration" so was incomplete. Adding the language there is less confusing and easier to find than adding it to A41-xds-rbac --- A36-xds-for-servers.md | 37 +++++++++++++++++++++++++++++++++++-- A41-xds-rbac.md | 2 -- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/A36-xds-for-servers.md b/A36-xds-for-servers.md index 6b6a89e0a..6a02b3f7e 100644 --- a/A36-xds-for-servers.md +++ b/A36-xds-for-servers.md @@ -181,8 +181,6 @@ Envoy's behavior. HttpConnectionManager support is required. HttpConnectionManager must have valid `http_filters`, as defined by [A39: xDS HTTP Filter Support][A39]. -RouteConfiguration (via `route_config` or indirect fields like `rds`) is not -used nor validated at this time, but may be in the future. If the `envoy.extensions.filters.http.router.v3.Router` is not present in `http_filters`, A39 calls for inserting a special filter that fails all RPCs. If @@ -193,8 +191,43 @@ required. This is to allow implementations that only support L4 xDS features to avoid L7 plumbing and implementation. This has no impact on the resource validation and NACKing behavior called for in A39. +If an XdsServer implementation uses RouteConfiguration or supports any HTTP +filters other than the hard-coded Router, then +`HttpConnectionManager.route_config` and `HttpConnectionManager.rds` must be +supported and RouteConfigurations must be validated. RouteConfiguration +validation logic inherits all previous validations made for client-side usage +as RDS does not distinguish between client-side and server-side. That is +predomenently defined in [gRFC A28][A28-validation], although note that +configuration for all VirtualHosts have been validated on client-side since +sharing the XdsClient was introduced, yet was not documented in a gRFC. The +validation must be updated to allow [Route.non_forwarding_action][] as a valid +`action`. The VirtualHost is selected on a per-RPC basis using the RPC's +requested `:authority`. Routes are matched the same as on client-side. +`Route.non_forwarding_action` is expected for all Routes used on server-side and +`Route.route` continues to be expected for all Routes used on client-side; a +Route with an inappropriate `action` causes RPCs matching that route to fail. + +[Like in Envoy][envoy lds], updates to a Listener cause all older connections on +that Listener to be gracefully shut down with a grace period of 10 minutes for +long-lived RPCs, such that clients will reconnect and have the updated +configuration apply. This applies equally to an update in a RouteConfiguration +provided inline via the `route_config` field as it is part of the Listener, but +it does not apply to an updated RouteConfiguration provided by reference via +`rds` field. Infrastructure for "not serving" mode may be used for the graceful +shutdown, but only if it can be used without causing spurious RPC or connection +failures. Applying updates to a Listener should be delayed until dependent +resources have been attempted to be loaded (e.g., via RDS). The existing +resource loading timeout in XdsClient prevents the update from being delayed +indefinitely. + +TODO: drain time is a command line flag for Envoy, not provided via xds. +https://www.envoyproxy.io/docs/envoy/latest/operations/cli#cmdoption-drain-time-s + [Filter.typed_config]: https://github.com/envoyproxy/envoy/blob/928a62b7a12c4d87ce215a7c4ebd376f69c2e080/api/envoy/config/listener/v3/listener_components.proto#L40 [TypedStruct.type_url]: https://github.com/cncf/udpa/blob/cc1b757b3eddccaaaf0743cbb107742bb7e3ee4f/udpa/type/v1/typed_struct.proto#L38 +[A28-validation]: https://github.com/grpc/proposal/blob/master/A28-xds-traffic-splitting-and-routing.md#response-validation +[Route.non_forwarding_action]: https://github.com/envoyproxy/envoy/blob/5963beae8842982803af1bef04fb5a2a0893c613/api/envoy/config/route/v3/route_components.proto#L242 +[envoy lds]: https://www.envoyproxy.io/docs/envoy/latest/configuration/listeners/lds ### FilterChainMatch diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 22f5a82fa..21aedbac9 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -73,8 +73,6 @@ between these fields, then `xff_num_trusted_hops` must be verified to be unset or zero. If it is any other value, the Listener must be NACKed. (TODO: Do this only on server-side, or both client and server? Or just implement the feature?) -TODO: Support RDS - The core policy matching logic should be split into an "RBAC engine" to allow reuse with non-xDS environments. However, there is no requirement that the RBAC engine have a specialized API; it could simply be an interceptor. From 392960ea2f5d41bd6a7f46a0dac7df4d2f45056e Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 11 Jun 2021 13:13:10 -0700 Subject: [PATCH 16/37] Clarify there will not be a public RBAC-authz API --- A41-xds-rbac.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 21aedbac9..c5cbdeae3 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -74,8 +74,10 @@ or zero. If it is any other value, the Listener must be NACKed. (TODO: Do this only on server-side, or both client and server? Or just implement the feature?) The core policy matching logic should be split into an "RBAC engine" to allow -reuse with non-xDS environments. However, there is no requirement that the RBAC -engine have a specialized API; it could simply be an interceptor. +internal reuse with a non-xDS API. Any non-xDS API will not be RBAC, so this +reuse is merely an implementation detail. The API is not part of this gRFC. +There is no requirement that the RBAC engine have a specialized API; it could +simply be an interceptor. The [RBAC policy][] has many fields. All current (Envoy-implemented) fields will be considered in gRPC. However, some fields may have pre-determined values or From a9d55912b429b940a285358df020a3bbee10e5a7 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 11 Jun 2021 13:19:52 -0700 Subject: [PATCH 17/37] Add mailing list link and update other headers --- A41-xds-rbac.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index c5cbdeae3..8b607dfb2 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -1,12 +1,12 @@ A41: xDS RBAC Support ---- * Author(s): [Eric Anderson](https://github.com/ejona86), [Ashitha - Santhosh](https://github.com/ashithasantosh) (RBAC engine behavior) + Santhosh](https://github.com/ashithasantosh) * Approver: markdroth -* Status: Draft {Draft, In Review, Ready for Implementation, Implemented} +* Status: In Review {Draft, In Review, Ready for Implementation, Implemented} * Implemented in: -* Last updated: 2021-05-07 -* Discussion at: (filled after thread exists) +* Last updated: 2021-06-11 +* Discussion at: https://groups.google.com/g/grpc-io/c/DQJfShLTrTQ/ ## Abstract From b9cbab4a622ff95aaae76d61f480924c520b25e0 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 11 Jun 2021 13:54:30 -0700 Subject: [PATCH 18/37] Reword REST+resource-names-in-path+RBAC background --- A41-xds-rbac.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 8b607dfb2..b7625a05b 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -32,13 +32,13 @@ Envoy defines the [RBAC HTTP filter][RBAC filter] that is well-suited for limiting which nodes can access which services within a mesh as encouraged by the principle of least privilege. -Resources in REST are present in the HTTP path so it would seem RBAC could be -used in non-gRPC environments as a precise service-level resource authz engine. -However, this 1) requires another filter to perform end-user authentication (as -TLS just provides service-level authn) and 2) does not scale as RBAC provides no -mechanism for loading rules on-demand. Since gRPC encodes service-level -resource identifiers in the request message, RBAC does not have access to the -requested resource for gRPC traffic. +The resource name in REST is present in the HTTP path so technically RBAC could +be used as a precise service-level resource authz engine with REST. However, +this 1) requires another filter to perform end-user authentication as TLS just +provides service-level authn and 2) does not scale as RBAC provides no mechanism +for loading rules on-demand. gRPC encodes service-level resource names (if they +exist) in the request message which RBAC cannot access, but this is not seen as +a major loss as RBAC is poorly suited for service-level resource authz. ### Related Proposals: From 08bed38f7eef55430529b591d21d7d34eb9400dd Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 11 Jun 2021 14:11:26 -0700 Subject: [PATCH 19/37] A29 is now in #243 --- A41-xds-rbac.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index b7625a05b..e1c0c5a0e 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -48,7 +48,7 @@ a major loss as RBAC is poorly suited for service-level resource authz. [A36]: https://github.com/grpc/proposal/blob/master/A36-xds-for-servers.md -[A29]: https://github.com/grpc/proposal/pull/184 +[A29]: https://github.com/grpc/proposal/pull/243 [A39]: https://github.com/grpc/proposal/blob/master/A39-xds-http-filters.md ## Proposal From 2a5b28921b8edb5ea4a742d3169438aac5edcd28 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Thu, 17 Jun 2021 15:25:22 -0700 Subject: [PATCH 20/37] Resolve TODO for xff_num_trusted_hops; it will be prohibited --- A41-xds-rbac.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index e1c0c5a0e..72cf09cbf 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -69,9 +69,10 @@ behavior. New validation should occur for `HttpConnectionManager` to allow equating `direct_remote_ip` and `remote_ip`. If the implementation does not distinguish -between these fields, then `xff_num_trusted_hops` must be verified to be unset -or zero. If it is any other value, the Listener must be NACKed. (TODO: Do this -only on server-side, or both client and server? Or just implement the feature?) +between these fields, then `xff_num_trusted_hops` must be unset or zero and +`original_ip_detection_extensions` must be empty. If either field has an +incorrect value, the Listener must be NACKed. For simplicity, this behavior +applies independent of the Listener type (both client-side and server-side). The core policy matching logic should be split into an "RBAC engine" to allow internal reuse with a non-xDS API. Any non-xDS API will not be RBAC, so this From 09a2cc880289f680da4e3e14253716d4e4ce8e94 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Mon, 28 Jun 2021 09:05:32 -0700 Subject: [PATCH 21/37] Disallow :scheme --- A41-xds-rbac.md | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 72cf09cbf..ca608bb6f 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -86,9 +86,9 @@ behavior. At this time, if the `RBAC.action` is `Action.LOG` then the policy will be completely ignored, as if RBAC was not configurated. CEL fields are not supported, so `Policy.condition` and `Policy.checked_condition` must cause a validation failure if present. It is also a validation failure if Permission or -Principal has a `header` matcher for a `grpc-`-prefixed header name. As -described in A39, validation failures for filter configuration causes the -listener to be NACKed. +Principal has a `header` matcher for a `grpc-`-prefixed header name or +`:scheme`. As described in A39, validation failures for filter configuration +causes the listener to be NACKed. The following fields of `Permission` may not be obvious how they map to gRPC: @@ -107,21 +107,19 @@ to "optimize out" the never-matching rules. The `header` field is not entirely 1:1 with gRPC Metadata, in part because which HTTP headers are present in Metadata is not 100% consistent cross-language. -For this design, `headers` can include `:method`, `:scheme`, `:authority`, -and`:path` matchers and they should match the values received on-the-wire -independent of whether they are stored in Metadata or in separate APIs. -`:scheme` is not universally available in gRPC APIs, so it may be hard-coded to -`http` if unavailable. `:method` can be hard-coded to `POST` if unavailable and -a code audit confirms the server denies requests for all other method types. -Implementations must not match [hop-by-hop headers][]. Since hop-by-hop headers -[are not used in HTTP/2 except for `te: trailers`][rfc7540 connection header], -implementations must ensure they are disallowing the `Connection` header and not -matching the `TE` header. Multi-valued metadata is represented as the -concatenation of the values along with a `,` (comma, no added spaces) separator, -as permitted by HTTP and gRPC. The `Content-Type` provided by the client must be -used; not a hard-coded value. Binary headers are represented in their -base64-encoded form, although we rarely expect binary header matchers other than -presence-checking. +For this design, `headers` can include `:method`, `:authority`, and`:path` +matchers and they should match the values received on-the-wire independent of +whether they are stored in Metadata or in separate APIs. `:method` can be +hard-coded to `POST` if unavailable and a code audit confirms the server denies +requests for all other method types. Implementations must not match [hop-by-hop +headers][]. Since hop-by-hop headers [are not used in HTTP/2 except for `te: +trailers`][rfc7540 connection header], implementations must ensure they are +disallowing the `Connection` header and not matching the `TE` header. +Multi-valued metadata is represented as the concatenation of the values along +with a `,` (comma, no added spaces) separator, as permitted by HTTP and gRPC. +The `Content-Type` provided by the client must be used; not a hard-coded value. +Binary headers are represented in their base64-encoded form, although we rarely +expect binary header matchers other than presence-checking. [hop-by-hop headers]: https://datatracker.ietf.org/doc/html/rfc7230#section-6.1 [rfc7540 connection header]: https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2.2 From 7416b2e8ad2636390048e7580c0485657ab1a442 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Mon, 28 Jun 2021 09:12:14 -0700 Subject: [PATCH 22/37] Sketch out major parts of authority handling --- A41-xds-rbac.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index ca608bb6f..d440b30f4 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -129,10 +129,12 @@ header map implementation, so they should be treated equivalent for the RBAC matchers; there must be no behavior change depending on which of the two header names is used in the _RBAC policy_. -TODO: Determine how to handle _requests_ (not policies) that have Host header, -since gRPC generally doesn't observe this header yet the RBAC policy could. -Options: deny such requests, move Host header to :authority, drop Host header. -Host and :authority could also disagree, and that needs to be handled. +The core gRPC implementation (not just xDS or RBAC) must observe both :authority +and Host headers. If either header contains a comma, the request must be failed +(TODO: any other characters to disallow; what status code?). If :authority is +missing, Host must be renamed to :authority. If :authority is present, Host must +be discarded. This produces a singular, unambiguous authority for every request +to be used by RBAC and the application itself. `metadata` will never match as `ValueMatcher` can only match if the value is present (even `NullMatch`). Be strongly aware that Envoy Metadata has no From f502d5609f9017701e31617e525cc2de7048c0bf Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Mon, 26 Jul 2021 15:53:24 -0700 Subject: [PATCH 23/37] Allow comma in :authority; disallow duplicate authority headers Comma is actually permitted as a literal in authorities via sub-delims in neg-name and IPvFuture. --- A41-xds-rbac.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index d440b30f4..07c3627c2 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -130,11 +130,13 @@ matchers; there must be no behavior change depending on which of the two header names is used in the _RBAC policy_. The core gRPC implementation (not just xDS or RBAC) must observe both :authority -and Host headers. If either header contains a comma, the request must be failed -(TODO: any other characters to disallow; what status code?). If :authority is -missing, Host must be renamed to :authority. If :authority is present, Host must -be discarded. This produces a singular, unambiguous authority for every request -to be used by RBAC and the application itself. +and Host headers. If :authority is missing, Host must be renamed to :authority. +If :authority is present, Host must be discarded. If multiple Host headers or +multiple :authority headers are present, the request must be rejected with an +HTTP status code 400 as required by Host validation in RFC 7230 ยง5.4, gRPC +status code INTERNAL, or RST_STREAM with HTTP/2 error code PROTOCOL_ERROR. These +restrictions and behavior produce a singular, unambiguous authority for every +request to be used by RBAC and the application itself. `metadata` will never match as `ValueMatcher` can only match if the value is present (even `NullMatch`). Be strongly aware that Envoy Metadata has no From ace1abd6373eb632bf97965f14d5c25522eb2b74 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Tue, 3 Aug 2021 15:35:24 -0700 Subject: [PATCH 24/37] Use relative links to other gRFCs --- A36-xds-for-servers.md | 2 +- A41-xds-rbac.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/A36-xds-for-servers.md b/A36-xds-for-servers.md index 6a02b3f7e..e4317a374 100644 --- a/A36-xds-for-servers.md +++ b/A36-xds-for-servers.md @@ -225,7 +225,7 @@ https://www.envoyproxy.io/docs/envoy/latest/operations/cli#cmdoption-drain-time- [Filter.typed_config]: https://github.com/envoyproxy/envoy/blob/928a62b7a12c4d87ce215a7c4ebd376f69c2e080/api/envoy/config/listener/v3/listener_components.proto#L40 [TypedStruct.type_url]: https://github.com/cncf/udpa/blob/cc1b757b3eddccaaaf0743cbb107742bb7e3ee4f/udpa/type/v1/typed_struct.proto#L38 -[A28-validation]: https://github.com/grpc/proposal/blob/master/A28-xds-traffic-splitting-and-routing.md#response-validation +[A28-validation]: A28-xds-traffic-splitting-and-routing.md#response-validation [Route.non_forwarding_action]: https://github.com/envoyproxy/envoy/blob/5963beae8842982803af1bef04fb5a2a0893c613/api/envoy/config/route/v3/route_components.proto#L242 [envoy lds]: https://www.envoyproxy.io/docs/envoy/latest/configuration/listeners/lds diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 07c3627c2..00e35f9a3 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -46,10 +46,10 @@ a major loss as RBAC is poorly suited for service-level resource authz. * [A29: xDS-Based Security for gRPC Clients and Servers][A29] * [A39: xDS HTTP Filter Support][A39] -[A36]: https://github.com/grpc/proposal/blob/master/A36-xds-for-servers.md +[A36]: A36-xds-for-servers.md [A29]: https://github.com/grpc/proposal/pull/243 -[A39]: https://github.com/grpc/proposal/blob/master/A39-xds-http-filters.md +[A39]: A39-xds-http-filters.md ## Proposal From 75c2c226d07003e99e5db9f78ae80b66a0025504 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 6 Aug 2021 16:57:30 -0700 Subject: [PATCH 25/37] Allow drain grace time to be configured --- A36-xds-for-servers.md | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/A36-xds-for-servers.md b/A36-xds-for-servers.md index e4317a374..f3886c00f 100644 --- a/A36-xds-for-servers.md +++ b/A36-xds-for-servers.md @@ -208,20 +208,20 @@ requested `:authority`. Routes are matched the same as on client-side. Route with an inappropriate `action` causes RPCs matching that route to fail. [Like in Envoy][envoy lds], updates to a Listener cause all older connections on -that Listener to be gracefully shut down with a grace period of 10 minutes for -long-lived RPCs, such that clients will reconnect and have the updated -configuration apply. This applies equally to an update in a RouteConfiguration -provided inline via the `route_config` field as it is part of the Listener, but -it does not apply to an updated RouteConfiguration provided by reference via -`rds` field. Infrastructure for "not serving" mode may be used for the graceful -shutdown, but only if it can be used without causing spurious RPC or connection -failures. Applying updates to a Listener should be delayed until dependent -resources have been attempted to be loaded (e.g., via RDS). The existing -resource loading timeout in XdsClient prevents the update from being delayed -indefinitely. - -TODO: drain time is a command line flag for Envoy, not provided via xds. -https://www.envoyproxy.io/docs/envoy/latest/operations/cli#cmdoption-drain-time-s +that Listener to be gracefully shut down (i.e., "drained") with a default grace +period of 10 minutes for long-lived RPCs, such that clients will reconnect and +have the updated configuration apply. This applies equally to an update in a +RouteConfiguration provided inline via the `route_config` field as it is part of +the Listener, but it does not apply to an updated RouteConfiguration provided by +reference via `rds` field. Infrastructure for "not serving" mode may be used for +the graceful shutdown, but only if it can be used without causing spurious RPC +or connection failures. Applying updates to a Listener should be delayed until +dependent resources have been attempted to be loaded (e.g., via RDS). The +existing resource loading timeout in XdsClient prevents the update from being +delayed indefinitely and the duplicate resource update detection in XdsClient +prevents replacing the Listener when nothing changes. The grace period should be +adjustable when building the XdsServer and should be described as the "drain +grace time." [Filter.typed_config]: https://github.com/envoyproxy/envoy/blob/928a62b7a12c4d87ce215a7c4ebd376f69c2e080/api/envoy/config/listener/v3/listener_components.proto#L40 [TypedStruct.type_url]: https://github.com/cncf/udpa/blob/cc1b757b3eddccaaaf0743cbb107742bb7e3ee4f/udpa/type/v1/typed_struct.proto#L38 From 14696426469617f1fffc672d3ff719077e4b3945 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 6 Aug 2021 16:57:45 -0700 Subject: [PATCH 26/37] Define behavior when RDS fails to load --- A36-xds-for-servers.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/A36-xds-for-servers.md b/A36-xds-for-servers.md index f3886c00f..f1fabbbd2 100644 --- a/A36-xds-for-servers.md +++ b/A36-xds-for-servers.md @@ -205,7 +205,12 @@ validation must be updated to allow [Route.non_forwarding_action][] as a valid requested `:authority`. Routes are matched the same as on client-side. `Route.non_forwarding_action` is expected for all Routes used on server-side and `Route.route` continues to be expected for all Routes used on client-side; a -Route with an inappropriate `action` causes RPCs matching that route to fail. +Route with an inappropriate `action` causes RPCs matching that route to fail +with UNAVAILABLE. If `HttpConnectionManager.rds` references a NACKed resource +without a previous good version, an unavailable resource because of +communication failures with control plane or a triggered loading timeout, or +a non-existent resource, then all RPCs processed by that HttpConnectionManager +will fail with UNAVAILABLE. [Like in Envoy][envoy lds], updates to a Listener cause all older connections on that Listener to be gracefully shut down (i.e., "drained") with a default grace From 583ac3af341d65ab44987efb72d022d6c8a40beb Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Wed, 11 Aug 2021 13:01:05 -0700 Subject: [PATCH 27/37] Flesh out implementation section, remove open issues --- A41-xds-rbac.md | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 00e35f9a3..22547f61e 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -219,8 +219,5 @@ support one day, but would most likely be a more advanced feature. ## Implementation -[A description of the steps in the implementation, who will do them, and when. If a particular language is going to get the implementation first, this section should list the proposed order.] - -## Open issues (if applicable) - -[A discussion of issues relating to this proposal for which the author does not know the solution. This section may be omitted if there are none.] +This will be implemented simultaneously in Java by @YifeiZhuang and @voidzcy, in +Go by @zasweq, and C++ by @yashykt. From 0736ffdb5c9c64634db213df0757e738f1e7d4cc Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Wed, 11 Aug 2021 14:33:37 -0700 Subject: [PATCH 28/37] Clarify Connection and TE handling --- A41-xds-rbac.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 22547f61e..a01da045c 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -107,14 +107,16 @@ to "optimize out" the never-matching rules. The `header` field is not entirely 1:1 with gRPC Metadata, in part because which HTTP headers are present in Metadata is not 100% consistent cross-language. -For this design, `headers` can include `:method`, `:authority`, and`:path` +For this design, `headers` can include `:method`, `:authority`, and `:path` matchers and they should match the values received on-the-wire independent of whether they are stored in Metadata or in separate APIs. `:method` can be hard-coded to `POST` if unavailable and a code audit confirms the server denies requests for all other method types. Implementations must not match [hop-by-hop headers][]. Since hop-by-hop headers [are not used in HTTP/2 except for `te: -trailers`][rfc7540 connection header], implementations must ensure they are -disallowing the `Connection` header and not matching the `TE` header. +trailers`][rfc7540 connection header], transports must consider requests +containing the `Connection` header as malformed, independent of xDS or RBAC, and +only the `TE` header may need special handling. If the transport exposes `TE` in +Metadata, then RBAC must special-case the header to prevent it from matching. Multi-valued metadata is represented as the concatenation of the values along with a `,` (comma, no added spaces) separator, as permitted by HTTP and gRPC. The `Content-Type` provided by the client must be used; not a hard-coded value. From c5ee9037fb07862e51621d81d620ca98795d30f1 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Wed, 11 Aug 2021 15:53:43 -0700 Subject: [PATCH 29/37] Reword service-level authz paragraph again. Third time's the charm, right? --- A41-xds-rbac.md | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index a01da045c..865e525f1 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -32,13 +32,12 @@ Envoy defines the [RBAC HTTP filter][RBAC filter] that is well-suited for limiting which nodes can access which services within a mesh as encouraged by the principle of least privilege. -The resource name in REST is present in the HTTP path so technically RBAC could -be used as a precise service-level resource authz engine with REST. However, -this 1) requires another filter to perform end-user authentication as TLS just -provides service-level authn and 2) does not scale as RBAC provides no mechanism -for loading rules on-demand. gRPC encodes service-level resource names (if they -exist) in the request message which RBAC cannot access, but this is not seen as -a major loss as RBAC is poorly suited for service-level resource authz. +RBAC is not well-suited for precise service-level resource authorization, even +if the resource name is exposed to the engine. Service-level resource authz 1) +requires another filter to perform end-user authentication as TLS just provides +service-level authn and 2) does not scale as RBAC provides no mechanism for +loading rules on-demand. Thus RBAC is only intended for service-to-service +authz. ### Related Proposals: From 1bce180e377f68b118c5f16666506d121a9880d3 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Wed, 11 Aug 2021 16:00:01 -0700 Subject: [PATCH 30/37] Provide context as to the location of the field --- A41-xds-rbac.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 865e525f1..04a3dff40 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -67,11 +67,13 @@ should be provided to each filter. A39 should be consulted for the expected behavior. New validation should occur for `HttpConnectionManager` to allow equating -`direct_remote_ip` and `remote_ip`. If the implementation does not distinguish -between these fields, then `xff_num_trusted_hops` must be unset or zero and -`original_ip_detection_extensions` must be empty. If either field has an -incorrect value, the Listener must be NACKed. For simplicity, this behavior -applies independent of the Listener type (both client-side and server-side). +RBAC's `direct_remote_ip` and `remote_ip`. If the RBAC implementation does not +distinguish between these fields, then +`HttpConnectionManager.xff_num_trusted_hops` must be unset or zero and +`HttpConnectionManager.original_ip_detection_extensions` must be empty. If +either field has an incorrect value, the Listener must be NACKed. For +simplicity, this behavior applies independent of the Listener type (both +client-side and server-side). The core policy matching logic should be split into an "RBAC engine" to allow internal reuse with a non-xDS API. Any non-xDS API will not be RBAC, so this From e8473369236cadfaa8987ab26184eabed5470af0 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Wed, 11 Aug 2021 16:12:11 -0700 Subject: [PATCH 31/37] Split out A36 changes to #255 --- A36-xds-for-servers.md | 42 ++---------------------------------------- 1 file changed, 2 insertions(+), 40 deletions(-) diff --git a/A36-xds-for-servers.md b/A36-xds-for-servers.md index f1fabbbd2..6b6a89e0a 100644 --- a/A36-xds-for-servers.md +++ b/A36-xds-for-servers.md @@ -181,6 +181,8 @@ Envoy's behavior. HttpConnectionManager support is required. HttpConnectionManager must have valid `http_filters`, as defined by [A39: xDS HTTP Filter Support][A39]. +RouteConfiguration (via `route_config` or indirect fields like `rds`) is not +used nor validated at this time, but may be in the future. If the `envoy.extensions.filters.http.router.v3.Router` is not present in `http_filters`, A39 calls for inserting a special filter that fails all RPCs. If @@ -191,48 +193,8 @@ required. This is to allow implementations that only support L4 xDS features to avoid L7 plumbing and implementation. This has no impact on the resource validation and NACKing behavior called for in A39. -If an XdsServer implementation uses RouteConfiguration or supports any HTTP -filters other than the hard-coded Router, then -`HttpConnectionManager.route_config` and `HttpConnectionManager.rds` must be -supported and RouteConfigurations must be validated. RouteConfiguration -validation logic inherits all previous validations made for client-side usage -as RDS does not distinguish between client-side and server-side. That is -predomenently defined in [gRFC A28][A28-validation], although note that -configuration for all VirtualHosts have been validated on client-side since -sharing the XdsClient was introduced, yet was not documented in a gRFC. The -validation must be updated to allow [Route.non_forwarding_action][] as a valid -`action`. The VirtualHost is selected on a per-RPC basis using the RPC's -requested `:authority`. Routes are matched the same as on client-side. -`Route.non_forwarding_action` is expected for all Routes used on server-side and -`Route.route` continues to be expected for all Routes used on client-side; a -Route with an inappropriate `action` causes RPCs matching that route to fail -with UNAVAILABLE. If `HttpConnectionManager.rds` references a NACKed resource -without a previous good version, an unavailable resource because of -communication failures with control plane or a triggered loading timeout, or -a non-existent resource, then all RPCs processed by that HttpConnectionManager -will fail with UNAVAILABLE. - -[Like in Envoy][envoy lds], updates to a Listener cause all older connections on -that Listener to be gracefully shut down (i.e., "drained") with a default grace -period of 10 minutes for long-lived RPCs, such that clients will reconnect and -have the updated configuration apply. This applies equally to an update in a -RouteConfiguration provided inline via the `route_config` field as it is part of -the Listener, but it does not apply to an updated RouteConfiguration provided by -reference via `rds` field. Infrastructure for "not serving" mode may be used for -the graceful shutdown, but only if it can be used without causing spurious RPC -or connection failures. Applying updates to a Listener should be delayed until -dependent resources have been attempted to be loaded (e.g., via RDS). The -existing resource loading timeout in XdsClient prevents the update from being -delayed indefinitely and the duplicate resource update detection in XdsClient -prevents replacing the Listener when nothing changes. The grace period should be -adjustable when building the XdsServer and should be described as the "drain -grace time." - [Filter.typed_config]: https://github.com/envoyproxy/envoy/blob/928a62b7a12c4d87ce215a7c4ebd376f69c2e080/api/envoy/config/listener/v3/listener_components.proto#L40 [TypedStruct.type_url]: https://github.com/cncf/udpa/blob/cc1b757b3eddccaaaf0743cbb107742bb7e3ee4f/udpa/type/v1/typed_struct.proto#L38 -[A28-validation]: A28-xds-traffic-splitting-and-routing.md#response-validation -[Route.non_forwarding_action]: https://github.com/envoyproxy/envoy/blob/5963beae8842982803af1bef04fb5a2a0893c613/api/envoy/config/route/v3/route_components.proto#L242 -[envoy lds]: https://www.envoyproxy.io/docs/envoy/latest/configuration/listeners/lds ### FilterChainMatch From ed508ceec1e6035a79d952e8741927ec3539954b Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Thu, 26 Aug 2021 13:53:12 -0700 Subject: [PATCH 32/37] Clarify language --- A41-xds-rbac.md | 43 ++++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 04a3dff40..fa5cd6525 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -32,12 +32,11 @@ Envoy defines the [RBAC HTTP filter][RBAC filter] that is well-suited for limiting which nodes can access which services within a mesh as encouraged by the principle of least privilege. -RBAC is not well-suited for precise service-level resource authorization, even -if the resource name is exposed to the engine. Service-level resource authz 1) -requires another filter to perform end-user authentication as TLS just provides -service-level authn and 2) does not scale as RBAC provides no mechanism for -loading rules on-demand. Thus RBAC is only intended for service-to-service -authz. +RBAC is not well-suited for precise resource authorization, even if the resource +name is exposed to the engine. Resource authz 1) requires another filter to +perform end-user authentication as TLS just provides client service authn and 2) +does not scale as RBAC provides no mechanism for loading rules on-demand. Thus +RBAC is only intended for service-to-service authz. ### Related Proposals: @@ -112,17 +111,18 @@ For this design, `headers` can include `:method`, `:authority`, and `:path` matchers and they should match the values received on-the-wire independent of whether they are stored in Metadata or in separate APIs. `:method` can be hard-coded to `POST` if unavailable and a code audit confirms the server denies -requests for all other method types. Implementations must not match [hop-by-hop -headers][]. Since hop-by-hop headers [are not used in HTTP/2 except for `te: -trailers`][rfc7540 connection header], transports must consider requests -containing the `Connection` header as malformed, independent of xDS or RBAC, and -only the `TE` header may need special handling. If the transport exposes `TE` in -Metadata, then RBAC must special-case the header to prevent it from matching. -Multi-valued metadata is represented as the concatenation of the values along -with a `,` (comma, no added spaces) separator, as permitted by HTTP and gRPC. -The `Content-Type` provided by the client must be used; not a hard-coded value. -Binary headers are represented in their base64-encoded form, although we rarely -expect binary header matchers other than presence-checking. +requests for all other method types. Implementations must consider the +request's [hop-by-hop headers][] to not be present. Since hop-by-hop headers +[are not used in HTTP/2 except for `te: trailers`][rfc7540 connection header], +transports must consider requests containing the `Connection` header as +malformed, independent of xDS or RBAC, and only the `TE` header may need special +handling. If the transport exposes `TE` in Metadata, then RBAC must special-case +the header to treat it as not present. Multi-valued metadata is represented as +the concatenation of the values along with a `,` (comma, no added spaces) +separator, as permitted by HTTP and gRPC. The `Content-Type` provided by the +client must be used; not a hard-coded value. Binary headers are represented in +their base64-encoded form, although we rarely expect binary header matchers +other than presence-checking. [hop-by-hop headers]: https://datatracker.ietf.org/doc/html/rfc7230#section-6.1 [rfc7540 connection header]: https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2.2 @@ -141,10 +141,11 @@ status code INTERNAL, or RST_STREAM with HTTP/2 error code PROTOCOL_ERROR. These restrictions and behavior produce a singular, unambiguous authority for every request to be used by RBAC and the application itself. -`metadata` will never match as `ValueMatcher` can only match if the value is -present (even `NullMatch`). Be strongly aware that Envoy Metadata has no -relation to gRPC Metadata. Envoy Metadata is generic state shared between -filters which has no gRPC equivalent. +In RBAC `metadata` refers to the Envoy metadata which has no relation to gRPC +metadata. Envoy metadata is generic state shared between filters, which has no +gRPC equivalent. RBAC implementations in gRPC will treat Envoy metadata as +empty. Since `ValueMatcher` can only match if a value is present (even +`NullMatch`), the `metadata` matcher is guaranteed not to match. `requested_server_name` can match if the matcher accepts empty string. From 17fdb3afdc542c946346e39068817eeb299c295f Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Thu, 26 Aug 2021 14:15:57 -0700 Subject: [PATCH 33/37] manage xDS-managed didn't flow --- A41-xds-rbac.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index fa5cd6525..730be6f30 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -17,7 +17,7 @@ client authorization (authz) on xDS-enabled gRPC servers. ## Background -[A29 xDS-Based Security][A29] introduced the ability to manage xDS-managed mTLS +[A29 xDS-Based Security][A29] introduced the ability to have xDS-managed mTLS meshes. As standard for TLS clients, the client verifies the server's identity is permitted to run the service. This is normally known as "hostname verification", but in a mesh scenario may be known as "server authorization" as From 8b5d8a2c2c6b9aee69859c050cb5ca7a26aef510 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Thu, 2 Sep 2021 17:51:50 -0700 Subject: [PATCH 34/37] Envoy metadata will be an empty _map_ --- A41-xds-rbac.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 730be6f30..41b397127 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -143,8 +143,8 @@ request to be used by RBAC and the application itself. In RBAC `metadata` refers to the Envoy metadata which has no relation to gRPC metadata. Envoy metadata is generic state shared between filters, which has no -gRPC equivalent. RBAC implementations in gRPC will treat Envoy metadata as -empty. Since `ValueMatcher` can only match if a value is present (even +gRPC equivalent. RBAC implementations in gRPC will treat Envoy metadata as an +empty map. Since `ValueMatcher` can only match if a value is present (even `NullMatch`), the `metadata` matcher is guaranteed not to match. `requested_server_name` can match if the matcher accepts empty string. From 13ee75925f3ca2802056de74c9b4c0f4e360ae36 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 3 Sep 2021 13:47:28 -0700 Subject: [PATCH 35/37] A29 is now merged --- A41-xds-rbac.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 41b397127..f60eea946 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -45,8 +45,7 @@ RBAC is only intended for service-to-service authz. * [A39: xDS HTTP Filter Support][A39] [A36]: A36-xds-for-servers.md - -[A29]: https://github.com/grpc/proposal/pull/243 +[A29]: A29-xds-tls-security.md [A39]: A39-xds-http-filters.md ## Proposal From 5c0c73c209df954873015490c96d769a0e19f77a Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 3 Sep 2021 15:07:56 -0700 Subject: [PATCH 36/37] Reword "RBAC is not for resource authz" paragraph again It is perfect this time. --- A41-xds-rbac.md | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index f60eea946..5c64af195 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -32,11 +32,15 @@ Envoy defines the [RBAC HTTP filter][RBAC filter] that is well-suited for limiting which nodes can access which services within a mesh as encouraged by the principle of least privilege. -RBAC is not well-suited for precise resource authorization, even if the resource -name is exposed to the engine. Resource authz 1) requires another filter to -perform end-user authentication as TLS just provides client service authn and 2) -does not scale as RBAC provides no mechanism for loading rules on-demand. Thus -RBAC is only intended for service-to-service authz. +Services that have state managed by their clients tend to need precise +authorization to that state, commonly driven by per-resource ACLs. RBAC is not +well suited for this precise authorization, even if the resource name +is exposed to the engine. Per-resource authorization 1) commonly uses end-user +identity so requires another filter to perform end-user authentication and +2) is too great of scale to enumerate all resource's ACLs in a single policy and +to absorb the rate of change to that policy. Thus RBAC is only intended for +service-to-service authorization and resource authorization is outside the scope +of this gRFC. ### Related Proposals: From d207a548a472dccec7b99be4896c8a8f780b692e Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 3 Sep 2021 16:57:44 -0700 Subject: [PATCH 37/37] Shadow rules are not supported --- A41-xds-rbac.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/A41-xds-rbac.md b/A41-xds-rbac.md index 5c64af195..8077a0233 100644 --- a/A41-xds-rbac.md +++ b/A41-xds-rbac.md @@ -57,7 +57,9 @@ of this gRFC. Support the [xDS RBAC HTTP filter][RBAC filter] as a registered filter type on xDS-enabled gRPC Servers, and support its `RBACPerRoute` configuration override. This does _not_ include [RBAC network filter][] support nor running the filter -on gRPC clients. xDS v2 support is not necessary. +on gRPC clients. xDS v2 support is not necessary. Shadow rules will not be +supported and is left as a potential future enhancement; only `RBAC.rules` will +be supported as they can function without stats. Supporting the RBAC HTTP filter on server-side leverages [A36: xDS-Enabled Servers][A36] and [A39: xDS HTTP Filter Support][A39]. Server-side HTTP filters