Draft: SlotInfo refactor and reimplement flags types

[vkkoskie]

Not ready for merge. Posting an initial draft for discussion.

This branch intends to rework how CK_FLAGS are abstracted in the API. As a test case, the commits so far update only SlotInfo and its flags member. I wanted to get a sense of whether this was likely to be merged if I did the same for the remaining handful of flags types as well.

The commit messages walk through individual changes, but here are the highlights:

• Flag types (as integers) are made private. All describe some containing struct and their meaning can be exposed through that outer type.
• Flag types are mostly written by the provider and only read by the user. Make the API enforce read and/or write access accordingly and don't, for instance provide setters where they cant actually change the provider's state.
• Use generics to bind a flag set to its outer type and prevent meaningless flag combinations or aliases (e.g., CKF_A may coincidentally have the same value as CKF_Z, but comparing them or using them interchangeably shouldn't be possible).
• Change how Debug and Display are implemented for flag sets, removing a lot of the C-style naming.

These are all piled into one PR because they all change the same few types, but most of these can be broken out for acceptance or rejection independent of one another. I'd like to get and idea of which ones are welcome to make a roadmap for what to revert and what to duplicate on the other flags.

[mike-boquard]

Thanks for adding this. It's a nice message to provide to the user to set expectations appropriately.

[mike-boquard]

I really like these changes, and definitely think it's worthwhile to continue making these changes (I'm particularly interested in how SessionFlags will change).

Really appreciate the effort you're putting into this crate. As someone relatively new to Rust this has been a fun learning experience!

[vkkoskie]

I don't plan to touch SessionFlags until your other PR is merged. I don't want to deal with the merge conflicts.

[ionut-arm]

Hey! I had a brief skim through and it looks pretty good, both the code and documentation, happy to move in this direction.

I have only one worry (and this comes a bit late, given your previous large PR!) - we should try and either cram all these changes in one big release - when we do end up releasing - or pushing them onto a develop branch and cherry-pick only the important fixes otherwise. I'm just a bit cautious of making constant, large changes to the API, and how sustainable that is for users. (I'm sure you're aware of this, just wanted to reach some broad consensus on it and maybe flag it out in a new issue, tracking what changes we want to make before the next release, etc.)

[vkkoskie]

I don't know what your expected release schedule or plan is, so I can't really speak to that.

I bumped the version in my first PR to avoid conflicts for clients. They might have to use a stricter semver specifier to stick to 0.2.0, but that's a one-line change in their TOML. 0.x.y versions don't guarantee stability. If it's still a concern, and you want to reset main to the end of 0.2.0 and name the current branch tip to 0.3.x or something, I can adjust.

Just for context, I found my way here after being equally frustrated by the existing pkcs11 crate. I started to write my own abstraction layer and learned a lot about the standard in the process. I found this crate part way into that project and (after realizing it wasn't empty 😉) saw that it was far enough along that contributing here would be a better use of time than starting from scratch. My ultimate goal is to be a user of this crate, though.

Anyway, that false start filled me with all sorts of ideas and opinions about what a rust API would look like, and those came with me for better or for worse. I've been trying to work incrementally to get buy-in at each step and not spend a lot of time on something that will never get merged. But I've got a lot in my head I'm still interested in committing. I've been fairly surprised and how open to you've all been to big, breaking changes. I also took that as a clue that there weren't looking to release any time soon, and I had plenty of time to really pile on.

[mike-boquard]

@ionut-arm how closely do you want to tie releases of this crate to say, Parsec? I'm of the opinion that, even though the Parsec program did start this crate and the majority of the maintainers are Parsec people, this crate should set it's own release schedule. As far as I can tell, the contents of the crate at v0.2.0 (which was the previous release) is "good enough" for use by the Parsec program. I haven't seen any feature requests/additions needed thus far to support them.

If you suspect that there are imminent changes required for it, maybe we create a v0.3.x branch back before the API changes occurred and rebase main up to that point onto that branch.

[mike-boquard]

I've been fairly surprised and how open to you've all been to big, breaking changes

Speaking for myself, I'm using this crate as an opportunity to learn Rust. I have a lot of PKCS11 experience from my current job and thought this would be a good place for me to get my hands dirty. So far, everything I've seen you add has helped the crate and has taught me a lot about the language. So my point of view is keep the changes coming!

[vkkoskie]

I have a lot of PKCS11 experience

Slight change of topic. I'm looking at MechanismFlags right now. Where did the extra four CKF_EC_* flags come from and where is the additional EC and RSA content coming from? It's in either version of the standard I've been looking at.

[mike-boquard]

I'm not sure where this pkcs11.h file came from. Normally I base off of the three header files oasis provides.

Where did the extra four CKF_EC_* flags come from

Looks like we're missing CKF_EC_ECPARAMETERS and CKF_EC_F_2M.

where is the additional EC and RSA content coming from

What do you mean? This should be coming from mechanism spec

[vkkoskie]

🙁 It looks like I've been reading the wrong thing. I've been using this as a reference. Both are marked 2.40, but yours is a year younger and is "curr" vs "base". Can you help me understand what the differences are, so I don't make the same mistake with the 3.0 draft draft I've also been glancing at occasionally?

Between you with the standard and me we Rust, this should be a very educational experience for us both!

[mike-boquard]

Ah, that makes sense now. What I linked was v2.40 with errata 01. I fault oasis with not updating the version number. I believe it mostly clarified some inconsistencies.

Raises a good point though, which spec should we be using? This pkcs11.h file is missing some fields that are defined in the "current" v2.40 spec.

[vkkoskie]

I assume that that header, which has copyright info from somewhere else, was just for a quick start with bindgen and wasn't written for the purpose it's being used for now. An audit/refactor there is another item I've thought a bit about, but consider it a distant enough goal that I haven't put much more thought than that into it.

[mike-boquard]

Created #65 to track this.

[ionut-arm]

how closely do you want to tie releases of this crate to say, Parsec?

We don't want to tie them, though in the future we might ask for a release to happen so that we get some feature that was implemented in this crate into Parsec as well, but everyone could put in that sort of request for a feature that's been implemented but not released yet. So Parsec is just a user of this crate, but that's about it.

I've been fairly surprised and how open to you've all been to big, breaking changes.

Well, we took over this crate in order to make PKCS11 better/easier for Rust developers, so anything that seems like a step in that direction is welcome!

I also took that as a clue that there weren't looking to release any time soon, and I had plenty of time to really pile on.

Indeed, we're not planning to release soon, but it'd be nice to have some (rough) draft for a roadmap. Then we could point to that if someone (new to the project) comes to us for a new feature and a release because they need it somewhere.

[vkkoskie]

I'm particularly interested in how SessionFlags will change

I'm more or less done with the read-only flag sets (mechanism, slot, token). This leaves three other sets:

1. The CKF_DONT_BLOCK flag belongs to a set of size 1, and controls a feature that isn't implemented.
2. The SessionFlags are a set of 2, one of which only exists for backward compatibility and must be true. The flags are "writable", but only in the sense that they are arguments when opening a session. There's no way to modify them after that. They're readable in the normal sense (via a get_*_info method).
3. The library initialization flags are "writable" in the same way as the session flags: they're really just pooled boolean arguments. They aren't readable; even get_info doesn't expose them, returning zero always.

So I think my plan is:

• Ignore CKF_DONT_BLOCK until there's an implementation around it to consider.
• Provide the same read API for SessionFlags as the read-only flag sets (after Implemented new way of holding the context within the session #59 is resolved).
• Update the Session opening API to take boolean arguments in some form, hiding any association with the underlying flag type.
• Update the library initialization API in a similar way.

The last two items would be covered by a different PR(s), and I'm pausing work on this PR until session types aren't in potential conflict.

[mike-boquard]

Update the Session opening API to take boolean arguments in some form, hiding any association with the underlying flag type.

Probably just need a simple read_write: bool argument for session creation. The CKF_SERIAL_SESSION flag needs not be exposed to a user of this crate.

The library initialization flags are "writable" in the same way as the session flags: they're really just pooled boolean arguments. They aren't readable; even get_info doesn't expose them, returning zero always.

I feel that the locking API that is provided is something from antiquity and probably not really used in more recent implementations of Cryptoki (e.g. some libraries will just happily accept a NULL_PTR and do whatever they want).

If you look at section 5.4 of the spec, there's four cases spelled out for what the library should do depending upon the state of the supplied locking mechanisms and the flag CKF_OS_LOCKING_OK. For this implementation, we should only look at the first two (where the mutexes are not supplied) and provide a single bool os_locking_ok: bool.

CKF_LIBRARY_CANT_CREATE_OS_THREADS by my reading, seems to override the other cases. Should this flag also be considered?

[mike-boquard]

I still have eyes on this, but life is a little busy/chaotic at the moment. I hope to get through the rest of the changes by the end of the week.

[mike-boquard]

Would this be cleaner if we used an enum like:

enum SomethingTokenInfo
{
    Unavailable,
    Infinite,
    Defined(u64)
}

[vkkoskie]

I definitely considered something like this, and could still be talked into it. I ended up going with this one for a few reasons:

• At the time, I wasn't sure how widely used the two special constants were that make this necessary, and wasn't sold on creating a new type if it turned out to be just this spot. Taking a look at the standard, that does appear to be the case.
• Between the two options, if I later decide I was wrong and want to switch, it's marginally easier to replace standard types with custom ones than the other way around.
• The Option type comes with a lot of functionality attached for free like converting into a Result, and flatten() which could be demonstrated in a doc comment.
• Personal bias: This crate (really PCKS#11 itself) requires the use of a lot of jargon like "token" and "slot" and "object" that all have meaning specific to the standard. In that context, I'm a bit biased toward a smaller number of custom types for the sake of having the main ones easier to identify. I wouldn't necessarily feel the same way for other crates.
• Rust provides Option::None precisely to be a semantic equivalent to "unavailable" and avoid having to encode it into the type itself (a la a null pointer).

That last item seems to imply that an idomatic new type would look more like

enum Limit {
    EffectivelyInfinite,
    Defined(u64),
}

and then return an Option<Limit>. But when I looked at that, I thought, "That's just anotherOption but it does less." and talked myself out of it.

[mike-boquard]

what is there todo?

[vkkoskie]

Ah, that's to myself, and is just part of my personal workflow. It would have (will be) removed before marking the PR ready for merge. I leave these behind for myself as I work (not always with explanatory text if the context is enough) when I have something I want to return to but not get sidetracked by right then or even have to switch contexts to a separate document. Any time I think I've reached my intended goal, I'll do a sweep for them, and they only occasionally make it into WIP commits.

In this case, I'd like to replace these raw bytes with an existing, functional date-time type TBD. But that's a completely different PR. So this tag is just going to get pulled out into a better note elsewhere.

[mike-boquard]

I'm a little sad about b1a449d. I like having the ability to "pretty-print" the flag values, but I understand your reasoning.

[mike-boquard]

Also apologies about the comments on the outdated code, I was stepping through the commits reviewing them.

[ionut-arm]

DAMN.... I meant to merge the other one and ended up clicking on the wrong tab. Sorry everyone, I'll open a PR to revert this and you can take it over from there. Sorry again.

[vkkoskie]

I'm a little sad about b1a449d. I like having the ability to "pretty-print" the flag values, but I understand your reasoning.

This may be worth revisiting when the more fundamental changes stabilize. Until then, I'll note that most documentation will point you to "{:?}" which produces inlined debug output because it's the simplest and default option, but "{:#?}" will add pretty formatting to the debug impl still in place because it's defined structurally instead of flattened into a string.