This PR implements a `non_secure_erase()` method on SecretKey,
KeyPair, SharedSecret, Scalar, and DisplaySecret. The purpose
of this method is to (attempt to) overwrite secret data with
valid default values. This method can be used by libraries
to implement Zeroize on structs containing secret values.
`non_secure_erase()` attempts to avoid being optimized away or
reordered using the same mechanism as the zeroize crate: first,
using `std::ptr::write_volatile` (which will not be optimized
away) to overwrite the memory, then using a memory fence to
prevent subtle issues due to load or store reordering.
Note, however, that this method is *very unlikely* to do anything
useful on its own. Effective use involves carefully placing these
values inside non-Copy structs and pinning those structs in place.
See the [`zeroize`](https://docs.rs/zeroize) documentation for tips
and tricks, and for further discussion.
[this commit includes a squashed-in commit from tcharding to fix docs
and helpful suggestions from apoelstra and Kixunil]
e0e575dde7 Run cargo fmt (Tobin C. Harding)
41449e455d Prepare codebase for formatting (Tobin C. Harding)
7e3c8935b6 Introduce rustfmt config file (Tobin C. Harding)
Pull request description:
(Includes the patch from #504, I pulled it out of this to merge faster)
Introduce `rustfmt` by doing:
- Copy the `rustfmt` config file from `rust-bitcoin`
- Prepare the codebase by adding `#[rustfmt::skip]` as needed and doing some manual format improvements.
- Run the formatter: `cargo +nightly fmt`
- Add formatting checks to CI and the pre-commit hook
Thanks in advance for doing the painful review on patch 3.
ACKs for top commit:
apoelstra:
ACK e0e575dde7
Tree-SHA512: 1b6fdbaf81480c0446e660cc3f6ab7ac0697f272187f6fdfd6b95d894a418cde8cf1c423f1d18ebbe03ac5c43489630a35ad07912afaeb6107cfbe7338a9bed7
Run the command `cargo +nightly fmt` to fix formatting issues.
The formatter got confused in one place, adding an incorrect
indentation, this was manually fixed.
The `ONE_KEY` is only used in two rustdoc examples, as such it
unnecessarily pollutes the crate root namespace. We can use
`SecretKey::from_str()` with no loss of clarity and remove the
`ONE_KEY`.
While we are touching the import statements in `secret.rs` elect to
remove the hide (use of `#`) for import statements relating to this
library. Doing so gives devs all the information they need in one place
if they are using the examples to copy code. It is also in line with the
rest of the codebase.
Currently we have a feature `bitcoin-hashes-std` and a dependency
`bitcoin_hashes`, this means one has to think about and change the `_`
and `-` when coding. The underscore in `bitcoin_hashes` is an artifact
of days gone by and we cannot fix it but we can cover it up and make our
lives easier, especially now we have `bitcoin-hashes-std`.
Improve feature usage of the `bitcoin_hashes` library by:
- Add a feature `bitcoin-hashes` that enables `bitcoin_hashes`.
- Use the new feature in all feature gated code
- Use `bitcoin-hashes-std` in feature gated code that includes other
`std` features (e.g. `rand-std`)
`SecretKey` implements `Copy` and it is fine to take owneship of it; we
have multiple methods called `from_secret_key` and they all borrow the
secret key parameter. Favour consistency over perfection.
Borrow secret key parameter as is done in other `from_secret_key`
methods.
Currently printing the `SharedSecret` using `Display` or `Debug` prints
the real secret, this is sub-optimal. We have a solution for other
secrets in the project where printing is obfuscated and we provide a
`display_secret` method for explicitly printing.
Mirror the logic for other secrets and obfuscate the `SharedSecret` when printing.
Improve rustdocs on `display_secret` by doing:
- Minor improvements to the rustdocs to aid readability in the editor.
- Do not guarantee (`assert_eq!`) debug output
The `serialize_secret` method is a getter method, it does not do any
serialisation. However we use the method on secret keys and key types so
in order for the name to be uniform use the descriptive name
`secret_bytes`.
Rename `serialize_secret` to be `secret_bytes`.
The identifier `i` is predominantly used for indexing an array but we
are using it as a place holder for the iterated value of an array that
is then printed. The identifier `byte` is more descriptive.
Done in preparation for adding similar code to the `ecdh` module.
In array initialisation we use magic number 64, this is the secret bytes
length multiplied by 2.
Please note; we still use the magic number 32, left as such because it
is used in various ways and its not immediately clear that using a
single const would be any more descriptive.
Use `SECRET_KEY_SIZE * 2` instead of magic number 64.
Hashing the debug output for secrets can be done with `bitcoin_hashes`
not just `std`. Mention this in the obfuscated string output when
neither are available.
The `Debug` implementation for secrets is feature gated on `std` because
it uses a hasher from `std`. If `bitcoin_hashes` is enabled we can use
it for hashing. If neither `std` nor `bitcoin_hashes` is enabled fall
back to outputting:
<secret requires std or bitcoin_hashes feature to display>
Remove the docs conditional since we now implement `Debug` always.
Rustc can warn us when we forget to add `Copy` and `Deubg` trait
implementations to types.
Add lint directives to enable warnings for missing `Copy` and `Debug`
implementations. Use the newly emitted warnings to find types that do
not implement our 'standard' traits. These 'standard' traits are defined
as the set of attributes that it has been found beneficial to
opportunistically add to all types, these are
- Copy
- Clone
- Debug
- PartialEq and Eq
- PartialOrd and Ord
- Hash
This documents the Cargo features making sure docs.rs shows warning for
feature-gated items. They are also explicitly spelled out in the crate
documentation.
kwantam
Andrew Poelstra
Tobin C. Harding
Elichai Turkel
Martin Habovstiak
Thomas Eizinger
Dr Maxim Orlovsky