doc: remove _optional_ designation for tls options #22545
Conversation
Options are, by definition, optional. Remove specification of some options as "optional".
nodejs-github-bot
added
doc
|
Node.js Collaborators, please, add 👍 here if you approve fast-tracking. |
vsemozhetbyt
added
the
author ready
| provide the client certificate. | ||
| * `crl` {string|string[]|Buffer|Buffer[]} Optional PEM formatted | ||
| CRLs (Certificate Revocation Lists). | ||
| * `cert` {string|string[]|Buffer|Buffer[]} Cert chains in PEM format. One cert |
There was a problem hiding this comment.
I'm not sure this is a good idea. It's hard to tell with options can be omitted now. It seems they must all be specified now.
There was a problem hiding this comment.
If I'm not mistaken, none of them are required. (If there were required things, it should probably be named config rather than options.) I think that by specifying "Optional" for some options, we mislead the reader into thinking that some options are required.
We have it called with an empty options object twice here.
I do think the function signature needs to be updated to put options in [] to make it clear that it can be omitted, like it is here and here.
There was a problem hiding this comment.
I do think the function signature needs to be updated
I've now done this in a separate commit. @vsemozhetbyt PTAL to confirm you're still 👍 on this with that addition. Thanks!
There was a problem hiding this comment.
LGTM, but maybe let's get at least one LGTM from @nodejs/crypto to be on the safe side.
There was a problem hiding this comment.
LGTM, but maybe let's get at least one LGTM from @nodejs/crypto to be on the safe side.
I'll remove the fast-track label, at least until that happens. No need to rush this.
There was a problem hiding this comment.
@nodejs/crypto Any chance someone can take a quick look at this?
The `options` argument to `tls.createSecureContext()` is optional. Indicate this by using `[` and `]` in the function signature.
|
Probably overkill to do this but just in case folks there would want to look this over: /ping @nodejs/security-wg |
|
LGTM @Trott with only a small concern that as @lpinca pointed out it might be misleading with regards to what is optional or not. In the future someone may refactor this code and rename I'm suggesting maybe we can specify in a comment that all |
|
yep, didn't notice that and makes sense 👍 |
addaleax
removed
the
author ready
Options are, by definition, optional. Remove specification of some options as "optional". PR-URL: #22545 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: George Adams <george.adams@uk.ibm.com>
The `options` argument to `tls.createSecureContext()` is optional. Indicate this by using `[` and `]` in the function signature. PR-URL: #22545 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: George Adams <george.adams@uk.ibm.com>
Options are, by definition, optional. Remove specification of some options as "optional". PR-URL: #22545 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: George Adams <george.adams@uk.ibm.com>
The `options` argument to `tls.createSecureContext()` is optional. Indicate this by using `[` and `]` in the function signature. PR-URL: #22545 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: George Adams <george.adams@uk.ibm.com>
Options are, by definition, optional. Remove specification of some options as "optional". PR-URL: #22545 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: George Adams <george.adams@uk.ibm.com>
The `options` argument to `tls.createSecureContext()` is optional. Indicate this by using `[` and `]` in the function signature. PR-URL: #22545 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: George Adams <george.adams@uk.ibm.com>
Options are, by definition, optional. Remove specification of some options as "optional". PR-URL: #22545 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: George Adams <george.adams@uk.ibm.com>
The `options` argument to `tls.createSecureContext()` is optional. Indicate this by using `[` and `]` in the function signature. PR-URL: #22545 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: George Adams <george.adams@uk.ibm.com>
Options are, by definition, optional. Remove specification of some
options as "optional".
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes