parallaxsecond / rust-cryptoki

Rust wrapper for the PKCS #11 API, Cryptoki
https://docs.rs/cryptoki/
Apache License 2.0
77 stars 61 forks source link

Expose more fine-grained control over `find_objects` #106

Closed jhagborgftx closed 2 months ago

jhagborgftx commented 2 years ago

Currently, the only way to search for objects is through Session::find_objects, which returns a vector of all objects. If there may be a huge number of objects, it's desirable to be able to find only a bounded number of objects. This is possible with PKCS#11, but the functionality is not exposed by this crate.

Further, it may be desirable to iterate through objects incrementally, i.e. calling C_FindObjects multiple times, with some processing in between each call. Also, the user may care whether the library makes several small calls to C_FindObjects, or one large one, especially if the HSM is being accessed over a network.

I believe all of these needs may be met safely by the following API:

impl Session {
    // ...

    pub fn find_objects_init<'a>(&'a mut self, template: &[Attribute]) -> Result<FindObjects<'a>> {
        // call `C_FindObjectsInit`
    }
}

pub FindObjects<'a> {
    session: '&mut Session // exclusive borrow to ensure at most one object search per session
}

impl<'a> FindObjects<'a> {
    pub fn find_next(&self, object_count: usize) -> Result<Option<Vec<ObjectHandle>>> {
        // call `C_FindObjects` once, with a buffer of size `object_count`.
        // return Ok(Some(...)) if we found objects, or Ok(None) if the search is over
    }
}

impl Drop for FindObjects { ... } // call `C_FindObjectsFinal` on drop

Although find_next could return just Result<Vec<ObjectHandle>>, I think returning an Option is more ergonomic, since

I believe the current implementation of Session::find_objects, as well as any other versions we might want to make (e.g. Session::find_objects_bounded(template, object_count)), can be implemented in terms of this one.

I will make a PR with something like this soon, but first I would like to collect some feedback about the API.

jhagborgftx commented 2 years ago

One downside to the API above the mutable borrow means it is not possible to interleave calls to find_next with calls to other session operations. I think it's possible to recover that by adding a method session(&self) -> &Session to FindObjects. Since it only returns an & reference, it should be possible to do any other session operation except starting a new object search.

Also, I just realized that it would be necessary to change find_objects to take a mutable reference as well, to prevent someone from calling find_objects while another object search is ongoing. That would make this a breaking change.

jhagborgftx commented 2 years ago

Another downside is that we can't handle errors from C_FindObjectsFinal, since it's called from drop. An alternative that would allow this is to use some sort of type-state: when we start a search, take the session by value, and then when you end the search you get the session back by value (only after checking for errors). One could still borrow a &Session during the search, to perform non-search operations.

I would consider this much less ergonomic than the API above. I doubt anyone cares much about handling errors from C_FindObjectsFinal, since there's not much you can do besides closing the session and trying to open a new one.

wiktor-k commented 2 years ago

The use-case is good and I like your ideas. :+1:

While looking at your proposed API it makes a striking resemblance to the Iterator API. Just wondering, have you considered providing an interface like this? Of course then you'd have to return Option<Result<ObjectHandle>> (just like fallible iterators) and probably do some caching (so that FindObjects is not too frequently called).

PRs of course welcome! Sorry in advance for the delay in reviewing :bow:

jhagborgftx commented 2 years ago

On Thu, 2022-11-24 at 01:14 -0800, Wiktor Kwapisiewicz wrote:

While looking at your proposed API it makes a striking resemblance to the Iterator API. Just wondering, have you considered providing an interface like this? Of course then you'd have to return Option<Result> (just like fallible iterators) and probably do some caching (so that FindObjects is not too frequently called).

I considered something like that. For the sake of simplicity, I only wanted to introduce one API at a time. I'm pretty sure an iterator- like API could be built on top of this one, but not the other way around. As you said, you'd have to make decisions about caching and error handling. I'd like to do the minimum possible to build a safe but flexible API first.

-- James Hagborg

keldonin commented 2 months ago

I guess this issue can be closed now?

hug-dev commented 2 months ago

Yes, thanks a lot for your work @keldonin! @jhagborgftx, if you don't find what you want in what #223 brought, feel free to re-open :smile_cat: