fix: validate proto messages before converting them to anypb.Any #4499

zhaohuabing · 2024-10-23T03:33:08Z

The validate and validateAll method of a proto message don't validate a member field of anypb.Any type as it doesn't have a validate method itself. Therefore, all proto messages must be validated before been converting to anypb.Any.

Fix #4504

Release note: No

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing · 2024-10-24T02:07:08Z

internal/xds/translator/testdata/in/xds-ir/accesslog.yaml

@@ -13,7 +13,8 @@ accesslog:
      protocol: "%PROTOCOL%"
      response_code: "%RESPONSE_CODE%"
  als:
-  - destination:
+  - name: als


--- FAIL: TestTranslateXds (0.00s) --- FAIL: TestTranslateXds/accesslog (0.00s) translator_test.go:142: accesslog translator_test.go:143: Error Trace: /home/ubuntu/gateway/internal/xds/translator/translator_test.go:143 Error: Received unexpected error: invalid HttpGrpcAccessLogConfig.CommonConfig: embedded message failed validation | caused by: invalid CommonGrpcAccessLogConfig.LogName: value length must be at least 1 runes Test: TestTranslateXds/accesslog

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing · 2024-10-24T02:11:33Z

internal/xds/translator/testdata/in/xds-ir/accesslog-without-format.yaml

@@ -11,7 +11,8 @@ accesslog:
      protocol: "%PROTOCOL%"
      response_code: "%RESPONSE_CODE%"
  als:
-  - destination:
+  - name: als


go test -timeout 30s github.com/envoyproxy/gateway/internal/xds/translator --override-testdata=true --- FAIL: TestTranslateXds (0.42s) --- FAIL: TestTranslateXds/accesslog-without-format (0.00s) translator_test.go:142: accesslog-without-format translator_test.go:143: Error Trace: /home/ubuntu/gateway/internal/xds/translator/translator_test.go:143 Error: Received unexpected error: invalid HttpGrpcAccessLogConfig.CommonConfig: embedded message failed validation | caused by: invalid CommonGrpcAccessLogConfig.LogName: value length must be at least 1 runes

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing · 2024-10-24T02:23:43Z

internal/xds/translator/testdata/in/xds-ir/authorization-multiple-principals.yaml

@@ -44,7 +44,7 @@ http:
              isIPv6: false
              maskLen: 24
            jwt:
-              issuer: https://one.example.com
+              provider: https://one.example.com


--- FAIL: TestTranslateXds (0.00s) --- FAIL: TestTranslateXds/authorization-multiple-principals (0.00s) translator_test.go:142: authorization-multiple-principals translator_test.go:143: Error Trace: /home/ubuntu/gateway/internal/xds/translator/translator_test.go:143 Error: Received unexpected error: invalid DynamicMetadataInput.Path[0]: embedded message failed validation | caused by: invalid DynamicMetadataInput_PathSegment.Key: value length must be at least 1 runes Test: TestTranslateXds/authorization-multiple-principals

codecov · 2024-10-24T02:30:29Z

Codecov Report

Attention: Patch coverage is 49.59350% with 62 lines in your changes missing coverage. Please review.

Project coverage is 65.54%. Comparing base (9353be2) to head (0e272b1).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/xds/translator/listener.go	58.13%	12 Missing and 6 partials ⚠️
internal/xds/translator/accesslog.go	34.78%	10 Missing and 5 partials ⚠️
internal/xds/translator/translator.go	15.38%	7 Missing and 4 partials ⚠️
internal/xds/translator/authorization.go	20.00%	0 Missing and 8 partials ⚠️
internal/utils/protocov/protocov.go	71.42%	3 Missing and 1 partial ⚠️
internal/xds/translator/custom_response.go	20.00%	0 Missing and 4 partials ⚠️
internal/xds/translator/basicauth.go	0.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4499      +/-   ##
==========================================
- Coverage   65.60%   65.54%   -0.07%     
==========================================
  Files         210      210              
  Lines       31612    31649      +37     
==========================================
+ Hits        20739    20744       +5     
- Misses       9670     9690      +20     
- Partials     1203     1215      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing · 2024-10-24T02:53:36Z

internal/utils/protocov/protocov.go

+func ToAny(msg proto.Message) *anypb.Any {
+	res, err := ToAnyWithValidation(msg)
+	if err != nil {
+		return nil


The validation error should not be ignored.

Leave the ToAny function for now as the impact are a bit large and we're approaching the rc1. I will raise a follow-up PR to clean up all the refrences to it after v1.2.0 release.

For cases like the following, 'ToAny' still makes sense, but you really should use ToAnyWithValidation in most cases.

https://github.com/envoyproxy/gateway/blob/988d4ed0be5a5f9dd39a784d52c3bb5c51ecc2aa/internal/xds/filters/wellknown.go

should we change the comment?

I prefer removing this function at all, but let's discuss and decide in a follow-up PR. Tracked in #4554

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

arkodg · 2024-10-28T17:47:24Z

hey @zhaohuabing can you rebase ?

zirain · 2024-10-29T00:02:29Z

this's worth to backport if possible.

arkodg

LGTM thanks for fixing it across the board

…oyproxy#4499) * validate proto message before converting to any Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> (cherry picked from commit 05817fc) Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing requested a review from a team as a code owner October 23, 2024 03:33

zhaohuabing marked this pull request as draft October 23, 2024 03:33

zhaohuabing changed the title ~~chore: validate proto message before converting to any~~ chore: validate proto messages before converting them to any Oct 23, 2024

zhaohuabing force-pushed the proto-validation branch from 5403023 to 87ab699 Compare October 23, 2024 03:43

validate proto message before converting to any

942de59

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing force-pushed the proto-validation branch from 87ab699 to 942de59 Compare October 23, 2024 03:47

validate proto message before converting to any

9156ad7

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing force-pushed the proto-validation branch from 9e01d42 to 9156ad7 Compare October 23, 2024 04:02

zhaohuabing added 3 commits October 23, 2024 04:15

fix test

ecb5ead

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

fix test

9fedc1c

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

add test for invalid message

5c5e9ed

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing changed the title ~~chore: validate proto messages before converting them to any~~ fix: validate proto messages before converting them to any Oct 23, 2024

zhaohuabing added 2 commits October 24, 2024 01:17

fix lint

452b877

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

fix accesslog translation test

d4206ab

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing commented Oct 24, 2024

View reviewed changes

fix accesslog translation test

745f752

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing commented Oct 24, 2024

View reviewed changes

fix authz translation test

303655a

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing commented Oct 24, 2024

View reviewed changes

zhaohuabing added 2 commits October 24, 2024 02:36

unified with ToAnyWithValidation

d03b269

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

Merge remote-tracking branch 'origin/main' into proto-validation

d9c9211

zhaohuabing commented Oct 24, 2024

View reviewed changes

zhaohuabing marked this pull request as ready for review October 24, 2024 03:08

zhaohuabing requested review from guydc and a team October 24, 2024 03:11

arkodg added this to the v1.2.0 milestone Oct 24, 2024

Merge branch 'main' into proto-validation

abd4f84

zhaohuabing changed the title ~~fix: validate proto messages before converting them to any~~ fix: validate proto messages before converting them to anypb.Any Oct 24, 2024

remove todo

861013b

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing added the cherrypick/release-v1.2 label Oct 28, 2024

zirain previously approved these changes Oct 28, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into proto-validation

51a5502

zhaohuabing dismissed zirain’s stale review via 51a5502 October 28, 2024 23:54

zhaohuabing force-pushed the proto-validation branch from 0e272b1 to 51a5502 Compare October 28, 2024 23:54

zirain approved these changes Oct 29, 2024

View reviewed changes

zhaohuabing added the cherrypick/release-v1.1 cherrypick to release/v1.1 label Oct 29, 2024

arkodg approved these changes Oct 29, 2024

View reviewed changes

zhaohuabing mentioned this pull request Oct 29, 2024

Clean up anypb.ToAny without validation #4554

Open

Merge branch 'main' into proto-validation

f679d53

arkodg merged commit 05817fc into envoyproxy:main Oct 29, 2024
20 of 21 checks passed

zhaohuabing deleted the proto-validation branch October 29, 2024 01:26

arkodg added the cherrypick/release-v1.1.3 label Oct 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: validate proto messages before converting them to anypb.Any #4499

fix: validate proto messages before converting them to anypb.Any #4499

zhaohuabing commented Oct 23, 2024 •

edited

Loading

zhaohuabing Oct 24, 2024

zhaohuabing Oct 24, 2024

zhaohuabing Oct 24, 2024

codecov bot commented Oct 24, 2024 •

edited

Loading

zhaohuabing Oct 24, 2024 •

edited

Loading

zirain Oct 26, 2024 •

edited

Loading

zirain Oct 29, 2024

zhaohuabing Oct 29, 2024 •

edited

Loading

arkodg commented Oct 28, 2024

zirain commented Oct 29, 2024

arkodg left a comment

fix: validate proto messages before converting them to anypb.Any #4499

fix: validate proto messages before converting them to anypb.Any #4499

Conversation

zhaohuabing commented Oct 23, 2024 • edited Loading

zhaohuabing Oct 24, 2024

Choose a reason for hiding this comment

zhaohuabing Oct 24, 2024

Choose a reason for hiding this comment

zhaohuabing Oct 24, 2024

Choose a reason for hiding this comment

codecov bot commented Oct 24, 2024 • edited Loading

Codecov Report

zhaohuabing Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

zirain Oct 26, 2024 • edited Loading

Choose a reason for hiding this comment

zirain Oct 29, 2024

Choose a reason for hiding this comment

zhaohuabing Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

arkodg commented Oct 28, 2024

zirain commented Oct 29, 2024

arkodg left a comment

Choose a reason for hiding this comment

zhaohuabing commented Oct 23, 2024 •

edited

Loading

codecov bot commented Oct 24, 2024 •

edited

Loading

zhaohuabing Oct 24, 2024 •

edited

Loading

zirain Oct 26, 2024 •

edited

Loading

zhaohuabing Oct 29, 2024 •

edited

Loading