Serde implementation for KeyPair type

[dr-orlovsky]

Serde implementation for KeyPair type (which hadn't it before).

Based on #312 (includes all commits from that PR, will be rebased upon merge)

[TheBlueMatt]

Why? Is Serde intended to be used for human-readable objects? Users would be totally reasonable to want to write secret key material to disk, and this makes them jump through hoops to do so. I'm all for removing foot guns, but not at the cost of disabling key library features without a flag.

[dr-orlovsky]

@TheBlueMatt am I getting you right that you propose to reverse feature flag introduced by this PR, so instead of #[cfg(feature = "serde-secrets")] we should do #[cfg(not(feature = "serde-protect-secrets"))]?

[elichai]

@TheBlueMatt am I getting you right that you propose to reverse feature flag introduced by this PR, so instead of #[cfg(feature = "serde-secrets")] we should do #[cfg(not(feature = "serde-protect-secrets"))]?

I don't believe this should be guarded by a unique feature, it's one thing to protect against accidental logging secrets via Debug implementation, but it's another thing to prevent serialization.
IMHO Almost any user of serde in this library will probably also want to serialize secret keys and KeyPairs

[dr-orlovsky]

Well, I am a user of this library and I do not want serde to default-serialize any private information. So I will prefer to be able to specify a feature which opts out from such thing.

[apoelstra]

We absolutely cannot do this with feature flags. It would cause users' secret key data serialization to change when their dependencies changed their set of feature flags.

[thomaseizinger]

Similar to how we don't implement Display on SecretKey but on an adapter struct, we could just not implement Serialize on SecretKey itself but provide two adapter structs and make it easy to create them from a SecretKey:

struct SerdeSecretKeyPlain(SecretKey);
struct SerdeSecretKeyRedacted(SecretKey);


impl SecretKey {
	pub fn serialize_plain() -> SerdeSecretKeyPlain;
	pub fn serialize_redacted() -> SerdeSecretKeyPlain;
}

impl Serialize for SerdeSecretKeyPlain {
   // serialize as bytes
}


impl Serialize for SerdeSecretKeyRedacted {
   // serialize as hashed bytes (or something else)
}

Bonus point for adding a trybuild test that ensures none ever implements Serialize on SecretKey directly:

fn can_serialize<T: Serialize>(_t: T)  { }

// this test needs to go into a trybuild configration that is expected to fail
#[test]
fn secret_key_does_not_serialize() {
	can_serialize(SecretKey::random())
}

[apoelstra]

Sure, but I can't think of a non-test example of secret key usage where you don't want to store them. Secret keys are not ephemeral. If you want to prevent them being stored then I think the onus is on you to create your own adaptor struct.

[real-or-random]

Sure, but I can't think of a non-test example of secret key usage where you don't want to store them.

One common example is that the secret key is derived from some seed/master key and so you don't need to store the derived key. Another example is ephemeral key exchange.

But I agree, it should be possible to serialize secret keys. Many very legit ways to use secret keys need storage.

[dr-orlovsky]

Removed from this PR everything related to preventing serde serialization. Now this PR is only about adding serde to KeyPair. It is still based on #312 since it requires display secret method.

[apoelstra]

Now that #312 is merged can this be a non-WIP?

[dr-orlovsky]

@apoelstra ready

[apoelstra]

ACK 266ea94

but now it needs a rebase :)

[dr-orlovsky]

@apoelstra rebased

[apoelstra]

ACK de77518