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

Fix broken build with NSS crypto back end #2074

Merged
merged 8 commits into from
Mar 12, 2024
Merged

Conversation

jan-cerny
Copy link
Member

@jan-cerny jan-cerny commented Jan 23, 2024

This PR will help us investigate possible alternatives to using GCrypt library in our project.

OpenSCAP had 2 alternatives of crypto libraries, GCrypt and NSS. GCrypt is the default back end.

We discovered that building with the NSS back end is broken at this moment.

This patch fixes the broken build. Then, it adds missing parts of code for the NSS implementation. Finally, it consolidates the duplicate code and simplifies the code. This simplification will make future changes easier, which will also help us with the investigation.

Use cmake -DWITH_CRYPTO=nss .. to configure with NSS. The crypto features are covered by tests probes/filehash58/test_probes_filehash58.sh and API/crypt/test_api_crypt.sh.

Related to: https://issues.redhat.com/browse/RHEL-22013

@evgenyz
Copy link
Contributor

evgenyz commented Feb 1, 2024

Can you please rebase the PR to include all CI fixes.

@evgenyz evgenyz self-assigned this Feb 1, 2024
OpenSCAP had 2 alternatives of crypto libraries, GCrypt and NSS.
GCrypt is the default backend. We discovered that building with
the NSS backend is broken at this moment. This patch fixes the
broken build. Use `cmake -DWITH_CRYPTO=nss ..` to configure
with NSS.
This commit adds an alternative NSS implementation of functions
from the `crapi/sha2.h` module. These functions were implemented
only in the gcrypt version, and the implementation in the NSS
variant was missing.
Simplify the 2 alternatives, merge them to a single one in case
the code is the same in both alternatives.
Remove duplicate code in files in the `crapi` directory. Unify
functions behind a single iface.
@jan-cerny
Copy link
Member Author

I have rebased this PR on the top of the latest upstream maint-1.3 branch.

@evgenyz
Copy link
Contributor

evgenyz commented Feb 6, 2024

Okay, so how about you'll also change the default GCrypt to NSS in this PR to run our CI with it (packit included)? Later we can contain it to Fedoras (just like we did with PCRE2) before merging (or just Rawhide, maybe, given we merge it at all, but we must be sure it is not broken anyway before making this descision).

@jan-cerny
Copy link
Member Author

@evgenyz Now it builds and tests on Linux, but not on OS X and Windows. What to do next?

@evgenyz
Copy link
Contributor

evgenyz commented Feb 7, 2024

We can pause for now. There is a decision to be made: use NSS everywhere and potentially drop GCrypt at some point or switch to OpenSSL.

I you have any insights that can sway this decision in any direction update the ticket.

@jan-cerny
Copy link
Member Author

OK. Let's keep this open until we investigate and decide.

But regarding dropping the Gcrypt, I think we can't drop it from upstream and I also think that we shouldn't change the default crypto library in the maint-1.3 branch. I think doing these things would break the compatibility of the maint-1.3 branch with 1.3.0 release that we keep there. I think that upstream we can only extend the CI and update the spec, but we shouldn't change the defaults in maint-1.3.

@evgenyz
Copy link
Contributor

evgenyz commented Feb 9, 2024

You're correct about defaults, but there is a nuance: we need either OpenSSL or NSS to be the bakend-in-use for openscap-1.3 in Fedora ELN at least. Probably it makes sense to do that for Rawhide or all Fedoras (in our upstream spec).

@jan-cerny jan-cerny added this to the 1.3.10 milestone Mar 11, 2024
@evgenyz
Copy link
Contributor

evgenyz commented Mar 11, 2024

Okay, so where we are going to gate this backend? Fedora in GH Actions? Fedoras in Packit? All Fedoras? New F Rawhide GH Action?

@jan-cerny
Copy link
Member Author

@evgenyz I think the easiest way would be to create a new job in GitHub actions using Fedora. Would you prefer to have that in this PR or in a separate PR.

@evgenyz
Copy link
Contributor

evgenyz commented Mar 12, 2024

@evgenyz I think the easiest way would be to create a new job in GitHub actions using Fedora. Would you prefer to have that in this PR or in a separate PR.

I don't mind it being a separate PR. Just link it here for posterity.

@evgenyz evgenyz merged commit 430dfe4 into OpenSCAP:maint-1.3 Mar 12, 2024
20 checks passed
jan-cerny added a commit to jan-cerny/openscap that referenced this pull request Mar 12, 2024
This commit adds a GitHub Actions CI gating job that runs on
Fedora and verifies build with NSS as a crypto backend.
Related to: OpenSCAP#2074
@jan-cerny
Copy link
Member Author

#2092

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

Successfully merging this pull request may close these issues.

2 participants