Skip to content

Use v3.0 OASIS header files #156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

arjennienhuis
Copy link
Contributor

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

Builds on #154 but that isn't required.

The platform specific bindings are similar, except for:
- the `packed` attribute
- the layout_tests

By removing the tests and adding `cfg_attr` for `packed` you get a
binding that works cross platform.

Signed-off-by: Arjen Nienhuis <[email protected]>
Use the original OASIS 3.0 headers with a custom minimal platform.h
wrapper.

Signed-off-by: Arjen Nienhuis <[email protected]>
Signed-off-by: Arjen Nienhuis <[email protected]>
@wiktor-k
Copy link
Collaborator

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: #66 (comment)

(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
Copy link
Contributor Author

  • The headers seem to be perfectly backwards compatible
  • If needed I can try to make a PR with the original v2.4 headers
  • How does a windows runner even work? Is that docker? Does anyone have a pointer to a getting started guide?

@wiktor-k
Copy link
Collaborator

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
Copy link
Member

ionut-arm commented Jun 27, 2023

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
Copy link
Member

ionut-arm commented Jun 28, 2023

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: softhsm/SoftHSMv2#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
Copy link
Collaborator

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

@arjennienhuis
Copy link
Contributor Author

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

@ionut-arm
Copy link
Member

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
Copy link
Member

hug-dev commented Oct 14, 2024

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 Direktor799 mentioned this pull request Oct 18, 2024
@Direktor799
Copy link
Contributor

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

@wiktor-k
Copy link
Collaborator

Closing this since #228 got merged.

@wiktor-k wiktor-k closed this Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants