fix: validate proto messages before converting them to anypb.Any (env…

…oyproxy#4499) * validate proto message before converting to any Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>
zhaohuabing · Oct 29, 2024 · 05817fc · 05817fc
1 parent b877bac
commit 05817fc
Show file tree

Hide file tree

Showing 19 changed files with 184 additions and 112 deletions.
diff --git a/internal/utils/protocov/protocov.go b/internal/utils/protocov/protocov.go
@@ -12,30 +12,30 @@ import (
 	"google.golang.org/protobuf/types/known/anypb"
 )
 
-const (
-	APIPrefix = "type.googleapis.com/"
-)
-
-var marshalOpts = proto.MarshalOptions{}
+// Deprecated: error should not be ignored, use ToAnyWithValidation instead.
+func ToAny(msg proto.Message) *anypb.Any {
+	res, err := ToAnyWithValidation(msg)
+	if err != nil {
+		return nil
+	}
+	return res
+}
 
-func ToAnyWithError(msg proto.Message) (*anypb.Any, error) {
+func ToAnyWithValidation(msg proto.Message) (*anypb.Any, error) {
 	if msg == nil {
 		return nil, errors.New("empty message received")
 	}
-	b, err := marshalOpts.Marshal(msg)
-	if err != nil {
-		return nil, err
+
+	// If the message has a ValidateAll method, call it before marshaling.
+	if validator, ok := msg.(interface{ ValidateAll() error }); ok {
+		if err := validator.ValidateAll(); err != nil {
+			return nil, err
+		}
 	}
-	return &anypb.Any{
-		TypeUrl: APIPrefix + string(msg.ProtoReflect().Descriptor().FullName()),
-		Value:   b,
-	}, nil
-}
 
-func ToAny(msg proto.Message) *anypb.Any {
-	res, err := ToAnyWithError(msg)
+	any, err := anypb.New(msg)
 	if err != nil {
-		return nil
+		return nil, err
 	}
-	return res
+	return any, nil
 }
diff --git a/internal/xds/translator/accesslog.go b/internal/xds/translator/accesslog.go
@@ -22,7 +22,6 @@ import (
 	"github.com/envoyproxy/go-control-plane/pkg/wellknown"
 	otlpcommonv1 "go.opentelemetry.io/proto/otlp/common/v1"
 	"golang.org/x/exp/maps"
-	"google.golang.org/protobuf/types/known/anypb"
 	"google.golang.org/protobuf/types/known/structpb"
 
 	egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
@@ -90,9 +89,9 @@ var (
 	}
 )
 
-func buildXdsAccessLog(al *ir.AccessLog, accessLogType ir.ProxyAccessLogType) []*accesslog.AccessLog {
+func buildXdsAccessLog(al *ir.AccessLog, accessLogType ir.ProxyAccessLogType) ([]*accesslog.AccessLog, error) {
 	if al == nil {
-		return nil
+		return nil, nil
 	}
 
 	totalLen := len(al.Text) + len(al.JSON) + len(al.OpenTelemetry)
@@ -133,8 +132,10 @@ func buildXdsAccessLog(al *ir.AccessLog, accessLogType ir.ProxyAccessLogType) []
 			filelog.GetLogFormat().Formatters = formatters
 		}
 
-		// TODO: find a better way to handle this
-		accesslogAny, _ := anypb.New(filelog)
+		accesslogAny, err := protocov.ToAnyWithValidation(filelog)
+		if err != nil {
+			return nil, err
+		}
 		accessLogs = append(accessLogs, &accesslog.AccessLog{
 			Name: wellknown.FileAccessLog,
 			ConfigType: &accesslog.AccessLog_TypedConfig{
@@ -185,7 +186,10 @@ func buildXdsAccessLog(al *ir.AccessLog, accessLogType ir.ProxyAccessLogType) []
 			filelog.GetLogFormat().Formatters = formatters
 		}
 
-		accesslogAny, _ := anypb.New(filelog)
+		accesslogAny, err := protocov.ToAnyWithValidation(filelog)
+		if err != nil {
+			return nil, err
+		}
 		accessLogs = append(accessLogs, &accesslog.AccessLog{
 			Name: wellknown.FileAccessLog,
 			ConfigType: &accesslog.AccessLog_TypedConfig{
@@ -228,7 +232,10 @@ func buildXdsAccessLog(al *ir.AccessLog, accessLogType ir.ProxyAccessLogType) []
 				alCfg.AdditionalResponseTrailersToLog = als.HTTP.ResponseTrailers
 			}
 
-			accesslogAny, _ := anypb.New(alCfg)
+			accesslogAny, err := protocov.ToAnyWithValidation(alCfg)
+			if err != nil {
+				return nil, err
+			}
 			accessLogs = append(accessLogs, &accesslog.AccessLog{
 				Name: wellknown.HTTPGRPCAccessLog,
 				ConfigType: &accesslog.AccessLog_TypedConfig{
@@ -241,7 +248,10 @@ func buildXdsAccessLog(al *ir.AccessLog, accessLogType ir.ProxyAccessLogType) []
 				CommonConfig: cc,
 			}
 
-			accesslogAny, _ := anypb.New(alCfg)
+			accesslogAny, err := protocov.ToAnyWithValidation(alCfg)
+			if err != nil {
+				return nil, err
+			}
 			accessLogs = append(accessLogs, &accesslog.AccessLog{
 				Name: tcpGRPCAccessLog,
 				ConfigType: &accesslog.AccessLog_TypedConfig{
@@ -297,7 +307,10 @@ func buildXdsAccessLog(al *ir.AccessLog, accessLogType ir.ProxyAccessLogType) []
 			al.Formatters = formatters
 		}
 
-		accesslogAny, _ := anypb.New(al)
+		accesslogAny, err := protocov.ToAnyWithValidation(al)
+		if err != nil {
+			return nil, err
+		}
 		accessLogs = append(accessLogs, &accesslog.AccessLog{
 			Name: otelAccessLog,
 			ConfigType: &accesslog.AccessLog_TypedConfig{
@@ -307,7 +320,7 @@ func buildXdsAccessLog(al *ir.AccessLog, accessLogType ir.ProxyAccessLogType) []
 		})
 	}
 
-	return accessLogs
+	return accessLogs, nil
 }
 
 func celAccessLogFilter(expr string) *accesslog.AccessLogFilter {

diff --git a/internal/xds/translator/authorization.go b/internal/xds/translator/authorization.go
@@ -26,6 +26,7 @@ import (
 
 	egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
 	"github.com/envoyproxy/gateway/internal/ir"
+	"github.com/envoyproxy/gateway/internal/utils/protocov"
 	"github.com/envoyproxy/gateway/internal/xds/types"
 )
 
@@ -75,7 +76,7 @@ func (*rbac) patchHCM(
 // buildHCMRBACFilter returns a RBAC filter from the provided IR listener.
 func buildHCMRBACFilter() (*hcmv3.HttpFilter, error) {
 	rbacProto := &rbacv3.RBAC{}
-	rbacAny, err := anypb.New(rbacProto)
+	rbacAny, err := protocov.ToAnyWithValidation(rbacProto)
 	if err != nil {
 		return nil, err
 	}
@@ -133,7 +134,7 @@ func (*rbac) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute) error {
 		return err
 	}
 
-	if cfgAny, err = anypb.New(rbacPerRoute); err != nil {
+	if cfgAny, err = protocov.ToAnyWithValidation(rbacPerRoute); err != nil {
 		return err
 	}
 
@@ -159,15 +160,15 @@ func buildRBACPerRoute(authorization *ir.Authorization) (*rbacv3.RBACPerRoute, e
 		Name:   "ALLOW",
 		Action: rbacconfigv3.RBAC_ALLOW,
 	}
-	if allowAction, err = anypb.New(allow); err != nil {
+	if allowAction, err = protocov.ToAnyWithValidation(allow); err != nil {
 		return nil, err
 	}
 
 	deny := &rbacconfigv3.Action{
 		Name:   "DENY",
 		Action: rbacconfigv3.RBAC_DENY,
 	}
-	if denyAction, err = anypb.New(deny); err != nil {
+	if denyAction, err = protocov.ToAnyWithValidation(deny); err != nil {
 		return nil, err
 	}
 
@@ -287,11 +288,6 @@ func buildRBACPerRoute(authorization *ir.Authorization) (*rbacv3.RBACPerRoute, e
 		rbac.Rbac.Matcher.MatcherType = nil
 	}
 
-	// We need to validate the RBACPerRoute message before converting it to an Any.
-	if err = rbac.ValidateAll(); err != nil {
-		return nil, err
-	}
-
 	return rbac, nil
 }
 
@@ -316,11 +312,11 @@ func buildIPPredicate(clientCIDRs []*ir.CIDRMatch) (*matcherv3.Matcher_MatcherLi
 		})
 	}
 
-	if ipMatcher, err = anypb.New(ipRangeMatcher); err != nil {
+	if ipMatcher, err = protocov.ToAnyWithValidation(ipRangeMatcher); err != nil {
 		return nil, err
 	}
 
-	if sourceIPInput, err = anypb.New(&networkinput.SourceIPInput{}); err != nil {
+	if sourceIPInput, err = protocov.ToAnyWithValidation(&networkinput.SourceIPInput{}); err != nil {
 		return nil, err
 	}
 
@@ -389,11 +385,11 @@ func buildJWTPredicate(jwt egv1a1.JWTPrincipal) ([]*matcherv3.Matcher_MatcherLis
 			},
 		}
 
-		if inputPb, err = anypb.New(input); err != nil {
+		if inputPb, err = protocov.ToAnyWithValidation(input); err != nil {
 			return nil, err
 		}
 
-		if matcherPb, err = anypb.New(scopeMatcher); err != nil {
+		if matcherPb, err = protocov.ToAnyWithValidation(scopeMatcher); err != nil {
 			return nil, err
 		}
 
@@ -454,7 +450,7 @@ func buildJWTPredicate(jwt egv1a1.JWTPrincipal) ([]*matcherv3.Matcher_MatcherLis
 			Path:   path,
 		}
 
-		if inputPb, err = anypb.New(input); err != nil {
+		if inputPb, err = protocov.ToAnyWithValidation(input); err != nil {
 			return nil, err
 		}
 
@@ -492,7 +488,7 @@ func buildJWTPredicate(jwt egv1a1.JWTPrincipal) ([]*matcherv3.Matcher_MatcherLis
 				}
 			}
 
-			if matcherPb, err = anypb.New(&metadatav3.Metadata{
+			if matcherPb, err = protocov.ToAnyWithValidation(&metadatav3.Metadata{
 				Value: valueMatcher,
 			}); err != nil {
 				return nil, err

diff --git a/internal/xds/translator/basicauth.go b/internal/xds/translator/basicauth.go
@@ -17,6 +17,7 @@ import (
 
 	egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
 	"github.com/envoyproxy/gateway/internal/ir"
+	"github.com/envoyproxy/gateway/internal/utils/protocov"
 	"github.com/envoyproxy/gateway/internal/xds/types"
 )
 
@@ -84,7 +85,7 @@ func buildHCMBasicAuthFilter(basicAuth *ir.BasicAuth) (*hcmv3.HttpFilter, error)
 	if err = basicAuthProto.ValidateAll(); err != nil {
 		return nil, err
 	}
-	if basicAuthAny, err = anypb.New(basicAuthProto); err != nil {
+	if basicAuthAny, err = protocov.ToAnyWithValidation(basicAuthProto); err != nil {
 		return nil, err
 	}
 
@@ -134,7 +135,7 @@ func (*basicAuth) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute) error
 		return err
 	}
 
-	if basicAuthAny, err = anypb.New(basicAuthProto); err != nil {
+	if basicAuthAny, err = protocov.ToAnyWithValidation(basicAuthProto); err != nil {
 		return err
 	}
 

diff --git a/internal/xds/translator/cluster.go b/internal/xds/translator/cluster.go
@@ -30,6 +30,7 @@ import (
 
 	egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
 	"github.com/envoyproxy/gateway/internal/ir"
+	"github.com/envoyproxy/gateway/internal/utils/protocov"
 )
 
 const (
@@ -509,7 +510,7 @@ func buildTypedExtensionProtocolOptions(args *xdsClusterArgs) map[string]*anypb.
 	if args.http1Settings != nil {
 		http1opts.EnableTrailers = args.http1Settings.EnableTrailers
 		if args.http1Settings.PreserveHeaderCase {
-			preservecaseAny, _ := anypb.New(&preservecasev3.PreserveCaseFormatterConfig{})
+			preservecaseAny, _ := protocov.ToAnyWithValidation(&preservecasev3.PreserveCaseFormatterConfig{})
 			http1opts.HeaderKeyFormat = &corev3.Http1ProtocolOptions_HeaderKeyFormat{
 				HeaderFormat: &corev3.Http1ProtocolOptions_HeaderKeyFormat_StatefulFormatter{
 					StatefulFormatter: &corev3.TypedExtensionConfig{
@@ -562,7 +563,7 @@ func buildTypedExtensionProtocolOptions(args *xdsClusterArgs) map[string]*anypb.
 		}
 	}
 
-	anyProtocolOptions, _ := anypb.New(&protocolOptions)
+	anyProtocolOptions, _ := protocov.ToAnyWithValidation(&protocolOptions)
 
 	extensionOptions := map[string]*anypb.Any{
 		extensionOptionsKey: anyProtocolOptions,
@@ -593,7 +594,7 @@ func buildProxyProtocolSocket(proxyProtocol *ir.ProxyProtocol, tSocket *corev3.T
 	// If existing transport socket does not exist wrap around raw buffer
 	if tSocket == nil {
 		rawCtx := &rawbufferv3.RawBuffer{}
-		rawCtxAny, err := anypb.New(rawCtx)
+		rawCtxAny, err := protocov.ToAnyWithValidation(rawCtx)
 		if err != nil {
 			return nil
 		}
@@ -608,7 +609,7 @@ func buildProxyProtocolSocket(proxyProtocol *ir.ProxyProtocol, tSocket *corev3.T
 		ppCtx.TransportSocket = tSocket
 	}
 
-	ppCtxAny, err := anypb.New(ppCtx)
+	ppCtxAny, err := protocov.ToAnyWithValidation(ppCtx)
 	if err != nil {
 		return nil
 	}

diff --git a/internal/xds/translator/custom_response.go b/internal/xds/translator/custom_response.go
@@ -24,6 +24,7 @@ import (
 
 	egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
 	"github.com/envoyproxy/gateway/internal/ir"
+	"github.com/envoyproxy/gateway/internal/utils/protocov"
 	"github.com/envoyproxy/gateway/internal/xds/types"
 )
 
@@ -85,7 +86,7 @@ func (c *customResponse) buildHCMCustomResponseFilter(ro *ir.ResponseOverride) (
 		return nil, err
 	}
 
-	any, err := anypb.New(proto)
+	any, err := protocov.ToAnyWithValidation(proto)
 	if err != nil {
 		return nil, err
 	}
@@ -237,7 +238,7 @@ func (c *customResponse) buildHTTPAttributeCELInput() (*cncfv3.TypedExtensionCon
 		err error
 	)
 
-	if pb, err = anypb.New(&matcherv3.HttpAttributesCelMatchInput{}); err != nil {
+	if pb, err = protocov.ToAnyWithValidation(&matcherv3.HttpAttributesCelMatchInput{}); err != nil {
 		return nil, err
 	}
 
@@ -253,7 +254,7 @@ func (c *customResponse) buildStatusCodeInput() (*cncfv3.TypedExtensionConfig, e
 		err error
 	)
 
-	if pb, err = anypb.New(&envoymatcherv3.HttpResponseStatusCodeMatchInput{}); err != nil {
+	if pb, err = protocov.ToAnyWithValidation(&envoymatcherv3.HttpResponseStatusCodeMatchInput{}); err != nil {
 		return nil, err
 	}
 
@@ -364,7 +365,7 @@ func (c *customResponse) buildStatusCodeCELMatcher(codeRange ir.StatusCodeRange)
 		return nil, err
 	}
 
-	if pb, err = anypb.New(matcher); err != nil {
+	if pb, err = protocov.ToAnyWithValidation(matcher); err != nil {
 		return nil, err
 	}
 
@@ -403,7 +404,7 @@ func (c *customResponse) buildAction(r ir.ResponseOverrideRule) (*matcherv3.Matc
 		return nil, err
 	}
 
-	if pb, err = anypb.New(response); err != nil {
+	if pb, err = protocov.ToAnyWithValidation(response); err != nil {
 		return nil, err
 	}
 

diff --git a/internal/xds/translator/fault.go b/internal/xds/translator/fault.go
@@ -20,6 +20,7 @@ import (
 
 	egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
 	"github.com/envoyproxy/gateway/internal/ir"
+	"github.com/envoyproxy/gateway/internal/utils/protocov"
 	"github.com/envoyproxy/gateway/internal/xds/types"
 )
 
@@ -71,7 +72,7 @@ func buildHCMFaultFilter() (*hcmv3.HttpFilter, error) {
 		return nil, err
 	}
 
-	faultAny, err := anypb.New(faultProto)
+	faultAny, err := protocov.ToAnyWithValidation(faultProto)
 	if err != nil {
 		return nil, err
 	}
@@ -165,7 +166,7 @@ func (*fault) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute) error {
 		return nil
 	}
 
-	routeCfgAny, err := anypb.New(routeCfgProto)
+	routeCfgAny, err := protocov.ToAnyWithValidation(routeCfgProto)
 	if err != nil {
 		return err
 	}