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

Add OpenSSL 3 support #19814

Merged
merged 9 commits into from
Aug 23, 2022
Merged

Add OpenSSL 3 support #19814

merged 9 commits into from
Aug 23, 2022

Conversation

FedericoCeratto
Copy link
Member

@FedericoCeratto FedericoCeratto commented May 21, 2022

Drop support for versions below 1.1.0
Move away from deprecated functions
Fixes: #19604

@ghost
Copy link

ghost commented May 21, 2022

So are you also planning on removing LibreSSL support as it relies on OpenSSL 1.0 APIs? It's really nice that you're doing the work with bringing up OpenSSL 3, but I think dropping support for OpenSSL < 1.1.0 is not a good idea.

@FedericoCeratto
Copy link
Member Author

FedericoCeratto commented May 22, 2022

@Yardanico supporting OpenSSL 3, 1.1, 1.0, 0.9 and LibreSSL, both with dynamic and static linking (and across multiple OSes) creates a combinatorial explosion of test scenarios, making it really difficult to ensure the libraries are used securely.
IMHO we should try to reduce the complexity in the wrapper. Maybe even remove a good fraction of unused procs.
See #19604 for more comments.

Copy link
Contributor

@heinthanth heinthanth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FedericoCeratto, I'm not sure but I think, the reason of failing on mac runners is due to this ...

.github/workflows/ci_packages.yml Outdated Show resolved Hide resolved
@heinthanth
Copy link
Contributor

heinthanth commented Jul 11, 2022

No Luck ... the same error.

I think it's due to macOS SIP. It don't pass VARIABLES starts with DYLD_ to subprocess.

Screen Shot 2022-07-11 at 2 40 04 PM

DYLD_LIBRARY_PATH variables exists. But when I execute levelOne.sh that var is wiped out. So, u can't set DYLD_* variable outside and use inside script that's the main issue. Other than that, the pull request works fine!

https://stackoverflow.com/questions/35568122/why-isnt-dyld-library-path-being-propagated-here
rbenv/rbenv#962

I found those issues.

@heinthanth
Copy link
Contributor

heinthanth commented Jul 11, 2022

Here are the proofs

Screen Shot 2022-07-11 at 3 05 07 PM

I've a script to run compiled nimble at bin dir. When I run that nimble, it works ( since var is passed to nimble process )
But when I run it via runNimble.sh, didn't work ( since var is wiped out ).
PS: I compiled nimble with -d:nimDebugDlOpen to debug.

@FedericoCeratto
Copy link
Member Author

Env vars get lost might get lost in different locations in nimInternalBuildKochAndRunCI.
Even nim r foo.nim drops them.
Besides, having to add special env vars for OSX would be an annoying requirement for end users. SSL should work out of the box.

@FedericoCeratto
Copy link
Member Author

The library import on Mac OS X is now fixed.

@FedericoCeratto
Copy link
Member Author

@ringabout @Araq the remaining issue seems to be a timeout due to slow CI. Do you think the PR can be reviewed and merged?

@FedericoCeratto FedericoCeratto requested review from ringabout and Varriount and removed request for dom96 August 2, 2022 16:22
@ringabout
Copy link
Member

the remaining issue seems to be a timeout due to slow CI.

I restarted the failed CI.

@Araq Araq closed this Aug 3, 2022
@Araq Araq reopened this Aug 3, 2022
@Araq
Copy link
Member

Araq commented Aug 3, 2022

One more attempt before I'll merge it anyway.

@heinthanth
Copy link
Contributor

Failed CI is due to timeout, I guess. Anyway, nice work @FedericoCeratto. Congrats!

@FedericoCeratto
Copy link
Member Author

@Araq is there any plan to make a minor release to add support for OpenSSL 3? A number of users are being impacted by this.

@FedericoCeratto
Copy link
Member Author

@Araq finally the CI is all green :)

@Araq Araq merged commit 2dcfd73 into nim-lang:devel Aug 23, 2022
@github-actions
Copy link
Contributor

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from 2dcfd73

Hint: mm: orc; threads: on; opt: speed; options: -d:release
163641 lines; 12.692s; 841.465MiB peakmem

@FedericoCeratto FedericoCeratto deleted the openssl3 branch August 25, 2022 17:55
@arnetheduck
Copy link
Contributor

These changes break CI on macos 11 for many of our projects that previously worked without having to install additional dependencies:

Nim Compiler Version 1.7.1 [MacOSX: amd64]
Compiled at 2022-08-26
Copyright (c) 2006-2022 by Andreas Rumpf

git hash: ea44c5cfed21951feb5978b74fbc6cdb24f54ac2
active boot switches: -d:release
could not load: libcrypto.1.1.dylib
(compile with -d:nimDebugDlOpen for more information)

https://github.com/status-im/nim-web3/runs/8027875139?check_suite_focus=true

narimiran added a commit to status-im/nim-chronos that referenced this pull request Aug 30, 2022
narimiran added a commit to status-im/nim-stew that referenced this pull request Aug 30, 2022
narimiran added a commit to status-im/nim-metrics that referenced this pull request Aug 31, 2022
narimiran added a commit to status-im/nim-ssz-serialization that referenced this pull request Sep 2, 2022
narimiran added a commit to status-im/nim-toml-serialization that referenced this pull request Sep 2, 2022
arnetheduck pushed a commit to status-im/nim-chronos that referenced this pull request Sep 2, 2022
* switch CI to the supported versions of ubuntu and macos

See:
- actions/runner-images#6002
- actions/runner-images#5583

* don't continue on error for Nim 1.6

* install openssl on macos for Nim devel

due to changes in nim-lang/Nim#19814
jangko pushed a commit to status-im/nim-toml-serialization that referenced this pull request Sep 3, 2022
Araq added a commit that referenced this pull request Sep 7, 2022
metagn added a commit to metagn/Nim that referenced this pull request Sep 13, 2022
Varriount pushed a commit that referenced this pull request Sep 14, 2022
* conservative partial revert of #19814

* fix

* revert tssl

* revert azure CI change

* keep azure, revert version range

* fully revert CI, add changelog

* useOpenssl3 as separate define, .3 is a version
metagn added a commit to metagn/Nim that referenced this pull request Oct 26, 2022
Araq pushed a commit that referenced this pull request Oct 27, 2022
* Revert "Add OpenSSL 3 support (#19814)"

This reverts commit 2dcfd73.

* openssl 3 support no longer opt in + some 1.0 support

* hopefully fix

* maybe fix

* final attempt

* actual fix hopefully
@metagn metagn mentioned this pull request Nov 3, 2022
@hkl615
Copy link

hkl615 commented Nov 23, 2022

Hiiii

@hkl615
Copy link

hkl615 commented Nov 23, 2022

FedericoCeratto:openssl3

jangko pushed a commit to status-im/nim-ssz-serialization that referenced this pull request Jan 24, 2023
jangko pushed a commit to status-im/nim-stew that referenced this pull request Feb 1, 2023
* switch CI to the supported version of ubuntu

See actions/runner-images#6002

* don't continue on error for Nim 1.6

* install openssl on macos for Nim devel

due to changes in nim-lang/Nim#19814

---------

Co-authored-by: Jacek Sieka <jacek@status.im>
capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
* Minor refactor

* Add OpenSSL 3 support

Remove symbols noOpenSSLHacksq and openssl10

* Drop loading of older openssl versions

* Add library path

* Use only versioned libssl soname os OSX

* Update .github/workflows/ci_packages.yml

Co-authored-by: Hein Thant <official.heinthanth@gmail.com>

* On Mac OS X CI, link OpenSSL in /usr/local/lib/

* Install OpenSSL on Mac OS X on azure pipeline

* Remove DYLD_LIBRARY_PATH

Co-authored-by: Hein Thant <official.heinthanth@gmail.com>

Co-authored-by: Andreas Rumpf <rumpf_a@web.de>
Co-authored-by: Hein Thant <official.heinthanth@gmail.com>
capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
)

* conservative partial revert of nim-lang#19814

* fix

* revert tssl

* revert azure CI change

* keep azure, revert version range

* fully revert CI, add changelog

* useOpenssl3 as separate define, .3 is a version
capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
* Revert "Add OpenSSL 3 support (nim-lang#19814)"

This reverts commit 2dcfd73.

* openssl 3 support no longer opt in + some 1.0 support

* hopefully fix

* maybe fix

* final attempt

* actual fix hopefully
bung87 pushed a commit to bung87/Nim that referenced this pull request Jul 29, 2023
* Revert "Add OpenSSL 3 support (nim-lang#19814)"

This reverts commit 2dcfd73.

* openssl 3 support no longer opt in + some 1.0 support

* hopefully fix

* maybe fix

* final attempt

* actual fix hopefully
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support OpenSSL 3
7 participants