[Tracking Issue] OpenSSL 3 #29817
Comments
jasnell
added
crypto
|
Its been on my list of things to look at, because FIPS will require OpenSSL 3.x, but I haven't had the time to try a build yet. OpenSSL 3.x is intended to be API compatible with OpenSSL 1.1.1 -- though NOT ABI compatible, but the proof of that will be in the build and testing. |
|
Ok, I'll see if I can at least do a preliminary check on it in the coming few weeks. Let's leave this issue open as a tracking issue for it. |
|
OpenSSL policy is to not release new features in patch releases, so OpenSSL 1.1.1 is feature frozen. However, OpenSSL also decided not to have a new 1.1.x release. So, QUIC support will only be possible on OpenSSL 3.x, or on our internal OpenSSL 1.1.1 with overlay patches. I'm not sure what to do about that. Once we have QUIC and HTTP/3 support it might be worth bringing this up with OpenSSL, because no OpenSSL 1.1.1 ABI compatible release will ever support HTTP/3. That might become an issue important enough for them to do an OpenSSL 1.1.2 (or 1.2.0). At this point, given lack of adoption or support for HTTP/3, I think its a bit early to try to get them to do a new release. |
|
@nodejs/crypto @nodejs/tsc ... just a heads up.. the OpenSSL OMC has announced the first alpha of OpenSSL 3: https://www.openssl.org/blog/blog/2020/04/23/OpenSSL3.0Alpha1/ |
|
@sam-github ... I've been thinking that it would be a good idea to set up an openssl-canary branch where we can start working with compiling openssl 3 into nodejs master to work out any issues that may exist. Wanted to coordinate with you on that before moving forward tho. |
|
Sure, so a branch like FYI, my first experience posted here, https://mta.openssl.org/pipermail/openssl-users/2020-April/012312.html (repeated below), but I used the configure flags to point to an out-of-tree openssl in order to do those tests. In terms of dep update, the deps/openssl/conf scripts failed when openssl was replaced with openssl 3. How much it will take to make them work I have not looked at. Fwiw, took a quick run at building and testing Node.js against the 3.x beta. It was API compatible enough to build. The DH_, ECDH_, HMAC_, etc. My assumption is that EVP versions of these exist in openssl 1.1.1, ERR_func_error_string, what is its replacement? I didn't see it Tests didn't go so well. Minor changes in error strings are to be I haven't looked at these yet other than scan the output, we might be https://gist.github.com/sam-github/5a3b3775029efb3d31109d7e6e390f85 |
|
I've started to take a look at this and I've pushed the following branch: I'm currently building this locally against OpenSSL 3.0 Alpha 3 as I ran into an issue with our snapshot building with Alpha 4 (I'll investigate that further once I got all the tests to pass). There are two tests that are failing at the moment which are a little more involved than just error message/code changes but I'm looking into them now. I'm using the following configuration options to build: $ ./configure --shared-openssl --shared-openssl-libpath=/work/security/openssl_build_master/lib --shared-openssl-includes=/work/security/openssl_build_master/include --shared-openssl-libname=crypto,ssl --debug |
This comment has been minimized.
This comment has been minimized.
This commit sets the error mark before calling old_priv_decode and if old_priv_decode returns false, and if EVP_PKCS82PKEY is successful, the errors are popped to the previously set mark. The motivation for this is an issue we found when linking Node.js against OpenSSL 3.0. Details can be found in the link below and the test case provided in this commit attempts cover this. Refs: https://github.com/danbev/learning-libcrypto#asn1-wrong-tag-issue Refs: nodejs/node#29817
This commit sets the error mark before calling old_priv_decode and if old_priv_decode returns false, and if EVP_PKCS82PKEY is successful, the errors are popped to the previously set mark. The motivation for this is an issue we found when linking Node.js against OpenSSL 3.0. Details can be found in the link below and the test case provided in this commit attempts cover this. Refs: https://github.com/danbev/learning-libcrypto#asn1-wrong-tag-issue Refs: nodejs/node#29817 Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from #13073)
This commit tries to address a locking issue in evp_pkey_reset_unlocked which can occur when it is called from evp_pkey_downgrade. evp_pkey_downgrade will acquire a lock for pk->lock and if successful then call evp_pkey_reset_unlocked. evp_pkey_reset_unlocked will call memset on pk, and then create a new lock and set pk->lock to point to that new lock. I believe there are two problems with this. The first is that after the call to memset, another thread would try to acquire a lock for NULL as that is what the value of pk->lock would be at that point. The second issue is that after the new lock has been assigned to pk->lock, that lock is different from the one currently locked so another thread trying to acquire the lock will succeed which can lead to strange behaviour. More details and a reproducer can be found in the Refs link below. This commit introduces a new function that is only called by evp_pkey_downgrade which does not use memset but instead "manually" sets the EVP_PKEY values to their default values, but does not modify pk->lock. This could perhaps be updated to go back to only having one function that is called for both evp_pkey_downgrade and EVP_PKEY_new and only create a new lock if one does not already exist. Refs: https://github.com/danbev/learning-libcrypto/blob/master/notes/issues.md#openssl-investigationtroubleshooting nodejs/node#29817
This commit tries to address a locking issue in evp_pkey_reset_unlocked which can occur when it is called from evp_pkey_downgrade. evp_pkey_downgrade will acquire a lock for pk->lock and if successful then call evp_pkey_reset_unlocked. evp_pkey_reset_unlocked will call memset on pk, and then create a new lock and set pk->lock to point to that new lock. I believe there are two problems with this. The first is that after the call to memset, another thread would try to acquire a lock for NULL as that is what the value of pk->lock would be at that point. The second issue is that after the new lock has been assigned to pk->lock, that lock is different from the one currently locked so another thread trying to acquire the lock will succeed which can lead to strange behaviour. More details and a reproducer can be found in the Refs link below. This changes the evp_pkey_reset_unlocked to not touch the lock and the creation of a new lock is done in EVP_PKEY_new. Refs: https://github.com/danbev/learning-libcrypto/blob/master/notes/issues.md#openssl-investigationtroubleshooting nodejs/node#29817 Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from #13374)
This commit enables node to dynamically link against OpenSSL 3.0. The motivation for opening this PR even though OpenSSL 3.0 has not been released yet is to allow a nightly CI job to be created. This will allow us stay on top of changes required for OpenSSL 3.0, and also to make sure that changes to node crypto do not cause issues when linking to OpenSSL 3.0. PR-URL: #37669 Refs: #29817 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
This commit enables node to dynamically link against OpenSSL 3.0. The motivation for opening this PR even though OpenSSL 3.0 has not been released yet is to allow a nightly CI job to be created. This will allow us stay on top of changes required for OpenSSL 3.0, and also to make sure that changes to node crypto do not cause issues when linking to OpenSSL 3.0. PR-URL: #37669 Refs: #29817 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
This commit enables node to dynamically link against OpenSSL 3.0. The motivation for opening this PR even though OpenSSL 3.0 has not been released yet is to allow a nightly CI job to be created. This will allow us stay on top of changes required for OpenSSL 3.0, and also to make sure that changes to node crypto do not cause issues when linking to OpenSSL 3.0. PR-URL: #37669 Refs: #29817 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
|
Part of the work for this issue has been done with the updates made to be able to dynamically link with OpenSSL 3.0 and have the test suite pass. And @richardlau has set up a CI build so that we can keep track of any changes that break this 🎉 The next task is to take a look at what is required to be able to replace the statically linked OpenSSL version so that at some point the current version can be replaced. Not sure if I should continue updating this issue or create separate issues for each task. If anyone has a preference please let me know. |
|
Just please be sure to source from the quictls/openssl repo, which has all the 3.0 bits. :) |
|
@jasnell I actually just realized this today that we should be using quictls/openssl for 3.0 as well, while updating the OpenSSL-Strategy.md. I've been using openssl/openssl until now and that is what the CI job is using as that is what I specified. I'll update the CI config so that it is testing the right version as well. |
Would you mind sharing the instructions to actually accomplish this? 🙃 |
|
@danbev Would you mind giving me a quick response to my previous message? I am happy to assist and collaborate - I just want to avoid that I also start "from scratch"! We are eagerly trying to get node 16 and FIPS working as this is a requirement of one of our customers... |
|
@mayrbenjamin92 Dan is currently on holiday and may not be checking notifications. |
|
@mayrbenjamin92 Like @richardlau mentioned I'm currently on PTO but I'll will be back next Friday and continue this work. The current state of #38512 is that I'm trying to get CI to pass for all architectures, and after that I'll iterate on this work (refactor and clean things up, and add some docs). |
|
Hi @richardlau thanks for your response! I am still running into issues when following the provided docs - this is how far I got so far: The openssl stuff works - I can build it (I have setup a pipeline for that). My "openssl" checkout is located in: My configured build path is
subdirectories. My As far as I understood, this is what I should suppose to do - adding the I then exported the following env variables: So far so good. I then cloned the "nodejs" source, checked-out the latest v16.X.X version explicitly and ran the following: I first looked okay: But when I actually then try to start compiling node, I will get the following linker error at the end: I am not sure whether I have missed something or whether the docs could be improved. |
|
Hi guys, maybe I am also confused, but what is meant with |
|
@mayrbenjamin92 Are you on Linux? If you are not then instead of setting
If you are on Linux, then the linker error is strange... you should have
This is what you pass to the "install" the built OpenSSL libraries.
Docs are always something that can be improved -- pull requests always welcome 🙂. |
|
@richardlau thanks for making this more clear. I will definitely improve the documentation as something is not "perfect" - I will create a pull request, soon. One other short question - it looks like that I am partially successful. I am at the following step (I called So it looks like that I am stuck in this step - nothing happens for more than 20 minutes. Is it really that heavy? |
|
I am one step further - I am, meanwhile, trying all of this on a big 16 CPU/30GB virtual machine on AWS. I think that I get quite far, but then I run into this: |
That does feel long. We did see a hang here with earlier alpha builds of OpenSSL 3.0.0 due to openssl/openssl#12290. I also hit a hang with a misconfigured You could try building with (Node.js)
Does |
|
Okay I think that I still have some fundamental issue. So I am doing all of this on an Ubuntu 20.04 server on AWS - 16CPU/30GB memory. Kernel is: My "script" does the following: Running all of this gives me a "standard" GIT checkout of "openssl" located under Output is: Contents of "lib": Output is: The "ossl-modules" folder only contains a single file: The Output is: The content of the As far as I understand, I should now alter the So this is the content of my Whats next? There is also an "SSL" folder in Where should I place
Does that make sense? |
|
Ah this might be a gap in the docs 😞 (or maybe something changed in OpenSSL). Can you try between in For reference, we run the CI builds in a Docker container where we build OpenSSL 3.0.0 when we create the container, https://github.com/nodejs/build/blob/001a5725c136d472c565a6b6fe90eac143956098/ansible/roles/docker/templates/ubuntu1804_sharedlibs.Dockerfile.j2#L83-L90. The FIPS build steps in the docs are then run via Jenkins inside the built container. Thanks for persevering with this -- it's always good to get new eyes on docs. |
|
So this is what I now came up with - sorry for the "repeating content": OpenSSLCloneBuildAlter OpenSSL config and "enable" fipsConfigure environmentNodeCloneBuildSo I actually end up with the following contents in That actually looks good. Please also check the "build steps" for NodeJS (above). I, sadly, still end up with: So something is still wrong :/. |
|
@richardlau My assumption that appending to the openssl.cnf file is correct, right? So since the environment variables (OPENSSL_MODULES and OPENSSL_CONF) now point to the |
Yes (to both questions). I don't see anything obvious that would explain why OpenSSL couldn't open/find |
|
@richardlau ohhh my god! That did the trick! I hate relative file-paths... Thank you soo much for your assistance! I will create a pull request and improve/update the documentation with my latest findings! Works as expected: |
|
Currently, if I try a build on Linux using system OpenSSL 3.0.0 and passing |
|
@dschepler opening a new issue is probably a good idea 👍🏻 |
|
Closing this issue as OpenSSL 3.0 has been released now. |
Hey @nodejs/crypto (and in particular @sam-github). For the QUIC implementation, for a variety of reasons, it's going to better for us to be able to move up to OpenSSL 3 as soon as possible. Yes, I know it hasn't been released yet. I wanted to check in to see if anyone has started the exploration of 3.0 yet, and if so, what the issues so far have been.
For QUIC, the reason we need it is because the BoringSSL QUIC APIs are being ported to OpenSSL 3.0. Once those are available, our implementation will be greatly simplified by being able to take advantage of the ngtcp2_crypto helper library. As it stands right now, we have to overlay and support a patch on OpenSSL 1.1.1 for it to work.
The text was updated successfully, but these errors were encountered: