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

Support custom signing routines in SignaturePrivateKey #424

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

Conversation

rcombs
Copy link
Contributor

@rcombs rcombs commented May 23, 2024

This enables support for signing with non-extractable private keys (e.g. TPM-bound).

This enables support for signing with non-extractable private keys (e.g. TPM-bound).
@bifurcation
Copy link
Contributor

Hi @rcombs, thanks for submitting this. Just to confirm I understand the architecture correctly:

  • The caller would represent a private key as a SignerFunc, and then use SignaturePrivateKey::from_func to construct the object they provide to MLSpp
  • A private key constructed in this way would have its _sign_func member populated, but its priv_data empty.

I would prefer to handle this a little more systematically, in the sense of not having an alternative path for functions like this as opposed to other keys. For example, we could have signing always be a function, and supply a default function when constructing a key from bytes. In line with #425, it would also be good to push most of the complexity here into the hpke module. The structs in crypto.cpp are designed to be pretty thin wrappers around the crypto types.

Given those considerations, how about an approach something like the following:

  • Move the sign() method from Signature to Signature::PrivateKey
  • Change SignaturePrivateKey to hold a std::unique_ptr<Signature::PrivateKey> (Honestly I don't remember why we store the bytes right now. I think maybe pure laziness.)
  • ... and have the existing constructors build that pointer as appropriate
  • Then add a new constructor that directly provides a pointer to Signature::PrivateKey

From the caller's perspective, you would just from Signature::PrivateKey instead of implementing SignerFunc.

The main drawback that occurs to me is that we end up exposing what are currently some internal interfaces. But we're going to have to expose that sort of thing anyway if we're going to allow this sort of runtime adaptation.

@rcombs
Copy link
Contributor Author

rcombs commented May 23, 2024

Your understanding is correct, yes. I considered having all keys use a _sign_func, but it introduced some potential API issues given that data can currently be modified by the caller after construction, and a signer function would need to hold its own copy of the key data to function.

I went with this route largely to try to keep the API change as small as possible (this doesn't introduce any API incompatibility with existing code). The data field is currently public, and can be used by callers to e.g. store a generated key on disk. Removing it would mean an API break for callers, and we'd need to either add a method to retrieve the serialized form from Signature::PrivateKey or instruct callers to always use the JWK form (which could be a problem if anybody's currently storing the raw form). The same applies to the public_key field.

API-wise, I think the change proposed here should be compatible with any future architecture change moving more of the implementation down into the hpke module (since the only new API surface is from_func, which would be needed regardless).

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