Implementation of PsaGenerateKey and PsaDestroyKey operations #354

RobertDrazkowskiGL · 2021-03-05T11:54:55Z

Refactor for a new, thread safe(er) rust-cryptoauthlib 0.2.0
Introduction of a CryptoAuthLib dedicated key management infrastructure
Implementation of PsaGenerateKey and PsaDestroyKey as users of this infrastructure

Signed-off-by: Robert Drazkowski Robert.Drazkowski@globallogic.com

hug-dev · 2021-03-08T17:21:20Z

Hey 👋
Thanks for the PR, great to see key management coming up.

I spent some time reading the code, trying to understand the logic behind, and before going too deep in reviewing specific things, I wanted to discuss the global design.

I will try to write here what I understood but please correct anything I say!

The ATECC chip has a number of slot with a predefined configuration for each one of them, noted with AtcaSlot.
This configuration is fetched during provider creation and stored in the KeySlotStorage structure, along with the information is a slot is occupied or not.
The key mappings map a KeyTriple to a slot ID (also the index of the KeySlotStorage) and attributes
When generating a key, we generate it in the first suitable slot: the first one with a compatible configuration for the attributes given.

A few questions now:

Do we assume that the Parsec service will be the only user of the ATECC device? Could multiple applications on the same host use the same ATECC device?
During the provider construction, how do you actually make sure that there is a valid key in the slot fetched from the mappings? To me, key_validate_and_mark_busy makes sure that the slot has still the correct configuration for the key and that the slot is free (which should always be the case during construction, unless somehow the mappings are corrupted and two slot IDs are the same?). Would it be possible that there is not a key at this slot? Could be if the mappings were badly managed or corrupted of if 1. is not assumed and that another application deleted the key in the slot?
More open to discussion, but would it make sense for the KeySlotStorage logic to be inside the rust-cryptoauthlib crate? One would call a method to generate a key with a specific type and it would return the slot where the key was created. The logic to cache the slot configs and verification would be inside the crate.

RobertDrazkowskiGL · 2021-03-09T13:21:59Z

Hey

Hey 👋
Thanks for the PR, great to see key management coming up.

I spent some time reading the code, trying to understand the logic behind, and before going too deep in reviewing specific
things, I wanted to discuss the global design.

Well, I am aware of the size of all those changes, but my intention was to provide not only the infrastructure, but also the users of that infrastructure.

I will try to write here what I understood but please correct anything I say!

* The ATECC chip has a number of slot with a predefined configuration for each one of them, noted with [`AtcaSlot`](https://docs.rs/rust-cryptoauthlib/0.2.0/rust_cryptoauthlib/struct.AtcaSlot.html).

This is correct.

* This configuration is fetched during provider creation and stored in the `KeySlotStorage` structure, along with the

information is a slot is occupied or not.

This is correct as well. There are two purposes of this fetching:

get and store the hardware configuration of a slot
check if the slot is locked by hardware

The KeySlotStorage is just an arbiter synchronizing accesses to hardware slots, with a pretty coarse granularity, I admit.

* The key mappings map a `KeyTriple` to a slot ID (also the index of the `KeySlotStorage`) and attributes

Yes, indirectly. There is already a mapping between KeyTriple and KeyInfo in Parsec. Then CAL interprets the field "id" from KeyInfo as a hardware slot number (id[0] is enough at the moment), and KeyInfo attributes are interpreted as "software attributes". The key in slot is usable only when the "software attributes" and "hardware slot attributes" are compatible.

* When generating a key, we generate it in the first suitable slot: the first one with a compatible configuration for the attributes given.

Yes, this is correct.

A few questions now:

1. Do we assume that the Parsec service will be the only user of the ATECC device? Could multiple applications on the same host use the same ATECC device?

Yes we do. This is the nature of I2C - any "i2c" group member is (usually) allowed to write to any i2c device. If there are two unsynchronized users of the same ATECC device, we have a BIG trouble.

2. During the provider construction, how do you actually make sure that there is a valid key in the slot fetched from the mappings? To me, `key_validate_and_mark_busy` makes sure that the slot has still the correct configuration for the key and that the slot is free (which should always be the case during construction, unless somehow the mappings are corrupted and two slot IDs are the same?). Would it be possible that there is not a key at this slot? Could be if the `mappings` were badly managed or corrupted of if 1. is not assumed and that another application deleted the key in the slot?

There is no in-hardware information whether there is anything usable in a slot or not. It may lead to some unpleasant results, e.g. if the slot is locked by hardware, but there is no KeyTriple -> KeyInfo mapping towards this slot, this slot is no longer usable at all. It is also possible that after a key info storage corruption a KeyInfo points at a slot that has no key at all. At the moment when two KeyInfos point at one slot the second referencing KeyTriple is reported but only a warning is emitted not an error. And yes, if there is another process accessing the same ATECC and it removes the key from the slot or (worse?) overwrites it, it is impossible to be noticed in Parsec.

Precisely speaking, there is an exception described here. Therefore, under certain circumstances, the public key may be invalidated.

3. More open to discussion, but would it make sense for the `KeySlotStorage` logic to be inside the `rust-cryptoauthlib` crate? One would call a method to generate a key with a specific type and it would return the slot where the key was created. The logic to cache the slot configs and verification would be inside the crate.

It is possible to implement rust-cryptoauthlib that way but it would violate two assumptions:

rust-cryptoauthlib is a thin wrapper for Microchip CryptoAuthentication Library, mimicking its behavior
Parsec is not the only user of rust-cryptoauthlib so it should be useful for other users as well.

Of course, both are already being violated one way or another, so I should rather refer to them as "clues", but any breach should be carefully considered anyway.
And honestly, I would need some time and also gain acceptance to take this path.

I hope this helps.

hug-dev · 2021-03-09T22:53:50Z

Well, I am aware of the size of all those changes, but my intention was to provide not only the infrastructure, but also the users of that infrastructure.

Oh that was not a criticism! I am ok with the size of the change too! I was just meaning that it's best to agree with the global logic before going into the tiny details 😄

Thanks for answering!

any "i2c" group member is (usually) allowed to write to any i2c device. If there are two unsynchronized users of the same ATECC device, we have a BIG trouble.

Ah ok. I think that would go under our threat model assumption 4: "Users with privilege rights are trusted to exercise them in a non-malicious way." Though, it seems pretty hard to enforce/verify that Parsec is the only process using it...

There is no in-hardware information whether there is anything usable in a slot or not.

Would it be possible to check if a basic (and fast) operation using the key fails or not? As in an operation that would fail if the slot is empty? Not sure if possible or if that makes sense. Something like atcab_get_zone_size()?

It is possible to implement rust-cryptoauthlib that way but it would violate two assumptions:

For 2., I was under the assumption that this is something (the KeySlotStorage) that would be needed for any user of rust-cryptoauthlib, not just Parsec. That in order to generate a key, you would first need to fetch the slots configuration and then find the appropriate one: what you did in Parsec could then be useful for all? But maybe I do not know all the use-cases.
For 1., I agree with you that you shouldn't go too far off the original spec but if some logic can be used by all and written in a way that is idiomatic in Rust then I'd say it can belong in the wrapper crate!

RobertDrazkowskiGL · 2021-03-10T14:53:13Z

Would it be possible to check if a basic (and fast) operation using the key fails or not? As in an operation that would fail if the slot is empty? Not sure if possible or if that makes sense. Something like atcab_get_zone_size()?

As far as I know, the answer is no, it is not possible. The only exception I am aware of is the slot with is_locked == true, but this is pretty easy to check anyway. And I am afraid there are no “fast” operations over I2C bus (up to 1Mbps, but more than 7 bytes of metadata overhead per request).

It is possible to implement rust-cryptoauthlib that way but it would violate two assumptions:

For 2., I was under the assumption that this is something (the KeySlotStorage) that would be needed for any user of rust-cryptoauthlib, not just Parsec. That in order to generate a key, you would first need to fetch the slots configuration and then find the appropriate one: what you did in Parsec could then be useful for all? But maybe I do not know all the use-cases.

I think the way Parsec uses ATECC and CALib is pretty unusual, at least at the moment. As you may have already noticed, it is not designed for a multi-user environment and safe privilege separation. There is no kernel driver owning exclusively the device and offering a common abstract interface or a user space daemon dynamically managing the globally shared ATECC configuration and contents.
Let me give you an example. It is not an unusual ATECC use case to have all the slots having a defined and known semantics, equipped with keys in the factory and just locked to avoid any manipulations. The whole key management requested by Parsec would be simply a waste of resources.
Therefore I believe that this is not the task for rust-cryptoauthlib to store this kind of information.

For 1., I agree with you that you shouldn't go too far off the original spec but if some logic can be used by all and written in a way that is idiomatic in Rust then I'd say it can belong in the wrapper crate!

In my opinion, this is not only a question of being far from spec, but also a question of a good design. The approach we try to follow is to define which information belongs to Parsec, which belongs to ATECC+CALib.

The free/busy slot management (and, less important, slot reference counting) belongs to Parsec, because only Parsec “knows” which slots are free or busy. It is not available directly but can be extracted from the key info manager. And this is where KeySlotStorage helps, managing these slots.

The hardware information, mostly slot configuration here, belongs to rust-cryptoauthlib and should be stored/cached/accessed using rust-cryptoauthlib.

There are things that belong to both - if the hardware slot is locked, the key is considered busy even without having a KeyTriple->KeyInfo mapping. If the KeyInfo attributes are not compatible with hardware slot configuration, it is considered “free” and corresponding KeyTriple->KeyInfo mapping is removed. All such cases belong to the interface between the two, i.e. to Parsec CryptoAuthLib provider. And if that needs a synchronization KeySlotStorage is used.

Please note, presented above “functionality ownership” is just my view and I am open for the discussion.

hug-dev · 2021-03-10T19:06:33Z

As far as I know, the answer is no, it is not possible. The only exception I am aware of is the slot with is_locked == true, but this is pretty easy to check anyway. And I am afraid there are no “fast” operations over I2C bus (up to 1Mbps, but more than 7 bytes of metadata overhead per request).

Got it, thanks.

Let me give you an example. It is not an unusual ATECC use case to have all the slots having a defined and known semantics, equipped with keys in the factory and just locked to avoid any manipulations. The whole key management requested by Parsec would be simply a waste of resources.

I see, that makes sense.

Please note, presented above “functionality ownership” is just my view and I am open for the discussion.

With your explanations, I now agree too with your choices. Let's start like this anyway and see how rust-cryptoauthlib evolves!

Thanks for the discussion, I will now start reviewing the code as it is 😃

RobertDrazkowskiGL · 2021-03-11T12:20:13Z

Thank you.

hug-dev

Here is my review, sorry for the load of comments 😓
Some of them are logic questions/comments and some are more to make the code more Rust idiomatic. I won't be too annoying about that last part but could be something good to have in the future (for example using more Result)!

e2e_tests/provider_cfg/cryptoauthlib/config.toml

e2e_tests/tests/per_provider/normal_tests/mod.rs

src/providers/cryptoauthlib/mod.rs

src/providers/cryptoauthlib/key_management.rs

src/providers/cryptoauthlib/key_slot.rs

hug-dev · 2021-03-22T09:44:16Z

Hello 👋 Don't hesitate to ping me here or on the Slack channel when my re-review is needed. Don't want to block you.

RobertDrazkowskiGL · 2021-03-22T10:27:58Z

Hello
Sorry for the delay - Artur took a few days off, he is back tomorrow with new corrections.

…toAuthLib provider. 1. Refactor for a new, thread safe(er) rust-cryptoauthlib 0.2.0 2. Introduction of a CryptoAuthLib dedicated key management infrastructure 3. Implementation of PsaGenerateKey and PsaDestroyKey as users of this infrastructure Signed-off-by: Robert Drazkowski <Robert.Drazkowski@globallogic.com>

Signed-off-by: Robert Drazkowski <Robert.Drazkowski@globallogic.com>

Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

…released rust-calib crate version Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

akazimierskigl · 2021-03-25T15:51:47Z

@hug-dev
Hello, I think we are ready for re-review. There is one test failing for PKCS 11 provider which we will try to resolve very soon. We've addressed most of comments you've posted if not I left comment with details. Update of e2e tests is integrated and pull requests which were merged to main branch on meantime are also merged to this branch.

hug-dev · 2021-03-25T15:56:23Z

Great, thanks! I will have a look again soon. Don't worry about the PKCS11 test, most probably it is not your fault, those tests are failing often for various reasons not under our control.

hug-dev

Alright second round! Sorry if this is taking a long time... Thanks a lot for sorting out the tests with is_operation_supported 👌

src/providers/cryptoauthlib/key_management.rs

src/providers/cryptoauthlib/key_slot.rs

e2e_tests/tests/per_provider/normal_tests/auth.rs

e2e_tests/tests/per_provider/normal_tests/create_destroy_key.rs

e2e_tests/tests/per_provider/normal_tests/create_destroy_key_calib.rs

hug-dev · 2021-03-25T17:37:19Z

e2e_tests/tests/per_provider/normal_tests/hash.rs

@@ -11,10 +11,12 @@ const SHA_256: [u8; 32] = [
 0x0d, 0xc4, 0xbc, 0x13, 0xfd, 0x91, 0x74, 0x52, 0x92, 0x24, 0xc3, 0x8e, 0x0e, 0xe0, 0x75, 0xfa,
 0x9e, 0xd8, 0x0b, 0x78, 0x47, 0xe6, 0xae, 0xa7, 0x6a, 0xe9, 0x8c, 0xf9, 0xdd, 0xd9, 0x26, 0x69,
 ];
+#[cfg(not(feature = "cryptoauthlib-provider"))]


Are those tests failing? Are yo utesting hashing with hash_calib?

ATECC supports SHA256 only. And hash_calib.rs willl be removed.

e2e_tests/tests/per_provider/normal_tests/hash_calib.rs

src/providers/cryptoauthlib/key_operations.rs

Signed-off-by: Robert Drazkowski <Robert.Drazkowski@globallogic.com>

hug-dev

Thank you! That looks good to me now 😄

ionut-arm

Thanks, only left a couple of comments but I'm ok with the change overall!

src/providers/cryptoauthlib/key_slot.rs

src/providers/cryptoauthlib/key_slot_storage.rs

hug-dev added the platforms Compatibility with different secure services or hardware platforms label Mar 5, 2021

hug-dev self-requested a review March 5, 2021 18:02

RobertDrazkowskiGL mentioned this pull request Mar 8, 2021

Investigate testing of Cryptoauthlib provider #315

Closed

hug-dev reviewed Mar 12, 2021

View reviewed changes

akazimierskigl force-pushed the calib-psa-generate-destroy-key branch from f08a666 to 65decc3 Compare March 18, 2021 13:40

akazimierskigl force-pushed the calib-psa-generate-destroy-key branch from 65decc3 to f08a666 Compare March 24, 2021 10:36

RobertDrazkowskiGL and others added 9 commits March 25, 2021 14:04

Commented out CryptoAuthLib provider references from tests run by CI.

d2f28d5

Signed-off-by: Robert Drazkowski <Robert.Drazkowski@globallogic.com>

fix destroy key logic

9c8751e

Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

refactor to simplify code readability

e1347b6

Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

review fixes

ffb0d55

Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

fix get_calib_key_type and revert configuration_is_locked changes to …

e93dd14

…released rust-calib crate version Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

updated tests to fix e2e calib testing

c7c6de8

Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

derive Default for AteccKeySlot

a408354

Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

revert derive Default

e6adfa4

Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

akazimierskigl force-pushed the calib-psa-generate-destroy-key branch from 667a3db to e6adfa4 Compare March 25, 2021 14:03

akazimierskigl added 3 commits March 25, 2021 15:24

e2e typo hotfix

080f507

Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

e2e typo hotfix 2

5898eb6

Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

e2e typo hotfix 3

548a6f1

Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

hug-dev reviewed Mar 25, 2021

View reviewed changes

RobertDrazkowskiGL added 2 commits March 26, 2021 10:24

Merge branch 'master' into calib-psa-generate-destroy-key

9690344

Post review fixes.

c72ab12

Signed-off-by: Robert Drazkowski <Robert.Drazkowski@globallogic.com>

hug-dev approved these changes Mar 26, 2021

View reviewed changes

hug-dev requested a review from ionut-arm March 26, 2021 15:35

ionut-arm approved these changes Mar 29, 2021

View reviewed changes

src/providers/cryptoauthlib/key_slot.rs Show resolved Hide resolved

src/providers/cryptoauthlib/key_slot_storage.rs Show resolved Hide resolved

ionut-arm merged commit c12b4c3 into parallaxsecond:master Mar 30, 2021

hug-dev mentioned this pull request Sep 22, 2021

Enable Squash and Merge on every repo and document merge procedure parallaxsecond/parsec-book#131

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of PsaGenerateKey and PsaDestroyKey operations #354

Implementation of PsaGenerateKey and PsaDestroyKey operations #354

RobertDrazkowskiGL commented Mar 5, 2021

hug-dev commented Mar 8, 2021

RobertDrazkowskiGL commented Mar 9, 2021

hug-dev commented Mar 9, 2021

RobertDrazkowskiGL commented Mar 10, 2021

hug-dev commented Mar 10, 2021

RobertDrazkowskiGL commented Mar 11, 2021

hug-dev left a comment

hug-dev commented Mar 22, 2021

RobertDrazkowskiGL commented Mar 22, 2021

akazimierskigl commented Mar 25, 2021

hug-dev commented Mar 25, 2021

hug-dev left a comment

hug-dev Mar 25, 2021

RobertDrazkowskiGL Mar 26, 2021

hug-dev left a comment

ionut-arm left a comment

Implementation of PsaGenerateKey and PsaDestroyKey operations #354

Implementation of PsaGenerateKey and PsaDestroyKey operations #354

Conversation

RobertDrazkowskiGL commented Mar 5, 2021

hug-dev commented Mar 8, 2021

RobertDrazkowskiGL commented Mar 9, 2021

hug-dev commented Mar 9, 2021

RobertDrazkowskiGL commented Mar 10, 2021

hug-dev commented Mar 10, 2021

RobertDrazkowskiGL commented Mar 11, 2021

hug-dev left a comment

Choose a reason for hiding this comment

hug-dev commented Mar 22, 2021

RobertDrazkowskiGL commented Mar 22, 2021

akazimierskigl commented Mar 25, 2021

hug-dev commented Mar 25, 2021

hug-dev left a comment

Choose a reason for hiding this comment

hug-dev Mar 25, 2021

Choose a reason for hiding this comment

RobertDrazkowskiGL Mar 26, 2021

Choose a reason for hiding this comment

hug-dev left a comment

Choose a reason for hiding this comment

ionut-arm left a comment

Choose a reason for hiding this comment