crypto_box: fix Scalar conversion to SecretKey
#137
Conversation
Avoids round tripping `Scalar` to/from bytes when leveraging the `From<Scalar>` impl for `SecretKey`.
|
@dignifiedquire can you give this a try and see if it fixes the problem you were encountering in dalek-cryptography/curve25519-dalek#564 ? |
|
Can confirm, fixes all the issues I have been seeing |
|
To give context, this addresses a compatibility bug with ed25519. A
These two procedures will result in different public keys. The solution here is to: 1) assume that the given scalar is already clamped and 2) save the original secret bytes. Question: does the new |
|
@rozbb I'm not sure how to solve that except by removing either the Alternatively we can add some documentation about this to the |
|
Removing |
How would I go about creating the matching secret key then from the |
|
One possibility is it takes the ed25519 secret key and computes the scalar bytes itself, ie |
But that sounds to me even more error prone than having these helper functions. Honestly I still think the cleanest and safest way would be to have a feature on |
|
I've been really hoping to avoid directly coupling |
|
I went ahead and added some notes in 6195d3a. If anyone feels particularly strongly about trying to find a better solution to this, I'd suggest opening a follow-up issue. |
## Description Moves all keys being used to `ed25519` keys, and only converts them to `crypto_box` keys only when needed. ### Breaking Changes - This is a breaking change to the derp protocol, as now only `ed25519` keys are sent over the wire. - The encrypted packets also changed, as first the sealed message and then the nonce is now sent - All configs and cli options now use `secret_key` not `keypair` or `private_key` - MSRV from `1.66` to `1.67` due to updated dependencies - `PeerId` is removed in favor of just using `PublicKey` directly ## Notes & open questions - [x] This is more expensive as the conversions are not cached at the moment, we should do so either in this PR or in a follow up. - [x] Depends on RustCrypto/nacl-compat#137 as otherwise the upgrade of `ed25519` breaks the code. ## Caching There is a performance benefit to caching, so for public key we always cache the `crypto_box::PublicKey`, and for `SecretKey`s we cache using `OnceCell`. --------- Co-authored-by: Diva M <[email protected]>
## Description Moves all keys being used to `ed25519` keys, and only converts them to `crypto_box` keys only when needed. ### Breaking Changes - This is a breaking change to the derp protocol, as now only `ed25519` keys are sent over the wire. - The encrypted packets also changed, as first the sealed message and then the nonce is now sent - All configs and cli options now use `secret_key` not `keypair` or `private_key` - MSRV from `1.66` to `1.67` due to updated dependencies - `PeerId` is removed in favor of just using `PublicKey` directly ## Notes & open questions - [x] This is more expensive as the conversions are not cached at the moment, we should do so either in this PR or in a follow up. - [x] Depends on RustCrypto/nacl-compat#137 as otherwise the upgrade of `ed25519` breaks the code. ## Caching There is a performance benefit to caching, so for public key we always cache the `crypto_box::PublicKey`, and for `SecretKey`s we cache using `OnceCell`. --------- Co-authored-by: Diva M <[email protected]>
## Description Moves all keys being used to `ed25519` keys, and only converts them to `crypto_box` keys only when needed. ### Breaking Changes - This is a breaking change to the derp protocol, as now only `ed25519` keys are sent over the wire. - The encrypted packets also changed, as first the sealed message and then the nonce is now sent - All configs and cli options now use `secret_key` not `keypair` or `private_key` - MSRV from `1.66` to `1.67` due to updated dependencies - `PeerId` is removed in favor of just using `PublicKey` directly ## Notes & open questions - [x] This is more expensive as the conversions are not cached at the moment, we should do so either in this PR or in a follow up. - [x] Depends on RustCrypto/nacl-compat#137 as otherwise the upgrade of `ed25519` breaks the code. ## Caching There is a performance benefit to caching, so for public key we always cache the `crypto_box::PublicKey`, and for `SecretKey`s we cache using `OnceCell`. --------- Co-authored-by: Diva M <[email protected]>
## Description Moves all keys being used to `ed25519` keys, and only converts them to `crypto_box` keys only when needed. ### Breaking Changes - This is a breaking change to the derp protocol, as now only `ed25519` keys are sent over the wire. - The encrypted packets also changed, as first the sealed message and then the nonce is now sent - All configs and cli options now use `secret_key` not `keypair` or `private_key` - MSRV from `1.66` to `1.67` due to updated dependencies - `PeerId` is removed in favor of just using `PublicKey` directly ## Notes & open questions - [x] This is more expensive as the conversions are not cached at the moment, we should do so either in this PR or in a follow up. - [x] Depends on RustCrypto/nacl-compat#137 as otherwise the upgrade of `ed25519` breaks the code. ## Caching There is a performance benefit to caching, so for public key we always cache the `crypto_box::PublicKey`, and for `SecretKey`s we cache using `OnceCell`. --------- Co-authored-by: Diva M <[email protected]>
## Description Moves all keys being used to `ed25519` keys, and only converts them to `crypto_box` keys only when needed. ### Breaking Changes - This is a breaking change to the derp protocol, as now only `ed25519` keys are sent over the wire. - The encrypted packets also changed, as first the sealed message and then the nonce is now sent - All configs and cli options now use `secret_key` not `keypair` or `private_key` - MSRV from `1.66` to `1.67` due to updated dependencies - `PeerId` is removed in favor of just using `PublicKey` directly ## Notes & open questions - [x] This is more expensive as the conversions are not cached at the moment, we should do so either in this PR or in a follow up. - [x] Depends on RustCrypto/nacl-compat#137 as otherwise the upgrade of `ed25519` breaks the code. ## Caching There is a performance benefit to caching, so for public key we always cache the `crypto_box::PublicKey`, and for `SecretKey`s we cache using `OnceCell`. --------- Co-authored-by: Diva M <[email protected]>
## Description Moves all keys being used to `ed25519` keys, and only converts them to `crypto_box` keys only when needed. ### Breaking Changes - This is a breaking change to the derp protocol, as now only `ed25519` keys are sent over the wire. - The encrypted packets also changed, as first the sealed message and then the nonce is now sent - All configs and cli options now use `secret_key` not `keypair` or `private_key` - MSRV from `1.66` to `1.67` due to updated dependencies - `PeerId` is removed in favor of just using `PublicKey` directly ## Notes & open questions - [x] This is more expensive as the conversions are not cached at the moment, we should do so either in this PR or in a follow up. - [x] Depends on RustCrypto/nacl-compat#137 as otherwise the upgrade of `ed25519` breaks the code. ## Caching There is a performance benefit to caching, so for public key we always cache the `crypto_box::PublicKey`, and for `SecretKey`s we cache using `OnceCell`. --------- Co-authored-by: Diva M <[email protected]>
## Description Moves all keys being used to `ed25519` keys, and only converts them to `crypto_box` keys only when needed. ### Breaking Changes - This is a breaking change to the derp protocol, as now only `ed25519` keys are sent over the wire. - The encrypted packets also changed, as first the sealed message and then the nonce is now sent - All configs and cli options now use `secret_key` not `keypair` or `private_key` - MSRV from `1.66` to `1.67` due to updated dependencies - `PeerId` is removed in favor of just using `PublicKey` directly ## Notes & open questions - [x] This is more expensive as the conversions are not cached at the moment, we should do so either in this PR or in a follow up. - [x] Depends on RustCrypto/nacl-compat#137 as otherwise the upgrade of `ed25519` breaks the code. ## Caching There is a performance benefit to caching, so for public key we always cache the `crypto_box::PublicKey`, and for `SecretKey`s we cache using `OnceCell`. --------- Co-authored-by: Diva M <[email protected]>
Avoids round tripping
Scalarto/from bytes when leveraging theFrom<Scalar>impl forSecretKey.