Improving safety of handling secret keys in different forms

[dr-orlovsky]

This PR tries to improve safety in working with data types containing secret key related information (SecretKey, ffi::KeyPair, KeyPair). In particular, it

• zeroes inner representations on drop operations
• prohibits copy
• removes explicit hex display, lower hex and debug representation
• prints debug information as a (partial) hash of the secret key data, either SHA256 (if bitcoin_hashes feature is used) or using rust default hasher (if the feature is not used, but either alloc or std is present)
• introduced special functions for disclosing secret hex in hexadecimal form, marked as deprecated (so compiler will always generate a warning)
• introduces "serde-secret" feature required to do a serde serialization of secret-key related types
• removes AsRef<[u8]> and index access methods to secret key values

Closes #226 and provides alternative to #262 without introducing zeroize dependency

[TheBlueMatt]

prints debug information as a (partial) hash of the secret key data, either SHA256 (if bitcoin_hashes feature is used) or using rust default hasher (if the feature is not used, but either alloc or std is present)

Wouldn't it be more useful to get the public key and print something meaningful like "secret key for pubkey: X"?

introduced special functions for disclosing secret hex in hexadecimal form, marked as deprecated (so compiler will always generate a warning)

I don't think we want to have functions that its impossible to use without warnings - if someone types "print_secrety_key_hex()" we don't need to nag them about it.

removes AsRef<[u8]> and index access methods to secret key values

Why?

[dr-orlovsky]

Wouldn't it be more useful to get the public key and print something meaningful like "secret key for pubkey: X"?

Good idea, will implement
Producing a public key requires Ssecp256k1 signing context (which is not available inside Debug::fmt, thus it has to be generated each time for printing out debug operation) and expensive elliptic curve multiplication operation. Thus, I think its better to keep hashes :(

I don't think we want to have functions that its impossible to use without warnings - if someone types "print_secrety_key_hex()" we don't need to nag them about it.

It is possible to use them w/o warning, as I did in tests: prefixing function call with #[allow(deprecated)], so it can be done only consciously and will be always visible in the code

removes AsRef<[u8]> and index access methods to secret key values
Why?

Because allowing access to underlying byt data leaves us without control of how many copies of secret keys are produced in the code

[TheBlueMatt]

It is possible to use them w/o warning, as I did in tests: prefixing function call with #[allow(deprecated)], so it can be done only consciously and will be always visible in the code

Frankly I find that really obnoxious. If the function is named appropriately I don't see any reason why adding an allow tag improves readability of the call site.

Because allowing access to underlying byt data leaves us without control of how many copies of secret keys are produced in the code

So? It's perfectly reasonable for a client application to make a byte copy, almost all probably have to at one time or another. I don't think a client may accidentally do so with the slice accessors.

Note that we absolutely do not have any control of how many copies of secret keys are in memory even with this patch - without forcing them all onto the stack inside a box (something we probably don't want to do), rustc will happily copy private keys all over the place.

[TheBlueMatt]

Producing a public key requires Ssecp256k1 signing context (which is not available inside Debug::fmt, thus it has to be generated each time for printing out debug operation) and expensive elliptic curve multiplication operation. Thus, I think its better to keep hashes :(

I think we should just stick with the private key bytes unless there's something more clear we can use. Maybe just a few of them?

[apoelstra]

concept NACK the attempted zeroing of secret data; see the other discussions in #262 and #102 for reasoning.

I like the idea of only outputting hashes on Debug/Display, this at least lets you distinguish distinct pubkeys.

I like the idea of outputting a truncated version of the secret key a lot less. If you were to output even a byte of a secret nonce, say, then using lattice methods and 30-40 signatures somebody could extract the entire secret key, so truncating doesn't really protect users who leak secrets into insecure logs.

[dr-orlovsky]

Closed in favor of #312 and #313 which has the concept ACKs part of this PR (and no Drop/Copy removal)