Account for OpenSSL library evolution #7172
Conversation
- account for documented buffer size openssl 1.1.x for error string (>=256 bytes) - use nullptr instead of NULL - fix/streamline null-checks
…fer in one place (for thread safety and code consistency reasons)
mcktr
added
area/api
|
If I remember correctly there was a problem with the Windows wizard which could not handle ECC certificates. |
|
Yes I think I read it the same. Possibly it's just that DSA implementation is to unimportant to justify efforts in coping with the setup wizard issue... |
|
No, it was about the bundled OpenSSL library within the msi package which is used to run Icinga 2 on as a Windows agent. This problem has been solved recently, or will be with 2.11 and the new build architecture. I would use everything from 1.1.x if I could, but unfortunately CentOS6 and Debian Jessie still provide 1.0.1. TlsStream::OnEvent is subject being removed with the network stack rewrite. That being said, thanks for the patch but I can only look into it after #7041 has been fully resolved. |
|
Aah ok, I didn't saw (and didn't searched) for details about what is the issue with windows. I just thought icinga2 runs with openssl independently of .NET so there shouldn't be any issue running it with a bundled DLL with current OpenSSL version. If You want to use all features of OpenSSL 1.1 You could simply bundle it in a similiar way You provide the more recent boost versions for the older systems? |
|
Bundling OpenSSL is something I'd like to avoid on Linux/Unix, this has severe security implications and also could mean that security departments disallow installing the agent or satellite with a bundled, non-vendor supported OpenSSL version. .NET is only used for the C# parts of the graphical setup wizard, everything else is pure C++ using the STL, Boost+OpenSSL as static libs and Visual Studio as compiler. In terms of the Windows problem, don't worry. This issue is from the time where issues lacked quite some quality, we're on the way to document such decisions more transparent. That's also the reason why I have directly commented here - without any actual help bringing this PR forward :-) One thing I can do though - @lippserd, remember the ECC cipher ticket discussion we had lately? Please put this onto our todo list :) |
|
Aah yes I forgot about security departments. But I think there are not too many features that are must have without working just automatically (like TLSv1.3). Or which ones You mean fall in that category? |
|
The thing is, I don't want to be responsible for security problems with OpenSSL. I also don't want to deal with CVEs which are created because a bundled library is affected. That's also the reason why not many libraries are put into If upstream vendor fixes OpenSSL, and prepares packages and delivers, that's a good thing. Also consider that the whole Icinga project is run by roughly 20 people, each overloaded multiple times with TODOs. The developer's dream is to use every new feature (like C++17) and don't care about dependencies or limitations. Enterprise environments proof otherwise. Even the Boost packages are only provided on platforms where they do not exist, with EPEL7 or SLES15 providing them upstream already. That being said, we always depend on the oldest supported distribution. For quite a long time, this was RHEL5, then SLES11 now it is RHEL6 and Debian Jessie where we're waiting for. It's been like this for 5 years now, and we cannot change it. Otherwise our users and customers will uninstall Icinga and use something else. Cheers, |
|
Some research, since I don't know all the OpenSSL changes and how they work.
https://www.openssl.org/docs/man1.0.2/man3/RSA_generate_key_ex.html |
|
Re-iterating the issues referenced here - yeah, this is a pretty fucked up situation with not merging something because of something else which got reverted later. I'm convinced that these human errors shouldn't happen with better project management in the future. I now fully understand the code origin and your points, thanks for taking them into account. I misunderstood the changes implied by this, and will test and merge this asap. Code cleanup for the TlsStream class happens when everything else is implemented anyways. Cheers, |
|
Yes probably better to check each issue/pr that referenced the pr on merge and possible revert... I also do not see any issue to merge these changes imediately as long as not many other branches exists that would need extensive rebasing or merging because of this. |
Analyzing an (at the end) unrelated built problem I encountered during build I found the #4635 which should have been fixed by #5574 but was dismissied because of #5555 (which was reverted later).
I do not understand the rationale behind #5555 and not why it was reverted, but I do not find any successor and I would like to resurvive the fix to avoid build warnings (and also fix the errbuf size to respect current requirements of OpenSSL lib).