fix localhost address check in xfcc authenticator (istio#41189)

* fix localhost address check in xfcc authenticator Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * fix log Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * fix ut Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * move up Signed-off-by: Rama Chavali <rama.rao@salesforce.com> Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
WillardHu · Sep 29, 2022 · 3edbfbf · 3edbfbf
1 parent ffe8a3f
commit 3edbfbf
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 7 deletions.
diff --git a/security/pkg/server/ca/authenticate/xfcc_authenticator.go b/security/pkg/server/ca/authenticate/xfcc_authenticator.go
@@ -26,6 +26,7 @@ import (
 
 	"istio.io/istio/pilot/pkg/features"
 	"istio.io/istio/pkg/security"
+	"istio.io/pkg/log"
 )
 
 const (
@@ -92,13 +93,18 @@ func buildSecurityCaller(xfccHeader string) (*security.Caller, error) {
 }
 
 func isTrustedAddress(addr string, trustedCidrs []string) bool {
+	ip, _, err := net.SplitHostPort(addr)
+	if err != nil {
+		log.Warnf("peer address %s can not be split in to proper host and port", addr)
+		return false
+	}
 	for _, cidr := range trustedCidrs {
-		if isInRange(addr, cidr) {
+		if isInRange(ip, cidr) {
 			return true
 		}
 	}
 	// Always trust local host addresses.
-	return net.ParseIP(addr).IsLoopback()
+	return net.ParseIP(ip).IsLoopback()
 }
 
 func isInRange(addr, cidr string) bool {

diff --git a/security/pkg/server/ca/authenticate/xfcc_authenticator_test.go b/security/pkg/server/ca/authenticate/xfcc_authenticator_test.go
@@ -16,6 +16,7 @@ package authenticate
 
 import (
 	"net"
+	"net/netip"
 	"reflect"
 	"strings"
 	"testing"
@@ -36,11 +37,18 @@ func TestIsTrustedAddress(t *testing.T) {
 		trusted bool
 	}{
 		{
-			name:    "localhost client",
+			name:    "localhost client with port",
 			cidr:    "",
-			peer:    "127.0.0.1",
+			peer:    "127.0.0.1:9901",
 			trusted: true,
 		},
+		{
+			// Should never happen, added test case for testing it.
+			name:    "localhost client without port",
+			cidr:    "",
+			peer:    "127.0.0.1",
+			trusted: false,
+		},
 		{
 			name:    "external client without trusted cidr",
 			cidr:    "",
@@ -50,13 +58,13 @@ func TestIsTrustedAddress(t *testing.T) {
 		{
 			name:    "cidr in range",
 			cidr:    "172.17.0.0/16,192.17.0.0/16",
-			peer:    "172.17.0.2",
+			peer:    "172.17.0.2:9901",
 			trusted: true,
 		},
 		{
 			name:    "cidr in range with both ipv6 and ipv4",
 			cidr:    "172.17.0.0/16,2001:db8:1234:1a00::/56",
-			peer:    "2001:0db8:1234:1aff:ffff:ffff:ffff:ffff",
+			peer:    "[2001:0db8:1234:1a00:0000:0000:0000:0000]:80",
 			trusted: true,
 		},
 		{
@@ -130,7 +138,8 @@ func TestXfccAuthenticator(t *testing.T) {
 			if len(tt.xfccHeader) > 0 {
 				md.Append(xfccparser.ForwardedClientCertHeader, tt.xfccHeader)
 			}
-			ctx := peer.NewContext(context.Background(), &peer.Peer{Addr: &net.IPAddr{IP: net.ParseIP("127.0.0.1").To4()}})
+			addr := net.TCPAddrFromAddrPort(netip.MustParseAddrPort("127.0.0.1:2301"))
+			ctx := peer.NewContext(context.Background(), &peer.Peer{Addr: addr})
 			ctx = metadata.NewIncomingContext(ctx, md)
 			result, err := auth.Authenticate(security.AuthContext{GrpcContext: ctx})
 			if len(tt.authenticateErrMsg) > 0 {