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

Be compatible to the updated botan's behaviour related to botan_cipher_update(). #2240

Merged
merged 8 commits into from
Jun 24, 2024

Conversation

ni4
Copy link
Contributor

@ni4 ni4 commented May 28, 2024

Looks like this commit changes behaviour to which we were stick for years:
randombit/botan@8336835

Fixes #2245

Copy link

codecov bot commented May 28, 2024

Codecov Report

Attention: Patch coverage is 92.06349% with 5 lines in your changes missing coverage. Please review.

Project coverage is 83.60%. Comparing base (a7ee088) to head (8eae86f).

Files Patch % Lines
src/librepgp/stream-parse.cpp 91.48% 4 Missing ⚠️
src/lib/crypto/symmetric.cpp 87.50% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #2240    +/-   ##
========================================
  Coverage   83.60%   83.60%            
========================================
  Files         106      107     +1     
  Lines       22964    23174   +210     
========================================
+ Hits        19198    19374   +176     
- Misses       3766     3800    +34     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ni4 ni4 force-pushed the ni4-fix-botan-aead-update branch from b85d93c to 0cb2663 Compare May 31, 2024 12:52
@ni4 ni4 force-pushed the ni4-fix-botan-aead-update branch 11 times, most recently from c1eeda6 to 5cbf852 Compare June 13, 2024 13:11
@ni4 ni4 mentioned this pull request Jun 17, 2024
@ni4 ni4 force-pushed the ni4-fix-botan-aead-update branch from 5cbf852 to 94ba422 Compare June 18, 2024 16:49
@ni4 ni4 force-pushed the ni4-fix-botan-aead-update branch from 94ba422 to 8eae86f Compare June 18, 2024 18:06
@ni4
Copy link
Contributor Author

ni4 commented Jun 18, 2024

@maxirmx Don't you mind that I picked here your commit, fixing macOS/Python issue?

@ni4 ni4 marked this pull request as ready for review June 18, 2024 19:28
@ni4 ni4 requested a review from desvxx June 19, 2024 08:10
Copy link
Member

@maxirmx maxirmx left a comment

Choose a reason for hiding this comment

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

LGTM

* @return true on success or false otherwise. On success exactly len processed bytes will be
* stored in out buffer
*/
bool pgp_cipher_aead_update(pgp_crypt_t *crypt, uint8_t *out, const uint8_t *in, size_t len);
bool pgp_cipher_aead_update(
pgp_crypt_t &crypt, uint8_t *out, const uint8_t *in, size_t len, size_t &read);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to change pointer to reference here? It will be weird from user perspective if other functions use pointer but this one not.

@@ -54,7 +54,7 @@
#define ST_FROM ("From")

/* Preallocated cache length for AEAD encryption/decryption */
#define PGP_AEAD_CACHE_LEN (PGP_INPUT_CACHE_SIZE + PGP_AEAD_MAX_TAG_LEN)
#define PGP_AEAD_CACHE_LEN (PGP_INPUT_CACHE_SIZE + 2 * PGP_AEAD_MAX_TAG_LEN)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why 2?

Copy link
Contributor

Choose a reason for hiding this comment

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

Found my answer in stream-write.cpp :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is to include the tag for the last block + the final tag after the encrypted stream.

@ni4
Copy link
Contributor Author

ni4 commented Jun 24, 2024

Two approvals so merhing this. Thanks all!

@ni4 ni4 merged commit 657b188 into main Jun 24, 2024
128 checks passed
@ni4 ni4 deleted the ni4-fix-botan-aead-update branch June 24, 2024 09:23
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.

Be compatible with update Botan 3.5 AEAD decryption behaviour.
3 participants