FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: createServer's key option can be an array by thefourtheye · Pull Request #3123 · nodejs/node · GitHub

/ node Public

doc: createServer's key option can be an array - #3123

Closed
thefourtheye wants to merge 3 commits into
nodejs:masterfrom
thefourtheye:improve-tls-keys-doc
ConversationCommits (3)ChecksFiles changed
Closed

doc: createServer's key option can be an array#3123
thefourtheye wants to merge 3 commits into
nodejs:masterfrom
thefourtheye:improve-tls-keys-doc

Conversation

thefourtheye commented Sep 30, 2015

Copy link
Copy Markdown
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

doc: createServer's key option can be an array
2d058c4 
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 added tls Issues and PRs related to the tls subsystem. doc Issues and PRs related to the documentations. labels Sep 30, 2015

indutny commented Sep 30, 2015

Copy link
Copy Markdown
Member

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

thefourtheye commented Sep 30, 2015

Copy link
Copy Markdown
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 commented Sep 30, 2015

Copy link
Copy Markdown
Member

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

thefourtheye commented Sep 30, 2015

Copy link
Copy Markdown
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 commented Sep 30, 2015

Copy link
Copy Markdown
Contributor

When would one want to use multiple keys?

indutny commented Sep 30, 2015

Copy link
Copy Markdown
Member

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

silverwind reviewed Oct 5, 2015
View reviewed changes
Comment thread doc/api/tls.markdown Outdated

silverwind Oct 5, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

silverwind Oct 9, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ping @thefourtheye: could you clarify? Otherwise LGTM

thefourtheye Oct 9, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

silverwind Oct 9, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

thefourtheye Oct 9, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

silverwind Oct 19, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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)

thefourtheye Oct 25, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

thefourtheye commented Oct 25, 2015

Copy link
Copy Markdown
Contributor Author

Bump!

silverwind commented Oct 26, 2015

Copy link
Copy Markdown
Contributor

LGTM

thefourtheye commented Oct 26, 2015

Copy link
Copy Markdown
Contributor Author

@indutny LGTY?

indutny commented Oct 28, 2015

Copy link
Copy Markdown
Member

LGTM

thefourtheye added a commit that referenced this pull request Oct 28, 2015
doc: createServer's key option can be an array
5d5a4c4 
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 <me@silverwind.io>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

thefourtheye commented Oct 28, 2015

Copy link
Copy Markdown
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 deleted the improve-tls-keys-doc branch October 28, 2015 02:18
jasnell mentioned this pull request Oct 28, 2015
Closed
rvagg pushed a commit to rvagg/io.js that referenced this pull request Oct 29, 2015
doc: createServer's key option can be an array
5e76587 
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 <me@silverwind.io>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
This was referenced Oct 29, 2015
Merged
Closed
thefourtheye added a commit that referenced this pull request Oct 30, 2015
doc: createServer's key option can be an array
db8e2f1 
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 <me@silverwind.io>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

jasnell commented Oct 30, 2015

Copy link
Copy Markdown
Member

Landed in v4.x-staging in db8e2f1

thefourtheye added a commit that referenced this pull request Oct 30, 2015
doc: createServer's key option can be an array
a5f0d64 
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 <me@silverwind.io>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

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.

5 participants

Back | FazBrowse Home | New Git URL