Replace third party header with OASIS official #66

vkkoskie · 2021-11-14T15:23:50Z

Not quite ready for merge. Read each commit message, which explain some potential areas of concern.

I was unable to update bindings for two targets, so this will fail normal CI tests until that's corrected in some way.

closes #65
moots #64

vkkoskie · 2021-11-14T15:50:45Z

Hmm. CI is seg faulting. My guess is the struct packing, but I won't have time to dig into this again for a while. One of the things I was scratching my head about while making these changes was how anything was working before with unpacked structs. Updating the packing is technically independent of the header swap, but still needed.

mike-boquard · 2021-11-16T03:26:25Z

What prompted the change in 8226a25? I'm of the opinion we shouldn't touch the headers unless we have to.

EDIT: If we do decide to change the headers because of bindgen, I believe that a script should be provided that shows how (and why) these changes are made.

cryptoki-sys/oasis-headers/pkcs11.h

mike-boquard · 2021-11-16T03:31:42Z

cryptoki-sys/oasis-headers/pkcs11.h

+// Restore prior pack setting
+#pragma pack(pop)
+


I believe explicit struct packing is really only needed for MSC based compilers.

Not sure what you mean by this exactly. Are you saying that elsewhere, the default packing is what PKCS11 requires?

If so, then I'm not sure what to make of the bindings I generated. I was curious enough about that detail that I split the bindings updates into before- and after-pack commits to create a nice diff.

vkkoskie · 2021-11-16T22:26:08Z

What prompted the change in 8226a25? I'm of the opinion we shouldn't touch the headers unless we have to.

This was a byproduct of me misinterpreting this comment or maybe just trusting it too much. Much later, when I started messing with the build script, I realized that preprocessor definitions were being excluded because they needed to be explicitly added with allowlist_var.

I didn't revert at that time because I liked the fact that const definitions caused the types to be propagated up into the bindings. But Rust's type aliasing really does just make these synonyms with whatever's backing CK_ULONG and is really only functioning as a documentation method.

It's fairly weak justification to keep them if having the headers be as pure as possible is a goal. That purity wasn't present in the headers I was replacing, though, so I figured I had a bit more freedom to customize.

EDIT: If we do decide to change the headers because of bindgen, I believe that a script should be provided that shows how (and why) these changes are made.

Yeah, agreed this wasn't my best commit message.

vkkoskie · 2021-11-17T23:55:06Z

Okay, I basically started over, cherry-picking the first draft of commits as needed. The branch now touches the headers as little as possible. There are still some modifications to look at though.

And again, I've disabled Darwin and Windows support because I don't have those toolchains available, and they'd just fail. Not exactly sure what the best way forward is there.

The only struct packing left is the MSC stuff you provided, which doesn't get used on my end. I'm still very confused about when this does and doesn't matter. But I'm comfortable not making it a condition of merging because it wasn't present before and so you're no worse off keeping it as is.

vkkoskie · 2021-11-18T01:06:58Z

EDIT: If we do decide to change the headers because of bindgen, I believe that a script should be provided that shows how (and why) these changes are made.

I've now managed to get all but one of the changes needed from the base header implemented in build.rs at generation time. The last one is a disagreement that can crop up between bindgen's preprocessor and compiler passes that I don't see a clean way around. I think it's minor enough to leave as is though.

ionut-arm · 2021-11-21T19:45:04Z

Hey,

Finally got some time to look through this.

could you please expand the comment in eff8917 to link to the place where those header files were taken from?
@mjb3279 - after this is merged could you regenerate the Windows bindings? I'll try to do the same for Mac OS
One of the things I was scratching my head about while making these changes was how anything was working before with unpacked structs. - what do you mean? I thought bindgen takes care of this (or maybe I'm thinking of the wrong thing)

vkkoskie · 2021-11-22T13:59:50Z

Hey,

Finally got some time to look through this.

1. could you please expand the comment in [eff8917](https://github.com/parallaxsecond/rust-cryptoki/commit/eff891738d3c669030ec7d8a09a01bea1d2d0191) to link to the place where those header files were taken from?

Sure. Just update the commit message or drop a README in the oasis-headers directory? Or both? Also, is the licensing information at the top of the headers sufficient? I'm trying to parse their legal pages but am having trouble finding anything directed at derivative products instead of OASIS official notices.

2. @mjb3279 - after this is merged could you regenerate the Windows bindings? I'll try to do the same for Mac OS

If you place these in any commit I have access to, I'll be able to cherry-pick them into this PR pre-merge.

3. `One of the things I was scratching my head about while making these changes was how anything was working before with unpacked structs.` - what do you mean? I thought `bindgen` takes care of this (or maybe I'm thinking of the wrong thing)

It's the difference between #[repr(C)] and #[repr(C,packed)] in the generated bindings. It's been rebased out of the current PR, but if you look at 60a3a3f you can see the difference between the two. In particular, struct sizes shrink in many places. It also makes the tests seg fault. But the packed representation is an explicit feature of the standard because that's the only one with any guarantee of ABI consistency.

There's definitely more investigating to do, and I haven't looked at it much myself. But it can be treated separately from this PR, and should probably have its own tracking issue.

ionut-arm · 2021-11-26T16:20:33Z

Sorry for the very sluggish replies.

Sure. Just update the commit message or drop a README in the oasis-headers directory? Or both? Also, is the licensing information at the top of the headers sufficient? I'm trying to parse their legal pages but am having trouble finding anything directed at derivative products instead of OASIS official notices.

Just the commit message should be fine. As for the license... hmm, good point, I'll have to find out.

If you place these in any commit I have access to, I'll be able to cherry-pick them into this PR pre-merge.

Should be ok to just do it in separate PRs, no need to complicate this one.

It's the difference between #[repr(C)] and #[repr(C,packed)] in the generated bindings. It's been rebased out of the current PR, but if you look at 60a3a3f you can see the difference between the two. In particular, struct sizes shrink in many places. It also makes the tests seg fault. But the packed representation is an explicit feature of the standard because that's the only one with any guarantee of ABI consistency.

There's definitely more investigating to do, and I haven't looked at it much myself. But it can be treated separately from this PR, and should probably have its own tracking issue.

Ah, that makes sense. Ok, a new issue it is - should I create it? I think you have the details lined up more finely.

I'll have another read through the PR, though I need to figure out if there are any restrictions/barriers for us to include those header files. Maybe we can just pull them at build time if need be?

ionut-arm · 2021-12-03T10:25:43Z

Hi @vkkoskie !

I'm not sure if you managed to find more information on the issue regarding Oasis licenses.

I've queried internally and it seems the headers are indeed under Oasis IPR, however the IPR is incompatible with our Apache-2.0 license (as it requires limiting distribution to members of Oasis only), so the PR can't be merged in its current form. However, I still don't know if pulling them in at build time to generate the FFI code is any better. Do you happen to have any more insight into that?

The Oasis headers appear to be more of a spec in code form. ~~At the same time, I don't know which other headers we should be using for our goals...~~ It appears SoftHSM essentially did the inverse of this PR a few years ago, replacing the Oasis ones with the ones we currently use, from p11-kit, PR here (thank you @paulhowardarm for pointing that out)

vkkoskie · 2021-12-03T17:39:36Z

Interesting. I followed a few links down from yours to this page that seems to at least acknowledge that someone might actually want to use OASIS work product instead of just contribute to it.

That led me to search github to see of a repo existed, which does exist. (What's currently committed came from from OASIS's own domain.) The license there is boilerplate (as noted in its commit message) and links to other places in the OASIS legal docs that are also beyond the limits of my legal knowledge. My best guess is that the headers can be used under BSD-3-Clause, but I've filed an issue on that repo to get clarification.

ionut-arm · 2021-12-03T18:01:54Z

Thanks for following that up, it sounds promising! I'll subscribe to the issue you raised, let's see what they reply :)

mike-boquard · 2021-12-06T12:48:19Z

Hey everyone. Sorry for disappearing as of late. I normally get to work on this repo when there is a lull in work, however lately I've been absolutely swamped with work and family stuff. Hopefully early next year things will calm down and I can get back to contributing.

ionut-arm · 2021-12-06T13:10:24Z

Hey everyone. Sorry for disappearing as of late. I normally get to work on this repo when there is a lull in work, however lately I've been absolutely swamped with work and family stuff. Hopefully early next year things will calm down and I can get back to contributing.

I'm in pretty much the same situation, hope we're not slowing @vkkoskie down too much 😅

Header content provided by oasis-open.org, cloned from https://github.com/oasis-tcs/pkcs11.git and using content under the `2-40-errata-1` tag. Signed-off-by: Keith Koskie <[email protected]>

Signed-off-by: Keith Koskie <[email protected]>

This is a convenience for the sake of upcoming commits. It allows the cfg check macro to be defined on the module and not have to add it to every single item. It's unformatted to so whitespace doesn't mask the content diff. The next commit applies rust fmt. Signed-off-by: Keith Koskie <[email protected]>

Signed-off-by: Keith Koskie <[email protected]>

Multiple things happening here: * Add allowlist_var() to make bindgen include constants, which are excluded by default. * Set integer types for generated constants. Bindgen will parse types in C declarations, but ignores any kind of type information (e.g., a "UL" suffix) on integer literals defined as macros. This sets the two bool constants to unsigned bytes, and assumes everything else will be an unsigned long. * Filter macro names that don't need bindings (e.g., include guards), * Filter out deprecations. In most cases these have been replaced with an identical type under a different name, so no functionality is being omitted. * Filter out some additional alternate spellings that aren't explicitly deprecated. These get used in Rust enums, where having one value with two descriminant names is disallowed, and breaks pattern matching. Signed-off-by: Keith Koskie <[email protected]>

A weird edge case. Based on observed behavior, bindgen parses preprocessor constants as either i64 (implied by the int_macro callback) or to the width of the type returned by that callback (ulong) *on the generating platform*. This may not agree with the specified target, though. Regardless, from my 64-bit platform, this ~0 gets converted to u64::MAX as a decimal literal. Then bindgen creates a declaration with a type equivalent to the target's ulong. If that type is only 32-bits wide, the compiler reports the overflowing literal as an error. To correct this, change the preprocessor definition to its C declaration equivalent because bindgen properly uses the target's types for both parsing and generation. Signed-off-by: Keith Koskie <[email protected]>

Signed-off-by: Keith Koskie <[email protected]>

Not sure if these are indespensible. These mechanisms appear in v3.0 but aren't defined in v2.40 spec or headers. It seems easier to to me to add 3.0 *after* a relatively clean 2.40 implementation is in place providing a stable(-ish) surface to layer onto, but there may be more context to why this was already implemented that makes a different path more appropriate. Signed-off-by: Keith Koskie <[email protected]>

Signed-off-by: Keith Koskie <[email protected]>

Reverts parallaxsecond#13 See also: parallaxsecond#12 It appears that bindgen no longer generates the code that this suppression was written for. The resulting bindings are identical. Signed-off-by: Keith Koskie <[email protected]>

Signed-off-by: Keith Koskie <[email protected]>

jhagborgftx · 2022-12-05T20:22:25Z

It seems that structure packing should be limited to Windows. This is a known inaccuracy of the spec, and there is a proposal to change this in v3.2. This was accepted without objection according to these meeting minutes.

Even in older versions of the spec, it seems nobody was actually packing structures on Unix. So following the spec literally will cause ABI incompatibility with every existing PKCS#11 implementation.

wiktor-k · 2023-01-04T08:57:57Z

I propose we close this PR. It doesn't see much progress lately and there are parallel efforts to update the PKCS version in other PRs/issues. Also closing doesn't hurt since the code is still here and in case something happens we can re-open.

Thoughts?

ionut-arm · 2023-01-11T11:31:08Z

I propose we close this PR. It doesn't see much progress lately and there are parallel efforts to update the PKCS version in other PRs/issues. Also closing doesn't hurt since the code is still here and in case something happens we can re-open.

Thoughts?

Agree, it's gone somewhat stale, I'd be happy to close. If we need to revisit the topic, we can re-open or link to this one from a new PR.

vkkoskie mentioned this pull request Nov 14, 2021

Add a missing flag value used by TokenInfo #64

Closed

vkkoskie force-pushed the header-rework branch from 03e1ea6 to 961bff2 Compare November 14, 2021 15:41

mike-boquard reviewed Nov 16, 2021

View reviewed changes

cryptoki-sys/oasis-headers/pkcs11.h Outdated Show resolved Hide resolved

mike-boquard reviewed Nov 16, 2021

View reviewed changes

vkkoskie force-pushed the header-rework branch from 961bff2 to 61221b1 Compare November 17, 2021 22:32

vkkoskie force-pushed the header-rework branch from 61221b1 to 5d64aaa Compare November 18, 2021 00:57

vkkoskie force-pushed the header-rework branch from 5d64aaa to b7cf1fc Compare November 22, 2021 13:09

vkkoskie mentioned this pull request Dec 3, 2021

Pack structs in C bindings #70

Open

vkkoskie force-pushed the header-rework branch 2 times, most recently from 21458b9 to ffd636c Compare December 4, 2021 17:59

vkkoskie added 7 commits January 28, 2022 12:57

Initial commit of OASIS C headers

f243094

Header content provided by oasis-open.org, cloned from https://github.com/oasis-tcs/pkcs11.git and using content under the `2-40-errata-1` tag. Signed-off-by: Keith Koskie <[email protected]>

Use OASIS pkcs11 headers, wrapped by *nix macros

2a45671

Signed-off-by: Keith Koskie <[email protected]>

Cargo fmt

9e42c77

Signed-off-by: Keith Koskie <[email protected]>

Update bindings for all targets

0a771af

Signed-off-by: Keith Koskie <[email protected]>

vkkoskie added 7 commits January 28, 2022 13:02

Increment cryptoki-sys version (0.1.2 -> 0.2.0)

1aa58ea

Signed-off-by: Keith Koskie <[email protected]>

Increment crate dependency version

c01e799

Signed-off-by: Keith Koskie <[email protected]>

Minor naming updates from header swap

73d6fa9

Signed-off-by: Keith Koskie <[email protected]>

Remove use of deprecated attribute types

5f647cc

Signed-off-by: Keith Koskie <[email protected]>

Remove bindgen type exclusion

54ed4bb

Reverts parallaxsecond#13 See also: parallaxsecond#12 It appears that bindgen no longer generates the code that this suppression was written for. The resulting bindings are identical. Signed-off-by: Keith Koskie <[email protected]>

Pack to 1-byte alignment on all platforms

890429c

Signed-off-by: Keith Koskie <[email protected]>

vkkoskie force-pushed the header-rework branch from ffd636c to 890429c Compare January 28, 2022 18:05

vkkoskie mentioned this pull request Aug 23, 2022

Improvement to unreleased open_session change? #97

Closed

ionut-arm closed this Jan 11, 2023

wiktor-k mentioned this pull request Jun 23, 2023

Use v3.0 OASIS header files #156

Closed

wiktor-k mentioned this pull request Oct 14, 2024

feat: add HKDF mechanisms #227

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace third party header with OASIS official #66

Replace third party header with OASIS official #66

vkkoskie commented Nov 14, 2021

vkkoskie commented Nov 14, 2021

mike-boquard commented Nov 16, 2021 •

edited

Loading

mike-boquard Nov 16, 2021

vkkoskie Nov 16, 2021

vkkoskie commented Nov 16, 2021

vkkoskie commented Nov 17, 2021

vkkoskie commented Nov 18, 2021

ionut-arm commented Nov 21, 2021 •

edited

Loading

vkkoskie commented Nov 22, 2021

ionut-arm commented Nov 26, 2021

ionut-arm commented Dec 3, 2021 •

edited

Loading

vkkoskie commented Dec 3, 2021 •

edited

Loading

ionut-arm commented Dec 3, 2021 •

edited

Loading

mike-boquard commented Dec 6, 2021

ionut-arm commented Dec 6, 2021

jhagborgftx commented Dec 5, 2022

wiktor-k commented Jan 4, 2023

ionut-arm commented Jan 11, 2023

Replace third party header with OASIS official #66

Replace third party header with OASIS official #66

Conversation

vkkoskie commented Nov 14, 2021

vkkoskie commented Nov 14, 2021

mike-boquard commented Nov 16, 2021 • edited Loading

mike-boquard Nov 16, 2021

Choose a reason for hiding this comment

vkkoskie Nov 16, 2021

Choose a reason for hiding this comment

vkkoskie commented Nov 16, 2021

vkkoskie commented Nov 17, 2021

vkkoskie commented Nov 18, 2021

ionut-arm commented Nov 21, 2021 • edited Loading

vkkoskie commented Nov 22, 2021

ionut-arm commented Nov 26, 2021

ionut-arm commented Dec 3, 2021 • edited Loading

vkkoskie commented Dec 3, 2021 • edited Loading

ionut-arm commented Dec 3, 2021 • edited Loading

mike-boquard commented Dec 6, 2021

ionut-arm commented Dec 6, 2021

jhagborgftx commented Dec 5, 2022

wiktor-k commented Jan 4, 2023

ionut-arm commented Jan 11, 2023

mike-boquard commented Nov 16, 2021 •

edited

Loading

ionut-arm commented Nov 21, 2021 •

edited

Loading

ionut-arm commented Dec 3, 2021 •

edited

Loading

vkkoskie commented Dec 3, 2021 •

edited

Loading

ionut-arm commented Dec 3, 2021 •

edited

Loading