Skip to content

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
@thefourtheyethefourtheye 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

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

@thefourtheye
Copy link
ContributorAuthor

@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

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

@thefourtheye
Copy link
ContributorAuthor

@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

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

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
ContributorAuthor

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
ContributorAuthor

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
ContributorAuthor

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
ContributorAuthor

Bump!

@silverwind
Copy link
Contributor

LGTM

@thefourtheye
Copy link
ContributorAuthor

@indutny LGTY?

@indutny
Copy link
Member

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
ContributorAuthor

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.

@thefourtheyethefourtheye 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

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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docIssues and PRs related to the documentations.tlsIssues and PRs related to the tls subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@thefourtheye@indutny@silverwind@jasnell@MylesBorins