Keypair: implement clone()

[ckamm]

Problem

Keypair::clone() was not implemented upstream in ed25519-dalek to force everyone to think twice before creating another copy of a potentially sensitive private key in memory.

See dalek-cryptography/ed25519-dalek#76

However, there are now 9 instances of

  Keypair::from_bytes(&keypair.to_bytes())

in the solana codebase and it would be preferable to have a function.

In particular since this also comes up when writing programs and can cause users to either start messing with lifetimes or discover the from_bytes() workaround themselves.

Summary of Changes

Add clone() to Keypair.

This patch opts to not implement the Clone trait. This avoids automatic use in order to preserve some of the original "let developers think twice about this" intention.

[mvines]

I see the utility but am concerned about using .clone() since that makes it very easy to misuse. Some options:

1. .clone_if_you_really_must_but_please_dont()
2. Mark the .clone() as unsafe to make it painful to use

[ckamm]

How about we put the clone() impl in a separate file and force use sdk::signer::keypair_clone_if_you_must before it becomes available? That would make using it in tests more straightforward and would still stick out a lot.

Alternatively there could be an UnimportantKeypair that implements clone() ;)

[mvines]

How about we put the clone() impl in a separate file and force use sdk::signer::keypair_clone_if_you_must before it becomes available? That would make using it in tests more straightforward and would still stick out a lot.

Yeah, something like that sgtm!

[ckamm]

@mvines Updated so you need to explicitly use a separate KeypairInsecureClone trait to use the function.

[mvines]

lgtm once CI is green

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[mvines]

Looks like a rebase is needed to resolve the cargo audit issue in CI

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[ckamm]

@mvines Please merge if you think this makes sense to have.

[CriesofCarrots]

Thanks for the contribution!

[behzadnouri]

What is the benefit of using a trait here?
Why not just add an unsecure_clone or mock_clone method to Keypair?
We have been using mock_clone already elsewhere in the code:
https://github.com/solana-labs/solana/search?q=mock_clone

The KeypairInsecureClone trait has the downside that if the file is already importing this trait then later on someone can erroneously add another keypair.clone elsewhere unknowingly that this is not right but the code compiles fine and there is no indication that they should avoid this.
It also does not help that method is just called .clone.

[ckamm]

There's no advantage to the trait. The goal was to gate the function via the suspicious-looking use. Making each callsite stick out sounds better. I wasn't aware of mock_clone.

[behzadnouri]

@ckamm Do you agree this is introducing this risk:

The KeypairInsecureClone trait has the downside that if the file is already importing this trait then later on someone can erroneously add another keypair.clone elsewhere unknowingly that this is not right but the code compiles fine and there is no indication that they should avoid this.

[ckamm]

@ckamm Do you agree this is introducing this risk:

The KeypairInsecureClone trait has the downside that if the file is already importing this trait then later on someone can erroneously add another keypair.clone elsewhere unknowingly that this is not right but the code compiles fine and there is no indication that they should avoid this.

Yes. Marking at each callsite would avoid accidental bugs like this and stick out more while reviewing.

[t-nelson]

wondering why the solution here was to implement Clone and not eliminate those unsafe instances by Arc wrapping them?