latchset / kryoptic

a pkcs#11 software token written in Rust
GNU General Public License v3.0
10 stars 4 forks source link

Packaging template + architecture specific fixes #96

Closed Jakuje closed 3 weeks ago

Jakuje commented 1 month ago

Add packaging template + some more metadata to the cargo.

I tried to build this in copr, but it looks like Fedora repositories exploded so not all worked:

https://copr.fedorainfracloud.org/coprs/jjelen/kryoptic/

And where the repositories worked, we are failing on some architecture/size specific issues that are also fixed with this change.

Overall, having packing to build RPM package in copr on different architectures might be a good CI.

Jakuje commented 1 month ago

Ugh ... it wont work this way. I guess I will have to cast to ::c_char to handle arch specific issues ...

Jakuje commented 1 month ago

oh .. there is more ...

Jakuje commented 1 month ago

Do we care about i686 builds? We assume that CK_ULONG is u64 in many places in the code:

error[E0308]: mismatched types
   --> src/storage/json.rs:96:62
    |
96  |                         Some(n) => attribute::from_ulong(id, n),
    |                                    ---------------------     ^ expected `u32`, found `u64`
    |                                    |
    |                                    arguments to this function are incorrect
    |
note: function defined here
   --> src/attribute.rs:331:29
    |
287 |         pub fn $fn1(t: CK_ULONG, val: $rtype) -> Attribute {
    |                                  -----------
...
331 | conversion_from_type! {make from_ulong; from_type_ulong; from_string_ulong; from CK_ULONG; as NumType; via ulong_to_vec}
    |                             ^^^^^^^^^^
help: you can convert a `u64` to a `u32` and panic if the converted value doesn't fit
    |
96  |                         Some(n) => attribute::from_ulong(id, n.try_into().unwrap()),
    |                                                               ++++++++++++++++++++
simo5 commented 4 weeks ago

Do we care about i686 builds? We assume that CK_ULONG is u64

No, we do not care that much, but it would be nice to get it working if possible, just not as part of this PR, feel free to declare i686 not supported.

All values in the PKCS#11 API, although declared CK_ULONG, never carry values larger than u32::MAX [even casting to (unsigned)-1 for CK_UNAVAILABLE_INFORMATION ends up being compatible as it is all bits set to 1 regardless of size] exactly to be compatible with 32 bit systems in terms of valid values.

Jakuje commented 4 weeks ago

The last build works everywhere but on i386: https://copr.fedorainfracloud.org/coprs/jjelen/kryoptic/build/8027167/

There is something that looks like an issue of the bindgen (but only warning though):

warning: literal out of range for `u32`
 --> src/conformance/../pkcs11/bindings.rs:7:50
  |
7 | pub const CK_UNAVAILABLE_INFORMATION: CK_ULONG = 18446744073709551615;
  |                                                  ^^^^^^^^^^^^^^^^^^^^
  |
  = note: the literal `18446744073709551615` does not fit into the type `u32` whose range is `0..=4294967295`
  = note: `#[warn(overflowing_literals)]` on by default

The following

#define CK_UNAVAILABLE_INFORMATION ~0UL

is converted to

pub const CK_UNAVAILABLE_INFORMATION: CK_ULONG = 18446744073709551615;

which does not sound right. I will probably add it as a separate issue later.

Jakuje commented 4 weeks ago

The code now compiles, but some all tests fail on 32b arch so it still needs some love:

https://copr.fedorainfracloud.org/coprs/jjelen/kryoptic/build/8027499/

I am not sure if I will be able to finalize this, but I think the changes do not make it worse than it was before so marking as ready.

simo5 commented 3 weeks ago

I force updated your branch as I have merged another PR that caused conflicts