fix(encryption): reject trailing bytes after key metadata datum - #3225
xanderbailey wants to merge 1 commit into
Conversation
`from_avro_datum` reads a single datum from the reader and ignores anything after it, so `StandardKeyMetadata::decode` accepted a valid datum followed by arbitrary trailing bytes. Check the reader was exhausted and fail with `DataInvalid` otherwise.
blackmwk
left a comment
blackmwk
left a comment
There was a problem hiding this comment.
Thanks @xanderbailey for this pr. Could you clarify what concrete issue this check fixes? Java’s RawDecoder.decode
(https://github.com/apache/iceberg/blob/c4784768b2f38c5a8c70d8e0e5a07f4302f9282f/core/src/main/java/org/apache/iceberg/data/avro/RawDecoder.java#L65)
reads one Avro datum without checking EOF, so this introduces stricter behavior than Java.
There is also a forward-compatibility concern: Java previously appended file_length to SCHEMA_V1 without changing the version byte
(apache/iceberg@11f3dcb). Ignoring trailing bytes lets older readers tolerate
appended fields; requiring EOF would prevent similar extensions.
I’d prefer preserving Java’s behavior unless there is a specific requirement or demonstrated bug that justifies this restriction.
|
You are correct, this came up in a review of the python bindings but your point here is very solid. We should keep the current version. Thanks for taking a look @blackmwk |
2 participants
from_avro_datumreads a single datum from the reader and ignores anything after it, soStandardKeyMetadata::decodeaccepted a valid datum followed by arbitrary trailing bytes. Check the reader was exhausted and fail withDataInvalidotherwise.Which issue does this PR close?
What changes are included in this PR?
Are these changes tested?
AI Disclosure