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

crypto: add support for chacha20-poly1305 for AEAD #24081

Merged
merged 1 commit into from
Nov 7, 2018

Conversation

chux0519
Copy link
Contributor

@chux0519 chux0519 commented Nov 4, 2018

Openssl support AEAD_CHACHA20_POLY1305(rfc7539) since 1.1.

Fixes: #24080
Refs: https://tools.ietf.org/html/rfc7539

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Nov 4, 2018
@targos
Copy link
Member

targos commented Nov 4, 2018

@nodejs/crypto

doc/api/crypto.md Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
@refack
Copy link
Contributor

refack commented Nov 5, 2018

Hello @chux0519 welcome, and thank you for your contribution 🥇
If you are not familiar with our review and landing process, it's covered in CONTRIBUTING.md

P.S. If you have any question you can also feel free to contact me directly.

@chux0519
Copy link
Contributor Author

chux0519 commented Nov 5, 2018

Hello @chux0519 welcome, and thank you for your contribution 🥇
If you are not familiar with our review and landing process, it's covered in CONTRIBUTING.md

P.S. If you have any question you can also feel free to contact me directly.

thx, I will change my code to follow that guide

@chux0519
Copy link
Contributor Author

chux0519 commented Nov 5, 2018

It seems make lint-md can not check the 80 characters limit.

@ryzokuken
Copy link
Contributor

@chux0519 lint-md lints the markdown in the codebase. It is geared towards doc-only contributions.

@chux0519
Copy link
Contributor Author

chux0519 commented Nov 5, 2018

@chux0519 lint-md lints the markdown in the codebase. It is geared towards doc-only contributions.

thx, i see

@Trott
Copy link
Member

Trott commented Nov 5, 2018

It seems make lint-md can not check the 80 characters limit.

@chux0519 It does not currently check that line lengths are 80 characters or less, although I've just opened a PR to implement that. #24094

@chux0519
Copy link
Contributor Author

chux0519 commented Nov 5, 2018

#24094

It would be great to have that, And will avoid many comments in reviews causing by doc lint like this one above.

doc/api/crypto.md Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
@refack
Copy link
Contributor

refack commented Nov 6, 2018

After rebasing to current master, Travis should not fail anymore for lint in commit message - #23739

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@@ -2855,8 +2855,7 @@ bool CipherBase::CheckCCMMessageLength(int message_len) {
bool CipherBase::IsAuthenticatedMode() const {
// Check if this cipher operates in an AEAD mode that we support.
CHECK(ctx_);
const int mode = EVP_CIPHER_CTX_mode(ctx_.get());
return IsSupportedAuthenticatedMode(mode);
return IsSupportedAuthenticatedMode(EVP_CIPHER_CTX_cipher(ctx_.get()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Would overloading IsSupportedAuthenticatedMode() to also accept the EVP_CIPHER_CTX type make sense? It would shorten these 3 repeated calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you, It's a good idea, I will add one more IsSupportedAuthenticatedMode to take the EVP_CIPHER_CTX as param

return mode == EVP_CIPH_CCM_MODE ||
static bool IsSupportedAuthenticatedMode(const EVP_CIPHER* cipher) {
const int mode = EVP_CIPHER_mode(cipher);
return EVP_CIPHER_nid(cipher) == NID_chacha20_poly1305 ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a comment here to state that chacha20-poly1305 is an AEAD cipher, but that its mode of 0 doesn't indicate that?

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 will add a comment to indicate it

@sam-github
Copy link
Contributor

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, LGTM with a request and a suggestion.

auth_tag_len_ = auth_tag_len;
}
} else {
// CCM / OCB / AEAD-chacha20-poly1305
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a CHECK here that verifies the cipher is one of these?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bnoordhuis maybe the comment can just be removed? The function is protected by a CHECK(IsAuthenticatedMode()); already, so we know this is an AEAD cipher, and what's happening here is that GCM has some special requirements on tag length that aren't shared by any other AEAD ciphers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for comment, will remove it

if (kind_ == kDecipher && IsSupportedAuthenticatedMode(mode)) {
MaybePassAuthTagToOpenSSL();
if (
kind_ == kDecipher &&
Copy link
Member

Choose a reason for hiding this comment

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

Style: should go on the previous line; i.e., no line break after the paren. The next line should have 4 spaces of indent.

If it gets too unwieldy / doesn't fit in 80 columns, assign the cipher to a variable first:

const EVP_CIPHER* cipher = EVP_CIPHER_CTX_cipher(ctx_.get());
if (kind_ == kDecipher && IsSupportedAuthenticatedMode(cipher)) {
  // ...
}

(Arguably a good idea in any case; easier to read, IMO.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I will fix this

@refack
Copy link
Contributor

refack commented Nov 7, 2018

Resume: https://ci.nodejs.org/job/node-test-commit/22946/

P.S. I'm self-assigned this so I'll get notifications from Github, and so that I will not lose track of it and help steward it to completion.

@refack refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 7, 2018
@refack refack self-assigned this Nov 7, 2018
@refack refack removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 7, 2018
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

LGTM, but it would be best to explicitely mention this change in the changes section of createCipheriv and createDecipheriv (in doc/api/crypto.md) like this:

  - version: REPLACEME
    pr-url: https://github.com/nodejs/node/pull/24081
    description: The cipher `chacha20-poly1305` is now supported.

Could you add this to the top of the respective changes: metadata sections in doc/api/crypto.md? (Probably right above OCB support.)

@chux0519
Copy link
Contributor Author

chux0519 commented Nov 7, 2018

@tniessen Of course, I have added them

@@ -1382,6 +1382,9 @@ Adversaries][] for details.
<!-- YAML
added: v0.1.94
changes:
- version: v12.0.0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- version: v12.0.0
- version: REPLACEME

@@ -1468,6 +1471,9 @@ to create the `Decipher` object.
<!-- YAML
added: v0.1.94
changes:
- version: v12.0.0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- version: v12.0.0
- version: REPLACEME

Copy link
Contributor Author

Choose a reason for hiding this comment

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

v10.14.0 right ? Sorry, I just used the ./out/Release/node -v

Copy link
Member

Choose a reason for hiding this comment

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

We do not know yet in which actual release this will land, so instead we write REPLACEME and the value is changed when the release that includes this change is done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, I misunderstood the REPLACEMENT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for being so patient.

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 you are the one being patient! Thanks.

@targos targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 7, 2018
@targos
Copy link
Member

targos commented Nov 7, 2018

I assume this is semver-minor?

@tniessen
Copy link
Member

tniessen commented Nov 7, 2018

@targos I don't think we have been consistent about that in the past, I'd be okay with marking it semver-minor.

@tniessen
Copy link
Member

tniessen commented Nov 7, 2018

openSSL supports AEAD_CHACHA20_POLY1305(rfc7539) since 1.1.

PR-URL: nodejs#24081
Fixes: nodejs#24080
Refs: https://tools.ietf.org/html/rfc7539
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@refack
Copy link
Contributor

refack commented Nov 7, 2018

Landed in 5c59622
Congratulations @chux0519 for GitHub promoting you from
image
to
image

BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
openSSL supports AEAD_CHACHA20_POLY1305(rfc7539) since 1.1.

PR-URL: #24081
Fixes: #24080
Refs: https://tools.ietf.org/html/rfc7539
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
openSSL supports AEAD_CHACHA20_POLY1305(rfc7539) since 1.1.

PR-URL: nodejs#24081
Fixes: nodejs#24080
Refs: https://tools.ietf.org/html/rfc7539
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@rvagg
Copy link
Member

rvagg commented Nov 18, 2018

Hey @chux0519, thanks for contributing this! I wasn't even aware we didn't support it but it'll help us get closer to solid TLS1.3 support (eventually). You'll note that it's in 11.2.0 now btw.

@chux0519
Copy link
Contributor Author

Hey @chux0519, thanks for contributing this! I wasn't even aware we didn't support it but it'll help us get closer to solid TLS1.3 support (eventually). You'll note that it's in 11.2.0 now btw.

I'm happy to make this little contrubution to nodejs, thank your all guys' help during the PR process

@refack refack removed their assignment Mar 11, 2019
@imcotton imcotton mentioned this pull request May 3, 2019
@tniessen tniessen mentioned this pull request Oct 21, 2019
BethGriggs pushed a commit that referenced this pull request Oct 21, 2019
openSSL supports AEAD_CHACHA20_POLY1305(rfc7539) since 1.1.

PR-URL: #24081
Fixes: #24080
Refs: https://tools.ietf.org/html/rfc7539
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
BethGriggs added a commit that referenced this pull request Oct 22, 2019
Notable changes:

* crypto:
  * add support for chacha20-poly1305 for AEAD (chux0519)
    #24081
  * increase maxmem range from 32 to 53 bits (Tobias Nießen)
    #28799
* deps:
  * update npm to 6.11.3 (claudiahdz)
    #29430
  * upgrade openssl sources to 1.1.1d (Sam Roberts)
    #29921
* dns:
  * remove dns.promises experimental warning (cjihrig)
    #26592
* fs:
  * remove experimental warning for fs.promises (Anna Henningsen)
    #26581
* http:
  * makes response.writeHead return the response (Mark S. Everitt)
    #25974
* http2:
  * makes response.writeHead return the response (Mark S. Everitt)
    #25974
* n-api:
  * make func argument of napi\_create\_threadsafe\_function optional
    (legendecas)
    #27791
  * mark version 5 N-APIs as stable (Gabriel Schulhof)
    #29401
  * implement date object (Jarrod Connolly)
    #25917
* process:
  * add --unhandled-rejections flag (Ruben Bridgewater)
    #26599
* stream:
  * implement Readable.from async iterator utility (Guy Bedford)
    #27660
  * make Symbol.asyncIterator support stable (Matteo Collina)
    #26989

PR-URL: #29875
BethGriggs added a commit that referenced this pull request Oct 22, 2019
Notable changes:

* crypto:
  * add support for chacha20-poly1305 for AEAD (chux0519)
    #24081
  * increase maxmem range from 32 to 53 bits (Tobias Nießen)
    #28799
* deps:
  * update npm to 6.11.3 (claudiahdz)
    #29430
  * upgrade openssl sources to 1.1.1d (Sam Roberts)
    #29921
* dns:
  * remove dns.promises experimental warning (cjihrig)
    #26592
* fs:
  * remove experimental warning for fs.promises (Anna Henningsen)
    #26581
* http:
  * makes response.writeHead return the response (Mark S. Everitt)
    #25974
* http2:
  * makes response.writeHead return the response (Mark S. Everitt)
    #25974
* n-api:
  * make func argument of napi\_create\_threadsafe\_function optional
    (legendecas)
    #27791
  * mark version 5 N-APIs as stable (Gabriel Schulhof)
    #29401
  * implement date object (Jarrod Connolly)
    #25917
* process:
  * add --unhandled-rejections flag (Ruben Bridgewater)
    #26599
* stream:
  * implement Readable.from async iterator utility (Guy Bedford)
    #27660
  * make Symbol.asyncIterator support stable (Matteo Collina)
    #26989

PR-URL: #29875
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AEAD_CHACHA20_POLY1305 support