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

FIX: botan_cipher_get_update_granularity() #4094

Closed
wants to merge 2 commits into from

Conversation

reneme
Copy link
Collaborator

@reneme reneme commented Jun 3, 2024

With #3951 botan_cipher_update() gained a documented way to communicate the required buffer size to finalize certain cipher modes if not enough bytes were provided by the caller. I feel that this actually obsoletes the internal helper function ffi_choose_update_size() altogether. Instead, I suggest to just pass through what the Cipher_Mode object reports.

Note that this might harm performance for users that currently use botan_cipher_get_update_granularity() (similarly to #3925). Such users should switch to botan_cipher_get_ideal_update_granularity(), as explained in the Botan 3.0 migration guide.

Previously, the FFI wrapper had to take the final invocation into
account. Now that botan_cipher_update() reports the required buffer
size when invoked with BOTAN_CIPHER_UPDATE_FLAG_FINAL this is not
a strict requirement anymore.
@reneme reneme added this to the Botan 3.5.0 milestone Jun 3, 2024
@reneme reneme self-assigned this Jun 3, 2024
@reneme reneme requested a review from randombit June 3, 2024 10:27
@coveralls
Copy link

Coverage Status

coverage: 91.822% (-0.002%) from 91.824%
when pulling efd9e69 on Rohde-Schwarz:fix/update_granularity
into 5649a10 on randombit:master.


/*
* Return the minimum possible granularity given the FFI API constraints that
* we require the returned size be > minimum final size.
Copy link
Owner

Choose a reason for hiding this comment

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

Does this condition no longer hold? I would think this would break callers.

Copy link
Collaborator Author

@reneme reneme Jun 4, 2024

Choose a reason for hiding this comment

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

To the best of my understanding it doesn't hold anymore (after the introduction of #3951). For instance: an AEAD that allows single-byte updates, could now be used via botan_cipher_update() with single-byte buffers. When trying to finalize the cipher with a single-byte bulffer, it will return a BOTAN_FFI_ERROR_INSUFFICIENT_BUFFER_SPACE and report the size of the required buffer via the out-variable. Repeating the finalize-call with a big-enough buffer will work.

Nevertheless, I totally agree that this could potentially break callers and force them to add similar handling as I explained above. See also the discussion here: #4090 (comment).

(I meant to open this as a draft-PR, sorry)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI: #4122 is an alternative that keeps this condition alive. ... to the best of my understanding.

@reneme
Copy link
Collaborator Author

reneme commented Jul 4, 2024

Closing as obsolete after #4122 got merged. We might bring this back with 4.0 as a new API.

@reneme reneme closed this Jul 4, 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

Successfully merging this pull request may close these issues.

3 participants