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

Update crypto.md to correct description of decipher.setAuthTag #33097

Closed
wants to merge 4 commits into from

Conversation

jbuhacoff
Copy link
Contributor

Calling decipher.setAuthTag after decipher.update will result in an error like Unsupported state or unable to authenticate data. The example code in CCM mode is correct, but to demonstrate the mistake in the documentation you can take the same example and move the setAuthTag call to in between update and final you will see the error.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Calling `decipher.setAuthTag` after `decipher.update` will result in an error like `Unsupported state or unable to authenticate data`. The example code in [CCM mode](https://nodejs.org/docs/latest-v14.x/api/crypto.html#crypto_ccm_mode) is correct, but to demonstrate the mistake in the documentation you can take the same example and move the `setAuthTag` call to in between `update` and `final` you will see the error.
@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels Apr 27, 2020
@mscdex
Copy link
Contributor

mscdex commented Apr 27, 2020

That's not quite correct since only CCM mode needs to have setAuthTag() before update(). For GCM and OCB modes you can have setAuthTag() any time before final() (in fact, we already have tests for this).

@jbuhacoff
Copy link
Contributor Author

Right, that should be documented.

I suggested changing it to mention update instead of final because I think that would work in all cases. If that's not desirable, we should at least mention that for CCM it actually needs to be called before decipher.update().

@mscdex
Copy link
Contributor

mscdex commented Apr 28, 2020

I think adding a note about the differences between the modes with regard to calling setAuthTag() would be best. For users of the GCM and OCB modes, it could be beneficial to call setAuthTag() at a later point in time, so it's best to continue to let them know they can do that.

Restore note about calling setAuthTag before decipher.final and add new note about calling it before decipher.update for CCM mode.
@jbuhacoff
Copy link
Contributor Author

Updated the pull request to address comments by @mscdex, please check it.

@@ -522,6 +522,8 @@ is invalid according to [NIST SP 800-38D][] or does not match the value of the

The `decipher.setAuthTag()` method must be called before
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just modify this sentence instead to clarify, something like:

The `decipher.setAuthTag()` method must be called before
[`decipher.update()`][] for `CCM` mode or before [`decipher.final()`][] for
`GCM` and `OCB` modes. `decipher.setAuthTag()` can only be called once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, thanks! I just pushed a correction with this advice.

Added detail to note about calling setAuthTag before decipher.final to mention that for CCM mode it must be called before decipher.update.
Added detail to descripption of `decipher.setAuthTag()` method that it must be called before `decipher.update()` for `CCM` mode.
@mscdex
Copy link
Contributor

mscdex commented Apr 29, 2020

/cc @nodejs/crypto to make sure this is the intended behavior

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Thanks, that's correct. GCM and OCB are online AEADs, CCM is not.

@addaleax
Copy link
Member

addaleax commented May 7, 2020

Landed in d135b50, thanks for the PR! 🎉

@addaleax addaleax closed this May 7, 2020
@jbuhacoff jbuhacoff deleted the patch-2 branch June 26, 2020 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants