Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A41: xDS RBAC Support #237

Merged
merged 38 commits into from
Sep 8, 2021
Merged
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
65ddc86
A41: xDS RBAC Support
ejona86 May 7, 2021
a9f7442
Nail down more details for matchers
ejona86 May 20, 2021
48c3a2c
Link to hop-by-hop documentation for those unlikely to read the HTTP …
ejona86 May 20, 2021
203a547
Define behavior for more headers
ejona86 May 21, 2021
f782dae
Grammar fixes
ejona86 May 21, 2021
2a30ba6
Add small explanation of Envoy metadata
ejona86 May 21, 2021
0eb14ca
Small clarifications
ejona86 May 21, 2021
bdece2b
Substantially change authenticated.principal_name
ejona86 May 24, 2021
be39479
TODO grpc-prefixed headers as well
ejona86 May 24, 2021
aa777e8
Define xff_num_trusted_hops behavior
ejona86 May 24, 2021
f3486d6
Binary headers and supported and grpc- headers are not
ejona86 May 26, 2021
88f00d5
Add to TODOs
ejona86 May 26, 2021
27ba988
Default authenticated behavior for unset matchers and missing client …
ejona86 Jun 7, 2021
2ab6d4a
Do not match hop-by-hop headers
ejona86 Jun 9, 2021
6228f60
Add RouteConfiguration handling to A36-xds-for-servers
ejona86 Jun 11, 2021
392960e
Clarify there will not be a public RBAC-authz API
ejona86 Jun 11, 2021
a9d5591
Add mailing list link and update other headers
ejona86 Jun 11, 2021
b9cbab4
Reword REST+resource-names-in-path+RBAC background
ejona86 Jun 11, 2021
08bed38
A29 is now in #243
ejona86 Jun 11, 2021
2a5b289
Resolve TODO for xff_num_trusted_hops; it will be prohibited
ejona86 Jun 17, 2021
09a2cc8
Disallow :scheme
ejona86 Jun 28, 2021
7416b2e
Sketch out major parts of authority handling
ejona86 Jun 28, 2021
f502d56
Allow comma in :authority; disallow duplicate authority headers
ejona86 Jul 26, 2021
ace1abd
Use relative links to other gRFCs
ejona86 Aug 3, 2021
75c2c22
Allow drain grace time to be configured
ejona86 Aug 6, 2021
1469642
Define behavior when RDS fails to load
ejona86 Aug 6, 2021
583ac3a
Flesh out implementation section, remove open issues
ejona86 Aug 11, 2021
0736ffd
Clarify Connection and TE handling
ejona86 Aug 11, 2021
c5ee903
Reword service-level authz paragraph again. Third time's the charm, r…
ejona86 Aug 11, 2021
1bce180
Provide context as to the location of the field
ejona86 Aug 11, 2021
e847336
Split out A36 changes to #255
ejona86 Aug 11, 2021
ed508ce
Clarify language
ejona86 Aug 26, 2021
17fdb3a
manage xDS-managed didn't flow
ejona86 Aug 26, 2021
8b5d8a2
Envoy metadata will be an empty _map_
ejona86 Sep 3, 2021
947d441
Merge branch 'master' into xds-rbac
ejona86 Sep 3, 2021
13ee759
A29 is now merged
ejona86 Sep 3, 2021
5c0c73c
Reword "RBAC is not for resource authz" paragraph again
ejona86 Sep 3, 2021
d207a54
Shadow rules are not supported
ejona86 Sep 3, 2021
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
232 changes: 232 additions & 0 deletions A41-xds-rbac.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,232 @@
A41: xDS RBAC Support
----
* Author(s): [Eric Anderson](https://github.com/ejona86), [Ashitha
Santhosh](https://github.com/ashithasantosh)
* Approver: markdroth
* Status: In Review {Draft, In Review, Ready for Implementation, Implemented}
* Implemented in: <language, ...>
* Last updated: 2021-06-11
* Discussion at: https://groups.google.com/g/grpc-io/c/DQJfShLTrTQ/

## 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 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
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 principle of least privilege.

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:

* [A36: xDS-Enabled Servers][A36]
* [A29: xDS-Based Security for gRPC Clients and Servers][A29]
* [A39: xDS HTTP Filter Support][A39]

[A36]: A36-xds-for-servers.md
[A29]: A29-xds-tls-security.md
[A39]: 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. 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
are less common than client-side at this point in time, so this may be the first
ejona86 marked this conversation as resolved.
Show resolved Hide resolved
usage of `RouteConfiguration` on server-side and so likely involves adding more
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.

New validation should occur for `HttpConnectionManager` to allow equating
markdroth marked this conversation as resolved.
Show resolved Hide resolved
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
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

Choose a reason for hiding this comment

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

there is an ongoing work to support dynamic typed proto matcher in RBAC, it is currently used to support upstream IP/port but the general framework allows other usages in the future, not sure if this is something the gRPC RBAC wants to support, see more details in envoyproxy/envoy#17645

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting. We may want a followup change that NACKs if the TypedExtensionConfig is set, simply because having ignored fields in matchers can cause havoc. But I don't think we have much of a need for that feature itself, at least yet.

RBAC here is server-side only, so "upstream IP/port" doesn't make sense. We could have client-side support, but it seems "upstream IP/port" has to be for a more conventional reverse proxy usecase instead of Envoy as a sidecar. I'd have to dig deeper to see if it can work load balancing, but it seems that specific extension is probably not too interesting to us.

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

Choose a reason for hiding this comment

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

A general question, does the gRPC allows specifying multiple RBAC config? One common use case in Envoy is to deploy multiple RBAC filter with different actions, for example, the 1st RBAC config with action DENY and the 2nd RBAC filter with action ALLOW.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. We allow multiple filters and filters can be repeated, just like in Envoy. That is covered in https://github.com/grpc/proposal/blob/master/A39-xds-http-filters.md

will be completely ignored, as if RBAC was not configurated. CEL fields are not
ejona86 marked this conversation as resolved.
Show resolved Hide resolved
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 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:

| Permission Field | gRPC Equivalent |
| ---------------- | --------------- |
| header | Metadata (with caveats below) |
| 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 |
| requested_server_name | Hard-coded as empty string |

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, in part because which
Copy link
Member

Choose a reason for hiding this comment

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

Some of the behavior described here is going to be a bit of a challenge for C-core, since the xDS HTTP filters will sit higher in the stack than the http_server filter, which removes a lot of the fields that we want to see here. In particular, it removes :method and Content-type, so they will no longer be visible by the time we get to the RBAC filter. CC @yashykt

Can you remind me how Java and Go handle these headers? Do they simply get returned to the application? CC @dfawley

Copy link
Member Author

Choose a reason for hiding this comment

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

:method will be hard-coded to POST in Java. Content-Type happens to still be present in Metadata in Java. Netty does validate it on a low-level, but doesn't remove it because 1) it doesn't remove any headers and 2) the higher levels would need it if we added support for the +proto/+json part.

Copy link
Member

Choose a reason for hiding this comment

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

In Go, we currently strip :method (we validate it is POST), but content-type is passed through to the application.

Copy link
Member

Choose a reason for hiding this comment

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

I think we've come up with a reasonable way to handle :method in C-core, but I'm not yet sure how we're going to handle content-type. We need to think about this some more.

HTTP headers are present in Metadata is not 100% consistent cross-language.
For this design, `headers` can include `:method`, `:authority`, and `:path`

Choose a reason for hiding this comment

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

does the :path goes through the same URL normalization done by Envoy? There are quite some normalization (e.g. resolve dot, dot-dot, remove multiple-forward-slashes, decode some of the percent encoded characters, etc.).

Some of the normalization are always needed and some other are configurable, we have seen multiple CVEs due to the mishandling of the path normalization.

Copy link
Member Author

Choose a reason for hiding this comment

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

No normalizations are done on the path. But no normalizations are done by the grpc service either. gRPC essentially just does string-equality to match RPC methods. (It is possible for applications to have some freedom here, but nobody really does differently.)

Envoy has a bigger problem here since the HTTP library processing the request in the application is a different one than in Envoy. That isn't really a problem in a proxyless environment, as long as we make sure RBAC runs with the same path that the service would see.

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

Choose a reason for hiding this comment

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

Hmmm... What if the C-core true-binary metadata extension is being used? In that case, the binary header will not actually be base64-encoded. Do we just match against the binary format in that case, or do we need to base64-encode it just to check the match?

Copy link
Member Author

Choose a reason for hiding this comment

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

The semantics of binary headers in HTTP/2 is they are normal base64 when unsupported. So yes, you'd need to match against the base64-encoded version. We don't really think binary header matching will be common, so we should optimize the code for simplicity and not performance.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense.

@yashykt Please note that this is something we'll need to handle in our implementation.

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

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

The core gRPC implementation (not just xDS or RBAC) must observe both :authority
Copy link
Member

Choose a reason for hiding this comment

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

@yashykt We'll need to make sure C-core is behaving correctly here.

and Host headers. If :authority is missing, Host must be renamed to :authority.

Choose a reason for hiding this comment

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

also note the match on host header should be case-insensitive, the match on other header should be case-sensitive.

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't case sensitivity defined by the matcher (StringMatcher.ignore_case), not the implementation or specific header being matched? I'd agree if you are saying that it is probably a configuration mistake if a matcher for a host was case sensitive. But that doesn't seem part of this specification.

You aren't suggesting that Envoy hard-coded different case matching behavior based on the header name for things like HeaderMatcher.exact_match, right? I've honestly not looked much at that sort of detail for HeaderMatcher as we had pre-existing support because of Routes and most things but StringMatcher are deprecated.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see any case-insensitivity code in HeaderMatcher for the old-style matchers:
https://github.com/envoyproxy/envoy/blob/327149fdc141ac7e057fb2c37fcf5f2e32047475/source/common/http/header_utility.cc#L152

It really just seems like users should set StringMatcher.ignore_case=true for headers like :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.

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

The following fields of `Principal` may not be obvious how they map to gRPC:

| Principal Field | gRPC Equivalent |
| ---------------- | --------------- |
| 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 |
| header | Same as in Permission |
| 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. 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
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
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
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

Choose a reason for hiding this comment

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

Another general question, does gRPC support RBAC in shadow mode? which is to evaluate the rules and record the result but do not enforce it. This is useful in trying out a RBAC config in production without rejecting any actual requests.

See api/envoy/extensions/filters/http/rbac/v3/rbac.proto

Copy link
Member Author

Choose a reason for hiding this comment

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

No, not as part of this. I've made a change to mention only RBAC.rules is supported in the RBAC message.

We are aware of it and agree it is really helpful. But it'll need some stats plumbing that is missing.

### Temporary environment variable protection

Choose a reason for hiding this comment

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

Another general question, does the gRPC RBAC emit the same stats for the evaluation result (e.g. allowed, denied number of requests), those are useful stats for quickly checking the status of the RBAC filter in Envoy.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are no stats as part of this. There have been some other discussions about stats (outside of RBAC) as stats are getting fleshed out. But as of yet, filters aren't contributing to stats. gRPC stats will not be the same as Envoy stats, but we'll obviously be aware of the Envoy stats and want similar stats for the really helpful cases.


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 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 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
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
ejona86 marked this conversation as resolved.
Show resolved Hide resolved

This will be implemented simultaneously in Java by @YifeiZhuang and @voidzcy, in
Go by @zasweq, and C++ by @yashykt.