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

Remove pad plaintext from hpke #525

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

benalleng
Copy link
Contributor

We had a function that checked and then padded the length of the plaintext when running message encryption. Instead we should use a fixed array length and return an error if the message is of the wrong length.

@benalleng benalleng marked this pull request as draft February 4, 2025 19:19
Comment on lines 159 to 161
// TODO: Try to return the error as a proper HpkeError::PayloadTooLarge
let body = encrypt_message_a(
body,
body.try_into().expect("Payload amount incorrect"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I failed to correctly apply a map_err() from a HpkeError to a CreateRequestError. But I'm not quite sure on how to accomplish this at the moment, I need to take a closer look at Error conversions

Copy link
Contributor

Choose a reason for hiding this comment

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

I think keeping the pad_plaintext around for deduplicate error handling is justifiable, the comments on the original may have been overly ambitious. Perhaps start with the first comment and then take a stab at removing pad_plaintext altogether. @nothingmuch can you remember why we said the pad_plaintext function should not exist at all?

ref:

https://github.com/payjoin/rust-payjoin/pull/405/files#diff-2236ea3589c5398fd90a3ac7e8a077689c3b66adf9fdc3738b04ef41580b1c88R247-R249

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have this currently, but ultimately It still feels like I moved the logic up to then simply duplicate the error handling.

I'm happy to scrap my current approach here and just go with using an array ref internally but I am perhaps also a little confused as you said about how the plan was to remove the pad plaintext as the plaintext is not really ever expected to be initially in the PADDED_PLAINTEXT_#_LENGTH as I first thought possible here to then just pass it down as an array but ultimately it does still need to be resized in my approach.

https://github.com/benalleng/rust-payjoin/blob/f00820bdb2735acf4305ac7259facd24cba1756a/payjoin/src/send/v2/mod.rs#L160-L169

@benalleng benalleng force-pushed the remove-pad-plaintext branch from 717a1a9 to 18e6985 Compare February 5, 2025 14:32
@coveralls
Copy link
Collaborator

coveralls commented Feb 5, 2025

Pull Request Test Coverage Report for Build 13160517763

Details

  • 25 of 32 (78.13%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 78.858%

Changes Missing Coverage Covered Lines Changed/Added Lines %
payjoin/src/receive/v2/mod.rs 7 10 70.0%
payjoin/src/send/v2/mod.rs 6 10 60.0%
Totals Coverage Status
Change from base Build 13150108794: -0.2%
Covered Lines: 3741
Relevant Lines: 4744

💛 - Coveralls

@benalleng benalleng force-pushed the remove-pad-plaintext branch from 18e6985 to 81a3c3c Compare February 5, 2025 15:07
We had a function to check the length and pad the plaintext
in our hpke encryption functions.
Instead we should just ensure that the text is passed as an
array with the correct length.
The plaintext passed to hpke message encrpytion needs to be the correct
A or B length respectively.
@benalleng benalleng force-pushed the remove-pad-plaintext branch from 81a3c3c to f00820b Compare February 5, 2025 15:14
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