-
Notifications
You must be signed in to change notification settings - Fork 61
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
Info flags refactor #68
Conversation
This flag indicates a failure in a token self-test. See: PKCS#11 2.40, Table 6 Signed-off-by: Keith Koskie <[email protected]>
Signed-off-by: Keith Koskie <[email protected]>
Signed-off-by: Keith Koskie <[email protected]>
Give the SlotInfo struct its own member definitions instead of being a wrapper around CK_SLOT_INFO. This causes the conversion for each memmber to happen once on construction. Signed-off-by: Keith Koskie <[email protected]>
Using the From trait here is both more flexible and also a better semantic match for how SlotInfo is constructed. Signed-off-by: Keith Koskie <[email protected]>
These functions differ only in a boolean. Them being distinct is even less necessary since they've been accessed through parent module stubs. Signed-off-by: Keith Koskie <[email protected]>
Fixes parallaxsecond#34 with respect to slots (still open for mechanisms) Signed-off-by: Keith Koskie <[email protected]>
Flags being accumulated into an integer is an implementation detail that has no significant meaning to its consumers. Instead, expose the information from the flags as booleans. Signed-off-by: Keith Koskie <[email protected]>
Information from SlotFlags is now available through SlotInfo, the only type that uses it. It's also a read-only value. The PCKS#11 provider sets these flags to be read, but setting the flags on the client side at best has no effect, and at worst is misleading about how the slot can be properly interacted with. Signed-off-by: Keith Koskie <[email protected]>
This commit is the first of several with the end goal of removing Flags types from the public interface. It makes several design changes. * Type state information encoded into an integer as flags is now exposed publically as booleans only. * The boolean values are read and/or written through the structure that contains the flag integer value. They do not exist as distinct entities, but as features of their outer type. * Flags are now type-bound to their container. Even though the CK_FLAGS type still backs them all, flag sets associated with one structure cannot interact with flags from a different structure. * Operations on the generic flag type are restricted to those required to set, unset, toggle, and test the values as needed by their outer types. These operations are limited to avoid common errors while still having the terseness of binary oprations. * Debug implementation for flags that display the full contents of the flag set as through it were a 'Flags' struct of named booleans. * Display implementation for flags that display only the relevant attributes as a set of string labels. Signed-off-by: Keith Koskie <[email protected]>
This replaces the prior documentation of SlotInfo members with wording from the standard itself. It also adds a crate-level Conformance Notes to provide context about documentation that would otherise have the appearance of a guarantee issued by this crate. Signed-off-by: Keith Koskie <[email protected]>
Signed-off-by: Keith Koskie <[email protected]>
Signed-off-by: Keith Koskie <[email protected]>
Signed-off-by: Keith Koskie <[email protected]>
This take the three previously refactored info+flag types and moves them together. Before, the flag types had to reach into their parent module to bring their associated info type into scope, while the info type had to do the same in the other direction. While this is legal, the tighter coupling made it natural to place them together. This has a side effect that all FlagBit constants can be made private within the new files. slot/{mod.rs:SlotInfo,flag.rs:CkFlag<SlotInfo>} -> slot/slot_info.rs slot/{mod.rs:TokenInfo,flag.rs:CkFlag<TokenInfo>} -> slot/token_info.rs mechanism/{mod.rs:MechanismInfo,flags.rs} -> mechanism/mechanism_info.rs Signed-off-by: Keith Koskie <[email protected]>
Signed-off-by: Keith Koskie <[email protected]>
Function added to support unit testing Signed-off-by: Keith Koskie <[email protected]>
Signed-off-by: Keith Koskie <[email protected]>
Having a human-readable format for the flag types seemed like a good idea initially. They were more compact than the Debug equivalents and had the flexibility of rephrasing for clarity. However, the format for these outputs was somewhat arbitrary which makes them potentially unstable. But most importantly, they were for a private type and served no purpose within the crate. In order for them to be usable in client code, the Display trait would need to be implemented on the containing info type as well. Again, the arbitrariness of the output presents concerns about fragility in client code, and there's no obvious use case that Debug can't also serve. This commit will be left behind rather than squashed out in case it becomes useful to revert. Signed-off-by: Keith Koskie <[email protected]>
Signed-off-by: Keith Koskie <[email protected]>
Signed-off-by: Keith Koskie <[email protected]>
Designs considered and rejected for TokenInfo::utc_time() return type * Option<String> of 14 original non-pad characters. This option is simplest but is an annoyance to anyone who wants to do anything except print. It might even be annoying to those people too. * Option<String> using ISO 8601 formatting. A step up in usability since it's common enough that most external tools already know how to parse it. However, it also implies additional constraints and guarantees from the ISO standard that PKCS#11 doesn't. Even with doc warnings it felt like a bad idea knowing how little validation is happening behind the scenes. * Option<(u16,u8,...)> tuple with the six parsed numerical fields. Serves the goal of not having custom types beyond necessity, but it's just too big to get away with unnamed std types only. * An Option<chrono::DateTime> or similar. Like ISO, appears to provide guarantees that PCKS#11 doesn't. Also, it would require assuming a specific notion of valid value ranges (e.g., January is month 0 or 1) beyond the specification that could report conformant strings as invalid. The final choice was a simple new type with minimal integral members. The members are made public so there's no need to implement getters. This is fine because mutating the struct is meaningless; it's not used as an input anywhere. The ISO formatting remains minimally present in the Display trait implementation, which provides much of the convenience of that format while weakening association with the ISO standard by not being the type's primary representation in code. Signed-off-by: Keith Koskie <[email protected]>
In the initial implementation, any parsing error was silently converted to None, which indicates the token doesn't have a clock. Signed-off-by: Keith Koskie <[email protected]>
cc6b852
to
46a2691
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks nice 👍
} | ||
} | ||
|
||
#[doc(hidden)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why #[doc(hidden)]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's best to avoid any direct linkage between this crate and the sys crate, and this is one aspect of that. The reasoning is to avoid implying that a user application should mix the two or need to dip into the ffi for any reason.
What I'd like to do is have this trait be private, but the visibility rules for traits don't allow that. So hiding the docs is a weaker version of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea. Maybe we should apply it more broadly. OTOH if someone knows the low-level type they may be surprised the conversion is not listed in the docs 🤔
@ionut-arm what do you think about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apply it more broadly
This was my intent. I've started a Discussion page (#69) that might be a better place for topics like this so they don't disappear when the PR closes. This item would fall under the second-to-last bullet in the "as I go" list. No one seems to have commented on (noticed?) that page yet, but it has plenty of stuff that's debatable or subjective.
One of my hopes in putting it up was to sanity check any ideas hat might raise objections well before sinking any real time into them. So if there is disagreement, there might be a good place to move them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure about this - as in, not sure it's a (big) benefit. Don't think it's something that would bother or distract the average user, and it might actually be helpful if they go ahead an use the FFI for something that's not currently implemented in the abstracted crate, and then want to convert to use the result with the abstracted stuff. But then again, I don't think that would be the average user either. I'm happy with this going either way, though, I guess
35f92e4
to
ff4aa06
Compare
The CkFlags type was written under the assumption that it would be part of the public API and would benefit from being bound (via generics) to the struct that contained each flag group. But the final API ended up keeping flags types private. With full control against incorrect usage being internal to the crate, CkFlags offers no benefit over bitflags, which is well established for this use case. bitflags auto-implements the Debug trait for its structures, which led to the corresponding tests being updated or deleted. Signed-off-by: Keith Koskie <[email protected]>
In particular, it * removes a lot of unnecessary interaction with ffi types, * removes the flags from the public API, * flattens the contents of CK_INFO into the struct, * moves individual field conversions out of public getters into a one-time, struct-level conversion, and * makes conversion from CK_INFO fallible. Signed-off-by: Keith Koskie <[email protected]>
b60441e
to
6dacb62
Compare
@ionut-arm @wiktor-k @mjb3279 I believe this is ready for review and/or approval. There may be some unfinished threads (here or #63) but I'm done making new additions. |
To clarify, I'm done on this PR. There are actually some places I resisted the urge to edit beyond the scope of this PR, which is already fairly large. I think this is a good checkpoint to merge on and then start those changes on a fresh branch. |
This looks very nice now. Thanks 👍
I agree. The PR is quite sizeable at this point and already takes a moment to digest. For me personally the end of the year is always particularly hard so sorry in advance for the slow process here but I hope we can manage to get this merged sooner rather than later 😊 |
Hey! I'm starting to review this now, should be done by the end of the day, sorry for the delay! I think the (generic) advice here would be to try and split PRs into smaller chunks. I know that's kinda what the commits should help with, but it's somewhat difficult to review commit-by-commit, for a number of reasons... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, went through all of it. Left a few comments, but happy to approve otherwise.
Also, writing some (generic) comments here as I go through the commits as well:
- what’s the worry with stability around
Display
andDebug
? Might’ve missed it from a different conversation. I don’t think anyone should be usingDebug
and/orDisplay
in a way that requires stability, they’re just intended for printing out various values in developer/user friendly ways, it doesn’t come with any guarantees - as in, it’s not supposed to be a proper serialization mechanism. - to reiterate the point from the previous comment I left - please split out PRs into smaller chunks, it makes them a lot easier to review and get merged. the UtcTime part, for example, could be a separate PR. Having gone through all of it now, this could’ve easily been split into 4-5 separate PRs.
cb1f095
to
91321af
Compare
Signed-off-by: Keith Koskie <[email protected]>
Signed-off-by: Keith Koskie <[email protected]>
91321af
to
40810ee
Compare
A silly mistake on my part. The token may not have a clock, so returning an Option is appropriate. But that designation was improperly split between the conversion function and the getter for the token. This removes the option from the conversion into the getter. Signed-off-by: Keith Koskie <[email protected]>
Agree, but I try to program with Hyrum's Law front-of-mind. I wanted to provide something more useful than dumping a raw integer, but I also didn't want to invent a new, mostly arbitrary formatting scheme that would have to be remembered anywhere else that this was necessary. Using the existing tools from the language seemed like the best way to do this, and anything "extra" felt like it would create more problems than it would solve.
Yeah, sorry about that. I primarily experience git as a CLI tool and tend to use a graph view to understand change sets. So good individual commits can be reviewed in sequence, and the size of an MR doesn't usually matter. I'll try to keep in mind not everyone uses the same tooling. I think one thing I dislike about many small PRs is the history it (often) creates. To my knowledge, github doesn't yet support a semi-linear merge strategy. This is why you see me constantly rebasing within my PRs, and why I don't like to have more than one or two submitted at a time. If they all get merged at once from an assortment of common ancestors, it creates a rat's nest that future-me will hate to have to mentally untangle. |
This function was added in 5bb40c6 to support unit testing but does not need to be public. Because it's only used during testing, it triggers a dead code lint unless limited to cfg(test). Signed-off-by: Keith Koskie <[email protected]>
I think the problem with that is how the Github interface for this works (or doesn't work) - you don't get the same controls when reviewing one commit at a time. And another issue (for me, at least) is that, like in this PR, some commits "overwrite" (or build on) changes made by a previous one, so you need to have the whole PR in mind when trying to understand what's going on - that's the kind of situation where the size ends up mattering. Obviously, if every commit is entirely independent of the others then it's not a problem at all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the changes after my review: 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for being so patient with the review process @vkkoskie! :)
I added some minor things but I think this is already good to merge as is 👍
cryptoki/src/slot/token_info.rs
Outdated
/// Information about a token | ||
#[derive(Debug, Clone)] | ||
pub struct TokenInfo { | ||
label: String, // 32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice to expand the comment to say sth like "Up to/exactly 32 characters, space-padded".
None | ||
} else { | ||
// Must have cast for when ulong is 32 bits | ||
// Must have lint suppression when ulong is 64 bits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice comment. Also: it'd be nice if we had CI running 32-bit builds too :)
@@ -158,7 +157,7 @@ impl Pkcs11 { | |||
} | |||
|
|||
/// Open a new session with no callback set | |||
pub fn open_session_no_callback(&self, slot_id: Slot, flags: SessionFlags) -> Result<Session> { | |||
session_management::open_session_no_callback(self, slot_id, flags) | |||
pub fn open_session_no_callback(&self, slot_id: Slot, read_write: bool) -> Result<Session> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I'm not really too much into having boolean parameters since it's not obvious what open_session_no_callback(..., true)
does (an enum with READ_WRITE
or READ_ONLY
would be more... readable) but on the other hand it's commonly used and the IDE can offer hints.
Or even export two functions: open_readonly_session_no_callback
or open_readwrite_session_no_callback
...
Btw very nice to see you providing explicit reasoning in the commit message for SessionFlags refactor: 67e7589 👏 thanks!
Signed-off-by: Keith Koskie <[email protected]>
One more commit to fix the typos mentioned above. Anyone mind if I do one final rebase onto 0.3.0 before it gets merged? (Or, if you're merging, mind rebasing when you do?) |
We don't have |
I don't mind it either way. Before working on these Rust crates I would rebase everything religiously but here the simple merge strategy also works well (modulo when CI is green on the top commit and red when merged, hehe). For the record I'm for enabling additional merge options in this repo. It doesn't hurt :) |
I'm for as well. We can discuss in the community meeting starting in a few minutes. @vkkoskie - if you'd like to join you're more than welcome! It's a more generic, Parsec-related meeting, but we do discuss issues related to all the repos we host. |
Don't want to interrupt you @ionut-arm but if you can please enable and "Rebase and merge" it when you have time. Thanks! |
Yes, was about to confirm that we've agreed to enable that. It should be available now! |
Reopens #63 which was closed by an accidental merge (now reversed).
I renamed the branch to reflect its intent a little better, but made no content changes beyond some minor typos.
Threads that may still be live/relevant at the time of opening form prior PR:
Tangent about Date type which doesn't affect this PR, but may still be relevant generally.EDIT: I've just added aUtcTime
type which is much simpler than the one I'd originally conceived.