Lax parser feature #363
Lax parser feature #363
Conversation
cschramm
requested review from
DGonzalezVillal,
larrydewey and
tylerfanelli
as code owners
cschramm
force-pushed
the
lax-decoders
branch
from
e6e308a to
629cd46
Compare
The parser strictly checks reserved areas to be zero (even though the specification does not even require that for all of them). This is an issue when parsing newer versions of attestation reports where those areas might not be reserved anymore. The `lax-parser` feature disables that check, currently with some tradeoffs: * No `WriteExt::skip_bytes` * No `Encoder` impl for `AttestationReport`, `Signature`, and `WrappedVlekHashstick` * No compatibility with `openssl` and `crypto_nossl` Closes virtee#343 Signed-off-by: Christopher Schramm <git@cschramm.eu>
cschramm
force-pushed
the
lax-decoders
branch
from
629cd46 to
e5c58d6
Compare
|
So in this solution, you would only be able to parse the attestation report, but you would not be able to write it anywhere correct? Is that still useful to you? |
|
Because the Write stuff can be modified to write the raw contents. Create something like a raw write |
Absolutely, yes. I only use the parser, but not the As I wrote in #343, writing could be achieved by actually reading reserved areas and carrying their contents to write them back instead of zeroes. I was under the impression, though, that you wanted to keep this a limited parse-only case. |
DGonzalezVillal
left a comment
DGonzalezVillal
left a comment
There was a problem hiding this comment.
Hello @cschramm,
Apologies for the delay—I was out on break.
After taking a closer look at your PR, I’m thinking a better approach might be to shift from the current lax-parsing idea to something more like a raw read/write (or unsafe read/write) mode. With that approach, this feature would simply read and write the raw binary contents as-is.
In our earlier discussions, I may have implied that I wanted this scoped strictly to parsing, but after reviewing the PR and realizing that lax parsing would require disabling or weakening other APIs, I’m less comfortable with that direction. I’d prefer not to reduce existing functionality to support a single feature. This alternative also results in fewer changes overall, since it would mainly involve updating the read and write paths to operate on raw bytes without safety checks.
This keeps the library behavior unchanged for most users, while clearly shifting responsibility for validation and safety checks to users who opt into this mode.
Please let me know what you think, and apologies again for the back and forth.
Thanks!
| Ok(()) | ||
| } | ||
|
|
||
| #[cfg(not(feature = "lax-parser"))] |
There was a problem hiding this comment.
Why do you have to disable this API for lax-parser?
| #[cfg(all( | ||
| any(feature = "openssl", feature = "crypto_nossl"), | ||
| feature = "lax-parser" | ||
| ))] |
There was a problem hiding this comment.
Why would lax-parser be part of this check?
4 participants
The parser strictly checks reserved areas to be zero (even though the specification does not even require that for all of them). This is an issue when parsing future (i.e., not known to the current codebase) versions of attestation reports, where those areas may no longer be reserved.
The
lax-parserfeature disables that check, currently with some tradeoffs:WriteExt::skip_bytesEncoderimplforAttestationReport,Signature, andWrappedVlekHashstickopensslandcrypto_nosslCloses #343