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.

Sign up for GitHub

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

Jump to bottom

doc: add tls.DEFAULT_ECDH_CURVE #10264

Closed
wants to merge 2 commits into from
Closed

doc: add tls.DEFAULT_ECDH_CURVE #10264

wants to merge 2 commits into from

Conversation

sam-github
Copy link
Contributor

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

A user can change the default curve for ECDH key agreement by
using tls.DEFAULT_ECDH_CURVE.

From #1495 (comment), forward-port 02a51cf to master.

/to @shigeki

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tls Issues and PRs related to the tls subsystem. dont-land-on-v7.x labels Dec 14, 2016
italoacasas
italoacasas reviewed Dec 16, 2016
View reviewed changes
doc/api/tls.md
The default curve name to use for ECDH key agreement. The default value is
`'prime256v1'` (NIST P-256). Consult [RFC 4492] for more details.


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra space

bnoordhuis
bnoordhuis approved these changes Dec 16, 2016
View reviewed changes
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.

LGTM if you drop the extra blank line.

shigeki
shigeki suggested changes Dec 16, 2016
View reviewed changes
doc/api/tls.md
@@ -1076,6 +1076,12 @@ For example:
console.log(tls.getCiphers()); // ['AES128-SHA', 'AES256-SHA', ...]
```

## tls.DEFAULT_ECDH_CURVE

The default curve name to use for ECDH key agreement. The default value is
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only effective on the key agreement on a tls server. I think that for ECDH key agreement in a tls server. is better.

doc/api/tls.md
## tls.DEFAULT_ECDH_CURVE

The default curve name to use for ECDH key agreement. The default value is
`'prime256v1'` (NIST P-256). Consult [RFC 4492] for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

RFC4492 seems to be old but the current RFC4492bis is under LastCall and not finished yet. The reference of prime256v1/NIST P-256 in RF4492 is outdated so I think it is better also to add the latest FIPS reference of FIPS.186-4 for NIST P-256. The reference link is also missed.

--- a/doc/api/tls.md
+++ b/doc/api/tls.md
@@ -1078,9 +1078,9 @@ console.log(tls.getCiphers()); // ['AES128-SHA', 'AES256-SHA', ...]

 ## tls.DEFAULT_ECDH_CURVE

-The default curve name to use for ECDH key agreement. The default value is
-`'prime256v1'` (NIST P-256). Consult [RFC 4492] for more details.
-
+The default curve name to use for ECDH key agreement in a tls
+server. The default value is `'prime256v1'` (NIST P-256). Consult [RFC
+4492] and [FIPS.186-4] for more details.

 ## Deprecated APIs

@@ -1219,3 +1219,5 @@ where `secure_socket` has the same API as `pair.cleartext`.
 [`tls.TLSSocket.getPeerCertificate()`]: #tls_tlssocket_getpeercertificate_detailed
 [`tls.createSecureContext()`]: #tls_tls_createsecurecontext_options
 [`tls.connect()`]: #tls_tls_connect_options_callback
+[RFC 4492]: https://www.rfc-editor.org/rfc/rfc4492.txt
+[FIPS.186-4]: http://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-4.pdf

Shigeki Ohtsu and others added 2 commits December 21, 2016 09:55
@sam-github
doc: add tls.DEFAULT_ECDH_CURVE
8517058 
A user can change the default curve for ECDH key agreement by
using tls.DEFAULT_ECDH_CURVE.
@sam-github
fixup! doc: add tls.DEFAULT_ECDH_CURVE
3b6f83a
@sam-github
Copy link
Contributor Author

@shigeki PTAL, I used your text verbatim, thanks.

shigeki
shigeki approved these changes Dec 26, 2016
View reviewed changes
Copy link
Contributor

@shigeki shigeki left a comment

Choose a reason for hiding this comment

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

LGTM.
@sam-github Please rebase the commits in your name not mine.

jasnell pushed a commit that referenced this pull request Dec 27, 2016
@sam-github @jasnell
doc: add tls.DEFAULT_ECDH_CURVE
97ab4b2 
A user can change the default curve for ECDH key agreement by
using tls.DEFAULT_ECDH_CURVE.

PR-URL: #10264
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
@jasnell
Copy link
Member

jasnell commented Dec 27, 2016

Landed in 97ab4b2

@jasnell jasnell closed this Dec 27, 2016
@sam-github
Copy link
Contributor Author

Thanks @jasnell and thanks for rewriting author.

@sam-github sam-github deleted the doc-default-ecdh-curve branch December 29, 2016 19:53
@evanlucas evanlucas mentioned this pull request Jan 3, 2017
Merged
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
@sam-github @evanlucas
doc: add tls.DEFAULT_ECDH_CURVE
dc0323c 
A user can change the default curve for ECDH key agreement by
using tls.DEFAULT_ECDH_CURVE.

PR-URL: #10264
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
@sam-github @evanlucas
doc: add tls.DEFAULT_ECDH_CURVE
5b7b457 
A user can change the default curve for ECDH key agreement by
using tls.DEFAULT_ECDH_CURVE.

PR-URL: #10264
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
@MylesBorins
Copy link
Contributor

@sam-github does this apply to the v4 and v6 implementation? If so feel free to backport

sam-github added a commit to sam-github/node that referenced this pull request Jan 24, 2017
@sam-github
doc: add tls.DEFAULT_ECDH_CURVE
148b713 
A user can change the default curve for ECDH key agreement by
using tls.DEFAULT_ECDH_CURVE.

PR-URL: nodejs#10264
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
@sam-github sam-github mentioned this pull request Jan 24, 2017
Closed
4 tasks
@sam-github
Copy link
Contributor Author

It does apply, and I just PRed #10984, but I have doubts.

The only reason it didn't land is it builds on #9800, which is marked
as don't land :-(. The docs apply, I'll try to figure out tomorrow why they were marked like that.

@sam-github
Copy link
Contributor Author

@MylesBorins this lands clean on v6.x, but isn't in v6.x-staging yet, is there some problem with it?

@sam-github
Copy link
Contributor Author

Its too much energy to backport docs to 4.x. Lands clean on 6.x.

MylesBorins pushed a commit that referenced this pull request Mar 7, 2017
@sam-github @MylesBorins
doc: add tls.DEFAULT_ECDH_CURVE
972a7c5 
A user can change the default curve for ECDH key agreement by
using tls.DEFAULT_ECDH_CURVE.

PR-URL: #10264
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@sam-github @MylesBorins
doc: add tls.DEFAULT_ECDH_CURVE
998fd1e 
A user can change the default curve for ECDH key agreement by
using tls.DEFAULT_ECDH_CURVE.

PR-URL: #10264
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@italoacasas italoacasas italoacasas approved these changes

@shigeki shigeki shigeki approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@silverwind silverwind silverwind approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@sam-github @jasnell @MylesBorins @silverwind @bnoordhuis @shigeki @italoacasas @nodejs-github-bot