lib, types: Implement string accessors for some padded fields

[woodruffw]

First of all, thanks a ton for your work on this! It's been invaluable.

This PR adds a library-private helper, str_from_blank_padded, and uses it to implement some convenience accessor functions for a few of the blank-padded fields in the PKCS#11 types.

It also implements the Display trait for CK_VERSION, using the common MAJOR.MINOR format.

Please let me know if there's anything else I can do!

[mheese]

@woodruffw First of all, thank you so much for your PR!! This is highly appreciated! 👍

And you are touching on a topic that I have avoided so far because I wanted to get the PKCS11 basics right before I deal with any convenience methods. Anyway, as you are already working on this, we should get that merged!

I have added one suggestion in a comment inline, however, more in general ... come to think of it, can you investigate if maybe implementing std::convert::Into (or probably even better std::convert::From) is even better? This way we wouldn't need any additional methods on CK_INFO and CK_SLOT_INFO and we could still access the properties just like simple strings when required.

Regarding your comment here ...

Please let me know if there's anything else I can do!

There is lots of things that could be done 🙂

Here are some suggestions, the low-hanging fruit:

• getting CI builds on Linux, macOS and Windows (there is Travis right now, but I have a PR going for github actions which will includ all 3 OSes - so I am actually working on this myself right now)
• finishing all unit tests: all PKCS11 functions should have tests which confirm low-level functionality
• adding code coverage information
• Rust docs and examples, etc.pp. - even reference docs are incomplete and there are no usable examples
• compatiblity matrix: for some time I wanted to start a compatibility matrix with HSMs that people are using this library with / have tested it with

and most importantly:

• helping design high-level API: this is what will make this library easy to use from Rust, currently the low-level API is supposed to be as close to C as possible. This helps to lock down the unsafe parts, and makes reasoning about this part of the code easier.

[woodruffw]

You're welcome! I'd be happy to help with those tasks (time permitting on my side 😄).

I have added one suggestion in a comment inline, however, more in general ... come to think of it, can you investigate if maybe implementing std::convert::Into (or probably even better std::convert::From) is even better? This way we wouldn't need any additional methods on CK_INFO and CK_SLOT_INFO and we could still access the properties just like simple strings when required.

Yeah, I considered this approach. I wasn't sure what to do about the multiple fields, though -- would std::convert::From<CK_SLOT_INFO> for String return the manufacturerID, the slotDescription, or both concatenated?

Alternatively, I think I could use the newtype pattern to strongly alias [CK_UTF8CHAR; N] and implement std::convert::From there. I haven't tried that yet, but I think that gets around the restrictions on external traits + types.

In either case, I completely agree that the additional methods are an eyesore and I'd prefer to remove them in this PR 🙂

[woodruffw]

Yeah, so I was hoping to do something like this:

pub struct PaddedString<'a, const N: usize>(pub &'a [CK_UTF8CHAR; N]);

impl std::convert::From<PaddedString<'_>> for String {
  fn from(field: PaddedString) -> String {
    str_from_blank_padded(field.0)
  }
}

And replace each use of [CK_UTF8CHAR; 32] or similar with PaddedString<32>.

But that requires const generics, which are still unstable.

I think the other option would be to newtype CK_UTF8CHAR itself. Will experiment with that.

[woodruffw]

Yeah, this doesn't work either:

/// an 8-bit UTF-8 character
pub struct CK_UTF8CHAR(CK_BYTE);
pub type CK_UTF8CHAR_PTR = *mut CK_UTF8CHAR;

impl std::convert::From<[CK_UTF8CHAR; 32]> for String {
  fn from(field: [CK_UTF8CHAR; 32]) -> String {
    // ...
  }
}

Since arrays are treated as always foreign, even when their members are local to the crate 😞

[woodruffw]

This works, but doesn't get us a transitive String::from:

pub struct PaddedString(pub String);

impl std::convert::From<PaddedString> for String {
  fn from(field: PaddedString) -> String {
    field.0
  }
}

// We'd need to duplicate or macro this for 16 and 64 as well.
impl std::convert::From<[CK_UTF8CHAR; 32]> for PaddedString {
  fn from(field: [CK_UTF8CHAR; 32]) -> PaddedString {
    PaddedString { 0: str_from_blank_padded(&field) }
  }
}

[mheese]

@woodruffw ah bummer :( ... but maybe the duplication route for 16 and 64 is the one which will provide us with the best experience at the moment. As long as it keeps a stable API which will convert well, we can "fix" this once Rust provides us with the toolkit for it.

What do you think? You have been experimenting with this more than me by now...

forgot to add:

Alternatively, I think I could use the newtype pattern to strongly alias [CK_UTF8CHAR; N] and implement std::convert::From there. I haven't tried that yet, but I think that gets around the restrictions on external traits + types.

This is sort of what I had in mind. I wouldn't do it on CK_INFO or CK_SLOT_INFO directly.

[woodruffw]

Yeah, I think the duplication route is probably the best for now -- it still requires a nested PaddedString::from on every String::from, but it avoids manual method implementations.

I have one more idea that I want to try: we might be able to use generic-array, which works with Rust stable (1.41+). I'll experiment with that a bit and see I can get it to work; if not then I think the double-wrapper is a decent compromise until const generics arrive 😄

[woodruffw]

This is sort of what I had in mind. I wouldn't do it on CK_INFO or CK_SLOT_INFO directly.

Yeah, that's what I figured 🙂. I tried that with the PaddedString impl in #26 (comment), but ran head-first into the const generics issue. Maybe I can do the duplication there, instead; will try.

[woodruffw]

Aha! This works:

#[derive(Debug, Copy, Clone, Default)]
pub struct PaddedString32(pub [CK_UTF8CHAR; 32]);

impl std::convert::From<PaddedString32> for String {
  fn from(field: PaddedString32) -> String {
    str_from_blank_padded(&field.0)
  }
}

So I can either macro or manually duplicate that for 16/32/64, and add a NOTE indicating that it should be refactored into a const generic when they become available 🙂.

[mheese]

@woodruffw cool! let's do the dupliction for now... If you look at all the places where CK_UTF8CHAR is used, it's pretty limited, and is also never returned as a slice anywhere. So they are in the fixed locations.

So I think we can live with this at the moment.

Can you also do the couple of places where CK_CHAR is used? It's the same as with the utf8 variant. Never returned as a slice, and just part of static structs.

Last but not least, maybe putting these helpers into a helper module within types might be a good idea. The general idea behind types.rs was to basically mirror pkcs11t.h.

[woodruffw]

Last but not least, maybe putting these helpers into a helper module within types might be a good idea. The general idea behind types.rs was to basically mirror pkcs11t.h.

Sounds good!

[woodruffw]

Done! We now have the types::padding module, which contains PaddedString{16,32,64} as newtypes for [CK_UTF8CHAR; N].

I'll add some additional assertions to the tests to make sure things pass through as expected 🙂

[mheese]

@woodruffw can you also add this one here while you are at it?

Can you also do the couple of places where CK_CHAR is used? It's the same as with the utf8 variant. Never returned as a slice, and just part of static structs.

[mheese]

ok, never mind ... let's merge this first! I'll do this and some minor changes afterwards myself.

This is good work! Thanks again!

[woodruffw]

Thanks a ton! Would it be possible to get a release cut for these changes (once you're done with your fixups)?

[mheese]

@woodruffw yes, I'm planning to do a 0.5.0 release soon (either today or in the next couple of days), there are some other breaking changes in master right now.

[mheese]

@woodruffw please check out this PR which I have just merged: #28

My worry right now with the change we made is that the arrays are now encapsulated in structs and that might have side-effects on the C representation of the types. I think it is fine and also the Windows build passed, but there are also not enough checks/assertions yet in the retrieved data from the info, slot_info, token_info objects.

[woodruffw]

Yeah -- I can work on some additional assertions for those structures.

From the language reference, it sounds like repr(C) (which you added) should be sufficient for most ABIs. repr(transparent) would also probably do the trick, but shouldn't be necessary.

[woodruffw]

It looks like Emscripten might be an ABI where repr(C) is insufficient, based on rust-lang/rust#41483. But that concerns a newtype that wraps a primitive, not an array like we do, so it may not apply.

[mheese]

@woodruffw awesome link!

I believe this is though exactly what we are looking for, no? repr(transparent) on the newtype

rust-lang/rfcs#1758 is merged as well and available.

[woodruffw]

I believe this is though exactly what we are looking for, no? repr(transparent) on the newtype

Yes, I think so. I'm still having a little trouble parsing the RFC to understand what underlying ABI it enforces, but this comment seems to suggest that it does what we want it to.

[mheese]

@woodruffw yes, that's exactly the comment on which I have based my conclusion as well! :D ... anyway, I changed it for now. I want to complete some more of the unit tests before I do another release. I already found a bug yesterday for a wrong assumption of mine on how to handle encrypt_final/decrypt_final.