-
Notifications
You must be signed in to change notification settings - Fork 30k
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 missing secureProtocol values, and some docs on protocol selection #24386
Conversation
doc/api/cli.md
Outdated
clients or servers. | ||
Enable TLSv1.0 and TLSv1.1 by default, in addition to TLSv1.2. This should only | ||
be used for compatibility with old TLS clients or servers. See | ||
[secureProtocol][] for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this?:
Enable TLSv1.0 and TLSv1.1 by default. Use only
for compatibility with old TLS clients or servers. See
[secureProtocol][] for more information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You removed the "in addition to TLSv1.2". Why? I guess one could argue that saying TLSv1.0 and 1.1 are enabled by default doesn't necessarily mean that ONLY TLS 1.0 and 1.1 are enabled by default, but I don't like to leave people wondering. Lets tell them exactly what will be enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to s/This should only be used/Use for/
.
af40ac7
to
818c1eb
Compare
This comment has been minimized.
This comment has been minimized.
818c1eb
to
25acf88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs LGTM.
Add the two TLS protocol method functions which were missing. They seem useful, and are already documented as being supported (indirectly, our docs just point to OpenSSL's docs).
Cross-reference the secureProtocol docs and the CLI docs for --tls-v1.0 and --tls-v1.1 and describe relationship. Make clear that --tls-v1.0 enables TLSv1.0 and TLSv1.1.
25acf88
to
1032db1
Compare
@nodejs/crypto @nodejs/documentation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Landed in eb42c1e...4327326 |
Add the two TLS protocol method functions which were missing. They seem useful, and are already documented as being supported (indirectly, our docs just point to OpenSSL's docs). PR-URL: #24386 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>
Cross-reference the secureProtocol docs and the CLI docs for --tls-v1.0 and --tls-v1.1 and describe relationship. Make clear that --tls-v1.0 enables TLSv1.0 and TLSv1.1. PR-URL: #24386 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>
Should this be backported to |
Add the two TLS protocol method functions which were missing. They seem useful, and are already documented as being supported (indirectly, our docs just point to OpenSSL's docs). PR-URL: nodejs#24386 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>
Cross-reference the secureProtocol docs and the CLI docs for --tls-v1.0 and --tls-v1.1 and describe relationship. Make clear that --tls-v1.0 enables TLSv1.0 and TLSv1.1. PR-URL: nodejs#24386 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes