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: createServer's key option can be an array #3123

Closed
wants to merge 3 commits into from

Conversation

thefourtheye
Copy link
Contributor

The tls module's createServer and createSecureContext accept
key option and it can be an array of keys as well. This patch
explains the format of the entries in that array.

Corresponding code:
https://github.com/nodejs/node/blob/v4.1.1/lib/_tls_common.js#L73-L90

cc @nodejs/crypto

The `tls` module's `createServer` and `createSecureContext` accept
`key` option and it can be an array of keys as well. This patch
explains the format of the entries in that array.

Corresponding code:
https://github.com/nodejs/node/blob/v4.1.1/lib/_tls_common.js#L73-L90
@thefourtheye thefourtheye added tls Issues and PRs related to the tls subsystem. doc Issues and PRs related to the documentations. labels Sep 30, 2015
@indutny
Copy link
Member

indutny commented Sep 30, 2015

@thefourtheye perhaps, it may be relevant to mention that the keys should use different algorithms? RSA, ECDSA, DSA?

@thefourtheye
Copy link
Contributor Author

@indutny But we don't validate if the keys use different algorithms, right? https://github.com/nodejs/node/blob/v4.1.1/src/node_crypto.cc#L457-L500

@indutny
Copy link
Member

indutny commented Sep 30, 2015

@thefourtheye hm... I'm sure we don't, but OpenSSL may.

@thefourtheye
Copy link
Contributor Author

@indutny Oh okay then. I included a line to say that the keys should use different algorithms. Should we explicitly give examples of algorithms?

@silverwind
Copy link
Contributor

When would one want to use multiple keys?

@indutny
Copy link
Member

indutny commented Sep 30, 2015

When you have two certs: ECDSA and RSA. Like I do on https://blog.indutny.com/

PEM format. It can also be an array of keys. The array can either be of
just keys or if you have different passphrases for the keys, then the
array elements can be of the form `{pem: key, passphrase: passphrase}` and
the keys should use different algorithms. (Required)
Copy link
Contributor

Choose a reason for hiding this comment

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

What kind of algorithms is and the keys should use different algorithms referring to?

Copy link
Contributor

Choose a reason for hiding this comment

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

ping @thefourtheye: could you clarify? Otherwise LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@silverwind The examples are ECDSA and RSA. Should we really mention them in the docs?

Copy link
Contributor

Choose a reason for hiding this comment

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

My gut thought is to remove that and the keys should use different algorithms altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@silverwind Hmmm, it was @indutny's suggestion. Let's see what he feels about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. I think your wording is a bit confusing. How about something like this?

`key`: A string or `Buffer` containing the private key of the server in
PEM format. To support multiple keys using different algorithms, an array
can be provided. It can either be a plain array of keys, or an array of
objects in the form of {pem: key, passphrase: passphrase}. (Required)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@silverwind Ya, it looks better. I updated the PR now. PTAL.

@thefourtheye
Copy link
Contributor Author

Bump!

@silverwind
Copy link
Contributor

LGTM

@thefourtheye
Copy link
Contributor Author

@indutny LGTY?

@indutny
Copy link
Member

indutny commented Oct 28, 2015

LGTM

thefourtheye added a commit that referenced this pull request Oct 28, 2015
The `tls` module's `createServer` and `createSecureContext` accept
`key` option and it can be an array of keys as well. This patch
explains the format of the entries in that array.

Corresponding code:
https://github.com/nodejs/node/blob/v4.1.1/lib/_tls_common.js#L73-L90

PR-URL: #3123
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
@thefourtheye
Copy link
Contributor Author

Thanks for the review :-) Landed at 5d5a4c4.

@silverwind I tweaked the text a little bit. Instead of in the form of, I used in the format. Hope that is okay.

@thefourtheye thefourtheye deleted the improve-tls-keys-doc branch October 28, 2015 02:18
rvagg pushed a commit to rvagg/io.js that referenced this pull request Oct 29, 2015
The `tls` module's `createServer` and `createSecureContext` accept
`key` option and it can be an array of keys as well. This patch
explains the format of the entries in that array.

Corresponding code:
https://github.com/nodejs/node/blob/v4.1.1/lib/_tls_common.js#L73-L90

PR-URL: nodejs#3123
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
thefourtheye added a commit that referenced this pull request Oct 30, 2015
The `tls` module's `createServer` and `createSecureContext` accept
`key` option and it can be an array of keys as well. This patch
explains the format of the entries in that array.

Corresponding code:
https://github.com/nodejs/node/blob/v4.1.1/lib/_tls_common.js#L73-L90

PR-URL: #3123
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
@jasnell
Copy link
Member

jasnell commented Oct 30, 2015

Landed in v4.x-staging in db8e2f1

thefourtheye added a commit that referenced this pull request Oct 30, 2015
The `tls` module's `createServer` and `createSecureContext` accept
`key` option and it can be an array of keys as well. This patch
explains the format of the entries in that array.

Corresponding code:
https://github.com/nodejs/node/blob/v4.1.1/lib/_tls_common.js#L73-L90

PR-URL: #3123
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants