parallaxsecond / rust-cryptoki

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

Use v3.0 OASIS header files #156

Closed arjennienhuis closed 1 month ago

arjennienhuis commented 1 year ago

Use the original OASIS 3.0 headers with a custom minimal platform.h wrapper.

Builds on #154 but that isn't required.

wiktor-k commented 1 year ago

I agree with this change although I remember someone (@ionut-arm) had concerns about bumping version from v2.4 to v3.0. This PR seems to solve issues reported with the previous approach: https://github.com/parallaxsecond/rust-cryptoki/pull/66#issuecomment-1338119056

(Since Windows support has been improved I'm wondering if we could use windows runner to actually test it... not sure about the effort to set it all up but even without it it doesn't seem that risky to merge it and if it fails on a Windows box we could improve that then).

arjennienhuis commented 1 year ago
wiktor-k commented 1 year ago

I don't mind v3.0 but Parsec, that's a main client of this crate, may. Let's wait to see what Ionut has to say (to fast lane it we could ask in the Parsec slack instance but I don't think it's that time critical).

As for runners some high level doc is here: https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners

ionut-arm commented 1 year ago

Oh, thank you for the patch! I was hoping to get some time to do that reorg of the header files, just for version v2.40 of the spec.

I think I would still prefer if we had the option to support both v2 and v3 - if not at cryptoki-sys level, at least in cryptoki, via some feature flag. Given that the current implementation is all v2, it means we'd only need to cordon off the new v3 additions. To catch off any problematic features, we could just test everything new and have a CI job against v2.

Which is to say, I'm probably in favour of this PR (if v3 is indeed a superset of v2), just need to go through the code as well.

ionut-arm commented 1 year ago

Ugh, now I remembered why we didn't do this before: licensing. If you look at the license header for the Oasis header files, you'll see they're not licensed under Apache 2 or something like that, it's some Oasis policy that's entirely free. It seems we've had this discussion before and we've had to reject the change for this reason. Actually, SoftHSM has gone the other way because of this: https://github.com/opendnssec/SoftHSMv2/pull/412

One interesting thing to note is that the native-pkcs11 folks seem to use the Oasis headers, I'll check what's up with that.

EDIT: Here's another place where this is discussed. As far as I can tell, the native-pkcs11 folks use a different license file in the header folder. In any case, I'm wondering if us doing the same would be permissible. I'd hope so, since we don't care about distributing those files under Apache 2.0, we only need them to generate our bindings.

wiktor-k commented 1 year ago

Ough! That's bad! Yep, then definitely as it stands it cannot go through... :cry:

arjennienhuis commented 1 year ago

We don't need to include them at all. Just let generate-bindings.sh download them.

ionut-arm commented 1 year ago

Hmm, that does sound like a good solution! BUT, one thing to note is that this will change the semantics and functioning of the generate-bindings feature. It'll have to take a local path now to the header that will hold the bindings. I don't see a major issue with that, it also means it'll be more flexible for people who want to alter those headers in some way.

hug-dev commented 1 month ago

Hey! It would be nice to continue with this and add the v3.0 version of the headers since this is blocking another PR depending on that version. As @ionut-arm mentioned, could we use another source of the v3.0 headers which are not affected by the non-free approach? ie doing similar that multiple people here. I think this is a good source.

Direktor799 commented 1 month ago

raised another PR #228 since there's no progress for a long time

wiktor-k commented 1 month ago

Closing this since https://github.com/parallaxsecond/rust-cryptoki/pull/228 got merged.