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

crypto: setting --tls-max-v1.2 and --tls-cipher-list seems to ignore --tls-min-* setting #43406

Closed
AdamMajer opened this issue Jun 13, 2022 · 3 comments · Fixed by #44086
Closed
Labels
crypto Issues and PRs related to the crypto subsystem.

Comments

@AdamMajer
Copy link
Contributor

AdamMajer commented Jun 13, 2022

Version

18.2.0, master

Platform

Linux localhost 5.17.7-1-default #1 SMP PREEMPT Thu May 12 12:38:04 UTC 2022 (c9a5fa1) x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

./node --tls-max-v1.2 --tls-min-v1.2 --tls-cipher-list='TLS_RSA_WITH_AES_256_CBC_SHA' -e "https.get('https://google.com/', (res) => {console.log('statusCode:', res.statusCode, res.client.getCipher()); }).on('error', (e) => console.error(e));"

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

TLSv1.2 connection or failure thereof

What do you see instead?

statusCode: 301 {
  name: 'AES256-SHA',
  standardName: 'TLS_RSA_WITH_AES_256_CBC_SHA',
  version: 'SSLv3'
}

Additional information

No response

@VoltrexKeyva VoltrexKeyva added the crypto Issues and PRs related to the crypto subsystem. label Jun 13, 2022
@bnoordhuis
Copy link
Member

TLS_RSA_WITH_AES_256_CBC_SHA is still around in TLSv1.2: https://ciphersuite.info/cs/TLS_RSA_WITH_AES_256_CBC_SHA/

Is your report about the version: 'SSLv3' line in the output? OpenSSL's SSL_CIPHER_get_version() reports the cipher's minimum SSL/TLS protocol version, not the protocol version of the current session.

@AdamMajer
Copy link
Contributor Author

Yes, indeed, I was indeed confused about the SSLv3 in the getCiphers() line. I've found getProtocol() and life makes sense again. Thanks for the clarification.

@bnoordhuis
Copy link
Member

If you're up for it, I think a documentation update to tlsSocket.getCipher() can help clarify the difference between the cipher and the session protocol version.

It is mentioned but the example uses a TLSv1.2-only cipher, making it an easy-to-miss nuance.

AdamMajer added a commit to AdamMajer/node that referenced this issue Aug 1, 2022
The getCipher() returns a tuple that includes protocol version string.
This string refers to the minimum protocol version string, as per
documentation. What is missing is a reference to the documentation
where to get the negotiated cipher for the socket connection and
a clearer example.

Fixes: nodejs#43406
AdamMajer added a commit to AdamMajer/node that referenced this issue Aug 1, 2022
The getCipher() returns a tuple that includes protocol version string.
This string refers to the minimum protocol version string, as per
documentation. What is missing is a reference to the documentation
where to get the negotiated cipher for the socket connection and
a clearer example.

Fixes: nodejs#43406
nodejs-github-bot pushed a commit that referenced this issue Aug 3, 2022
The getCipher() returns a tuple that includes protocol version string.
This string refers to the minimum protocol version string, as per
documentation. What is missing is a reference to the documentation
where to get the negotiated cipher for the socket connection and
a clearer example.

Fixes: #43406
PR-URL: #44086
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
danielleadams pushed a commit that referenced this issue Aug 16, 2022
The getCipher() returns a tuple that includes protocol version string.
This string refers to the minimum protocol version string, as per
documentation. What is missing is a reference to the documentation
where to get the negotiated cipher for the socket connection and
a clearer example.

Fixes: #43406
PR-URL: #44086
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
ruyadorno pushed a commit that referenced this issue Aug 23, 2022
The getCipher() returns a tuple that includes protocol version string.
This string refers to the minimum protocol version string, as per
documentation. What is missing is a reference to the documentation
where to get the negotiated cipher for the socket connection and
a clearer example.

Fixes: #43406
PR-URL: #44086
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
targos pushed a commit that referenced this issue Sep 5, 2022
The getCipher() returns a tuple that includes protocol version string.
This string refers to the minimum protocol version string, as per
documentation. What is missing is a reference to the documentation
where to get the negotiated cipher for the socket connection and
a clearer example.

Fixes: #43406
PR-URL: #44086
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Fyko pushed a commit to Fyko/node that referenced this issue Sep 15, 2022
The getCipher() returns a tuple that includes protocol version string.
This string refers to the minimum protocol version string, as per
documentation. What is missing is a reference to the documentation
where to get the negotiated cipher for the socket connection and
a clearer example.

Fixes: nodejs#43406
PR-URL: nodejs#44086
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
juanarbol pushed a commit that referenced this issue Oct 10, 2022
The getCipher() returns a tuple that includes protocol version string.
This string refers to the minimum protocol version string, as per
documentation. What is missing is a reference to the documentation
where to get the negotiated cipher for the socket connection and
a clearer example.

Fixes: #43406
PR-URL: #44086
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
juanarbol pushed a commit that referenced this issue Oct 11, 2022
The getCipher() returns a tuple that includes protocol version string.
This string refers to the minimum protocol version string, as per
documentation. What is missing is a reference to the documentation
where to get the negotiated cipher for the socket connection and
a clearer example.

Fixes: #43406
PR-URL: #44086
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this issue Jan 3, 2023
The getCipher() returns a tuple that includes protocol version string.
This string refers to the minimum protocol version string, as per
documentation. What is missing is a reference to the documentation
where to get the negotiated cipher for the socket connection and
a clearer example.

Fixes: nodejs/node#43406
PR-URL: nodejs/node#44086
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this issue Jan 3, 2023
The getCipher() returns a tuple that includes protocol version string.
This string refers to the minimum protocol version string, as per
documentation. What is missing is a reference to the documentation
where to get the negotiated cipher for the socket connection and
a clearer example.

Fixes: nodejs/node#43406
PR-URL: nodejs/node#44086
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants