Handling key handle closing is noisy and error prone

[sbailey-arm]

As discussed in other PRs, having to deal with cleanup operations after a normal operation and returning any errors in the correct order is noisy and error prone.

E.g.:

let export_res = Status::from(unsafe {
        psa_crypto_sys::psa_export_public_key(
            handle,
            data.as_mut_ptr(),
            data.len(),
            &mut data_length,
        )
    })
    .to_result();
    key.close_handle(handle)?;
    export_res?;
    Ok(data_length)

(not sure if the errors are returned in the 'correct' order here!)

[sbailey-arm]

@ionut-arm has suggested:
Maybe we should really return a generic error when some of these "internal" methods fail, that the user should not really know about.

An alternative would be to make a "layered" function, where for each variable that needs cleaning up we wrap everything after it in a function that returns a Result. If the result is Ok, attempt to clean up and if that fails return an error, otherwise return the Ok you got. If what you got was Err, then do the clean up anyway and return the Err you got from the next layer function. If you have two things that you need to clean, unfortunately you'd have 2 "layer" functions. But the benefit is that you're sure that you clean up everything and you return the Err that's most appropriate.

[sbailey-arm]

I had the idea of using a 'black box' (? not sure if that's a very accurate name...!) that you pass the operation you want done to it with the key handle and it will do the operation and return Errors/Oks in the correct order.

Here's an implementation:

fn do_op(key_id: Id, op: &mut dyn FnMut(u16) -> Result<()>) -> Result<()> {
    let handle = key_id.handle()?;
    let op_result = op(handle);
    let close_handle_res = key_id.close_handle(handle);
    op_result?;
    close_handle_res
}

This is an example of using it:

pub fn sign_hash(
    key: Id,
    alg: AsymmetricSignature,
    hash: &[u8],
    signature: &mut [u8],
) -> Result<usize> {
    initialized()?;

    let mut signature_length = 0;

    let mut sign_op = |handle: u16| {
        Status::from(unsafe {
            psa_crypto_sys::psa_sign_hash(
                handle,
                alg.into(),
                hash.as_ptr(),
                hash.len(),
                signature.as_mut_ptr(),
                signature.len(),
                &mut signature_length,
            )
        }).to_result()
    };
    do_op(key, &mut sign_op)?;

    Ok(signature_length)
}

It's probably one of the more basic solutions but it does work! Thoughts?

[hug-dev]

I think at some point Mbed Crypto will align to PSA Crypto and remove the key handle to only kepe the key IDs, at which point hopefully we can simplify our code.

[hug-dev]

Looks like it started in part? cf Mbed-TLS/mbedtls#3547

[hug-dev]

Closing as we will use the openless version of Mbed Crypto hopefully soon.