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

Add aead_encrypt and aead_decrypt convenience functions #155

Merged
merged 6 commits into from
Aug 19, 2022

Conversation

thibault-martinez
Copy link
Member

Change checklist

Add an x to the boxes that are relevant to your changes, and delete any items that are not.

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Contributor

@Wollac Wollac left a comment

Choose a reason for hiding this comment

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

Two remarks:

  • While I am not aware of any particular specification and there are cases with a different order, I would say the "usual" order is nonce || ciphertext || tag. So should we also maybe use that order here?
  • I absolutely agree, we should add such a util function. Especially since associated_data and tag is hardly ever needed explicitly. However, I see this a bit differently for the nonce. There are common scenarios where the nonce is not chosen randomly, e.g. all-zero for ephemeral keys or derived from a hash. Good arguments can be made, why the nonce should still be explicit (like e.g. in crypto_secretbox_easy and libsodium is usually a great example for API design).
    However, nonce selection is security critical. So, when this better fits the use-case your are having in mind, let's leave it like this and hide the nonce.

@thibault-martinez
Copy link
Member Author

thibault-martinez commented Aug 18, 2022

  • While I am not aware of any particular specification and there are cases with a different order, I would say the "usual" order is nonce || ciphertext || tag. So should we also maybe use that order here?

I kind of assumed that was the usual order as it has been chosen this way for the stronghold integration in the wallet. We obviously can't change it now. I would understand if choosing an arbitrary order was a no-go for an inclusion in crypto.rs

However, nonce selection is security critical. So, when this better fits the use-case your are having in mind, let's leave it like this and hide the nonce.

I am not fully certain what your conclusion is. Do you want me to make the nonce a parameter?

@Wollac
Copy link
Contributor

Wollac commented Aug 19, 2022

If that order is already in use let's stick with it 👍
Maybe you could add a comment describing the order and that the nonce is initialized randomly.

@thibault-martinez
Copy link
Member Author

Maybe you could add a comment describing the order and that the nonce is initialized randomly.

Done, let me know if this is fine.

@thibault-martinez thibault-martinez merged commit 1910a54 into iotaledger:dev Aug 19, 2022
@thibault-martinez thibault-martinez deleted the aead-helpers branch August 19, 2022 15:45
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.

2 participants