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

tls.DEFAULT_MIN_VERSION according to docs is v11 feature, but its present in v10.16.0 and is incorrect #28758

Closed
nitinsurana opened this issue Jul 18, 2019 · 14 comments
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. tls Issues and PRs related to the tls subsystem.

Comments

@nitinsurana
Copy link

nitinsurana commented Jul 18, 2019

  • Version: 10.16.0
  • Platform: Ubuntu 14.04

According to the docs tls.DEFAULT_MIN_VERSION is available starting v11 and default value is TLSv1.2

Issue :

  1. Ubuntu 14.04 with Nodejs v10.16.0 has tls.DEFAULT_MIN_VERSION defined
  2. It is returning incorrect value viz TLSv1 instead of TLSv1.2
@nitinsurana
Copy link
Author

Possibly, this commit needs to be merged with 10x

@Trott
Copy link
Member

Trott commented Jul 18, 2019

Possibly, this commit needs to be merged with 10x

No, that's just a changelog entry.

@Trott
Copy link
Member

Trott commented Jul 18, 2019

Seems like #26821 needs to be added to v10.x-staging (so it can go out in the next release).

@sam-github
Copy link
Contributor

It is returning incorrect value viz TLSv1 instead of TLSv1.2

Value is correct, you are reading 12.x docs, the 11.x docs are slightly more relevant to 10.x: https://nodejs.org/docs/latest-v11.x/api/tls.html#tls_tls_default_min_version

@sam-github sam-github added doc Issues and PRs related to the documentations. tls Issues and PRs related to the tls subsystem. v10.x good first issue Issues that are suitable for first-time contributors. labels Jul 22, 2019
@ckarande
Copy link
Contributor

ckarande commented Jul 22, 2019

I would like to assist on this issue and starting to exploring it further.
cc: @mcollina

@mcollina
Copy link
Member

@ckarande
Copy link
Contributor

@mcollina Thanks for the link. I am all set to go ahead with it.

@ckarande
Copy link
Contributor

@sam-github @nitinsurana @Trott I am comparing the changes in #26821 with what is applicable to v10.x. Can you please confirm these findings:

  1. In protocol versions, remove references of TLSv1.3,as it was not supported in v10
  2. Remove references to CLI options such as --tls-min-v1.0, --tls-min-v1.2, --tls-min-v1.3, --tls-max-v1.2 --tls-max-v1.3 as they do not seem to be supported in v10.x.

ckarande added a commit to ckarande/node that referenced this issue Jul 23, 2019
Add documentation for tls.DEFAULT_MAX_VERSION and
tls.DEFAULT_MIN_VERSION, which existed in v10.6.0

Fixes: nodejs#28758
Refs: nodejs#26821
@sam-github
Copy link
Contributor

looks about right to me.

#27946 is trying to add the CLI switches, but its blocked on a problem with a container build I haven't been able to replicate outside docker, or had the time to replicate inside docker.

@nitinsurana
Copy link
Author

@ckarande The findings & the PR both looks good to me 👍
The build is failing because of missing reference to the new constants. Please see my comment on the PR.

@ckarande
Copy link
Contributor

ckarande commented Jul 24, 2019

Thanks @nitinsurana . The build was successful with the latest fixes.

@Trott , @sam-github, the fix so far adds missing documentation about tls.DEFAULT_MAX_VERSION and tls.DEFAULT_MIN_VERSION options to the Node v10 docs. Should we also fix v11.x and v12.x docs to indicate that these options were added in v10.6.0 instead of v11.4.0? The corresponding sections in v11.x and v12.x docs also refer to the CLI options, which were not present on v10.6.0. So it may not be completely accurate to just indicate that these features were added in v10.6.0.

@Trott
Copy link
Member

Trott commented Jul 24, 2019

Should we also fix v11.x and v12.x docs to indicate that these options were added in v10.6.0 instead of v11.4.0?

No, leave those docs as they are. The features were added in 11.4.0, then backported to the v10 line. So 11.4.0 is correct for versions 11 and later. If we change it to 10.6.0, then someone running 11.3.0 will wonder why it doesn't exist in 11.3.0.

@ckarande
Copy link
Contributor

Makes sense. Thanks for the clarification. In my PR for v10.x docs, I indicated that these options were added in v10.6.0 (diff1 and diff2). Should that be fixed to indicate these were added in 11.4.0? I was not sure if it is normal to specify that a feature was added in a future release.

BethGriggs pushed a commit that referenced this issue Oct 7, 2019
Add documentation for tls.DEFAULT_MAX_VERSION and
tls.DEFAULT_MIN_VERSION, which existed in v10.6.0

Fixes: #28758
Refs: #26821

PR-URL: #28827
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
BethGriggs pushed a commit that referenced this issue Oct 7, 2019
PR-URL: #28827
Fixes: #28758
Refs: #26821
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
@sam-github
Copy link
Contributor

https://nodejs.org/docs/latest-v10.x/api/tls.html#tls_tls_default_min_version

docs are present and correct in 10.x line

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. good first issue Issues that are suitable for first-time contributors. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

6 participants