-
Notifications
You must be signed in to change notification settings - Fork 99
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
Add authenticated encryption. #127
base: main
Are you sure you want to change the base?
Conversation
5fb3ea2
to
37d9893
Compare
9aba397
to
c2a413a
Compare
c2a413a
to
025e8c8
Compare
025e8c8
to
f1f8e38
Compare
|
||
if *tag != computed_tag { | ||
return Err(Error::TagMismatch); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If plaintext length does not not divide arity
we should check that other elements are 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In essence there is implicit padding applied during encryption (by not adding anything when calling duplex with smaller number of inputs), we need to verify that padding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we do need to verify it explicitly. It is implicitly checked by the final authentication tag. If you decrypt with the 'wrong' padding values, then you won't get the correct tag, and decryption will fail.
Do you mean something different? Can you give an example where what I described fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the nonce is part of a key in the implementation, not verifying the padding allows the same plaintext and key-nonce combination to encrypt into different ciphertext+tag. Which can be an issue for some applications.
Other than that I can't think off top of my head more significant negative for not verifying the padding in this case but it is recommended in both source documents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we resolved this in our conversation elsewhere, but please let me know if it still seems problematic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I will probably switch to always using explicit padding treated uniformly, for a different reason though. That will allow for a more uniform circuit — which will make proofs simpler in most cases.
Thanks for review, @Kubuxu. I decided to add variable-length messages, and will probably also extend to include multiple messages and headers in the same stream. So another commit with those changes (and a fix for the problem you noted) will be coming. Your having reviewed this one first will make following those changes simpler though. |
No problem. I will review updates as well. |
c5c0628
to
7e3a9c4
Compare
@Kubuxu This still needs more work, but you can see generally where I'm going with it if you're interested. If you want to wait, I will also ping you when it's ready. |
624354c
to
1b17425
Compare
1b17425
to
f6e31ca
Compare
@porcuquine I think we should add nonce there, I know that nonce can be considered "part of the key" here because it is no different but for ease of use we should add it explicitly. |
As described in LNCS 7118 - Duplexing the Sponge: Single-Pass Authenticated Encryption and Other Applications and recommended in the Poseidon paper.
Details:
NOTE: This needs more attention and should not be merged. I don't yet have confidence in its overall correctness yet. I do want eyes on the DST construction, which is plausibly correct and will stay as it is unless a problem is found.
UPDATE: I somehow managed not to find this document (https://drive.google.com/file/d/1EVrP3DzoGbmzkRmYnyEDcIQcXVU7GlOd/edit) initially and instead chased pointers from the main Poseidon paper to https://link.springer.com/content/pdf/10.1007%2F978-3-642-28496-0_19.pdf . I may change this PR (potentially including the DST also) once I've had a chance to look at it in more detail.
It's not all bad: this made me think and understand what's going on better than I probably would have otherwise.
TODO:
support collection of intermediate authentication tags