Provide attribute type in return from get_attribute_info

[mike-boquard]

Currently get_attribute_info takes in a slice of AttributeType's and returns a Vec<AttributeInfo> ,however other than indexing into the slice and assuming the corresponding attribute information occupies the same index , the returned data does not correlate the attribute length to said attribute.

I can think of two solutions (but my limited Rust experience tells me there's probably better ways of doing this):

1. Modify AttributeInfo to contain the AttributeType by modifying the enum variants to be either:

pub enum AttributeInfo {
    Unavailable(AttributeType),
    Available(AttributeType, usize),
}

or

pub enum AttributeInfo {
    Unavailable { type: AttributeType},
    Available { type: AttributeType, size: usize },
}

2. Return a map which is keyed on the attribute, such as:

pub get_attribute_info(
&self,
object: ObjectHandle,
attributes: &[AttributeType],
) -> Result<HashMap<AttributeType, AttributeInfo>>

Furthermore, get_attribute_info does not return whether or not C_GetAttributeValue returns CKR_ATTRIBUTE_SENSITIVE or CKR_ATTRIBUTE_INVALID. Operationally, these may not be the most helpful returns (frankly the spec for C_GetAttributeValue leaves a lot to be desired when it comes to determining why an attribute is not returned) but I believe the wrapper library should not be masking away that information.

This part of the issue may warrant a separate ticket/discussion, but one possibility would be returning the CKR as part of a tuple (along with the attribute information) in the successful part of the Result enum.

[hug-dev]

Hey!

Back from holidays 🌴
Thanks for your contribution in the crate, that's really appreciated.

I thought that because the get_attribute_info keeps the order, it would be ok to check which attribute type maps to which attribute info but maybe it would be better to do it in the function already.

Something like this could be possible, having keys as the vector of AttributeType and values as the vector returned by get_attribute_info. If needed we could do the same with your 2 option above. The only thing I see with that is that you would have to allocate more data but that might be fine as AttributeType is Copy and small in size.

Furthermore, get_attribute_info does not return whether or not C_GetAttributeValue returns CKR_ATTRIBUTE_SENSITIVE or CKR_ATTRIBUTE_INVALID.

Good point! In the current implementation we make no difference if the attribute is unavailable because sensitive/unextractable or because invalid. We could add one more variant to the AttributeInfo enum with Sensitive for example?

[mike-boquard]

Welcome back! Sorry for the delay in the response. Been busy with life and work and such, but I appreciate the response.

I thought that because the get_attribute_info keeps the order, it would be ok to check which attribute type maps to which attribute info but maybe it would be better to do it in the function already.

That is a fair point. My thinking behind this is if you have a long list of attributes you want the information of, it would feel a little "kludgy" to figure out the index into the vector of attributes and corresponding that to the index location of the AttributeInfo vector that is returned.

The only thing I see with that is that you would have to allocate more data

Maybe we could pass in a mutable reference to a HashMap<AttributeType,AttributeInfo> and populate that within the function?

We could add one more variant to the AttributeInfo enum with Sensitive for example?

Unfortunately, you don't know which attribute is "sensitive" from the return of C_GetAttributeValue. All you're told is one of the requested attributes is either sensitive or invalid.

For example, if you attempt to get the value of CKA_PRIVATE_EXPONENT and CKA_EC_POINT from an RSA private key whose attribute CKA_SENSITIVE is CK_TRUE, the ulValueLen field of both entries will be CK_UNAVAILABLE_INFORMATION (as CKA_PRIVATE_EXPONENT would be considered sensitive and CKA_EC_POINT is not a valid attribute for an RSA private key) and the return from C_GetAttributeValue would be either CKR_ATTRIBUTE_TYPE_INVALID or CKR_ATTRIBUTE_SENSITIVE. The spec doesn't specify which return code takes precedence.

However, in my experience, a lot of applications that use the spec tend to call C_GetAttributeValue one attribute at a time due to this inability to know what attribute is sensitive/invalid or to get the size or an attribute if it is unknown.

I think it may make sense to return a tuple on the success end that contains the return code?

[hug-dev]

I tried and one could use the following to create a HashMap out of the current method:

    let attribute_types = [
        AttributeType::Token,
        AttributeType::Private,
        AttributeType::Modulus,
        AttributeType::KeyType,
        AttributeType::Verify,
    ];
    let attribute_info = session.get_attribute_info(object, &attribute_types)?;

    let hash = attribute_types
        .iter()
        .zip(attribute_info.iter())
        .collect::<HashMap<_, _>>();

otherwise I am fine to just return the HashMap from this function. The allocation should be very small.

Thanks for the explanation on the second problem, I see what you mean now.

So, if I'm correct, currently you get AttributeInfo::Unavailable so you know if your attribute was either INVALID or SENSITIVE but you can't use this function with only one attribute type to know exactly which one of the two it was.

I think it may make sense to return a tuple on the success end that contains the return code?

Could work but does not seem ideal to add something that can only be useful in some particular cases (when calling the function with just one attribute type) 😕. What I can propose as well is to add one more method only taking one single attribute type and returning all possible error messages? That way people can use that to check attribute by attribute. We should also add the limitation in the original method.

[mike-boquard]

Maybe we can add in two functions. A get_attribute_info_map that returns the map and a get_single_attribute that does what you described.

And I can put in a comment to get_attribute_info with that sample code as a helper and note that something returning Unavailable could be sensitive or invalid.

[mike-boquard]

Maybe we can add in two functions. A get_attribute_info_map that returns the map

This is probably over-kill. I'll just make a comment similar to what you wrote out above.

Thanks for the input (and reminding me about zip()!)

[hug-dev]

I don't mind about adding a wrapper which return the map. As for the get_single_attribute, it's probably a good idea in case someone needs to know :)