Skip to content

/ node

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

Jump to bottom

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
Closed

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

nitinsurana opened this issue Jul 18, 2019 · 14 comments
Labels
doc good first issue tls v10.x

Comments

@nitinsurana
Copy link

@nitinsurana nitinsurana commented Jul 18, 2019 •
edited

  • 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

This comment has been minimized.

Sign in to view
Copy link
Author

@nitinsurana nitinsurana commented Jul 18, 2019

Possibly, this commit needs to be merged with 10x
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

@Trott Trott commented Jul 18, 2019

Possibly, this commit needs to be merged with 10x

No, that's just a changelog entry.
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

@Trott 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).
@Trott Trott mentioned this issue Jul 18, 2019
Closed
0 of 4 tasks complete
@sam-github

This comment has been minimized.

Sign in to view
Copy link
Member

@sam-github sam-github commented Jul 22, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

@ckarande ckarande commented Jul 22, 2019 •
edited

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

This comment has been minimized.

Sign in to view
Copy link
Member

@mcollina mcollina commented Jul 23, 2019

@ckarande do you need any direction/help? Check out https://github.com/nodejs/node/blob/master/doc/guides/backporting-to-release-lines.md.
@ckarande

This comment has been minimized.

Sign in to view
Copy link
Contributor

@ckarande ckarande commented Jul 23, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

@ckarande ckarande commented Jul 23, 2019

@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
@ckarande
doc: describe tls.DEFAULT_MIN_VERSION/_MAX_VERSION
Loading status checks…
e3ebd51 
Add documentation for tls.DEFAULT_MAX_VERSION and
tls.DEFAULT_MIN_VERSION, which existed in v10.6.0

Fixes: nodejs#28758
Refs: nodejs#26821
@ckarande ckarande mentioned this issue Jul 23, 2019
Closed
3 of 3 tasks complete
@sam-github

This comment has been minimized.

Sign in to view
Copy link
Member

@sam-github sam-github commented Jul 23, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Author

@nitinsurana nitinsurana commented Jul 24, 2019

@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

This comment has been minimized.

Sign in to view
Copy link
Contributor

@ckarande ckarande commented Jul 24, 2019 •
edited

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

This comment has been minimized.

Sign in to view
Copy link
Member

@Trott Trott commented Jul 24, 2019 •
edited

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

@ckarande ckarande commented Jul 24, 2019

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 added a commit that referenced this issue Oct 7, 2019
@ckarande @BethGriggs
doc: describe tls.DEFAULT_MIN_VERSION/_MAX_VERSION
Verified
This commit was signed with a verified signature.
BethGriggs Bethany Nicolle Griggs
GPG key ID: D7062848A1AB005C Learn about signing commits
cef5010 
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 added a commit that referenced this issue Oct 7, 2019
@ckarande @BethGriggs
doc: fix the links tls default version sections
Verified
This commit was signed with a verified signature.
BethGriggs Bethany Nicolle Griggs
GPG key ID: D7062848A1AB005C Learn about signing commits
Loading status checks…
c285e69 
PR-URL: #28827
Fixes: #28758
Refs: #26821
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
@Trott Trott removed the Hacktoberfest label Nov 3, 2019
@sam-github

This comment has been minimized.

Sign in to view
Copy link
Member

@sam-github sam-github commented Nov 26, 2019

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

docs are present and correct in 10.x line
@sam-github sam-github closed this Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
doc good first issue tls v10.x
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
@nitinsurana @Trott @sam-github @ckarande @mcollina @trivikr
You can’t perform that action at this time.