Skip to content

xds/rbac: add additional handling for addresses with ports #8990

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.

Sign up for GitHub

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

Merged
mbissa merged 2 commits into from
Mar 26, 2026
Merged

xds/rbac: add additional handling for addresses with ports #8990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
323fae8
fix ip matching bug
mbissa Mar 20, 2026
8b2575b
review comments
mbissa Mar 26, 2026
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
21 changes: 19 additions & 2 deletions internal/xds/rbac/matchers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package rbac
import (
"errors"
"fmt"
"net"
"net/netip"
"regexp"

Expand Down Expand Up @@ -344,7 +345,15 @@ func newRemoteIPMatcher(cidrRange *v3corepb.CidrRange) (*remoteIPMatcher, error)
}

func (sim *remoteIPMatcher) match(data *rpcData) bool {
Copy link
Copy Markdown
Contributor

@arjan-bal arjan-bal Mar 24, 2026

Choose a reason for hiding this comment

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

How big of a change would it be to make match return a (bool, error) to enable callers to handle parsing errors?

Copy link
Copy Markdown
Contributor Author

@mbissa mbissa Mar 26, 2026

Choose a reason for hiding this comment

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

There are 11 implementations and each implementation has on an average of 4-5 references. This would need thorough testing and effort. I suggest we merge this and do that as a follow up.

Copy link
Copy Markdown
Contributor

@arjan-bal arjan-bal Mar 26, 2026

Choose a reason for hiding this comment

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

Sounds good, can you file a buganizer issue for this?

ip, _ := netip.ParseAddr(data.peerInfo.Addr.String())
host, _, err := net.SplitHostPort(data.peerInfo.Addr.String())
if err != nil {
// Fallback for addresses without a port.
host = data.peerInfo.Addr.String()
}
ip, err := netip.ParseAddr(host)
if err != nil {
return false
}
return sim.ipNet.Contains(ip)
}

Expand All @@ -362,7 +371,15 @@ func newLocalIPMatcher(cidrRange *v3corepb.CidrRange) (*localIPMatcher, error) {
}

func (dim *localIPMatcher) match(data *rpcData) bool {
ip, _ := netip.ParseAddr(data.localAddr.String())
host, _, err := net.SplitHostPort(data.localAddr.String())
if err != nil {
// Fallback for addresses without a port.
host = data.localAddr.String()
}
ip, err := netip.ParseAddr(host)
if err != nil {
return false
}
return dim.ipNet.Contains(ip)
}

Expand Down
36 changes: 18 additions & 18 deletions internal/xds/rbac/rbac_engine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -760,7 +760,7 @@ func (s) TestChainEngine(t *testing.T) {
rpcData: &rpcData{
fullMethod: "localhost-fan-page",
peerInfo: &peer.Peer{
Addr: &addr{ipAddress: "0.0.0.0"},
Addr: &addr{ipAddress: "0.0.0.0:8080"},
},
},
wantStatusCode: codes.OK,
Expand Down Expand Up @@ -823,7 +823,7 @@ func (s) TestChainEngine(t *testing.T) {
rpcData: &rpcData{
fullMethod: "some method",
peerInfo: &peer.Peer{
Addr: &addr{ipAddress: "0.0.0.0"},
Addr: &addr{ipAddress: "0.0.0.0:8080"},
AuthInfo: credentials.TLSInfo{
State: tls.ConnectionState{
PeerCertificates: []*x509.Certificate{
Expand Down Expand Up @@ -855,7 +855,7 @@ func (s) TestChainEngine(t *testing.T) {
rpcData: &rpcData{
fullMethod: "get-product-list",
peerInfo: &peer.Peer{
Addr: &addr{ipAddress: "0.0.0.0"},
Addr: &addr{ipAddress: "0.0.0.0:8080"},
},
},
wantStatusCode: codes.PermissionDenied,
Expand Down Expand Up @@ -907,7 +907,7 @@ func (s) TestChainEngine(t *testing.T) {
rpcData: &rpcData{
fullMethod: "/regular-content",
peerInfo: &peer.Peer{
Addr: &addr{ipAddress: "0.0.0.0"},
Addr: &addr{ipAddress: "0.0.0.0:8080"},
},
},
wantStatusCode: codes.OK,
Expand Down Expand Up @@ -945,7 +945,7 @@ func (s) TestChainEngine(t *testing.T) {
{
rpcData: &rpcData{
peerInfo: &peer.Peer{
Addr: &addr{ipAddress: "0.0.0.0"},
Addr: &addr{ipAddress: "0.0.0.0:8080"},
},
},
wantStatusCode: codes.OK,
Expand Down Expand Up @@ -995,7 +995,7 @@ func (s) TestChainEngine(t *testing.T) {
{
rpcData: &rpcData{
peerInfo: &peer.Peer{
Addr: &addr{ipAddress: "10.0.0.0"},
Addr: &addr{ipAddress: "10.0.0.0:8080"},
},
},
wantStatusCode: codes.PermissionDenied,
Expand Down Expand Up @@ -1072,7 +1072,7 @@ func (s) TestChainEngine(t *testing.T) {
{
rpcData: &rpcData{
peerInfo: &peer.Peer{
Addr: &addr{ipAddress: "0.0.0.0"},
Addr: &addr{ipAddress: "0.0.0.0:8080"},
},
},
wantStatusCode: codes.OK,
Expand All @@ -1093,7 +1093,7 @@ func (s) TestChainEngine(t *testing.T) {
{
rpcData: &rpcData{
peerInfo: &peer.Peer{
Addr: &addr{ipAddress: "10.0.0.0"},
Addr: &addr{ipAddress: "10.0.0.0:8080"},
},
},
wantStatusCode: codes.PermissionDenied,
Expand Down Expand Up @@ -1139,7 +1139,7 @@ func (s) TestChainEngine(t *testing.T) {
rpcData: &rpcData{
fullMethod: "some method",
peerInfo: &peer.Peer{
Addr: &addr{ipAddress: "0.0.0.0"},
Addr: &addr{ipAddress: "0.0.0.0:8080"},
AuthInfo: credentials.TLSInfo{
State: tls.ConnectionState{
PeerCertificates: []*x509.Certificate{
Expand Down Expand Up @@ -1218,7 +1218,7 @@ func (s) TestChainEngine(t *testing.T) {
rpcData: &rpcData{
fullMethod: "some method",
peerInfo: &peer.Peer{
Addr: &addr{ipAddress: "0.0.0.0"},
Addr: &addr{ipAddress: "0.0.0.0:8080"},
},
},
wantStatusCode: codes.OK,
Expand Down Expand Up @@ -1328,7 +1328,7 @@ func (s) TestChainEngine(t *testing.T) {
rpcData: &rpcData{
fullMethod: "localhost-fan-page",
peerInfo: &peer.Peer{
Addr: &addr{ipAddress: "0.0.0.0"},
Addr: &addr{ipAddress: "0.0.0.0:8080"},
},
},
wantStatusCode: codes.PermissionDenied,
Expand All @@ -1338,7 +1338,7 @@ func (s) TestChainEngine(t *testing.T) {
{
rpcData: &rpcData{
peerInfo: &peer.Peer{
Addr: &addr{ipAddress: "10.0.0.0"},
Addr: &addr{ipAddress: "10.0.0.0:8080"},
},
},
wantStatusCode: codes.PermissionDenied,
Expand Down Expand Up @@ -1427,7 +1427,7 @@ func (s) TestChainEngine(t *testing.T) {
rpcData: &rpcData{
fullMethod: "localhost-fan-page",
peerInfo: &peer.Peer{
Addr: &addr{ipAddress: "0.0.0.0"},
Addr: &addr{ipAddress: "0.0.0.0:8080"},
AuthInfo: credentials.TLSInfo{
State: tls.ConnectionState{
PeerCertificates: []*x509.Certificate{
Expand Down Expand Up @@ -1460,7 +1460,7 @@ func (s) TestChainEngine(t *testing.T) {
{
rpcData: &rpcData{
peerInfo: &peer.Peer{
Addr: &addr{ipAddress: "10.0.0.0"},
Addr: &addr{ipAddress: "10.0.0.0:8080"},
},
},
wantStatusCode: codes.PermissionDenied,
Expand Down Expand Up @@ -1566,7 +1566,7 @@ func (s) TestChainEngine(t *testing.T) {
rpcData: &rpcData{
fullMethod: "localhost-fan-page",
peerInfo: &peer.Peer{
Addr: &addr{ipAddress: "0.0.0.0"},
Addr: &addr{ipAddress: "0.0.0.0:8080"},
},
},
wantStatusCode: codes.PermissionDenied,
Expand All @@ -1577,7 +1577,7 @@ func (s) TestChainEngine(t *testing.T) {
{
rpcData: &rpcData{
peerInfo: &peer.Peer{
Addr: &addr{ipAddress: "10.0.0.0"},
Addr: &addr{ipAddress: "10.0.0.0:8080"},
},
},
wantStatusCode: codes.PermissionDenied,
Expand Down Expand Up @@ -1675,7 +1675,7 @@ func (s) TestChainEngine(t *testing.T) {
rpcData: &rpcData{
fullMethod: "localhost-fan-page",
peerInfo: &peer.Peer{
Addr: &addr{ipAddress: "0.0.0.0"},
Addr: &addr{ipAddress: "0.0.0.0:8080"},
},
},
wantStatusCode: codes.PermissionDenied,
Expand All @@ -1694,7 +1694,7 @@ func (s) TestChainEngine(t *testing.T) {
{
rpcData: &rpcData{
peerInfo: &peer.Peer{
Addr: &addr{ipAddress: "10.0.0.0"},
Addr: &addr{ipAddress: "10.0.0.0:8080"},
},
},
wantStatusCode: codes.PermissionDenied,
Expand Down
42 changes: 42 additions & 0 deletions test/xds/xds_server_rbac_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,48 @@ func (s) TestRBACHTTPFilter(t *testing.T) {
wantStatusEmptyCall: codes.PermissionDenied,
wantStatusUnaryCall: codes.PermissionDenied,
},
{
name: "match-on-principal-remote-ip",
rbacCfg: &rpb.RBAC{
Rules: &v3rbacpb.RBAC{
Action: v3rbacpb.RBAC_ALLOW,
Policies: map[string]*v3rbacpb.Policy{
"match-on-principal-remote-ip": {
Permissions: []*v3rbacpb.Permission{
{
Rule: &v3rbacpb.Permission_Header{
Header: &v3routepb.HeaderMatcher{
Name: "host",
HeaderMatchSpecifier: &v3routepb.HeaderMatcher_PrefixMatch{PrefixMatch: "my-service-fallback"},
},
},
},
},
Principals: []*v3rbacpb.Principal{
{
Identifier: &v3rbacpb.Principal_RemoteIp{
RemoteIp: &v3corepb.CidrRange{
AddressPrefix: "127.0.0.0",
PrefixLen: &wrapperspb.UInt32Value{Value: 8},
},
},
},
{
Identifier: &v3rbacpb.Principal_RemoteIp{
RemoteIp: &v3corepb.CidrRange{
AddressPrefix: "::1",
PrefixLen: &wrapperspb.UInt32Value{Value: 128},
},
},
},
},
},
},
},
},
wantStatusEmptyCall: codes.OK,
wantStatusUnaryCall: codes.OK,
},
// This test tests that RBAC ignores the TE: trailers header (which is
// hardcoded in http2_client.go for every RPC). Since the RBAC
// Configuration says to only ALLOW RPC's with a TE: Trailers, every RPC
Expand Down
Loading