Update DR External Keys

[jazzz]

This PR fixes how keys are handled across all the various crates.

Currently Conversations made use of x25519-dalek to provide safe handling of secrets, and DHKE. The double ratchet implementation also used the same library internally however accepted keys via [u8;32]s. This required that keymaterial be extracted, copied leading to potential unsafe handling of keys.

Changes

• Isolates x25519-dalek library to the crypto crate.
• Wraps PublicKey, StaticSecret in Newtypes.
• Updates Conversations to use these Newtypes.
• Updates double-ratchets to use these newtypes in its initialization functions, so that memory can be zeroized safely without needless copies.
• Removes IdentityKeyPair to save on copies, and reduce unneeded structs.

Not Changed

• Keys internal to DR were left untouched. This can be addressed in a followup PR, later.

Reviewer Tips

• The change set is quite large however its mostly many instances of the same change. It's recommended to review the commits in order.
• Happy to rollback the later commits if there is any tension.

[kaichaosun]

• Adding 32 everywhere seems tedious, consider removing it.
• What's the difference between SecretKey32 and PrivateKey32, can we just use one and make the convertion when needed?

[jazzz]

Adding 32 everywhere seems tedious, consider removing it.

We will be adding PQE in march, which will add addition sets of keys. With Curve25519 being the choice for 32Byte cryptography. Being able to differentiate between keysizes would be helpful. I can add a type aliases in double-ratchets if that would be preferred.

[jazzz]

What's the difference between SecretKey32 and PrivateKey32, can we just use one and make the convertion when needed?

PublicKey32/PrivateKey32 represent asymmetric key pairs used in key exchange protocols like Diffie-Hellman (X25519).
SecretKey32 represents symmetric keys used for encryption/decryption operations.
While they're both 32 bytes, they have different cryptographic properties and uses:

PrivateKey32: Used to derive shared secrets via DH.
SecretKey32: Used directly for symmetric encryption.

Conversion between them isn't generally safe or meaningful. Having separate types allows for stricter type checking.

[kaichaosun]

We will be adding PQE in march, which will add addition sets of keys. With Curve25519 being the choice for 32Byte cryptography.

It seems adding 32 is not a good way to identify different version of cryptography, what about use Version number like V1?

PrivateKey32: Used to derive shared secrets via DH.
SecretKey32: Used directly for symmetric encryption.

It makes sense two differentiate the two scenarios, would be good use some clearer names and a little comments.

DhPrivateKey (actually should it live in DR crate?)      // used only for Diffie–Hellman
SymmetricKey      // used directly for encryption

[jazzz]

It seems adding 32 is not a good way to identify different version of cryptography,

I think we are still not aligned. It's not a version, its a type. this Library will support multiple cryptosystems simultaneously. Classic PreQuantum cryptography based on 32 Byte keys, and PostQuantum Cryptography based. They will exist side by side.

I hear this distinction does not work for you; What If:

• I changed PublicKey32 -> PublicKey.
• We deal with multiple implementations of PublicKey by using namespaces where appropriate to avoid confusion.
• We then do a large refactor later to rename the keys to some appropriate then.

Does this work @kaichaosun? What needs to change in relation to the 32 Suffix?

[jazzz]

DhPrivateKey // used only for Diffie–Hellman

Its not a DhPrivateKey its also used to signing, among other tasks. It's a privateKey to call it something else goes against convention.

(actually should it live in DR crate?)
The goal was to provide consist use of Keys across implementations to provide consistency and safe key usage. Moving SecretKey to DR is the opposite of what I'm trying to achieve.

If you were suggesting to perform another NewType wrapper e.g struct DoubleratchetInitializationKey(PrivateKey); I won't block it - but not where I think time is best spent at this moment

[kaichaosun]

PublicKey32 -> PublicKey.

That solves my problem, jut removing 32 and we may find good solution when PQE landed.
And if secret key vs private key need to both exist, please add necessary comments.

[jazzz]

Updated to PublicKey, PrivateKey

[kaichaosun]

We may want this to wrap the PublicKey32 and PrivateKey32 for easily consolidating the helper functions that required by DR.
Without it, readers need to read code deeply to understand the required crypto related to installation.
Maybe worth to find a better name for it though.

[jazzz]

Without it, readers need to read code deeply to understand the required crypto related to installation.

I don't follow.

• What are you defining as the operation "Installation?" is that Account Provisioning?

We may want this to wrap the PublicKey32 and PrivateKey32 for easily consolidating the helper functions that required by DR.

I completely disagree here - (especially focusing on DR). If a PublicKeys are required; they can be derived safely where they are needed.

• Simplicity: I don't see how Struct KeyPair(PrivateKey32, PublicKey32) is more simple that PrivateKey32 - Both in terms of readability and code maintenance.
• Safer: Deriving PublicKeys in advance means that checks are required to ensure that the publicKeys were derived from the PrivateKey provided.
• Efficient: Every instance where a PublicKey is needed, Ownership is also required. Deriving and storing the publickeys in advance does not provide any performance improvements.

consolidating the helper functions that required by DR.

Which exact helper functions are these?

[kaichaosun]

What are you defining as the operation "Installation?" is that Account Provisioning?

I actually don't know how to define Installation, renaming it actually makes a lot of sense, I think it would be good to still wrap the key material in a struct or alias of DR crate, (struct is better because the type safety check).

I completely disagree here - (especially focusing on DR). If a PublicKeys are required; they can be derived safely where they are needed.

yes, I agree with removing public keys in the wrapper, if performance needed, we may add it back later. (I do see a few libraries has private key and public both included, not sure if it's because performance issue though.)

Which exact helper functions are these?

PrivateKey32 provides a large scope of functions, DR only require a little like dh, public key derivation, make itself a submodule clearly show what's cryptography requirements needed by DR.

[jazzz]

I still don't follow.

Originally I believed the ask was to "wrap Public and Private keys into a struct to pass to double-ratchets::state::init_sender. So My comments reflected that.

But that appears to not be the case

yes, I agree with removing public keys in the wrapper

I think it would be good to still wrap the key material in a struct or alias of DR crate, (struct is better because the type safety check).

If I am reading this correctly you are asking for:

• InstallationKey to have a concrete type so that it is different from PrivateKey32?
• Stop the wrong PrivateKey32 from being used as an InstallationKey?

e.g. struct InstallationKey(PrivateKey32)

Is that Correct @kaichaosun ?

[kaichaosun]

struct InstallationKey(PrivateKey32)

yeah, that's what I mean, should have better name than InstallationKey.
DR crate should use the functions from it, not directly deal with PrivateKey32.

[jazzz]

Defining an entire NewType for that code seems excessive. crate crypto wraps the external dependence and provides common keys to be used in the workspace.

Maintaining an additional set of types for little benefit. There is currently no interest in a independently usable double ratchet library. I'm not sure what we are insulating it from? I'm content with a type alias, if truely desired. I still believe that double-ratchet being its own crate is producing less readable code due to the extra layers of indirection. If the desire is to keep clarity and review-ability as goals, then using consistent types across Libchat would lower mental load on reviewers.

[kaichaosun]

x25519_dalek::PublicKey can be aliased to DalekPublicKey when import.

[jazzz]

If you think that is clearer, sure

[jazzz]

Since this was last reviewed, xed25519::Public has also been added which adds more ambiguity around publicKeys.

I've added EdPublicKey and DrPublicKey as asked but I'm not convinced it makes it any clearer. I'm used to the x25519_dalek types, so that could be biasing my opinions here.