search

TBD54566975 / web5-rs

Apache License 2.0
6 stars 4 forks source link

Move import & export functions to new trait called `UnsafeKeyManager` #158

Closed KendallWeihe closed 2 months ago

KendallWeihe commented 2 months ago

Currently the export_private_keys() and import_private_keys() are a part of the KeyManager trait, but this is undesirable for two reasons. First, we want to promote the exclusion of the implementation of these methods because moving keys out of the given key store poses security risks. Second, we want to avoid implementations which return an error for "unsupported" or "not implemented" and instead shift-left such that the implementation is determinable at build time (throwing an error isn't discovered until runtime whereas the implementation of a rust trait is determinable at build time).

Instead, let's introduce a new trait, appropriately named UnsafeKeyManager which resolves both of these issues; it's named as such as to promote the exclusion of it (b/c what developer wants to use something "unsafe"? I suppose there is an assumption in there but I'm okay making it) & also the implementation of such a trait will be determinable at build time.

frankhinek commented 2 months ago

Alternative Proposal

Follow an approach similar to what has been implemented in JS, Kotlin, Dart, and Go:

  1. Remove the export_private_keys and import_private_keys methods from the Rust KeyManager trait.

  2. Add two additional traits:

    • KeyImporter
    • KeyExporter

These can optionally be implemented by concrete implementations of KeyManager when this functionality is desirable.

Background

Not all KeyManager implementations can support exporting keys. For example, this typically isn't possible with both mobile device and cloud-based HSMs.

Additionally, some key management systems can support importing of externally created key material, but this can be rather involved (e.g., see docs on AWS KMS and Google CloudKMS key import).

By maintaining separate traits, concrete implementations can choose to implement KeyImporter and KeyExporter when it is possible / makes sense.

KendallWeihe commented 2 months ago

It's a good point. I hadn't considered a use case wherein a key manager supports importing but not exporting (or vice versa). KeyImporter and KeyExporter doesn't convey the intent of moving keys around being an unsafe behavior, but I suppose it's fine. Developer beware. I'm in favor of the proposed alternative @frankhinek 👍

KendallWeihe commented 2 months ago

Completed in #204

Opened #211 for exporter