Note: As this is a bundled spec, I uploaded the vendor tarball to a public server to help automatic fedora-review process. Spec has commented the Source entry that should be used and also the steps to generate it.

Copr build:
https://copr.fedorainfracloud.org/coprs/build/6885806
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2257948-bpfman/fedora-rawhide-x86_64/06885806-bpfman/fedora-review/review.txt

Found issues:

- No gcc, gcc-c++ or clang found in BuildRequires
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/

Please know that there can be false-positives.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Package looks good in general, but the way the crate dependencies are handled is not good right now.

1. In general, using vendored sources is strongly discouraged (in general, and also for Rust packages). I have looked at the contents of the vendor tarball, and it looks like almost all dependencies are already packaged for Fedora. Do you have a list of crate dependencies that are missing from Fedora? If it is not too long, I would *very much* prefer to not use vendored dependencies here (except for the git snapshot of aya that is apparently necessary). I (and other Rust SIG members) can help with packaging and / or reviewing missing dependencies.

2. *If* it turns out that using *all* vendored dependencies is indeed necessary (see Rust Packaging Guidelines for replacing Git snapshots with path-based dependencies), you will need to do at least *some* auditing of the contents of the vendor tarball.

I can tell just by looking at the list of vendored crates that there are is least some content that is not acceptable for redistributing in Fedora (most importantly, cryptography algorithm implementations that are not legally allowed in Fedora in the fiat-crypto crate).

You can also use automation to generate the list of bundled crates with the `%cargo_vendor_manifest` macro and adding that file as a `%license` file, there is no need to list the bundled crates manually anymore. In fact, you are already using that, so there is no need to list "Provides: bundled(crate(...))" at all, since it is redundant with the Provides generator.

Thanks for reviewing Fabio!

I tried %cargo_generate_buildrequires against bpfman workspace only and the following deps would be missing:

Problem 1: nothing provides requested (crate(aya) >= 0.11.0 with crate(aya) < 0.12.0~)
 Problem 2: nothing provides requested (crate(comfy-table) >= 7.1.0 with crate(comfy-table) < 8.0.0~)
 Problem 3: nothing provides requested (crate(comfy-table/tty) >= 7.1.0 with crate(comfy-table/tty) < 8.0.0~)
 Problem 4: nothing provides requested (crate(netlink-packet-route) >= 0.17.1 with crate(netlink-packet-route) < 0.18.0~)
 Problem 5: nothing provides requested (crate(oci-distribution) >= 0.9.0 with crate(oci-distribution) < 0.10.0~)
 Problem 6: nothing provides requested (crate(oci-distribution/rustls-tls) >= 0.9.0 with crate(oci-distribution/rustls-tls) < 0.10.0~)
 Problem 7: nothing provides requested (crate(oci-distribution/trust-dns) >= 0.9.0 with crate(oci-distribution/trust-dns) < 0.10.0~)
 Problem 8: nothing provides requested (crate(prost) >= 0.12.3 with crate(prost) < 0.13.0~)
 Problem 9: nothing provides requested (crate(prost-types) >= 0.12.3 with crate(prost-types) < 0.13.0~)
 Problem 10: nothing provides requested (crate(prost/prost-derive) >= 0.12.3 with crate(prost/prost-derive) < 0.13.0~)
 Problem 11: nothing provides requested (crate(prost/std) >= 0.12.3 with crate(prost/std) < 0.13.0~)
 Problem 12: nothing provides requested (crate(rtnetlink) >= 0.13.1 with crate(rtnetlink) < 0.14.0~)
 Problem 13: nothing provides requested (crate(rtnetlink/tokio_socket) >= 0.13.1 with crate(rtnetlink/tokio_socket) < 0.14.0~)
 Problem 14: nothing provides requested (crate(sigstore) >= 0.7.2 with crate(sigstore) < 0.8.0~)
 Problem 15: nothing provides requested (crate(sigstore/cached-client) >= 0.7.2 with crate(sigstore/cached-client) < 0.8.0~)
 Problem 16: nothing provides requested (crate(sigstore/cosign-rustls-tls) >= 0.7.2 with crate(sigstore/cosign-rustls-tls) < 0.8.0~)
 Problem 17: nothing provides requested (crate(sigstore/tuf) >= 0.7.2 with crate(sigstore/tuf) < 0.8.0~)
 Problem 18: nothing provides requested (crate(sled) >= 0.34.7 with crate(sled) < 0.35.0~)
 Problem 19: nothing provides requested (crate(systemd-journal-logger) >= 1.0.0 with crate(systemd-journal-logger) < 2.0.0~)
 Problem 20: nothing provides requested (crate(tonic) >= 0.10.2 with crate(tonic) < 0.11.0~)
 Problem 21: nothing provides requested (crate(tonic/codegen) >= 0.10.2 with crate(tonic/codegen) < 0.11.0~)
 Problem 22: nothing provides requested (crate(tonic/prost) >= 0.10.2 with crate(tonic/prost) < 0.11.0~)
 Problem 23: nothing provides requested (crate(tonic/transport) >= 0.10.2 with crate(tonic/transport) < 0.11.0~)

I can try to build them to see how feasible having bpfman unbundled would be, but things like oci-distribution at least in go-sig it's a huge challenge.

Taking a look at the missing dependencies:

- aya: appears to require a git snapshot according to the comments in the spec file, cannot be un-bundled
- comfy-table: packaged for Fedora, just too old (v4 vs. v7)
- netlink-packet-route: packaged for Fedora, but too new (v0.18 vs. v0.17)
- oci-distribution: not packaged for Fedora (does not look too bad to add it)
- prost / prost-types: packaged for Fedora, but too old (v0.12 vs v0.10)
- rtnetlink: packaged for Fedora, but too new (v0.14 vs v0.13)
- sigstore: not packaged for Fedora (does not look too bad to add it)
- sled: not packaged for Fedora (looks like we already have all its dependencies though)
- systemd-journal-logger: not packaged for Fedora (we already have all its dependencies)
- tonic: not packaged for Fedora (I *think* we have all its dependencies)

====================

To sum it up, these are missing:

- oci-distribution
- sigstore
- sled
- systemd-journal-logger
- tonic

These are too new in Fedora, you might be able to patch bpfman to use the new versions or request that this happens upstream:

- netlink-packet-route
- rtnetlink

These are too old in Fedora, I should be able to update them soon in Fedora if necessary:

- comfy-table
- prost / prost-types

So that would mean (estimated) ~10-15 new packages for missing dependencies, plus updates for some existing packages.

====================

If you want to get bpfman into Fedora quickly, I do not want to block on building against vendored dependencies as it is done right now.
However, in that case, I would like to work on getting the missing pieces in so it could stop using vendored dependences at some point in the future.

What do you think?

I like your idea and if we can have bpfman vendored now we can work during February to have all deps in place.

That sounds good to me, however you will still need to verify that the vendor tarball contains only permissible content: see point 2 in my previous comment https://bugzilla.redhat.com/show_bug.cgi?id=2257948#c3

This will likely require at least some amount of modifying and / or patching the sources of some vendored dependencies, similar to what we do in the Fedora package for the fiat-crypto crate. I have not done a complete audit of the vendor tarball though, so there is likely other stuff that would need to be cleaned up for legal reasons.

(In reply to Fabio Valentini from comment #5)
> 
> - netlink-packet-route
> - rtnetlink
> 

Updated bpfman to match these dependencies, so this wouldn't be an issue anymore

(In reply to Fabio Valentini from comment #7)
> That sounds good to me, however you will still need to verify that the
> vendor tarball contains only permissible content: see point 2 in my previous
> comment https://bugzilla.redhat.com/show_bug.cgi?id=2257948#c3
> 
> This will likely require at least some amount of modifying and / or patching
> the sources of some vendored dependencies, similar to what we do in the
> Fedora package for the fiat-crypto crate. I have not done a complete audit
> of the vendor tarball though, so there is likely other stuff that would need
> to be cleaned up for legal reasons.

Thanks, that make sense, we'll take a look at that crate but, are there any specific guidelines for this audit to be done?

> Thanks, that make sense, we'll take a look at that crate but, are there any specific guidelines for this audit to be done?

In principle, the same source audit that should be done for every package review:

- Are there any pre-built binaries that are used during the build process? If yes, can they be replaced and / or rebuilt from source?
- Are there any contents that are under licenses that would prohibit it from being distributed by Fedora?
- Are there any implementations of cryptographic algorithms that Fedora is not allowed to distribute?
- etc.

Hi @fab, I've checked mainly re: implementations of algorithms with some dedicated folks (thanks @cclang) and it should be fine.

Can we move forward with the bundled version? 

We'll still commit to provide the unbundled and switch to it but as you can see we're waiting on some packages and iterating on a few others and the list keeps growing. I'd like to unblock this while keeping the work, is there any issue from your side? Thanks! Pointing to specifics we're waiting on:

# Updated on bpfman

- netlink-packet-route: packaged for Fedora, but too new (v0.18 vs. v0.17) -> updated in bpfman, should be ok now
- rtnetlink: packaged for Fedora, but too new (v0.14 vs v0.13)

# Waiting to be updated on Fedora
- comfy-table: packaged for Fedora, just too old (v4 vs. v7)


- aya: cannot be un-bundled, trying to get a new release so we can package it instead of git
- sled: https://bugzilla.redhat.com/show_bug.cgi?id=2259675
  PR open to sled -> https://github.com/spacejam/sled/pull/1500 updates rand_distr to 0.4.3
  PR open to sled -> https://github.com/spacejam/sled/pull/1501 updates zerocopyu to 0.7


# These brings a lot of dependencies, with several dependency tree levels and somehow blocked. Can provide exact details if you'd like to discuss.

- sigstore: not packaged for Fedora
- oci-distribution: not packaged for Fedora (does not look too bad to add it)
- systemd-journal-logger: not packaged for Fedora 

# We discussed about these ones and you mentioned that they'll be tackled later from rust-sig
- prost / prost-types: packaged for Fedora, but too old (v0.12 vs v0.10)
- tonic: not packaged for Fedora (I *think* we have all its dependencies)

As a follow up, taking for example sigstore gets us to:

A few of those are in review and I already provided  rpm and upstream fixes for a few of those such as
 Problem 1: nothing provides requested (crate(cached/async) >= 0.46.0 with crate(cached/async) < 0.47.0~)   - In Review https://bugzilla.redhat.com/show_bug.cgi?id=2259225
 Problem 2: nothing provides requested (crate(cached/default) >= 0.46.0 with crate(cached/default) < 0.47.0~) - In Review https://bugzilla.redhat.com/show_bug.cgi?id=2259225
 Problem 3: nothing provides requested (crate(crypto_secretbox/default) >= 0.1.1 with crate(crypto_secretbox/default) < 0.2.0~) - In Review https://bugzilla.redhat.com/show_bug.cgi?id=2259330

Pending or with different versions in Fedora 
 Problem 4: nothing provides requested (crate(docker_credential/default) >= 1.1.0 with crate(docker_credential/default) < 2.0.0~)

Initial build, pending from some update https://bugzilla.redhat.com/show_bug.cgi?id=1981119 
 Problem 5: nothing provides requested (crate(ed25519-dalek/default) >= 2.0.0~rc.2 with crate(ed25519-dalek/default) < 3.0.0~)
 Problem 6: nothing provides requested (crate(ed25519-dalek/pkcs8) >= 2.0.0~rc.2 with crate(ed25519-dalek/pkcs8) < 3.0.0~)
 Problem 7: nothing provides requested (crate(ed25519-dalek/rand_core) >= 2.0.0~rc.2 with crate(ed25519-dalek/rand_core) < 3.0.0~)
 Problem 8: nothing provides requested (crate(ed25519/alloc) >= 2.2.1 with crate(ed25519/alloc) < 3.0.0~)
 Problem 9: nothing provides requested (crate(ed25519/default) >= 2.2.1 with crate(ed25519/default) < 3.0.0~)

 Not in Fedora
 Problem 10: nothing provides requested (crate(oci-distribution) >= 0.10.0 with crate(oci-distribution) < 0.11.0~)
 Problem 11: nothing provides requested (crate(oci-distribution/native-tls) >= 0.10.0 with crate(oci-distribution/native-tls) < 0.11.0~)
 Problem 12: nothing provides requested (crate(olpc-cjson/default) >= 0.1.0 with crate(olpc-cjson/default) < 0.2.0~)
 Problem 13: nothing provides requested (crate(openidconnect) >= 3.0.0 with crate(openidconnect) < 4.0.0~)
 Problem 14: nothing provides requested (crate(openidconnect/native-tls) >= 3.0.0 with crate(openidconnect/native-tls) < 4.0.0~)
 Problem 15: nothing provides requested (crate(openidconnect/reqwest) >= 3.0.0 with crate(openidconnect/reqwest) < 4.0.0~)
 Problem 16: nothing provides requested (crate(p256/default) >= 0.13.2 with crate(p256/default) < 0.14.0~)

Packaged in Fedora but different version  https://src.fedoraproject.org/rpms/rust-rsa
 Problem 17: nothing provides requested (crate(rsa/default) >= 0.9.2 with crate(rsa/default) < 0.10.0~)
 
Not in Fedora 
 Problem 18: nothing provides requested (crate(rustls-pki-types/default) >= 1.0.0 with crate(rustls-pki-types/default) < 2.0.0~)
 Problem 19: nothing provides requested (crate(rustls-pki-types/std) >= 1.0.0 with crate(rustls-pki-types/std) < 2.0.0~)
 Problem 20: nothing provides requested (crate(rustls-webpki/alloc) >= 0.102.0 with crate(rustls-webpki/alloc) < 0.103.0~)
 Problem 21: nothing provides requested (crate(rustls-webpki/default) >= 0.102.0 with crate(rustls-webpki/default) < 0.103.0~)

 Not in Fedora and would need LICENSE files fix
 Problem 22: nothing provides requested (crate(testcontainers/default) >= 0.15.0 with crate(testcontainers/default) < 0.16.0~)

 Not in Fedora
 Problem 23: nothing provides requested (crate(tough/default) >= 0.14.0 with crate(tough/default) < 0.15.0~) 
 Problem 24: nothing provides requested (crate(tough/http) >= 0.14.0 with crate(tough/http) < 0.15.0~)
 Problem 25: nothing provides requested (crate(x509-cert/default) >= 0.2.2 with crate(x509-cert/default) < 0.3.0~)
 Problem 26: nothing provides requested (crate(x509-cert/pem) >= 0.2.2 with crate(x509-cert/pem) < 0.3.0~)
 Problem 27: nothing provides requested (crate(x509-cert/std) >= 0.2.2 with crate(x509-cert/std) < 0.3.0~)

I looked through your dependency tree from a cryptographic point of view, and most of it is fine.

A lot of the cryptography is implemented in rust and thus does not use one of the implementations we validate according to various standards on RHEL. This means that, for example, a system running in FIPS mode or a CommonCriteria-certified system should not make security assumptions about bpfman (i.e., whatever signatures it verifies should not be considered trusted). That's not an issue for Fedora, though.

As for specific questionable algorithms that are in your vendored source tree:

- You already identified p434 in the fiat-crypto crate. See [1,2,3] for reference, background discussion and patches. Maybe you can not bundle fiat-crypto and use the version from Fedora?
- cruve25519-dalek contains an implementation of the Ristretto group [4,5]. I'm not aware of prior legal review of that group, so maybe we shouldn't include it.

I'm also somewhat generally concerned about side-channel vulnerabilities in those cryptographic implementations. We test the common implementations on RHEL and Fedora, but not those Rust ones. We already know that some of those crates are vulnerable to side-channel attacks [6,7].


My recommendation would thus be to drop support for the p343 and Ristretto curves, and tread carefully when it comes to some of the other libraries with respect to side-channel issues.


[1]: https://lists.fedoraproject.org/archives/list/legal%40lists.fedoraproject.org/thread/FBZU2X7ZKTK2BVZKBHFUCI44SMY4UQCE/
[2]: https://src.fedoraproject.org/rpms/rust-fiat-crypto/blob/rawhide/f/gen_clean_tarball.sh
[3]: https://fedoraproject.org/wiki/Legal:ECC
[4]: https://github.com/dalek-cryptography/curve25519-dalek/blob/curve25519-4.1.1/curve25519-dalek/src/ristretto.rs
[5]: https://ristretto.group/ristretto.html
[6]: https://github.com/RustCrypto/RSA/issues/19
[7]: https://github.com/RustCrypto/RSA/blob/master/src/algorithms/pkcs1v15.rs#L59-L63

I'm working through my backlog and will come back to this review ASAP.

Regarding the previous comment:

> Maybe you can not bundle fiat-crypto and use the version from Fedora?

The way cargo treats dependencies is more or less an all-or-nothing approach when it comes to vendored dependencies. Either you use all dependencies from the system registry, or you use all dependencies from the vendor directory. In most cases, it's not possible to do something in the middle. So I'm pretty sure using vendored dependencies for most things and using the system dependencies for *some* things is not feasible.

(In reply to Mikel Olasagasti Uranga from comment #0)
> Spec URL: https://mikel.olasagasti.info/tmp/fedora/bpfman.spec
> SRPM URL:
> https://mikel.olasagasti.info/tmp/fedora/bpfman-0.4.0-0.1.20240110gitc6d1da2.
> fc39.src.rpm
> Description: bpfman operates as an eBPF manager, focusing on simplifying the
> deployment and administration of eBPF programs.
> Fedora Account System Username: mikelo2

I've already reached out to you, but we'd need to get this updated. If you don't have the bandwidth to do so, would you mind dropping the issue so I either take over or submit a new one? There's been (and will be) a few changes in the deps, updating a few and moving from sigstore to native-tls. Thanks

superseded by https://bugzilla.redhat.com/show_bug.cgi?id=2269411 in order to be able to take this over from Mikel

Updated and resubmitted due to lack of response.

*** This bug has been marked as a duplicate of bug 2269411 ***