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

Add serialization of private key material (KeyExporter) #211

Open
KendallWeihe opened this issue May 15, 2024 · 4 comments
Open

Add serialization of private key material (KeyExporter) #211

KendallWeihe opened this issue May 15, 2024 · 4 comments

Comments

@KendallWeihe
Copy link
Contributor

Requirement

Enable re-instantiation of BearerDid instances from serialized private key material, which implies we must support serializing private key material.

Use Cases

Test Suites

We want to rely on deterministic test vectors (composed of static data, some of which is encrypted or signed by private key material) which are freely interoperable across languages.

Workaround for Secure Enclave & HSM Limitations

(@mistermoe help me out here, fill gaps and correct me where I'm wrong)

There are no guarantees that all key management solutions will support our set of cryptographic requirements. For example, Ed25519 isn't supported in AWS KMS. In such a case, we must workaround this by serializing the private key material, storing it in a secure environment (ex. AWS Secrets Manager), deserializing the private key material at runtime, and relying on memory safety for security requirements. In other words, we can't assume the usage of a secure enclave or HSM solution by the application.

Others???

If there are other use cases for the serialization of private key material, then please comment such use cases below!

Solution

We can agree for certain to add a KeyExporter trait to the keys crate (inspiration can be taken from KeyImporter #204).

The contested matter is whether or not to first-class conceptualize a PortableDid type struct. The inclusion of the PortableDid type struct gives us an explicit data structure which represents a fully self-contained serialized instance of a BearerDid. The alternative would be to rely on the calling code (the app developer) to maintain the state associating the serialized private key(s) with the given DID.

It's not obvious to me that it's beneficial for us, at this time, to introduce a PortableDid at the rust core layer. It's not obvious it'll assist us with bindings. There's also a layer of complexity here with regards to #142 and if we decide to full-commit to JWK as the sole key representation or not.

For the sake of this ticket, I would recommend we do not introduce the PortableDid concept. With recognition we may if it becomes abundantly clear to do so. I'm open to challenging this though. By adding the KeyExporter, at a minimum, we enable the concept of a PortableDid even though, without the implementation, we don't provide it.

This ticket is relevant to decentralized-identity/web5-spec#112 (comment)


All of the below web5 SDKs have PortableDid as a dedicated type struct.

@KendallWeihe
Copy link
Contributor Author

KendallWeihe commented May 15, 2024

Actually, this triggered a concern with portable DID in our other SDKs. What happens in the following case:

  1. Create a did:dht
  2. Export it as a portable DID (which includes the DID Document)
  3. Update the did:dht (as in, update the DID Document in the network)
  4. At this point, what's in the portable DID's Document is different than the Document persisted in the network
  5. Instantiate a bearer DID from the portable DID
  6. At this point the developer has an out-of-date DID Document in their bearer DID instance

I think we should consider removing the DID Document from the portable DID type and instead resolve upon each instantiation (upon each "import from a portable DID"). As I see it, if we want to create a dedicated type for portable DID's then that type should merely consist of:

{
    "uri": "did:dht...",
    "privateKeyJwks": [...]
}

@mistermoe
Copy link

aye we definitely chatted about this during the design phase of portable did. @frankhinek do you remember our rationale for choosing to include the did document as part of portable did?

ah i think i remember. w.r.t. did:dht specifically, it may be the case that a did was created but not published. if so, without including the did document in the portable did itself, fidelity is lost (e.g. on creation, n services were provided to create)

This definitely doesn't negate the issue you've raised @KendallWeihe. The scenario you describe is entirely possible. At the time we decided that it was unlikely and felt that the furthest we should probably go (but didn't in any of the sdks) for the time being would be, on import, attempt to resolve. if resolution is successful, diff the portable did doc and the published did doc. if there's a difference, throw exception

@KendallWeihe
Copy link
Contributor Author

Excellent, let's move the discussion in the above two comments to decentralized-identity/web5-spec#156

@KendallWeihe
Copy link
Contributor Author

Then again, now that I think about this use case:

Workaround for Secure Enclave & HSM Limitations

In such a use case, wouldn't the solution be to have the key manager implementation:

  1. Generate the private key
  2. Immediately store in the secure environment (ex: AWS Secrets Manager)
  3. Subsequently read from the secure environment when the key material is needed

In this use case, the private key material should still never leave the key manager, granted this would require a key manager implementation specific to the given secure environment (ex. AWS Secrets Manager) but I think that's appropriate and compatible with our approach of supporting "bring-your-own-key-manager" (#160).

And so really, the only use case we need to support is for generating test vectors. In which case, I think it's appropriate to build one-off code for that (or better yet, a CLI feature) rather than implementing it in the core SDK which would enable accidental security vulnerabilities.

Going to leave this ticket open for the sake of discussion but I don't see a concrete need for this feature.

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

No branches or pull requests

2 participants