Skip to content
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.

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

metadata: validate metadata keys and values #4886

Merged
merged 26 commits into from
Feb 23, 2022
Merged

Conversation

Patrick0308
Copy link
Contributor

@Patrick0308 Patrick0308 commented Oct 19, 2021

validate grpc metadata to ensure grpc to follow http standards

close #468

RELEASE NOTES:

  • TBD

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 19, 2021

CLA Signed

The committers are authorized under a signed CLA.

@zasweq zasweq added this to the 1.43 release milestone Nov 2, 2021
@easwars
Copy link
Contributor

easwars commented Nov 17, 2021

Based on #468 (comment), I'm guessing we also need to change client and server side header sending routines to perform this validation.

@Patrick0308
Was your plan to also do those changes?

Also, it would be better if a regex is used instead of manually checking every character in the metadata key/value.

@dfawley
Copy link
Member

dfawley commented Nov 17, 2021

Also, it would be better if a regex is used instead of manually checking every character in the metadata key/value.

I wouldn't be certain of this. Regexes are probably going to be slower. We'd want to do some performance testing if this is done per-RPC (which it probably should).

@Patrick0308
Copy link
Contributor Author

Patrick0308 commented Nov 18, 2021

@easwars Yeah, I'll do it. I think regexes are slower than checking every character. But I'm not sure too.

@Patrick0308 Patrick0308 changed the title (WIP) fix: does not validate metadata keys and values fix: does not validate metadata keys and values Nov 24, 2021
@Patrick0308
Copy link
Contributor Author

@dfawley @easwars PTAL

@dfawley
Copy link
Member

dfawley commented Nov 30, 2021

@dfawley @easwars PTAL

Looks like we also need this validation in stream.go:(*ClientConn).NewStream

@menghanl it seems to me this validation should just be private to the grpc package, instead of a method on the metadata itself. WDYT? And did you say you found a function we can use for this in a stdlib package?

@menghanl
Copy link
Contributor

Moving this to a private package sounds good.

There's no function does exactly what we want.
We can use https://pkg.go.dev/unicode#IsDigit for the digits.
https://pkg.go.dev/unicode#IsLetter checks all unicode letters, not just ascii (we can check <=MaxASCII but that's an extra step).

metadata/metadata.go Outdated Show resolved Hide resolved
metadata/metadata_test.go Outdated Show resolved Hide resolved
metadata/metadata.go Outdated Show resolved Hide resolved
test/metadata_test.go Outdated Show resolved Hide resolved
test/metadata_test.go Show resolved Hide resolved
test/metadata_test.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned Patrick0308 and unassigned dfawley Jan 27, 2022
@dfawley
Copy link
Member

dfawley commented Feb 1, 2022

Friendly ping. This is very close to being ready to merge; please let me know if you have any questions or need help with anything.

@Patrick0308
Copy link
Contributor Author

@dfawley I'm back. In the last few days, I've been spending Chinese New Year.

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

LGTM except for the one small formatting nit. Thank you!

test/metadata_test.go Outdated Show resolved Hide resolved
@Patrick0308
Copy link
Contributor Author

@dfawley PTAL

@easwars easwars modified the milestones: 1.45 Release, 1.46 Release Feb 22, 2022
@easwars easwars merged commit e601f1a into grpc:master Feb 23, 2022
@easwars easwars changed the title fix: does not validate metadata keys and values metadata: validate metadata keys and values Feb 23, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Go does not validate metadata keys and values
5 participants