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(identity): mark sign as infaillible #5728

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/signed_envelope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ impl SignedEnvelope {
) -> Result<Self, SigningError> {
let buffer = signature_payload(domain_separation, &payload_type, &payload);

let signature = key.sign(&buffer)?;
let signature = key.sign(&buffer).unwrap();

Ok(Self {
key: key.public(),
Expand Down
6 changes: 4 additions & 2 deletions identity/src/keypair.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
// DEALINGS IN THE SOFTWARE.

use std::convert::Infallible;

#[cfg(any(
feature = "ecdsa",
feature = "secp256k1",
Expand Down Expand Up @@ -55,7 +57,7 @@
#[cfg(feature = "secp256k1")]
use crate::secp256k1;
use crate::{
error::{DecodingError, SigningError},

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Compile with select features (mdns tcp dns async-std)

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Compile with select features (mdns tcp dns tokio)

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Compile on wasm32-unknown-unknown

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Compile on wasm32-unknown-emscripten

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / IPFS Integration tests

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Compile on wasm32-wasi

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Check rustdoc intra-doc links

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / clippy (1.80.0)

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / examples

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Compile with MSRV

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / clippy (beta)

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-autonat

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-webrtc

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-allow-block-list

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-perf

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-core

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-floodsub

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-connection-limits

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-dcutr

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-quic

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-dns

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-identify

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-gossipsub

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-metrics

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-kad

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-memory-connection-limits

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-mdns

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-ping

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-identity

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-plaintext

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-noise

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-mplex

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-pnet

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-relay

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-rendezvous

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-stream

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-request-response

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-server

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-swarm

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-uds

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-tcp

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-tls

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-upnp

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-webrtc-utils

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-swarm-test

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-webrtc-websys

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-webtransport-websys

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-websocket

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-websocket-websys

unused import: `SigningError`

Check failure on line 60 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / Test libp2p-yamux

unused import: `SigningError`
KeyType,
};

Expand Down Expand Up @@ -176,12 +178,12 @@
/// Sign a message using the private key of this keypair, producing
/// a signature that can be verified using the corresponding public key.
#[allow(unused_variables)]
pub fn sign(&self, msg: &[u8]) -> Result<Vec<u8>, SigningError> {
pub fn sign(&self, msg: &[u8]) -> Result<Vec<u8>, Infallible> {

Check failure on line 181 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / clippy (1.80.0)

this function's return value is unnecessarily wrapped by `Result`

Check failure on line 181 in identity/src/keypair.rs

View workflow job for this annotation

GitHub Actions / clippy (beta)

this function's return value is unnecessarily wrapped by `Result`
Copy link
Member

Choose a reason for hiding this comment

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

Since we are returning Ok throughout the inner functions, should we go on and remove the Result here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can. But what about upstream? there are a couple of functions that return a Result only because sign does so. Should we address them here? The biggest change will be noise security upgrade become infallible but the trait bound in swarm builder expect an associated Error type.

Copy link
Member

Choose a reason for hiding this comment

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

We could change it so it is no longer expecting an associated error type

Copy link
Member

Choose a reason for hiding this comment

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

Im not against making the change here. Thoughts on that @jxs ?

Copy link
Member

Choose a reason for hiding this comment

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

yeah I think if it's api that doesn't make sense we should address it and take the next release cycle to release a new libp2p-identity release wdyt?

match self.keypair {
#[cfg(feature = "ed25519")]
KeyPairInner::Ed25519(ref pair) => Ok(pair.sign(msg)),
#[cfg(all(feature = "rsa", not(target_arch = "wasm32")))]
KeyPairInner::Rsa(ref pair) => pair.sign(msg),
KeyPairInner::Rsa(ref pair) => Ok(pair.sign(msg)),
#[cfg(feature = "secp256k1")]
KeyPairInner::Secp256k1(ref pair) => Ok(pair.secret().sign(msg)),
#[cfg(feature = "ecdsa")]
Expand Down
8 changes: 4 additions & 4 deletions identity/src/rsa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,12 @@ impl Keypair {
}

/// Sign a message with this keypair.
pub fn sign(&self, data: &[u8]) -> Result<Vec<u8>, SigningError> {
pub fn sign(&self, data: &[u8]) -> Vec<u8> {
let mut signature = vec![0; self.0.public().modulus_len()];
let rng = SystemRandom::new();
match self.0.sign(&RSA_PKCS1_SHA256, &rng, data, &mut signature) {
Ok(()) => Ok(signature),
Err(e) => Err(SigningError::new("RSA").source(e)),
Ok(()) => signature,
Err(_) => unreachable!("length of pubkey remain unchanged with immutability"),
}
}
}
Expand Down Expand Up @@ -359,7 +359,7 @@ mod tests {
#[test]
fn rsa_sign_verify() {
fn prop(SomeKeypair(kp): SomeKeypair, msg: Vec<u8>) -> Result<bool, SigningError> {
kp.sign(&msg).map(|s| kp.public().verify(&msg, &s))
Ok(kp.public().verify(&msg, &kp.sign(&msg)))
}
QuickCheck::new()
.tests(10)
Expand Down
2 changes: 1 addition & 1 deletion protocols/gossipsub/src/behaviour.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2659,7 +2659,7 @@ where
// the signature is over the bytes "libp2p-pubsub:<protobuf-message>"
let mut signature_bytes = SIGNING_PREFIX.to_vec();
signature_bytes.extend_from_slice(&buf);
Some(keypair.sign(&signature_bytes)?)
Some(keypair.sign(&signature_bytes).unwrap())
};

Ok(RawMessage {
Expand Down
2 changes: 1 addition & 1 deletion transports/noise/src/protocol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ impl Keypair {
self,
id_keys: &identity::Keypair,
) -> Result<AuthenticKeypair, Error> {
let sig = id_keys.sign(&[STATIC_KEY_DOMAIN.as_bytes(), self.public.as_ref()].concat())?;
let sig = id_keys.sign(&[STATIC_KEY_DOMAIN.as_bytes(), self.public.as_ref()].concat()).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

ditto; we should use expect here to provide context if it ever does fails and panics. Unlikely though but its more of a just-in-case.


let identity = KeypairIdentity {
public: id_keys.public(),
Expand Down
Loading