Skip to content

Commit f1d1ce5

Browse files
mbissambissa
authored
xds/rbac: add additional handling for addresses with ports (grpc#8990)
This PR enhances the rbac matcher to handle IP address string with a port attached. The fix introduces the `net.SplitHostPort` utility function, ensuring the port is properly stripped out of the underlying `peerInfo.Addr.String()` and `localAddr.String()` values before parsing them with `netip.ParseAddr`. A fallback mechanism is also included in case `SplitHostPort` fails due to a missing port. RELEASE NOTES: * xds/rbac: Add additional handling for addresses with ports
1 parent c20fba0 commit f1d1ce5

File tree

3 files changed

+79
-20
lines changed

3 files changed

+79
-20
lines changed

‎internal/xds/rbac/matchers.go‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package rbac
1919
import (
2020
"errors"
2121
"fmt"
22+
"net"
2223
"net/netip"
2324
"regexp"
2425

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

346347
func (sim *remoteIPMatcher) match(data *rpcData) bool {
347-
ip, _ := netip.ParseAddr(data.peerInfo.Addr.String())
348+
host, _, err := net.SplitHostPort(data.peerInfo.Addr.String())
349+
if err != nil {
350+
// Fallback for addresses without a port.
351+
host = data.peerInfo.Addr.String()
352+
}
353+
ip, err := netip.ParseAddr(host)
354+
if err != nil {
355+
return false
356+
}
348357
return sim.ipNet.Contains(ip)
349358
}
350359

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

364373
func (dim *localIPMatcher) match(data *rpcData) bool {
365-
ip, _ := netip.ParseAddr(data.localAddr.String())
374+
host, _, err := net.SplitHostPort(data.localAddr.String())
375+
if err != nil {
376+
// Fallback for addresses without a port.
377+
host = data.localAddr.String()
378+
}
379+
ip, err := netip.ParseAddr(host)
380+
if err != nil {
381+
return false
382+
}
366383
return dim.ipNet.Contains(ip)
367384
}
368385

‎internal/xds/rbac/rbac_engine_test.go‎

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -760,7 +760,7 @@ func (s) TestChainEngine(t *testing.T) {
760760
rpcData: &rpcData{
761761
fullMethod: "localhost-fan-page",
762762
peerInfo: &peer.Peer{
763-
Addr: &addr{ipAddress: "0.0.0.0"},
763+
Addr: &addr{ipAddress: "0.0.0.0:8080"},
764764
},
765765
},
766766
wantStatusCode: codes.OK,
@@ -823,7 +823,7 @@ func (s) TestChainEngine(t *testing.T) {
823823
rpcData: &rpcData{
824824
fullMethod: "some method",
825825
peerInfo: &peer.Peer{
826-
Addr: &addr{ipAddress: "0.0.0.0"},
826+
Addr: &addr{ipAddress: "0.0.0.0:8080"},
827827
AuthInfo: credentials.TLSInfo{
828828
State: tls.ConnectionState{
829829
PeerCertificates: []*x509.Certificate{
@@ -855,7 +855,7 @@ func (s) TestChainEngine(t *testing.T) {
855855
rpcData: &rpcData{
856856
fullMethod: "get-product-list",
857857
peerInfo: &peer.Peer{
858-
Addr: &addr{ipAddress: "0.0.0.0"},
858+
Addr: &addr{ipAddress: "0.0.0.0:8080"},
859859
},
860860
},
861861
wantStatusCode: codes.PermissionDenied,
@@ -907,7 +907,7 @@ func (s) TestChainEngine(t *testing.T) {
907907
rpcData: &rpcData{
908908
fullMethod: "/regular-content",
909909
peerInfo: &peer.Peer{
910-
Addr: &addr{ipAddress: "0.0.0.0"},
910+
Addr: &addr{ipAddress: "0.0.0.0:8080"},
911911
},
912912
},
913913
wantStatusCode: codes.OK,
@@ -945,7 +945,7 @@ func (s) TestChainEngine(t *testing.T) {
945945
{
946946
rpcData: &rpcData{
947947
peerInfo: &peer.Peer{
948-
Addr: &addr{ipAddress: "0.0.0.0"},
948+
Addr: &addr{ipAddress: "0.0.0.0:8080"},
949949
},
950950
},
951951
wantStatusCode: codes.OK,
@@ -995,7 +995,7 @@ func (s) TestChainEngine(t *testing.T) {
995995
{
996996
rpcData: &rpcData{
997997
peerInfo: &peer.Peer{
998-
Addr: &addr{ipAddress: "10.0.0.0"},
998+
Addr: &addr{ipAddress: "10.0.0.0:8080"},
999999
},
10001000
},
10011001
wantStatusCode: codes.PermissionDenied,
@@ -1072,7 +1072,7 @@ func (s) TestChainEngine(t *testing.T) {
10721072
{
10731073
rpcData: &rpcData{
10741074
peerInfo: &peer.Peer{
1075-
Addr: &addr{ipAddress: "0.0.0.0"},
1075+
Addr: &addr{ipAddress: "0.0.0.0:8080"},
10761076
},
10771077
},
10781078
wantStatusCode: codes.OK,
@@ -1093,7 +1093,7 @@ func (s) TestChainEngine(t *testing.T) {
10931093
{
10941094
rpcData: &rpcData{
10951095
peerInfo: &peer.Peer{
1096-
Addr: &addr{ipAddress: "10.0.0.0"},
1096+
Addr: &addr{ipAddress: "10.0.0.0:8080"},
10971097
},
10981098
},
10991099
wantStatusCode: codes.PermissionDenied,
@@ -1139,7 +1139,7 @@ func (s) TestChainEngine(t *testing.T) {
11391139
rpcData: &rpcData{
11401140
fullMethod: "some method",
11411141
peerInfo: &peer.Peer{
1142-
Addr: &addr{ipAddress: "0.0.0.0"},
1142+
Addr: &addr{ipAddress: "0.0.0.0:8080"},
11431143
AuthInfo: credentials.TLSInfo{
11441144
State: tls.ConnectionState{
11451145
PeerCertificates: []*x509.Certificate{
@@ -1218,7 +1218,7 @@ func (s) TestChainEngine(t *testing.T) {
12181218
rpcData: &rpcData{
12191219
fullMethod: "some method",
12201220
peerInfo: &peer.Peer{
1221-
Addr: &addr{ipAddress: "0.0.0.0"},
1221+
Addr: &addr{ipAddress: "0.0.0.0:8080"},
12221222
},
12231223
},
12241224
wantStatusCode: codes.OK,
@@ -1328,7 +1328,7 @@ func (s) TestChainEngine(t *testing.T) {
13281328
rpcData: &rpcData{
13291329
fullMethod: "localhost-fan-page",
13301330
peerInfo: &peer.Peer{
1331-
Addr: &addr{ipAddress: "0.0.0.0"},
1331+
Addr: &addr{ipAddress: "0.0.0.0:8080"},
13321332
},
13331333
},
13341334
wantStatusCode: codes.PermissionDenied,
@@ -1338,7 +1338,7 @@ func (s) TestChainEngine(t *testing.T) {
13381338
{
13391339
rpcData: &rpcData{
13401340
peerInfo: &peer.Peer{
1341-
Addr: &addr{ipAddress: "10.0.0.0"},
1341+
Addr: &addr{ipAddress: "10.0.0.0:8080"},
13421342
},
13431343
},
13441344
wantStatusCode: codes.PermissionDenied,
@@ -1427,7 +1427,7 @@ func (s) TestChainEngine(t *testing.T) {
14271427
rpcData: &rpcData{
14281428
fullMethod: "localhost-fan-page",
14291429
peerInfo: &peer.Peer{
1430-
Addr: &addr{ipAddress: "0.0.0.0"},
1430+
Addr: &addr{ipAddress: "0.0.0.0:8080"},
14311431
AuthInfo: credentials.TLSInfo{
14321432
State: tls.ConnectionState{
14331433
PeerCertificates: []*x509.Certificate{
@@ -1460,7 +1460,7 @@ func (s) TestChainEngine(t *testing.T) {
14601460
{
14611461
rpcData: &rpcData{
14621462
peerInfo: &peer.Peer{
1463-
Addr: &addr{ipAddress: "10.0.0.0"},
1463+
Addr: &addr{ipAddress: "10.0.0.0:8080"},
14641464
},
14651465
},
14661466
wantStatusCode: codes.PermissionDenied,
@@ -1566,7 +1566,7 @@ func (s) TestChainEngine(t *testing.T) {
15661566
rpcData: &rpcData{
15671567
fullMethod: "localhost-fan-page",
15681568
peerInfo: &peer.Peer{
1569-
Addr: &addr{ipAddress: "0.0.0.0"},
1569+
Addr: &addr{ipAddress: "0.0.0.0:8080"},
15701570
},
15711571
},
15721572
wantStatusCode: codes.PermissionDenied,
@@ -1577,7 +1577,7 @@ func (s) TestChainEngine(t *testing.T) {
15771577
{
15781578
rpcData: &rpcData{
15791579
peerInfo: &peer.Peer{
1580-
Addr: &addr{ipAddress: "10.0.0.0"},
1580+
Addr: &addr{ipAddress: "10.0.0.0:8080"},
15811581
},
15821582
},
15831583
wantStatusCode: codes.PermissionDenied,
@@ -1675,7 +1675,7 @@ func (s) TestChainEngine(t *testing.T) {
16751675
rpcData: &rpcData{
16761676
fullMethod: "localhost-fan-page",
16771677
peerInfo: &peer.Peer{
1678-
Addr: &addr{ipAddress: "0.0.0.0"},
1678+
Addr: &addr{ipAddress: "0.0.0.0:8080"},
16791679
},
16801680
},
16811681
wantStatusCode: codes.PermissionDenied,
@@ -1694,7 +1694,7 @@ func (s) TestChainEngine(t *testing.T) {
16941694
{
16951695
rpcData: &rpcData{
16961696
peerInfo: &peer.Peer{
1697-
Addr: &addr{ipAddress: "10.0.0.0"},
1697+
Addr: &addr{ipAddress: "10.0.0.0:8080"},
16981698
},
16991699
},
17001700
wantStatusCode: codes.PermissionDenied,

‎test/xds/xds_server_rbac_test.go‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -605,6 +605,48 @@ func (s) TestRBACHTTPFilter(t *testing.T) {
605605
wantStatusEmptyCall: codes.PermissionDenied,
606606
wantStatusUnaryCall: codes.PermissionDenied,
607607
},
608+
{
609+
name: "match-on-principal-remote-ip",
610+
rbacCfg: &rpb.RBAC{
611+
Rules: &v3rbacpb.RBAC{
612+
Action: v3rbacpb.RBAC_ALLOW,
613+
Policies: map[string]*v3rbacpb.Policy{
614+
"match-on-principal-remote-ip": {
615+
Permissions: []*v3rbacpb.Permission{
616+
{
617+
Rule: &v3rbacpb.Permission_Header{
618+
Header: &v3routepb.HeaderMatcher{
619+
Name: "host",
620+
HeaderMatchSpecifier: &v3routepb.HeaderMatcher_PrefixMatch{PrefixMatch: "my-service-fallback"},
621+
},
622+
},
623+
},
624+
},
625+
Principals: []*v3rbacpb.Principal{
626+
{
627+
Identifier: &v3rbacpb.Principal_RemoteIp{
628+
RemoteIp: &v3corepb.CidrRange{
629+
AddressPrefix: "127.0.0.0",
630+
PrefixLen: &wrapperspb.UInt32Value{Value: 8},
631+
},
632+
},
633+
},
634+
{
635+
Identifier: &v3rbacpb.Principal_RemoteIp{
636+
RemoteIp: &v3corepb.CidrRange{
637+
AddressPrefix: "::1",
638+
PrefixLen: &wrapperspb.UInt32Value{Value: 128},
639+
},
640+
},
641+
},
642+
},
643+
},
644+
},
645+
},
646+
},
647+
wantStatusEmptyCall: codes.OK,
648+
wantStatusUnaryCall: codes.OK,
649+
},
608650
// This test tests that RBAC ignores the TE: trailers header (which is
609651
// hardcoded in http2_client.go for every RPC). Since the RBAC
610652
// Configuration says to only ALLOW RPC's with a TE: Trailers, every RPC

0 commit comments

Comments
 (0)