Expose security levels #56601
Expose security levels #56601
Conversation
Distros may compile with a different openssl security level than the default. In addition there has been some discussion with respect to shipping with a different default security security level in different Node.js versions in order to main stabilty. Exposing the default openssl security level with let us have tests that work in these situations as well as allow applications to better cope with the avialable crypto algorithms. - add API to get openssl security level - modify one test to use security level instead of openssl version as an example Signed-off-by: Michael Dawson <[email protected]>
Signed-off-by: Michael Dawson <[email protected]>
|
Review requested:
|
nodejs-github-bot
added
c++
|
Run which tests on the different OpenSSL versions - https://ci.nodejs.org/job/node-test-commit-linux-containered/48401/ All passed, so I think that confirms it is working correctly. In terms of testing I don't think we can change the default except at compile time, and I also think comparing against a specific version could cause problems for the shared library testing. I can add a test that just calls the method and makes sure it is within the range documented which is currently 1-5. Does that makes sense to people? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56601 +/- ##
==========================================
- Coverage 89.19% 89.19% -0.01%
==========================================
Files 662 662
Lines 191797 191858 +61
Branches 36923 36925 +2
==========================================
+ Hits 171074 171124 +50
- Misses 13569 13572 +3
- Partials 7154 7162 +8
|
|
|
||
| * Returns: {number} The [`default OpenSSL security level`][]. | ||
| Useful to be able to adjust based on the algorithms available given the | ||
| security level. |
There was a problem hiding this comment.
Some explanation as to how to interpret the returned values would be helpful.
And just for my knowledge because I'm not that familiar with the security levels... are the return values specific at all the version of OpenSSL? Is it supported by BoringSSL?
There was a problem hiding this comment.
Ok just digging into the API a bit more ... https://docs.openssl.org/3.0/man3/SSL_CTX_set_security_level/#description
Looks like there are six security levels 0 through 5.... honestly we should document these and/or link to the relevant openssl definitions of them here.
The docs should also acknowledge that the API will not work when Node.js is compiled to use BoringSSL.
There was a problem hiding this comment.
And just for my knowledge because I'm not that familiar with the security levels... are the return values specific at all the version of OpenSSL? Is it supported by BoringSSL?
Yes, the values mean different things for different OpenSSL versions.
Compare the level descriptions for 1.1.1, 3.0 and 3.4:
| return ThrowCryptoError(env, ERR_get_error(), "SSL_new"); | ||
| } | ||
|
|
||
| int sec_level = SSL_get_security_level(ssl); |
There was a problem hiding this comment.
consider adding a method to ncrypto::SSLPointer?
| return ThrowCryptoError(env, ERR_get_error(), "SSL_new"); | ||
| } | ||
|
|
||
| int sec_level = SSL_get_security_level(ssl); |
There was a problem hiding this comment.
I could be missing it but based on a code search it does not appear as if this is implemented in boringssl at the moment so this will need to be gated and will need to have a good fallback behavior defined when openssl is not used.
| @@ -44,6 +44,7 @@ const { getOptionValue } = require('internal/options'); | |||
| const { | |||
| getFipsCrypto, | |||
| setFipsCrypto, | |||
| getOpenSSLSecLevelCrypto, | |||
There was a problem hiding this comment.
| getOpenSSLSecLevelCrypto, | |
| getOpenSSLSecLevelCrypto: getOpenSSLSecLevel, |
| function getOpenSSLSecLevel() { | ||
| return getOpenSSLSecLevelCrypto(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Can remove the indirection through the function by aliasing the import
| function getOpenSSLSecLevel() { | |
| return getOpenSSLSecLevelCrypto(); | |
| } |
| @@ -43,7 +44,7 @@ const dheCipher = 'DHE-RSA-AES128-SHA256'; | |||
| const ecdheCipher = 'ECDHE-RSA-AES128-SHA256'; | |||
| const ciphers = `${dheCipher}:${ecdheCipher}`; | |||
|
|
|||
| if (!common.hasOpenSSL(3, 2)) { | |||
| if (secLevel < 2) { | |||
There was a problem hiding this comment.
I know this was a problem with the original code here also but it would be helpful to add a comment explaining what the 2 here is.... a pointer to the docs would suffice.
No description provided.