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

feature: Add a new SortMode to encode struct fields in a less predictable order #514

Closed
benluddy opened this issue Apr 2, 2024 · 3 comments

Comments

@benluddy
Copy link
Contributor

benluddy commented Apr 2, 2024

Is your feature request related to a problem? Please describe.

When encoding a Go struct to a CBOR map with SortNone, keys are encoded based on the field order stored in the fields field of encodingStructType. The current implementation happens to sort fields in a certain way, but even if it did not, the encodingStructType for a given struct type is built once and cached for the life of the program. The result is that encoding a Go struct to a CBOR map appears to be deterministic, even with SortNone.

This is of course not wrong, but it does create opportunity for users to erroneously build into their programs and APIs the assumption that struct encoding is deterministic. I would like to make it harder for users to accidentally depend on this behavior.

Describe the solution you'd like

I propose a new SortMode that begins struct encoding at a random field offset (in the spirit of map iterator initialization https://github.com/golang/go/blob/b6efc3b755b74147a3700ad51773b01fa68f76e8/src/runtime/map.go#L837-L840). This would have negligible runtime overhead but would be, I think, sufficient to prevent programs from relying on order.

Describe alternatives you've considered

  • Changing SortNone may be a breaking change for someone, precisely because they have inadvertently assumed that struct encoding is deterministic (https://www.hyrumslaw.com/). A new mode would be fine for my use case, but in that case it may still be a good idea to rename SortNone based on its implicit order (SortFieldIndex?) and add a const SortNone SortMode = SortFieldIndex // DEPRECATED to save future users. Edit: I tried changing the behavior of SortNone in a POC and it reinforced my concerns.
  • Randomly permuting the field encode order on every encode seems likely to incur some runtime overhead. I don't think that would be worth it since randomness is not necessary.

Additional context

@benluddy
Copy link
Contributor Author

benluddy commented Apr 2, 2024

I drafted a quick POC of this proposal in #515. Some of the unit tests use SortNone and rely on deterministic struct encoding (https://github.com/fxamacker/cbor/actions/runs/8529815861/job/23366362431?pr=515).

@benluddy
Copy link
Contributor Author

The POC reinforced this risks of changing the behavior of SortNone. I'm now proposing a new sort mode to handle this, and #515 has been updated accordingly.

@fxamacker
Copy link
Owner

Thanks Ben! Closed by #515

@fxamacker fxamacker changed the title feature: Protect users from assuming a deterministic encoding of structs using SortNone feature: Add a new SortMode to encode struct fields in a less predictable order Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants