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

Build of cryptoki v0.6.1 failing on Fedora 39+ #198

Closed nullr0ute closed 8 months ago

nullr0ute commented 9 months ago

I'm seeing the following error when building cryptoki v0.6.1 on Fedora, nothing in the commits since 0.6.1 appear to address this but I'm not sure.

   Compiling cryptoki v0.6.1 (/builddir/build/BUILD/cryptoki-0.6.1)
     Running `/usr/bin/rustc --crate-name cryptoki --edition=2018 src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -C embed-bitcode=no -C codegen-units=1 -C debuginfo=2 --cfg 'feature="generate-bindings"' --cfg 'feature="psa-crypto"' --cfg 'feature="psa-crypto-conversions"' --cfg 'feature="serde"' -C metadata=44bab49b06b82f38 -C extra-filename=-44bab49b06b82f38 --out-dir /builddir/build/BUILD/cryptoki-0.6.1/target/rpm/deps -L dependency=/builddir/build/BUILD/cryptoki-0.6.1/target/rpm/deps --extern bitflags=/builddir/build/BUILD/cryptoki-0.6.1/target/rpm/deps/libbitflags-6a2dfb57bd5a0567.rmeta --extern cryptoki_sys=/builddir/build/BUILD/cryptoki-0.6.1/target/rpm/deps/libcryptoki_sys-bf3a80c2574f0e1e.rmeta --extern libloading=/builddir/build/BUILD/cryptoki-0.6.1/target/rpm/deps/liblibloading-7bed22b62b3fab93.rmeta --extern log=/builddir/build/BUILD/cryptoki-0.6.1/target/rpm/deps/liblog-36a0428a1ae46d43.rmeta --extern paste=/builddir/build/BUILD/cryptoki-0.6.1/target/rpm/deps/libpaste-c4e0531ade32c89a.so --extern psa_crypto=/builddir/build/BUILD/cryptoki-0.6.1/target/rpm/deps/libpsa_crypto-32e5c548160cd902.rmeta --extern secrecy=/builddir/build/BUILD/cryptoki-0.6.1/target/rpm/deps/libsecrecy-6befe5a62119802e.rmeta -Copt-level=3 -Cdebuginfo=2 -Ccodegen-units=1 -Cstrip=none -Cforce-frame-pointers=yes -Clink-arg=-Wl,-z,relro -Clink-arg=-Wl,-z,now -Clink-arg=-specs=/usr/lib/rpm/redhat/redhat-package-notes --cap-lints=warn`
error[E0726]: implicit elided lifetime not allowed here
   --> src/mechanism/mod.rs:964:59
    |
964 | impl TryFrom<psa_crypto::types::algorithm::Algorithm> for Mechanism {
    |                                                           ^^^^^^^^^ expected lifetime parameter
    |
help: indicate the anonymous lifetime
    |
964 | impl TryFrom<psa_crypto::types::algorithm::Algorithm> for Mechanism<'_> {
    |                                                                    ++++
error[E0433]: failed to resolve: could not find `PkcsOaepSourceType` in `rsa`
   --> src/mechanism/mod.rs:995:34
    |
995 |                     source: rsa::PkcsOaepSourceType::DATA_SPECIFIED,
    |                                  ^^^^^^^^^^^^^^^^^^
    |                                  |
    |                                  could not find `PkcsOaepSourceType` in `rsa`
    |                                  help: a struct with a similar name exists: `PkcsOaepSource`
error: cannot construct `PkcsOaepParams<'_>` with struct literal syntax due to private fields
   --> src/mechanism/mod.rs:992:43
    |
992 |                 Ok(Mechanism::RsaPkcsOaep(rsa::PkcsOaepParams {
    |                                           ^^^^^^^^^^^^^^^^^^^
993 |                     hash_alg: Mechanism::try_from(Algorithm::from(hash_alg))?.mechanism_type(),
    |                     -------------------------------------------------------------------------- private field
994 |                     mgf: rsa::PkcsMgfType::from_psa_crypto_hash(hash_alg)?,
    |                     ------------------------------------------------------ private field
995 |                     source: rsa::PkcsOaepSourceType::DATA_SPECIFIED,
    |                     ----------------------------------------------- private field
996 |                     source_data: std::ptr::null(),
    |                     ----------------------------- private field
997 |                     source_data_len: 0.into(),
    |                     ------------------------- private field
    |
    = note: ... and other private field `_marker` that was not provided
Some errors have detailed explanations: E0433, E0726.
For more information about an error, try `rustc --explain E0433`.
error: could not compile `cryptoki` (lib) due to 3 previous errors
gowthamsk-arm commented 9 months ago

Thanks for reporting the issue @nullr0ute We will check this soon.

billatarm commented 9 months ago

@nullr0ute I cannot reproduce:

$ cargo --version
cargo 1.77.0-nightly (ac6bbb332 2023-12-26)

cat /etc/os-release 
NAME="Fedora Linux"
VERSION="39 (Workstation Edition)"
ID=fedora
VERSION_ID=39

git describe --always --dirty --tags 
cryptoki-0.6.1

Is it just my cargo version perhaps?

nullr0ute commented 9 months ago

The full build logs for building in the build system are here: https://koji.fedoraproject.org/koji/taskinfo?taskID=112631130

billatarm commented 9 months ago

@nullr0ute I just grabbed rawhide and built with the packaged cargo command it builds. However, it looks like you have a patch applied, is their a way to see that patch?

nullr0ute commented 9 months ago

I pushed my local changes to rawhide repo, no patches, should be able to repo from there with a "fedpkg scratch-build"

gowthamsk-arm commented 8 months ago

The cause of the failure is due to an update in the Cargo.lock file. I see in the build steps, + /usr/bin/rm -f Cargo.lock

The issue has been fixed by this commit on main.

@nullr0ute, just checking, are you specifically removing the Cargo.lock file for cryptoki or is it a standard process for packaging the crate?

billatarm commented 8 months ago

I cherry picked that patch back to 0.6.1, Their is still some build issues, apply this @nullr0ute and you should be good:

diff --git a/cryptoki/src/mechanism/mod.rs b/cryptoki/src/mechanism/mod.rs
index e968434..e7de375 100644
--- a/cryptoki/src/mechanism/mod.rs
+++ b/cryptoki/src/mechanism/mod.rs
@@ -18,7 +18,7 @@ use std::ops::Deref;
 use std::ptr::null_mut;

 pub use mechanism_info::MechanismInfo;
-use crate::mechanism::rsa::{PkcsOaepParams, PkcsOaepSource};
+use crate::mechanism::rsa::PkcsOaepParams;

 #[derive(Copy, Debug, Clone, PartialEq, Eq)]
 // transparent so that a vector of MechanismType should have the same layout than a vector of
@@ -997,6 +997,7 @@ impl TryFrom<psa_crypto::types::algorithm::Algorithm> for Mechanism<'_> {
         use psa_crypto::types::algorithm::{
             Algorithm, AsymmetricEncryption, AsymmetricSignature, Hash, SignHash,
         };
+        use crate::mechanism::rsa::PkcsOaepSource;

         match alg {
             Algorithm::Hash(Hash::Sha1) => Ok(Mechanism::Sha1),
nullr0ute commented 8 months ago

@nullr0ute, just checking, are you specifically removing the Cargo.lock file for cryptoki or is it a standard process for packaging the crate?

The Cargo.lock file isn't shipped in the crates from crates.io so I don't believe it's anything to do with the rpm process.

billatarm commented 8 months ago

@nullr0ute, just checking, are you specifically removing the Cargo.lock file for cryptoki or is it a standard process for packaging the crate?

The Cargo.lock file isn't shipped in the crates from crates.io so I don't believe it's anything to do with the rpm process.

Sorry, late to the party. IIUC, which is always suspect, Cargo.lock is only used in direct "development" builds of the crate for reproducible builds, ie it's intended to represent a state of a known successful build. Cargo.toml is used for resolution of supported dependencies for the package manager per the semver conventions. If the Cargo.toml says versions 1.0 up to 2.0 work for dependency X and the user has the crate X version 1.2 and the Cargo.lock is locked at 1.2.2, the dependency on X will be resolved with 1.2.

However, if the package contains binaries, they include the Cargo.lock file AFAIK, because of:

tgonzalezorlandoarm commented 8 months ago

There was a change in Guidance on Lockfiles: https://blog.rust-lang.org/2023/08/29/committing-lockfiles.html

"For years, the Cargo team has encouraged Rust developers to commit their Cargo.lock file for packages with binaries but not libraries. We now recommend people do what is best for their project."

billatarm commented 8 months ago

do what is best for their project

Thanks for confirming what I said :-p. That document is speaking for the dev tree, the language is around committing it into version control not wether cargo itself packages it into the crate. These are separate things. The crate may have the Cargo.lock included under the specific condition that the package contains binaries and then cargo install will only consume it if the --locked option is passed in, not by default.

Generally speaking Cargo.lock is for developers, Cargo.toml is for downstream consumers with the exception of crates containing binaries.

gowthamsk-arm commented 8 months ago

@nullr0ute will a new release from main (v0.7.0) suffice or do you need a (v0.6.2) with just the fix patch?

nullr0ute commented 8 months ago

@nullr0ute will a new release from main (v0.7.0) suffice or do you need a (v0.6.2) with just the fix patch?

Will the 0.7 release be compatible with the 0.6 series, or will things that depend on it need changes and releases too, if the later a 0.6.2 would be great as it will allow just building all the current parsec pieces against without major bumps :)

billatarm commented 8 months ago

@nullr0ute will a new release from main (v0.7.0) suffice or do you need a (v0.6.2) with just the fix patch?

Will the 0.7 release be compatible with the 0.6 series, or will things that depend on it need changes and releases too, if the later a 0.6.2 would be great as it will allow just building all the current parsec pieces against without major bumps :)

I vote for a 0.6.2, just because we already know the pain required to package it downstream.

gowthamsk-arm commented 8 months ago

Will the 0.7 release be compatible with the 0.6 series

There will be a breaking API change with 0.7.0

gowthamsk-arm commented 8 months ago

0.6.2 would be great as it will allow just building all the current parsec pieces against without major bumps :)

Just to understand things. Since Parsec is a binary crate and has a lock file, it will continue to use cryptoki v0.6.1 version (unless we update it) and build successfully isn't it? v0.6.2 would be mainly aimed at getting the cryptoki CI green. Can you please let me know if my understanding is correct here?

nullr0ute commented 8 months ago

Just to understand things. Since Parsec is a binary crate and has a lock file, it will continue to use cryptoki v0.6.1 version (unless we update it) and build successfully isn't it? v0.6.2 would be mainly aimed at getting the cryptoki CI green. Can you please let me know if my understanding is correct here?

So Fedora builds parsec itself, when we build we deal with the dependencies itself and it will use anything in the 0.6.x from what we specify so when we build it if we have a build for 0.6.2 it will consume that in the Fedora build system. Upstream would need a bump separately but with a API change a 0.6.2 would allow them to have the fix if necessary without bumping to 0.7 with an API change

gowthamsk-arm commented 8 months ago

Thanks for the explanation :) I will verify the parsec build with cryptoki v0.6.2 changes and then create a PR.

nullr0ute commented 8 months ago

What's the status of the release? Do we have an ETA.

gowthamsk-arm commented 8 months ago

I've created a PR for the required changes here https://github.com/parallaxsecond/rust-cryptoki/pull/202 Once this is merged will be publishing a new version soon.

gowthamsk-arm commented 8 months ago

@nullr0ute We have now published v0.6.2.

nullr0ute commented 8 months ago

Awesome, thanks! Testing now.

nullr0ute commented 8 months ago

Builds, looks good thanks!