Display and Debug for secret keys prints a hash #312

dr-orlovsky · 2021-06-27T17:30:04Z

Extract of concept ACK part of #311 related to changing the way secret keys are displayed/printed out

real-or-random

Concept ACK

elichai · 2021-06-28T09:02:16Z

src/key.rs

+            #[cfg(any(feature = "std", feature = "alloc"))] {
+                #[allow(deprecated)]
+                s.serialize_str(&self.format_secret())
+            }
+            #[cfg(not(any(feature = "std", feature = "alloc")))]
+            s.serialize_bytes(&self[..])


idk if we really want that.
serializing differently depending on features can cause serious breakage.
we can also easily implement stack based str here

Hm, can you elaborate how to do that?

See the last 2 commits here: https://github.com/elichai/rust-secp256k1/tree/pr-312 (feel free to cherry-pick them)

@elichai cherry-picked, thanks!

elichai · 2021-06-28T09:03:56Z

Is there an actual need to print a hash of the private key instead of just <redacted private key>?

dr-orlovsky · 2021-06-28T11:06:57Z

@elichai I think since we need to distinguish two different keys it's good to print their hashes

elichai · 2021-06-29T08:37:52Z

@elichai I think since we need to distinguish two different keys it's good to print their hashes

idk, I'm not a big fun of actual logic inside of Debug prints, and also not sure assuming that it's safe to print a hash of a private key is such a great assumption, @real-or-random @apoelstra what do you think?

real-or-random · 2021-06-29T08:47:42Z

idk, I'm not a big fun of actual logic inside of Debug prints

No strong opinions here. I think it's acceptable for Debug.

and also not sure assuming that it's safe to print a hash of a private key is such a great assumption

I think it's fine. I think even printing the keys out is fine, even though it's better to avoid that footgun. (But if you have Debug prints in your code, it's your fault in the end...)

Hashing is for sure a better idea, and secret keys should have enough entropy. But we should nevertheless tag the tash, i.e., add a prefix like "rust-secp256k1DEBUG" to make sure we don't collide with other Maybe even BIP340-style tagging if bitcoinhashes supports this.

dr-orlovsky · 2021-06-29T11:10:22Z

@devrandom I did tagged hashing. Doing it with bitcoin_hashes::sha256t will require introduction of two new types (for hash tag and for tagged hash) which is an overkill, so I just reproduced the logic right in the code without optimization of tagged hash midstate generation.

devrandom · 2021-06-29T11:22:59Z

Ironically enough, the wrong random person was tagged in the previous message. ;-)

dr-orlovsky · 2021-06-29T12:25:52Z

Ha! That's funny! Sorry :)

@real-or-random, I meant you

TheBlueMatt · 2021-06-30T03:58:56Z

src/macros.rs

+            /// This is the only method that outputs the actual secret key value, and, thus,
+            /// should be used with extreme precaution.
+            #[deprecated(
+                note = "Caution: you are explicitly outputting secret key value! This can be done


If a user says they want to do something, let them do it. Forcing a warning print or an allow statement after they explicitly stated they want it (via an aptly named method call) is just making the library harder to use for no obvious gain. The goal here should be to make sure that the users' code is readable - if they're writing out the secret key it should be really painfully obvious that they are - not to make it actively hard for the user to write out secret key material.

I assume the point of the warning is that users don't forget to remove debug prints from their code after debugging. And this seems indeed helpful.

And if a user actually wants to print a secret for some reason? eg in lightning you might imagine a secret which just represents the payment claim, and which the sender should not consider materially "secret"? Not all cryptographic protocols require that secrets stay out of debug logs, nor should we be the ones to decide that even if the secrets control lots of bitcoin! It's not our job to "fix" client applications, only remove foot guns; if they don't do code review or catch bugs that our well-named functions make obvious, there isn't anything we can do about it, and we shouldn't make the lives of our users harder.

And if a user actually wants to print a secret for some reason? eg in lightning you might imagine a secret which just represents the payment claim, and which the sender should not consider materially "secret"?

Yeah, and you may want to build protocols where public keys are secret. secp256k1 is supposed to be high-level library hard to misuse. As a wrapper we should follow the same philosophy. And this means we need to make an educated guess which values are supposed to be secret, and I think then a secret key is a good candidate. ;)

Not all cryptographic protocols require that secrets stay out of debug logs, nor should we be the ones to decide that even if the secrets control lots of bitcoin! It's not our job to "fix" client applications, only remove foot guns; if they don't do code review or catch bugs that our well-named functions make obvious, there isn't anything we can do about it, and we shouldn't make the lives of our users harder.

I think in this case is foot-gunny. You introduce some Debug prints for debugging, then fix your bug and forget to remove the Debug prints. If you want to print secrets in production, even for logging, there are other ways to do. It's not impossible to do it, we're just forcing the users to think about their intentions.

But of course, the distinction between avoiding foot guns and imposing our will on users is fuzzy, and it's no surprise that different people draw the line in different places. So in the end, I don't care too much.

Look at it another way, what if a user, for whatever reason, legitimately wants to print a secret key? Adding a giant warning every time they compile is just being obnoxious. We can be anti-footgun by making the warning something clearer (I dunno, fmt_secret_key_printing_secret), forcing users to see a scary compile warning is going beyond anti-footgun and assuming users are using the library in exactly the way we intend (which I guess is just on-chain bitcoin, cause this doesn't make sense for lighting).

And if a user actually wants to print a secret for some reason?

Then you should mark such calls with #[allow(deprecated)] as like here: b284e44#diff-0988d5d2259e1a401001675578c38d09c46290b877b66b07fb96a7e034ccdf70R674

Frankly, I find that kind of thing obnoxious to our users. It is not our job to tell users exactly how they must use this library, and we shouldn't force our users to add stupid hacks and workarounds just to use the library. We can help them avoid bugs by making sure their code is clear in their calls to clearly-named functions, but that's about it. Also, this method is not deprecated, it's intended to be in the API, so it's just completely a misnomer and confusing to users on top.

Removed that depreciation part

src/macros.rs

dr-orlovsky · 2021-08-09T15:42:19Z

@TheBlueMatt @thomaseizinger @elichai Tried to fulfill all suggestions, completelly refactored PR history. Now there is no depricated thing, and everything done in the same manner with display_secret() alike it is done for std::Path

thomaseizinger

Thanks!

A few nits, but generally ACK a828af6

src/secret.rs

src/key.rs

dr-orlovsky · 2021-08-21T20:14:51Z

~~@thomaseizinger covered your review comments with the last commit~~

UPDT: @thomaseizinger actually had to revert that change: its impossible to use to_string() as you suggested without relying on the alloc feature, which is not always present for this crate.

apoelstra · 2021-09-13T21:57:31Z

Should remove the commit from #314 and I'm not sure why hashes::hex or formatting with {:x} is insufficient and we need to implement manual hex encoding.

dr-orlovsky · 2021-09-27T11:22:33Z

@thomaseizinger all all suggestions are implemented

@apoelstra commit removed. As for manual hex, we need this function b/c of no-std

Debug-print secrets as tagged hashes Refactoring Display/Debug for secret values with display_secret

apoelstra

ACK 6810c2b

thomaseizinger

ACK 6810c2b

dr-orlovsky · 2021-09-28T04:19:14Z

I assume this can be merged now? At least github says all criteria are fulfilled.

apoelstra · 2021-11-02T17:58:19Z

Yep, thanks

de77518 Serde serialization for KeyPair (Dr Maxim Orlovsky) Pull request description: Serde implementation for `KeyPair` type (which hadn't it before). Based on #312 (includes all commits from that PR, will be rebased upon merge) ACKs for top commit: apoelstra: ACK de77518 Tree-SHA512: 1e75c4fc772dcba5ce7edb30235a58550342cf986c6a77b4affd81defeba456c9655e28b081e0040c1f8440da3f7ad2224485d35222c1921099567b4d1533794

Needed to pump the lightning version used and had to pump bitcoin, bitcoincore-rpc aswell to match dep verions in lightining merkle root computation changed in `bitcoin`, thus some methods in the test_utils needed to adapt (basically by adding a tx if there is none in a block). See: rust-bitcoin/rust-bitcoin@b454cf8 Also SecretKey had it's `to_string` method removed, so TEOS now encodes its tower key using `secret_bytes()` and `from_slice()`. See: rust-bitcoin/rust-secp256k1#312

Needed to bump the lightning version used and had to bump bitcoin, bitcoincore-rpc aswell to match dep verions in lightining merkle root computation changed in `bitcoin`, thus some methods in the test_utils needed to adapt (basically by adding a tx if there is none in a block). See: rust-bitcoin/rust-bitcoin@b454cf8 Also SecretKey had it's `to_string` method removed, so TEOS now encodes its tower key using `secret_bytes()` and `from_slice()`. See: rust-bitcoin/rust-secp256k1#312

Needed to bump the lightning version used and had to bump bitcoin, bitcoincore-rpc aswell to match dep verions in lightining merkle root computation changed in `bitcoin`, thus some methods in the test_utils needed to adapt (basically by adding a tx if there is none in a block). See: rust-bitcoin/rust-bitcoin@b454cf8 Also SecretKey had it's `to_string` method removed, so TEOS now encodes its tower key using `display_secret().to_string()`. See: rust-bitcoin/rust-secp256k1#312

dr-orlovsky changed the title ~~Display and Debug foe secret keys prints a hash~~ Display and Debug for secret keys prints a hash Jun 27, 2021

dr-orlovsky force-pushed the extrakeys/display branch from c7315bb to 2d260f1 Compare June 27, 2021 17:42

This was referenced Jun 27, 2021

Serde implementation for KeyPair type #313

Merged

Improving safety of handling secret keys in different forms #311

Closed

dr-orlovsky force-pushed the extrakeys/display branch 6 times, most recently from adfda2e to 739d36f Compare June 27, 2021 18:47

real-or-random reviewed Jun 28, 2021

View reviewed changes

elichai reviewed Jun 28, 2021

View reviewed changes

TheBlueMatt reviewed Jun 30, 2021

View reviewed changes

elichai reviewed Jun 30, 2021

View reviewed changes

src/macros.rs Outdated Show resolved Hide resolved

thomaseizinger reviewed Jul 14, 2021

View reviewed changes

src/macros.rs Outdated Show resolved Hide resolved

src/macros.rs Outdated Show resolved Hide resolved

src/macros.rs Outdated Show resolved Hide resolved

tcharding mentioned this pull request Jul 29, 2021

Add feature zeroize #262

Closed

dr-orlovsky force-pushed the extrakeys/display branch 4 times, most recently from 0858c9f to a828af6 Compare August 9, 2021 15:40

dr-orlovsky requested review from elichai and TheBlueMatt August 9, 2021 15:42

thomaseizinger previously approved these changes Aug 10, 2021

View reviewed changes

src/secret.rs Outdated Show resolved Hide resolved

src/secret.rs Outdated Show resolved Hide resolved

src/key.rs Outdated Show resolved Hide resolved

src/key.rs Outdated Show resolved Hide resolved

dr-orlovsky dismissed thomaseizinger’s stale review via efaf561 August 21, 2021 20:14

dr-orlovsky force-pushed the extrakeys/display branch 2 times, most recently from e209663 to 73c0779 Compare August 22, 2021 08:53

Add to_hex converter and add tests for hex conversion

635a6ae

dr-orlovsky force-pushed the extrakeys/display branch from 73c0779 to 7cd29e0 Compare September 27, 2021 11:21

dr-orlovsky force-pushed the extrakeys/display branch 2 times, most recently from 1f5d6da to 60f7e58 Compare September 27, 2021 11:52

Dedicated display_secret fn for secret-containing types

6810c2b

Debug-print secrets as tagged hashes Refactoring Display/Debug for secret values with display_secret

dr-orlovsky force-pushed the extrakeys/display branch from 60f7e58 to 6810c2b Compare September 27, 2021 12:04

apoelstra approved these changes Sep 27, 2021

View reviewed changes

thomaseizinger approved these changes Sep 28, 2021

View reviewed changes

apoelstra merged commit 88196bd into rust-bitcoin:master Nov 2, 2021

This was referenced Feb 17, 2022

Don't let Debug or Display accidentally log SecretKeys #226

Closed

Obfuscate shared secret when printing #396

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display and Debug for secret keys prints a hash #312

Display and Debug for secret keys prints a hash #312

dr-orlovsky commented Jun 27, 2021

real-or-random left a comment

elichai Jun 28, 2021

dr-orlovsky Jun 28, 2021

elichai Jun 28, 2021

dr-orlovsky Jun 29, 2021

elichai commented Jun 28, 2021

dr-orlovsky commented Jun 28, 2021

elichai commented Jun 29, 2021

real-or-random commented Jun 29, 2021

dr-orlovsky commented Jun 29, 2021

devrandom commented Jun 29, 2021

dr-orlovsky commented Jun 29, 2021

TheBlueMatt Jun 30, 2021

real-or-random Jun 30, 2021

TheBlueMatt Jun 30, 2021

real-or-random Jun 30, 2021

TheBlueMatt Jun 30, 2021 •

edited

dr-orlovsky Jul 1, 2021

TheBlueMatt Jul 2, 2021

dr-orlovsky Sep 28, 2021

dr-orlovsky commented Aug 9, 2021

thomaseizinger left a comment

dr-orlovsky commented Aug 21, 2021 •

edited

apoelstra commented Sep 13, 2021

dr-orlovsky commented Sep 27, 2021

apoelstra left a comment

thomaseizinger left a comment

dr-orlovsky commented Sep 28, 2021

apoelstra commented Nov 2, 2021

Display and Debug for secret keys prints a hash #312

Display and Debug for secret keys prints a hash #312

Conversation

dr-orlovsky commented Jun 27, 2021

real-or-random left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elichai commented Jun 28, 2021

dr-orlovsky commented Jun 28, 2021

elichai commented Jun 29, 2021

real-or-random commented Jun 29, 2021

dr-orlovsky commented Jun 29, 2021

devrandom commented Jun 29, 2021

dr-orlovsky commented Jun 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Jun 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-orlovsky commented Aug 9, 2021

thomaseizinger left a comment

Choose a reason for hiding this comment

dr-orlovsky commented Aug 21, 2021 • edited

apoelstra commented Sep 13, 2021

dr-orlovsky commented Sep 27, 2021

apoelstra left a comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

dr-orlovsky commented Sep 28, 2021

apoelstra commented Nov 2, 2021

TheBlueMatt Jun 30, 2021 •

edited

dr-orlovsky commented Aug 21, 2021 •

edited