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

std::span for Botan::Cipher_Mode #3392

Merged
merged 4 commits into from
Mar 21, 2023
Merged

std::span for Botan::Cipher_Mode #3392

merged 4 commits into from
Mar 21, 2023

Conversation

reneme
Copy link
Collaborator

@reneme reneme commented Mar 20, 2023

Two things:

  • allow Cipher_Mode::process() and Cipher_Mode::start() for arbitrary contiguous containers
  • allow concepts::resizable_byte_buffer for the in/out parameters of Cipher_Mode::update() and Cipher_Mode::finish().

To do that properly a "virtual entrypoint" indirection was introduced. Namely Cipher_Mode::process_msg() and Cipher_Mode::finish_msg().

Note that Cipher_Mode::finish() with anything but Botan::secure_vector<uint8_t> will introduce additional data copies. We might be able to address this later on (by generalizing the virtual entrypoint for Cipher_Mode). But that felt entirely out of scope here. FWIW: Users that had their data stored in some other container were previously forced to do the same copy ceremony before in their application code. That's a hot take, maybe. I'm open for better approaches.

@reneme reneme force-pushed the span/cipher_modes branch from 7fc8b1f to 9f63da9 Compare March 20, 2023 11:23
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (eb51e40) 88.13% compared to head (9f63da9) 88.14%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3392   +/-   ##
=======================================
  Coverage   88.13%   88.14%           
=======================================
  Files         617      617           
  Lines       70358    70350    -8     
  Branches     6984     6987    +3     
=======================================
- Hits        62011    62010    -1     
+ Misses       5402     5398    -4     
+ Partials     2945     2942    -3     
Impacted Files Coverage Δ
src/lib/modes/aead/ccm/ccm.cpp 90.37% <100.00%> (ø)
...b/modes/aead/chacha20poly1305/chacha20poly1305.cpp 94.31% <100.00%> (ø)
src/lib/modes/aead/eax/eax.cpp 97.72% <100.00%> (ø)
src/lib/modes/aead/gcm/gcm.cpp 92.77% <100.00%> (ø)
src/lib/modes/aead/ocb/ocb.cpp 98.38% <100.00%> (ø)
src/lib/modes/aead/siv/siv.cpp 93.13% <100.00%> (ø)
src/lib/modes/cbc/cbc.cpp 92.10% <100.00%> (ø)
src/lib/modes/cfb/cfb.cpp 95.49% <100.00%> (ø)
src/lib/modes/xts/xts.cpp 98.36% <100.00%> (ø)
src/lib/tls/tls12/tls_cbc/tls_cbc.cpp 83.41% <100.00%> (ø)

... and 9 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@randombit
Copy link
Owner

I'm open for better approaches.

TBH I'm pretty unhappy with the whole cipher API as it exists. For context, originally it was designed to handle arbitrary transformations with minimal copies. (Until #475 cipher modes and compression shared a base class.) But as a cipher-specific interface it leaves a bit to be desired, and is not so easy to use, especially in incremental mode.

I think a new cipher API is happening at some point, and we can try to apply some lessons learned.

@reneme
Copy link
Collaborator Author

reneme commented Mar 20, 2023

I think a new cipher API is happening at some point, and we can try to apply some lessons learned.

I read that you don't foresee this for Botan 3.0, right?

With today's PRs I'm trying to reach a somewhat consistent API re: the adoption of std::span and concepts::resizable_byte_buffer.

@randombit
Copy link
Owner

I read that you don't foresee this for Botan 3.0, right?

No, not at this stage of things certainly. I don't even know what exactly a new API would look like at this point.

With today's PRs I'm trying to reach a somewhat consistent API re: the adoption of std::span and concepts::resizable_byte_buffer.

👍 absolutely, this PR series is a good improvement over status quo

@reneme reneme merged commit 9f63da9 into master Mar 21, 2023
@randombit randombit deleted the span/cipher_modes branch March 21, 2023 22:21
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