Add OpenSSL #113
Add OpenSSL #113
Conversation
|
Thanks for working on this, there seems to be a fair bit of demand for it. However there is a policy issues to note: Wrapdb uses the exact unmodified release file provided by upstream. Thus a package called |
|
Should I rename it to Making this work with upstream OpenSSL is possible, but it is going to be very, very messy and I'd rather reuse the work done by Node.js. |
|
Hm, I will try to replace OpenSSL with upstream version and see how it goes. |
nazar-pc
force-pushed
the
openssl
branch
2 times, most recently
from
87344a6
to
9a72f8a
Compare
|
Yeah, I made it work with upstream OpenSSL and it got really messy (all of If this is acceptable and |
|
Split changes into 2 commits to ease review: one without auto-generated files and another with just generated files |
|
Nice. Though this now faces its own problem. Because OpenSSL is security critical, shipping prebuilt source files that come from "somewhere" is problematic. If downstream developers want to audit the full product (as they should and many do) then they need to go through all that code. Ideally those asm files would get regenerated during Meson reconfigure. That is probably a fair bit of work, though. Would it be possible to only support the asmless version for now and not provide any of those asm files? The generation logic can be added later if there is a demand for it. |
It is not necessarily as bad.
Ideally yes, but on top of being significant amount of work (though certainly doable), it is also a challenge for adoption since it will add Perl 5 as a dependency for build process for instance. Mediasoup wants to migrate away from GYP (which made me start this in the first place) and adding Perl to build dependencies for all users (especially on Windows) would be a challenge.
Certainly possible, but there are some simple generated header files even for non-asm cases as you can see here for example: https://github.com/nazar-pc/wrapdb/tree/6737e80d4aa76d651132fef4c926916437b354b3/subprojects/packagefiles/openssl/generated-config/archs/linux-x86_64/no-asm Also there is already an option in |
|
I haven't looked at this yet, but I was wondering if it was possible to not require perl5 as build-time dep, but then instead have an optional target that uses perl to regenerate/verify the produced/shipped build files in some way (which could be run on a CI for example). I agree we need confidence, but if perl is required for the build that'd render it useless for the purpose I wanted an openssl wrap for most (which is for gst-build on Windows). |
|
If perl is required then better use external-project module, a lot less work for us... IMHO the whole point here is to not require it. I'm not sure I'm comfortable with supporting a non trivial fork of the build system for such a core security module, it means we need to be reactive in updating the wrap for each security fixes... I feel we are understaffed for that unless @nazar-pc commits himself in doing that for the years to come. That said, it is super useful in ci or developer use-cases, but when a product that uses OpenSSL is distributed it probably should not rely on the wrap. Thinking out loud here, but a warning() in the meson.build could be needed? |
|
One way of not having a Perl dependency is to rewrite the scripts to Python. Which is some amount of work, yes, but it only needs to be done once. |
|
I have a suspicion you might underestimate a bit the involvement of Perl in OpenSSL. For instance, Perl is used as a templating language to conditionally generate platform-specific versions of assembly files like here: https://github.com/openssl/openssl/blob/ca001524971ccd595bc0e9843611e6784adfc981/crypto/aes/asm/aes-x86_64.pl I do not think it is feasible to do anything except actually using real Perl during build time in such cases or pre-generating all of such files (that this PR does, really smart workaround by Node.js authors). So it is not just the "external" build system that wraps everything together, it is inherent to OpenSSL implementation in general. |
I have done my fair share of converting old Perl code to Python. I'm not saying that it would be easy or that it would not take effort. Most definitely it would. But we have the advantage that WrapDB does not need to support all of OpenSSL. Starting with no ASM at all is already functional (if not as performant) as I understand it. Then adding x64_64, arm32, aarch64 and maybe x86 would get most use cases. People who care about the remaining platforms can then submit fixes to their arches if they find it useful. Trying chomp all of OpenSSL in a single bite would most definitely lead to failure, so let's not even try. |
|
That is fair, but I don't see how ASM support can be added without Perl at the moment, except if maintaining a significantly diverged fork. And no-ASM version leaves a lot of performance on the table, for mediasoup the difference in CPU usage with A healthier solution would be if upstream could include these pre-generated files in official tarballs, but I haven't seen too much enthusiasm in actively supporting other build systems from tickets in their repo. |
|
maybe far fetched, but... is it possible to build perl as subproject? Are there stripped down versions of perl that could be enough for openssl build system? |
|
Do I understand correctly that Perl is only needed to generate those files? If so, maybe we can make the generated files reproducible, and have a CI step that verifies that the checked-in files are the same as would be generated by the openssl Perl-based build system? Having looked at the openssl build system in detail (for painful-to-remember reasons), I agree with Nazar that it is not feasible to port it to Python. Even if you could spend the dozens / hundreds of hours needed to do that, keeping it in-sync with upstream changes would be too hard. I like this approach overall, but the auditability of the generateds file is something we need to fix. |
|
In the interest of getting something merged, maybe it makes sense to publish a non-ASM version first and then we can figure out how to fix the generated files issue? Is that possible? That would enable subproject use of openssl for development use at least, which is valuable. Generated headers are easier to audit than generated assembly. |
We could special-case OpenSSL in WrapDB CI script and generate those files when generating the patch tarball. That way we don't even have to check them in. |
|
Is there anything I can do here to move things forward somehow? |
Yes, like I said above, I think for a first iteration it would be ok to not generate assembly and only generate (and check-in) headers. I can do the review for those. The next step would be what Xavier said. |
|
I can do that. What about files that do not pass sanity check script, do you want me to move them somewhere else, like creating a directory in the root of the repo? |
|
What does |
|
Oh that's the wrapdb script. nvm. Let me take a look. |
|
@xclaesse should we add a per-project list of extensions that are permitted? In a file that's named something obvious like |
I don't mind having some specific exceptions, yes. Many wraps actually don't pass the sanity check already, the idea is we'll need to fix or whitelist when someone update them. |
|
Updated PR with just non-ASM header files |
|
What tradeoff? :D The current state of affairs is that it only works on *nix CI and via downloading the generated release archive, this is a pure win if now "some of the time it no longer fails with -Dwraps=openssl" |
|
Oh also we probably want to squash it all (while retaining the commit messages). |
|
I don't think any of the commit messages are useful for wrapdb history, they all express facts about that commit's difference from the previous unready commit which is only relevant to the PR process itself -- and IMO, force-pushing already shows the range-diff (or something close enough). Squashing is especially important, because some of those commits include approaches which were ultimately rejected, like checking in tons of generated files... one reason for which was "this bloats the repository quite a bit, all things considered". So this really needs to be properly squashed and the commit messages cleaned up before merging. |
|
I don't think incremental commit messages are important in this case as this is the initial implementation. You can squash everything during merge, such that PR will still have multiple commits for history, but there will be just one merged commit. Or I can squash and force-push if you prefer it. |
|
Yes please squash and force push to the PR. |
add separate list of permitted files just for OpenSSL wrap generate OpenSSL files during release archive creation
|
Are we just waiting for someone to click a button or is there something else left to be done? |
This adds OpenSSL 1.1.1k
Important notes:
meson setupif those aren't present alreadyFixes #109