P256 keys using Secure Enclave and Android StrongBox #245

berendsliedrecht · 2024-05-02T14:11:52Z

This PR contains the functionality to add support for hardware-backed P256 keys on iOS and Android Using iOS Secure Enclave and Android StrongBox. This is done by using the secure-env library. iOS is accessed via Security.framework and Android via JNI. Using these features you must build the library with mobile_secure_element or askar-crypto directly with p256_hardware.

During the implementation I ran into some issues due to askar-crypto being no_std. because of this I made the rng: impl KeyMaterial optional as well as the id: &str. My initial intention was to create a structure or enum for KeyGenOps, but due to not always having access to an allocator I could not leverage Box and dynamic dispatch. If there are any solutions for this, I would be happy to change the current implementation.

askar-crypto now also has a method to get the key by id. Which ties askar-crypto directly to storage, but I could not find any other way of doing this due to the nature of Secure Elements.

Over FFI, when supplying an id to askar_key_generate, the key will be created on hardware. This seems a bit too implicit for me. My next approach would be generate a random id, uuid or something and simply pass in a boolean for hardware_backed.

Implemented that we generate an id now for secure element keys and the user just has to provide hardware_backed argument over FFI as an i8.

Still in draft for now, as I have to do some testing.

Will also include an updated python wrapper (only software as key_backend allowed) and an updated node.js wrapper. React Native will be done in #247 as that will also include the React Native Example app and move to pnpm.

TimoGlastra

Nice 👍

Will the is_hardware_backed also be exposed over FFI?

TimoGlastra · 2024-05-06T06:53:05Z

include/libaries_askar.h

+ * id is an optional argument only required for hardware-bound keys
+ */
+ErrorCode askar_key_generate(FfiStr alg,
+                             int8_t hardware_backed,


Instead of marking something as hardware_backed, should this be a more generic parameters so it's a bit more ready for future changes? E.g. "key_backend" which can take a value of secure_env, or something like strongbox? I foresee us using the same capabilities for cloud HSM, with possibly multiple backends configured, and it might be nice to make it a backend rather than strictly hardware_backed?

Just a thought, this approach seems fine as well

TimoGlastra · 2024-05-06T10:51:25Z

In credo we sign based on the public key, will that also be possible with the hardware keys? Or do we need to pass the key id specifically?

berendsliedrecht · 2024-05-06T11:59:58Z

In credo we sign based on the public key, will that also be possible with the hardware keys? Or do we need to pass the key id specifically?

Do you have an example of this code? If I can check which path it takes I can see whether it'll work or not. I don't think we can go from a public key to storage and get the HSM key. Software keys are stored as a JWK, but for secure element keys I chose to store just the id. If this is a very strong requirement I could do something like:

{
  "id": "CUSTOM_USER_DEFINED_ID",
  "public_key_base58": "bla..."
}

instead of the id alone.

TimoGlastra · 2024-05-06T12:09:56Z

Could we make the id the public key? Or is it generated by the secure enclave?

this is the code in Credo, we fetch the key based on the publicKeyBase58: https://github.com/openwallet-foundation/credo-ts/blob/8f49d84d52143bd9136cebd502bd9d10b5881463/packages/askar/src/wallet/AskarBaseWallet.ts#L214

If we can't do that, it will require A LOT of effort in Credo, as we would have to introduce a new concept of keyIds (which is a good thing probably), but it will be a big breaking chagne as we pass a Key instance around on MANY levels and layers

genaris · 2024-05-06T12:16:35Z

Could we make the id the public key? Or is it generated by the secure enclave?

this is the code in Credo, we fetch the key based on the publicKeyBase58: https://github.com/openwallet-foundation/credo-ts/blob/8f49d84d52143bd9136cebd502bd9d10b5881463/packages/askar/src/wallet/AskarBaseWallet.ts#L214

If we can't do that, it will require A LOT of effort in Credo, as we would have to introduce a new concept of keyIds (which is a good thing probably), but it will be a big breaking chagne as we pass a Key instance around on MANY levels and layers

In Credo we store the key using publicKeyBase58 as its name (id). Does this new approach prevent us from specifying the id we want when creating a key in hardware?

berendsliedrecht · 2024-05-06T12:33:35Z

Could we make the id the public key? Or is it generated by the secure enclave?

Yes! The name is not equal to the underlying Secure Element id, they are separate identifiers.

If we can't do that, it will require A LOT of effort in Credo, as we would have to introduce a new concept of keyIds (which is a good thing probably), but it will be a big breaking chagne as we pass a Key instance around on MANY levels and layers.

We can defer it for now, and possibly introduce it later, but it is not required.

In Credo we store the key using publicKeyBase58 as its name (id). Does this new approach prevent us from specifying the id we want when creating a key in hardware?

No it does not. It will work exactly the same as before. askar_key_generate now takes a key_backend argument which can be Software (default) or Secure Element. And based on that backend we generate a uuid for the Secure Element and only use that internally. After creation you can fetch the public key and use that as the name of the row.

TimoGlastra · 2024-05-06T12:35:35Z

Okay perfect, sounds good :)

andrewwhitehead · 2024-05-06T17:47:22Z

src/kms/entry.rs

@@ -14,9 +43,11 @@ pub struct KeyParams {

    /// An optional external reference for the key
    #[serde(default, rename = "ref", skip_serializing_if = "Option::is_none")]
-    pub reference: Option<String>,
+    pub reference: Option<KeyReference>,


I expected that the reference field would be something like "secure_element:{key_id}" and the data field would be empty in this case, but I suppose this works too and might be better for indexing keys.

I am not too set on either approach. I can leave it as-is or change it to w/e is your preferred method.

andrewwhitehead · 2024-05-06T17:49:20Z

askar-crypto/src/alg/any.rs

+    fn generate_with_rng(alg: KeyAlg, rng: impl KeyMaterial) -> Result<Self, Error>;
+
+    /// Generate a new key with an id for the given key algorithm.
+    fn generate_with_id(alg: KeyAlg, id: &str) -> Result<Self, Error>;


This seems to be restricted to hardware backed keys, and the naming doesn't really reflect that for me. Do we need to accept the ID as an argument or could it just be generated automatically?

Right now the id is generated in the FFI layer. I could also defer this to askar-crypto where I replace generate_any_with_id with generate_any_hardware and generate the id there. This would also remove the uuid dependency from aries-askar, but would introduce it into askar-crypto.

I will implement it like this, but also open to other suggestions.

andrewwhitehead · 2024-05-06T17:52:16Z

src/ffi/key.rs

+            KeyBackend::Software => LocalKey::generate_with_rng(alg, ephemeral != 0),
+            KeyBackend::SecureElement =>  {
+                let id = Uuid::new_v4().to_string();
+                LocalKey::generate_with_id(alg, &id, ephemeral != 0)


Is it appropriate to support 'ephemeral' for these keys?

Is it appropriate to support 'ephemeral' for these keys?

The underlying library does not yet implement the deletion of keys, so ATM not really no. Once it is there, it would be fine imo to support it. Otherwise we would also have to create a new FFI function for it or make it an optional argument.

andrewwhitehead · 2024-05-06T17:54:56Z

Looks great so far, maybe there would be a need to inspect what backends are supported by Askar for the current platform?

berendsliedrecht · 2024-05-07T06:46:53Z

Looks great so far, maybe there would be a need to inspect what backends are supported by Askar for the current platform?

Yes that should be quite easy based on the feature flags. I will add it to askar-crypto and expose it over ffi as askar_get_supported_key_backends(out: *mut FfiStringListHandle) -> ErrorCode.

berendsliedrecht · 2024-05-07T13:02:44Z

CI is finally finished so it is ready.

Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>

TimoGlastra · 2024-05-08T13:06:22Z

wrappers/javascript/aries-askar-nodejs/tests/keys.test.ts

+  test('supported backends', () => {
+    const backends = ariesAskarNodeJS.keyGetSupportedBackends()
+
+    expect(backends.length).toStrictEqual(1)
+    expect(backends).toStrictEqual(expect.arrayContaining(['software']))
+  })


Maybe it should also support returning which backends are supported for which key, but I'm als fine to add that in the future

Yeah that would be a good one. A bit more difficult to check dynamically, but it should be fine to track that statically.

TimoGlastra

LGTM. Maybe @andrewwhitehead can also give a final ✅ if everythings looks ok?

berendsliedrecht marked this pull request as draft May 2, 2024 14:12

TimoGlastra reviewed May 6, 2024

View reviewed changes

berendsliedrecht force-pushed the mobile-hardware-es256 branch from 8949028 to 13a66c8 Compare May 6, 2024 09:54

berendsliedrecht mentioned this pull request May 6, 2024

chore(js): use pnpm as the package manager #246

Closed

swcurran requested a review from andrewwhitehead May 6, 2024 14:04

andrewwhitehead reviewed May 6, 2024

View reviewed changes

berendsliedrecht force-pushed the mobile-hardware-es256 branch 10 times, most recently from bd315cc to 9e9ae56 Compare May 7, 2024 12:53

berendsliedrecht marked this pull request as ready for review May 7, 2024 13:02

added p256_hardware to askar-crypto

c4425f2

Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>

berendsliedrecht force-pushed the mobile-hardware-es256 branch from 9e9ae56 to c4425f2 Compare May 8, 2024 09:26

Berend Sliedrecht added 4 commits May 8, 2024 11:28

expose p256_hardware over FFI and storage

fec3c57

Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>

expose p256_hardware functionality at node.js wrapper

88d48b8

Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>

expose p256_hardware functionality at python wrapper

492d154

Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>

update secure-env dependency and add secure-env error message

8b64450

Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>

berendsliedrecht force-pushed the mobile-hardware-es256 branch from e64a60e to 8b64450 Compare May 8, 2024 11:05

berendsliedrecht mentioned this pull request May 8, 2024

chore(js): add expo example app and move to pnpm #247

Merged

TimoGlastra reviewed May 8, 2024

View reviewed changes

TimoGlastra approved these changes May 8, 2024

View reviewed changes

andrewwhitehead approved these changes May 8, 2024

View reviewed changes

berendsliedrecht merged commit 81a925b into openwallet-foundation:main May 10, 2024
30 checks passed

berendsliedrecht deleted the mobile-hardware-es256 branch May 10, 2024 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

P256 keys using Secure Enclave and Android StrongBox #245

P256 keys using Secure Enclave and Android StrongBox #245

berendsliedrecht commented May 2, 2024 •

edited

Loading

TimoGlastra left a comment

TimoGlastra May 6, 2024

TimoGlastra commented May 6, 2024

berendsliedrecht commented May 6, 2024

TimoGlastra commented May 6, 2024

genaris commented May 6, 2024

berendsliedrecht commented May 6, 2024

TimoGlastra commented May 6, 2024

andrewwhitehead May 6, 2024

berendsliedrecht May 7, 2024

andrewwhitehead May 6, 2024

berendsliedrecht May 7, 2024

andrewwhitehead May 6, 2024

berendsliedrecht May 7, 2024

andrewwhitehead commented May 6, 2024

berendsliedrecht commented May 7, 2024

berendsliedrecht commented May 7, 2024

TimoGlastra May 8, 2024

berendsliedrecht May 8, 2024

TimoGlastra left a comment

P256 keys using Secure Enclave and Android StrongBox #245

P256 keys using Secure Enclave and Android StrongBox #245

Conversation

berendsliedrecht commented May 2, 2024 • edited Loading

TimoGlastra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimoGlastra commented May 6, 2024

berendsliedrecht commented May 6, 2024

TimoGlastra commented May 6, 2024

genaris commented May 6, 2024

berendsliedrecht commented May 6, 2024

TimoGlastra commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewwhitehead commented May 6, 2024

berendsliedrecht commented May 7, 2024

berendsliedrecht commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimoGlastra left a comment

Choose a reason for hiding this comment

berendsliedrecht commented May 2, 2024 •

edited

Loading