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

doc: fix crypto update() signatures #5500

Merged
merged 1 commit into from
Mar 18, 2016

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Mar 1, 2016

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

  • doc
  • crypto

Description of change

Fixes: #5499

@mscdex mscdex added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels Mar 1, 2016
@mscdex mscdex mentioned this pull request Mar 1, 2016
@shigeki
Copy link
Contributor

shigeki commented Mar 1, 2016

@mscdex Does Sign/Verify.update() also need to add the same descriptions?

@mscdex mscdex force-pushed the doc-crypto-fix-hmac-update-signature branch 2 times, most recently from 8de1720 to 92a60fd Compare March 1, 2016 02:45
@mscdex
Copy link
Contributor Author

mscdex commented Mar 1, 2016

@shigeki Fixed.

@mscdex mscdex changed the title doc: fix crypto hash.update() signature doc: fix crypto update() signatures Mar 1, 2016
@shigeki
Copy link
Contributor

shigeki commented Mar 1, 2016

Thanks. LGTM.

@kirked
Copy link

kirked commented Mar 1, 2016

As the originator of #5499, I have to say that the most misleading part of the doc is near the top of the page where it states

The crypto module provides cryptographic functionality that includes a set of wrappers for OpenSSL's hash, HMAC, cipher, decipher, sign and verify functions.

Once I saw that it should be OpenSSL compatible, it was easy to prove that it isn't.

I don't know of a way to arrive at the same digest that node does using OpenSSL when multibyte characters are in the digest stream (which is probably just ignorance on my part). My real problem now is that other systems (e.g., JDK) digest the same as OpenSSL (there is no such thing as binary encoding), so node seems to be the outlier.

@mscdex mscdex force-pushed the doc-crypto-fix-hmac-update-signature branch from 92a60fd to af09a9c Compare March 18, 2016 02:31
@mscdex mscdex merged commit af09a9c into nodejs:master Mar 18, 2016
@mscdex mscdex deleted the doc-crypto-fix-hmac-update-signature branch March 18, 2016 02:38
Fishrock123 pushed a commit that referenced this pull request Mar 22, 2016
@MylesBorins
Copy link
Contributor

@mscdex should this go to lts?

@mscdex
Copy link
Contributor Author

mscdex commented Mar 23, 2016

Probably, I think the API has been this way at least since v0.10 IIRC.

@jasnell
Copy link
Member

jasnell commented Mar 23, 2016

+1

MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
PR-URL: #5500
Reviewed-By: Shigeki Ohtsu <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
PR-URL: #5500
Reviewed-By: Shigeki Ohtsu <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Jul 19, 2016
The default encoding for crypto methods was changed in v6.0.0,
with v4.x keeping a default of `binary`.

Ref: nodejs#5500
Fixes: nodejs#7712
jasnell pushed a commit that referenced this pull request Aug 1, 2016
The default encoding for crypto methods was changed in v6.0.0,
with v4.x keeping a default of `binary`.

Ref: #5500
Fixes: #7712
PR-URL: #7799
Reviewed-By: Claudio Rodriguez <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@JacksonTian
Copy link
Contributor

This should not be landed in LTS v4.

lmoe pushed a commit to lmoe/node that referenced this pull request Feb 16, 2017
PullRequest nodejs#5522 and nodejs#5500 described the change
of the default encoding into UTF8 in crypto functions.

This however was only changed for the non-streaming API.
The streaming API still used binary as the default encoding.

This commit will change the default streaming API encoding to UTF8
to make both APIs behave the same.

It will also add tests to validate the behavior.
addaleax pushed a commit that referenced this pull request Mar 11, 2017
PullRequest #5522 and #5500 described the change
of the default encoding into UTF8 in crypto functions.

This however was only changed for the non-streaming API.
The streaming API still used binary as the default encoding.

This commit will change the default streaming API encoding to UTF8
to make both APIs behave the same.

It will also add tests to validate the behavior.

Refs: #5522
Refs: #5500
PR-URL: #8611
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
PullRequest nodejs#5522 and nodejs#5500 described the change
of the default encoding into UTF8 in crypto functions.

This however was only changed for the non-streaming API.
The streaming API still used binary as the default encoding.

This commit will change the default streaming API encoding to UTF8
to make both APIs behave the same.

It will also add tests to validate the behavior.

Refs: nodejs#5522
Refs: nodejs#5500
PR-URL: nodejs#8611
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
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.

Incorrect HMAC result!
6 participants