Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support custom KeyProvider #366

Merged
merged 4 commits into from
May 5, 2023
Merged

feat: support custom KeyProvider #366

merged 4 commits into from
May 5, 2023

Conversation

driftluo
Copy link
Collaborator

This is a feature with a wide influence and break change. I have tried several methods, and finally feel that the current writing method is relatively appropriate.

It will allow users to decide how to keep the private key used for ecdh, such as using some private key management services, requiring network communication signatures, and of course a default implementation is also provided. Users don't even need to use the secp256k1 algorithm, such as ed25519, as long as they conform to the behavior specification of the trait.

Of course, it also contains some minor modifications, including api changes, etc.

@driftluo driftluo requested a review from quake April 21, 2023 03:29
@driftluo driftluo changed the title feat: support custom singer feat: support custom signer Apr 21, 2023
@driftluo driftluo force-pushed the support-custom-signer branch 6 times, most recently from 286ac25 to 6a85133 Compare April 23, 2023 08:54
@driftluo driftluo marked this pull request as ready for review April 23, 2023 08:54
@driftluo driftluo requested a review from TheWaWaR as a code owner April 23, 2023 08:54
@doitian
Copy link
Member

doitian commented Apr 25, 2023

Could you provide an example how the library user can add a custom signer?

@driftluo
Copy link
Collaborator Author

Could you provide an example how the library user can add a custom signer?

https://github.com/nervosnetwork/tentacle/pull/366/files#diff-c50024690d098fb18317beafd9497d081b8710447a2cfc6286c0f19ba53eb159R153-R209

Implement these two traits, secio just provides the default implementation of secp256k1

@driftluo driftluo force-pushed the support-custom-signer branch from 6a85133 to 0791074 Compare April 25, 2023 02:39
Copy link
Member

@quake quake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are several things to improve in this PR

  1. The naming needs to be unified: Config struct field named as key and ServiceBuilder struct field named as key_pair, however, the trait is named as Signer, it's really confusing for reviewer. Can we unify the field naming as signer and the trait naming as Signer?

  2. We should not restrict the user to implement a Debug trait for Signer, suggest to remove this trait bound from Signer and Pubkey.

  3. The message type of T should not be restricted to Send if I understand correctly, suggest to remove this trait bound in Signer and PubKey trait:

-    fn sign_ecdsa<T: AsRef<[u8]> + Send>(&self, message: T) -> Result<Vec<u8>, Self::Error>;
+    fn sign_ecdsa<T: AsRef<[u8]>>(&self, message: T) -> Result<Vec<u8>, Self::Error>;
  1. It is not necessary to use Arc<K> as a Signer field by default, the only usage of Arc is for cloning, we may change it to key_pair: Option<K> by adding a Clone trait bound to Signer: Clone + . User may choose to pass a Arc<K> or K to ServiceBuilder

  2. In the handshake procedure, why we need to use a local signer to recover the remote public key, shouldn't these two be unrelated?

    let remote_public_key =
        <K as Signer>::pubkey_from_slice(ephemeral_context.state.remote.public_key.inner_ref())
            .map_err(Into::into)?;

    if !remote_public_key.verify_ecdsa(&data_to_verify, &remote_exchanges.signature) {
        debug!("failed to verify the remote's signature");
        return Err(SecioError::SignatureVerificationFailed);
    }

@driftluo
Copy link
Collaborator Author

driftluo commented Apr 25, 2023

@quake

  1. Under the current abstraction, two tentacles may fail to verify the handshake due to inconsistent algorithm choices.

The reason for using signer to restore the public key here is to let users understand that verification and signature need to use the same algorithm. Of course, this function of restoring the public key can also be placed on the pubkey trait. This is what I have thought about for a long time. It's will like follow:

pub trait Signer: std::fmt::Debug + Send + Sync + 'static {
    /// Error
    type Error: Into<crate::error::SecioError>;
    /// Public key
    type Pubkey: Pubkey;

    /// Constructs a signature for `msg` using the secret key `sk`
    #[cfg(feature = "async-trait")]
    async fn sign_ecdsa_async<T: AsRef<[u8]> + Send>(
        &self,
        message: T,
    ) -> Result<Vec<u8>, Self::Error> {
        self.sign_ecdsa(message)
    }

    /// Constructs a signature for `msg` using the secret key `sk`
    fn sign_ecdsa<T: AsRef<[u8]>>(&self, message: T) -> Result<Vec<u8>, Self::Error>;

    /// Creates a new public key from a [`Signer`].
    fn pubkey(&self) -> Self::Pubkey;
}

/// Public key for Signer
pub trait Pubkey: Send + Sync + 'static {
    /// Error
    type Error: Into<crate::error::SecioError>;
    /// Checks that `sig` is a valid ECDSA signature for `msg` using the public
    /// key `pubkey`.
    fn verify_ecdsa<T: AsRef<[u8]>, F: AsRef<[u8]>>(&self, message: T, signature: F) -> bool;

    /// serialized key into a bytes
    fn serialize(&self) -> Vec<u8>;

    /// Recover public key from slice
    fn from_slice<T: AsRef<[u8]>>(key: T) -> Result<Self, Self::Error>
    where
        Self: Sized;
}

let remote_public_key =
        <K as Signer>::Pubkey::from_slice(ephemeral_context.state.remote.public_key.inner_ref())
            .map_err(Into::into)?;

@driftluo driftluo force-pushed the support-custom-signer branch from 67ce726 to 6586f3a Compare April 25, 2023 09:22
@driftluo driftluo force-pushed the support-custom-signer branch 2 times, most recently from 07c00cd to 6876a2b Compare April 25, 2023 09:30
@driftluo driftluo changed the title feat: support custom signer feat: support custom KeyProvider Apr 25, 2023
@quake
Copy link
Member

quake commented Apr 25, 2023

fn verify_ecdsa<T: AsRef<[u8]>, F: AsRef<[u8]>>(&self, message: T, signature: F) -> bool;

in this case, we may simplified trait to

pub trait Signer {
    fn sign_ecdsa<T: AsRef<[u8]>>(&self, message: T) -> Result<Vec<u8>, Self::Error>;

    fn pubkey(&self) -> Vec<u8>;

    fn verify_ecdsa<P: AsRef<[u8]>, T: AsRef<[u8]>, F: AsRef<[u8]>>(pubkey: P, message: T, signature: F) -> bool;
}

secio/src/lib.rs Outdated Show resolved Hide resolved
@@ -157,7 +176,7 @@ impl ServiceBuilder {
///
/// for example, set all tcp bind to `127.0.0.1:1080`, set keepalive:
///
/// ```rust
/// ```ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ignore doc test here?

Copy link
Collaborator Author

@driftluo driftluo Apr 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since ServiceBuilder added a new generic type

let mut server = ServiceBuilder::new();

must specify the type,I think it's noise, so I ignore this doc test

@driftluo driftluo requested a review from quake April 27, 2023 02:47
@driftluo driftluo merged commit 9f322db into master May 5, 2023
@driftluo driftluo deleted the support-custom-signer branch May 5, 2023 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants