Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposed API change: accept key material by reference #57

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

marlonbaeten
Copy link

Thank you for this useful library!

I noticed while using the library that the interface forces the caller to transfer memory ownership of the key material to an instance of the operation mode enum (OpModeS).

Since the PSK is also part of this data structure, one cannot reuse the OpModeS::AuthPsk value for multiple sessions and is thus forced to clone the private / public key for each PSK / OpModeS::AuthPsk instance.

The API can be changed to take a reference to the key material; see this PR as an example.

Would you consider such a change to the API? I could assist in updating examples and documentation.

@tweedegolf-marc
Copy link

I would also be curious to know if this was a deliberate design decision, especially since in the Auth and AuthPSK case, this forces a caller to make copies of secret key material; an application might want to keep those in a "zeroizing" data structure for some extra guardrails.

@rozbb
Copy link
Owner

rozbb commented Jul 3, 2024

Hi, thank you for this! I agree there is unnecessary cloning happening. IIRC the reason I made the API this way was purely convenience. I'm not super concerned with copying secret keys, since all of those types are zeroize-on-drop anyway.

Right now the only reason I could see to modify the API is to avoid an unnecessary clone of a public key. Given that public keys in post-quantum KEMs can be up to 1.5kB, this is slightly nontrivial. Not clear though if this really matters.

That said, the reasons against it are:

  1. This would be a breaking change, and
  2. It kinda breaks the agility code. The try_lift functions can no longer return an OpMode type because they're not owned anymore. I'm not sure if anyone was relying on this behavior tbh

Do you have a strong opinion here? Does the current API impede a use case of yours?

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.

3 participants