parallaxsecond / rust-cryptoki

Rust wrapper for the PKCS #11 API, Cryptoki
https://docs.rs/cryptoki/
Apache License 2.0
75 stars 61 forks source link

Bump bindgen to 0.69.4 #199

Closed hug-dev closed 8 months ago

hug-dev commented 9 months ago

Bumps bindgen in order to also bump shlex which version we currently use has a security vulnerability.

ionut-arm commented 9 months ago

Ok, I've ran regenerate_bindings.sh locally, it seems only the Loongarch build fails for some reason. I've created https://github.com/rust-lang/rust-bindgen/issues/2765 based on that.

Any chance you have the bandwidth to run that? Or if you give me push rights to your fork I can take care of it.

cc @heiher

heiher commented 9 months ago

Interesting. I can successfully cross generate these bindings locally, including LoongArch.

Steps:

# Install rust 1.76.0 (stable)
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh

git clone -b shlexed https://github.com/hug-dev/rust-cryptoki
# HEAD commit 9765405f7be9445aed972f24b41d5bb638bd12e8
cd rust-cryptoki
cargo build --features generate-bindings

cd cryptoki-sys
./regenerate_bindings.sh

Logs:

+ for target in $targets
+ rustup target list
+ grep -q 'loongarch64-unknown-linux-gnu (installed)'
+ rustup target install loongarch64-unknown-linux-gnu
info: downloading component 'rust-std' for 'loongarch64-unknown-linux-gnu'
info: installing component 'rust-std' for 'loongarch64-unknown-linux-gnu'
+ TARGET_INSTALLED=loongarch64-unknown-linux-gnu
+ cargo build --target loongarch64-unknown-linux-gnu --features generate-bindings
   Compiling cfg-if v1.0.0
   Compiling cryptoki-sys v0.1.7 (/home/hev/git/rust/rust-cryptoki/cryptoki-sys)
   Compiling libloading v0.7.4
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.46s
+ find ../target/loongarch64-unknown-linux-gnu/ -name pkcs11_bindings.rs
+ xargs -I '{}' cp '{}' src/bindings/loongarch64-unknown-linux-gnu.rs
+ '[' loongarch64-unknown-linux-gnu == loongarch64-unknown-linux-gnu ']'
+ rustup target remove loongarch64-unknown-linux-gnu
info: removing component 'rust-std' for 'loongarch64-unknown-linux-gnu'

Results:

diff --git a/cryptoki-sys/src/bindings/loongarch64-unknown-linux-gnu.rs b/cryptoki-sys/src/bindings/loongarch64-unknown-linux-gnu.rs
index 3854d02..81bdfa3 100644
--- a/cryptoki-sys/src/bindings/loongarch64-unknown-linux-gnu.rs
+++ b/cryptoki-sys/src/bindings/loongarch64-unknown-linux-gnu.rs
@@ -1,4 +1,4 @@
-/* automatically generated by rust-bindgen 0.66.1 */
+/* automatically generated by rust-bindgen 0.69.4 */

 pub const PKCS11_H: u32 = 1;
 pub const CRYPTOKI_VERSION_MAJOR: CK_BYTE = 2;
ionut-arm commented 9 months ago

Interesting. That means that the problem is either with the Rust toolchain I was using (an older version), or with cross-compilaton. I'll check again.

heiher commented 9 months ago

Interesting. That means that the problem is either with the Rust toolchain I was using (an older version), or with cross-compilaton. I'll check again.

Thanks. The LoongArch support start from Rust 1.71.0.

hug-dev commented 9 months ago

Oh sorry, forgot about that! I got the same error running the script than Ionut, running with Rust 1.76.0.

wiktor-k commented 9 months ago

For the record it works on my machine (as in: ./regenerate_bindings.sh succeeds, loongarch is in the script output). (I had to adjust the target dir though as I'm using custom CARGO_TARGET_DIR).

tgonzalezorlandoarm commented 9 months ago

Hi, thanks for the PR.

Parsec's, release (which is coming sometime around April) depends on rust-cryptoki's. For easily packaging distros, we need alignment between the different dependencies' versions: Each crate should be brought, ideally, with only one version. Parsec also depends on bindgen as a dependency, and currently we are planning on releasing with version 0.66.1:

From what I've noticed in our research on the shlex vulnerability for Parsec, the concerning APIs were not being used on our bindgen calls, and shlex was only being used for testing on bindgen elsewhere.

I will have to check for this repository, but I think the same may possibly apply here. If this is the case, I think there would be no urgency in upgrading bindgen to solve the shlex vulnerability as this one would not directly apply. I will confirm shortly if this is the case.

To align bingen versions between rust-cryptoki and parsec, it would be great if we could postpone this PR from being merged until parsec updates its bindgen version, which would happen after release.

Please let me know if this is okay or if there are any other questions/comments

hug-dev commented 9 months ago

Yes that is perfectly fine! I can close it for now and maybe replace it with an ignore flag on the cargo audit command so that the nightly is not failing anymore? I can do that once it is verified that the vulnerable shlex API are not used!

tgonzalezorlandoarm commented 9 months ago

I've proposed a way to update bindgen in Parsec as well before release and the team agreed, so this can be unblocked. Thanks a lot and sorry for the noise!

hug-dev commented 9 months ago

Can someone for who the script work out of the box take over this PR :pray: It became more work than I thought to make it cross-compile

ionut-arm commented 9 months ago

@hug-dev - yeah, no worries, I'll take it over

ionut-arm commented 9 months ago

@heiher - I've tried again with the latest Rust compiler. I think the issue is with cross-compiling, I'm doing so from x86_64. Should I update this PR with all other bindings, and let you open a PR afterwards updating the loongarch bindings?

heiher commented 9 months ago

@heiher - I've tried again with the latest Rust compiler. I think the issue is with cross-compiling, I'm doing so from x86_64. Should I update this PR with all other bindings, and let you open a PR afterwards updating the loongarch bindings?

I have also cross-compiled on x86_64 before. I'll try to reproduce it using Github Action.

heiher commented 9 months ago

@heiher - I've tried again with the latest Rust compiler. I think the issue is with cross-compiling, I'm doing so from x86_64. Should I update this PR with all other bindings, and let you open a PR afterwards updating the loongarch bindings?

Thanks for everything you have done for LoongArch.

The root cause is that the libclang of the Ubuntu distribution is too old and does not support LoongArch, but my locally ArchLinux does. I propose to install the snapshot version from LLVM offical to solve it:

https://github.com/heiher/rust-cryptoki/blob/main/.github/workflows/ci.yml#L15-L19

      - name: Setup clang
        run: |
          wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key | sudo apt-key add -
          sudo add-apt-repository -y 'deb http://apt.llvm.org/jammy/ llvm-toolchain-jammy main'
          sudo apt install libclang-dev

Action results: https://github.com/heiher/rust-cryptoki/actions

ionut-arm commented 9 months ago

Excellent, thank you for pointing that out, @heiher ! I've now updated all the bindings.

wiktor-k commented 9 months ago

The root cause is that the libclang of the Ubuntu distribution is too old and does not support LoongArch, but my locally ArchLinux does.

Ah, that explains it, since I also use Arch, btw 😏 😉

hug-dev commented 8 months ago

Thank you so much @ionut-arm !! Can we now approve and merge?