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

Protobuf unknown field rejection #6192

Closed
2 of 3 tasks
aaronc opened this issue May 11, 2020 · 2 comments
Closed
2 of 3 tasks

Protobuf unknown field rejection #6192

aaronc opened this issue May 11, 2020 · 2 comments

Comments

@aaronc
Copy link
Member

aaronc commented May 11, 2020

Summary

As discussed in #6078, we need to generally reject unknown protobuf fields and this is not done automatically in signature verification with our current approach. Also, we want to add a reserved range of "non-critical" fields for forwards compatibility with newer clients.

Basically, a transaction processor should generally reject transactions that it does not understand because not doing so would give clients the expectation that something had been handled when it had actually just been ignored. However, there may be cases where we want fields that aren't understood to be silently ignored, thus the "non-critical" field range.

Therefore we need code that:

  • parses a protobuf message with its schema and rejects unknown fields even tranversing inside Anys
  • except does not reject fields in the "non-critical" range of 1028-2047 (i.e. bit 11 set)

This will likely be a custom protobuf parser pass that does not actually unmarshal any values but simply checks a message for compatibility.

TODO

@aaronc aaronc mentioned this issue May 13, 2020
27 tasks
@clevinson clevinson added this to the v0.39 milestone May 29, 2020
@clevinson clevinson assigned clevinson and unassigned clevinson Jun 26, 2020
@odeke-em
Copy link
Collaborator

Thanks for filing this issue @aaronc! I have started working on this issue today.

@aaronc
Copy link
Member Author

aaronc commented Aug 7, 2020

Closing this, improved tests can continue in #6977

@aaronc aaronc closed this as completed Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants