Skip to content

feat: introduce unsafe_parser#372

Open
DGonzalezVillal wants to merge 1 commit intovirtee:mainfrom
DGonzalezVillal:simple-unsafe
Open

feat: introduce unsafe_parser#372
DGonzalezVillal wants to merge 1 commit intovirtee:mainfrom
DGonzalezVillal:simple-unsafe

Conversation

@DGonzalezVillal
Copy link
Member

@DGonzalezVillal DGonzalezVillal commented Jan 29, 2026

When this feature is enabled, the zero-byte check performed by skip_bytes is disabled. As a result, parsing will no longer fail when fields that are expected to be zero contain non-zero values.

This enables forward compatibility when reserved fields are repurposed in newer report versions. Users enabling this feature must take care to validate reserved fields manually if they rely on their contents for security or correctness.

The approaches of preserving values in reserved fields or treating the report as an opaque blob both have potential. However, based on concerns raised by @cschramm and @hyperfinitism, I think there is a larger design discussion to be had—one that could ultimately change how some of these structs are built and may require a breaking API release. To avoid dependency issues like the one @cshramm pointed out in #370
, I’m hesitant to put such changes behind a feature flag. The alternative—introducing a separate report type—is also not ideal.

I don’t want to block @cshramm any longer, so this change immediately addresses the forward compatibility problem. I’m aware this approach has trade-offs, such as losing access to the parsed bytes of invalid reserved fields. However, since we currently have no appropriate fields to store that data, the report maintains type safety by only exposing known, validated fields. Additionally, the API already returns the raw binary report, so those bytes can still be inspected if needed.

If there are additional functions or features you think would make this more immediately usable, please let me know. I’ll start a separate discussion to explore how we might incorporate @hyperfinitism’s ideas into a future major release.

When this feature is enabled, the zero-byte check performed by skip_bytes
is disabled. As a result, parsing will no longer fail when fields that are
expected to be zero contain non-zero values.

This enables forward compatibility when reserved fields are repurposed in
new report versions. Users enabling this feature must take care to
validate reserved fields manually if they rely on their contents for
security or correctness.

Signed-off-by: DGonzalezVillal <Diego.GonzalezVillalobos@amd.com>
Copy link

@cschramm cschramm left a comment

Choose a reason for hiding this comment

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

Thank you, @DGonzalezVillal. Your push to release any solution to unblock me is very kind.

This very minimal solution is perfectly fine for me.

Just bare in mind that the Encoder impls still assume that reserved bytes are zero, leading to invalid binary and broken features that rely on it, especially the cryptographic verification. #363 thus disables those parts while #366 reads and writes reserved areas to avoid that (leading to overhead even without the feature enabled).

I'd personally favor the #363 approach to avoid the footgun that comes with the feature otherwise, but that's up to you.

@DGonzalezVillal
Copy link
Member Author

Hey @cschramm — I was looking more closely at your comment, and I think the Encoder issue is really a consequence of the current typed design, which effectively ignores reserved fields.

On the decoding side, we already assume reserved bytes are zero and simply skip them. In both #363
and here, what we’re really doing is disabling the failure that occurs when those reserved bytes are non-zero. However, that data is still lost once the report is parsed into the struct. Even if the Encoder didn’t skip bytes and attempted to re-emit the AttestationReport “as is,” the information has already been discarded at parse time.

This is also why @hyperfinitism has been pushing for a raw model that preserves all bytes and only interprets fields when needed.

For that reason, I’m intentionally calling this quick implementation unsafe rather than lax. As you pointed out, it can lead to invalid binaries and broken behavior, and that risk fundamentally comes from how AttestationReport is currently structured.

I do want to design a safer implementation that addresses what you and @hyperfinitism are asking for, but I don’t see a way to do that without significant changes to AttestationReport. The point you raised in #370
in particular made me pause, since it highlights a real compatibility issue between crates that depend on different feature configurations. Because of that, I’d like to slow down and think this through more carefully.

That said, I think your point is valid: if an unsafe (or “lax”) parser is enabled, then report verification should probably be disabled, since parsing and re-encoding the report no longer preserves integrity. Would that change make this a more acceptable interim solution?

Thoughts, @hyperfinitism and @tylerfanelli?

@cschramm
Copy link

Any chance to get this (or a different short-term solution) merged?

@DGonzalezVillal
Copy link
Member Author

@tylerfanelli Are you ok with merging this or PR #363?

I think after that we cut a minor release, because the more complete solution in #378 will have major changes.

For the next major release we can also address the concerns in #369

thoughts??

Copy link
Member

@tylerfanelli tylerfanelli left a comment

Choose a reason for hiding this comment

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

LGTM.

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

Successfully merging this pull request may close these issues.

4 participants

Comments