Skip to content

[KCC] implement destroy_key_pair FFI#634

Open
meetrajvala wants to merge 2 commits intogoogle:mainfrom
meetrajvala:kcc2des
Open

[KCC] implement destroy_key_pair FFI#634
meetrajvala wants to merge 2 commits intogoogle:mainfrom
meetrajvala:kcc2des

Conversation

@meetrajvala
Copy link
Contributor

No description provided.

Copy link
Collaborator

@NilanjanDaw NilanjanDaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this. A few early comments as you refine the implementation further.

/// Assumes `key_handle_ptr` is a valid pointer to 16 bytes.
#[unsafe(no_mangle)]
pub unsafe extern "C" fn key_manager_destroy_key(key_handle_ptr: *const u8) -> i32 {
let handle_bytes = slice::from_raw_parts(key_handle_ptr, 16);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me a bit nervous, probably a good idea to add some checks to see if the key_handle_ptr is valid?

}

pub fn destroy_key(&self, handle: KeyHandle) -> Result<(), Error> {
if self.binding_keys.destroy_key(handle).is_ok() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this delete function will work. This assumes the the Binding and KEM keys will have unique UUIDs. But that is not the case, each Binding Key, KEM Key pair will share the same UUID. We can probably pass a reference to the key-registry (binding_keys or kem_keys) to lookup as a parameter.

/// # Safety
/// Assumes `key_handle_ptr` is a valid pointer to 16 bytes.
#[unsafe(no_mangle)]
pub unsafe extern "C" fn key_manager_destroy_key(key_handle_ptr: *const u8) -> i32 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets have two separate FFIs, key_manager_destroy_kem_key and key_manager_destroy_binding_key to disambiguate the KeyRecord to delete the keys from.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants