xds/rbac: apply header matcher checks to nested and/or/not rules - #9258
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9258 +/- ##
==========================================
- Coverage 83.30% 83.15% -0.15%
==========================================
Files 420 423 +3
Lines 34106 35261 +1155
==========================================
+ Hits 28413 29323 +910
- Misses 4260 4427 +167
- Partials 1433 1511 +78
🚀 New features to boost your workflow:
|
Contributor
Author
|
gentle ping |
easwars
approved these changes
Aug 6, 2026
easwars
left a comment
Contributor
There was a problem hiding this comment.
LGTM, modulo couple of minor nits
Comment on lines
+173
to
+175
| if name == ":scheme" || strings.HasPrefix(name, "grpc-") { | ||
| return fmt.Errorf("rbac: header matcher for %v is :scheme or starts with grpc", name) | ||
| } |
Contributor
There was a problem hiding this comment.
Nit: Can we split the check so that we get a finer grained error and use %q formatting directive:
if name == ":scheme" {
return fmt.Errorf("rbac: header matcher for %q is %q", name, ":scheme")
}
if strings.HasPrefix(name, "grpc-") {
return fmt.Errorf("rbac: header matcher for %q starts with %q", name, "grpc-")
}
Contributor
Author
There was a problem hiding this comment.
Done, split into two checks with %q.
|
|
||
| gotPerm := perm.GetRule().(*v3rbacpb.Permission_NotRule).NotRule.GetRule().(*v3rbacpb.Permission_Header).Header.GetName() | ||
| if gotPerm != ":authority" { | ||
| t.Errorf("nested permission host matcher name = %q, want %q", gotPerm, ":authority") |
Contributor
There was a problem hiding this comment.
Nit: here and down below s/nested/Nested
Contributor
Author
There was a problem hiding this comment.
Done, capitalized both.
easwars
approved these changes
Aug 10, 2026
Contributor
|
@nvxbug Thank you for your contribution! |
easwars
pushed a commit
to easwars/grpc-go
that referenced
this pull request
Aug 19, 2026
…c#9258) parseConfig only walks the top-level Permissions and Principals of each RBAC policy when it applies the A41 header-name rules, so a header matcher nested inside an and_rules, or_rules, or not_rule is never checked. A control plane can put a `:scheme` or `grpc-` prefixed matcher inside a nested rule to slip past the validation A41 says must reject it, and a nested `host` matcher never gets rewritten to `:authority`, so it silently fails to match the header grpc-go actually carries (a deny policy on a nested host matcher fails open). Walk the full permission and principal trees so both the :scheme/grpc- rejection and the host to :authority rewrite reach matchers at any depth. Doing it in parseConfig keeps the check in the one place that already owns A41 validation, and folds the two former top-level passes into a single recursive walk shared by permissions and principals. RELEASE NOTES: - xds/rbac: Fix a bug where nested `Principal` or `Permission` rules with `:scheme` or `grpc-` prefixed header matchers were not rejected, which could cause DENY rules to fail open. - xds/rbac: Fix a bug where the `host` header matcher was not being replaced with `:authority` in nested `Principal` or `Permission` rules. (cherry picked from commit 1f4c6f3)
easwars
added a commit
that referenced
this pull request
Aug 19, 2026
Original PRs: #9258 and #9332 RELEASE NOTES: - xds/rbac: Fix a bug where nested `Principal` or `Permission` rules with `:scheme` or `grpc-` prefixed header matchers were not rejected, which could cause DENY rules to fail open. - xds/rbac: Fix a bug where the `host` header matcher was not being replaced with `:authority` in nested `Principal` or `Permission` rules. - xds/rbac: Fix a bug where a header matcher whose name was not lowercase, such as `X-Role`, matched no header, which could cause DENY rules to fail open. - xds/rbac: Fix a bug where a `:scheme` or `grpc-` prefixed header matcher was accepted when its name was not lowercase. - xds/rbac: Fix a bug where a `Host` header matcher was not replaced with `:authority`. --------- Co-authored-by: Naveed <naveed@bugqore.com> Co-authored-by: Markus Magnuson <331091+alimony@users.noreply.github.com>
eleboucher
pushed a commit
to eleboucher/runner-k8s-plugin
that referenced
this pull request
Aug 19, 2026
…(#83) This PR contains the following updates: | Package | Change | [Age](https://docs.renovatebot.com/merge-confidence/) | [Confidence](https://docs.renovatebot.com/merge-confidence/) | |---|---|---|---| | [google.golang.org/grpc](https://github.com/grpc/grpc-go) | `v1.82.0` → `v1.83.1` |  |  | --- ### Release Notes <details> <summary>grpc/grpc-go (google.golang.org/grpc)</summary> ### [`v1.83.1`](https://github.com/grpc/grpc-go/releases/tag/v1.83.1): Release 1.83.1 [Compare Source](grpc/grpc-go@v1.83.0...v1.83.1) ### Security - xds/rbac: Fix a bug where nested `Principal` or `Permission` rules with `:scheme` or `grpc-` prefixed header matchers were not rejected, which could cause DENY rules to fail open. ([#​9258](grpc/grpc-go#9258)) - Special Thanks: [@​nvxbug](https://github.com/nvxbug) - xds/rbac: Fix a bug where the `host` header matcher was not being replaced with `:authority` in nested `Principal` or `Permission` rules. ([#​9258](grpc/grpc-go#9258)) - Special Thanks: [@​nvxbug](https://github.com/nvxbug) - xds/rbac: Fix a bug where a header matcher whose name was not lowercase, such as `X-Role`, matched no header, which could cause DENY rules to fail open. ([#​9332](grpc/grpc-go#9332)) - Special Thanks: [@​alimony](https://github.com/alimony) - xds/rbac: Fix a bug where a `:scheme` or `grpc-` prefixed header matcher was accepted when its name was not lowercase. ([#​9332](grpc/grpc-go#9332)) - Special Thanks: [@​alimony](https://github.com/alimony) - xds/rbac: Fix a bug where a `Host` header matcher was not replaced with `:authority`. ([#​9332](grpc/grpc-go#9332)) - Special Thanks: [@​alimony](https://github.com/alimony) ### Performance - transport: Restrict memory overhead of buffering small data frames. ([#​9331](grpc/grpc-go#9331)) ### [`v1.83.0`](https://github.com/grpc/grpc-go/releases/tag/v1.83.0): Release 1.83.0 [Compare Source](grpc/grpc-go@v1.82.1...v1.83.0) ### Security - server: Stop reading from connections when flooded by HTTP/2 frames to mitigate resource exhaustion. The default value for this limit is 100 frames, excluding DATA and HEADERS, and may be changed by setting environment variable `GRPC_GO_EXPERIMENTAL_CONTROL_BUFFER_THROTTLE_LIMIT`. - xds/rbac: Support `Metadata` and `RequestedServerName` permissions matcher fields. If present in a DENY rule, previously these would be ignored and fail-open. - xds/rbac: Fix panic when parsing unsupported fields in `NotRule`/`NotId` permissions. - xds/rbac: Support the deprecated `source_ip` principal identifier by treating it as equivalent to `direct_remote_ip`. - xds: Fix panic when parsing route header matchers configured with empty `exact_match`, `prefix_match`, or `suffix_match` strings. ([#​9223](grpc/grpc-go#9223)) ### New Features - xds/googlec2p: Enable DirectPath over Interconnect support for on-premises clients via the `force-xds` target URI query parameter. ([#​9133](grpc/grpc-go#9133)) - xds: Enable xDS configuration to control which fields get propagated from ORCA backend metric reports to LRS load reports. ([#​9145](grpc/grpc-go#9145)) - authz: Add `OnPolicyUpdate` callback to `FileWatcherOptions` to notify when an authz policy is loaded or updated. ([#​9142](grpc/grpc-go#9142)) - Special Thanks: [@​hnefatl](https://github.com/hnefatl) - xds: Add support for the GCP Authentication HTTP Filter, which automatically fetches and attaches GCP Service Account Identity JWT tokens to outgoing RPCs. - This feature can be enabled by setting environment variable `GRPC_EXPERIMENTAL_XDS_GCP_AUTHENTICATION_FILTER=true`. ([#​9119](grpc/grpc-go#9119)) - xds: Add support for xDS-based HTTP CONNECT proxies. - This feature can be enabled by setting environment variable `GRPC_EXPERIMENTAL_XDS_HTTP_CONNECT=true`. ([#​9151](grpc/grpc-go#9151)) - xds: Add support for `contains_match` in route header matchers. ([#​9223](grpc/grpc-go#9223)) ### Bug Fixes - credentials/alts: Fix panic when processing malformed frames by validating that the message frame length exceeds the message type field size. ([#​9197](grpc/grpc-go#9197)) - grpc: Fix compilation on Plan 9 targets (`GOOS=plan9`), broken since v1.81.0. ([#​9255](grpc/grpc-go#9255)) - Special Thanks: [@​Yusufihsangorgel](https://github.com/Yusufihsangorgel) ### [`v1.82.1`](https://github.com/grpc/grpc-go/releases/tag/v1.82.1): Release 1.82.1 [Compare Source](grpc/grpc-go@v1.82.0...v1.82.1) ### Security - server: Stop reading from the connection when flooded by HTTP/2 frames. The default value for this limit is 100 frames, excluding DATA and HEADERS, and may be changed by setting environment variable `GRPC_GO_EXPERIMENTAL_CONTROL_BUFFER_THROTTLE_LIMIT`. - xds/rbac: Support `Metadata` and `RequestedServerName` permissions matcher fields. If present in a DENY rule, previously these would be ignored and fail-open. - xds/rbac: Fix panic when parsing unsupported fields in `NotRule`/`NotId` permissions. - xds/rbac: Support the deprecated `source_ip` principal identifier by treating it as equivalent to `direct_remote_ip`. </details> --- ### Configuration 📅 **Schedule**: (in timezone Europe/Paris) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate CLI](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4xMDEuMSIsInVwZGF0ZWRJblZlciI6IjQ0LjMxLjAiLCJ0YXJnZXRCcmFuY2giOiJtYWluIiwibGFiZWxzIjpbInR5cGUvbWlub3IiXX0=--> Reviewed-on: https://git.erwanleboucher.dev/eleboucher/runner-k8s-plugin/pulls/83
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
parseConfig only walks the top-level Permissions and Principals of each RBAC policy when it applies the A41 header-name rules, so a header matcher nested inside an and_rules, or_rules, or not_rule is never checked. A control plane can put a
:schemeorgrpc-prefixed matcher inside a nested rule to slip past the validation A41 says must reject it, and a nestedhostmatcher never gets rewritten to:authority, so it silently fails to match the header grpc-go actually carries (a deny policy on a nested host matcher fails open).Walk the full permission and principal trees so both the :scheme/grpc- rejection and the host to :authority rewrite reach matchers at any depth. Doing it in parseConfig keeps the check in the one place that already owns A41 validation, and folds the two former top-level passes into a single recursive walk shared by permissions and principals.
RELEASE NOTES:
PrincipalorPermissionrules with:schemeorgrpc-prefixed header matchers were not rejected, which could cause DENY rules to fail open.hostheader matcher was not being replaced with:authorityin nestedPrincipalorPermissionrules.