This lib, and bitcoin_hashes, should have a cfg param for logging everything that it does

[apoelstra]

We should have a cfg flag, which if set alongside an environment variable CRYPTO_DEBUG_FILE or something, logs all actions to that file. Meaning, at the very least:

• When hashing data (or feeding it into a preimage), log the data and the resulting hash/midstate
• When signing data, log the pubkey, message and resulting signature
• When generating keys, log the privkey and corresponding pubkey
• When tweaking keys, log the inputs and results

Obviously this would be extremely unsafe and the cfg flag should be named danger_leak_secret_data or something long and scary, but it would be really helpful when debugging higher level crypto protocols (which seem to always fail in the form of "this uniformly random number does not equal this other uniformly random number") it would be extremely helpful for figuring out what went wrong.

For rust-secp-zkp it may also make sense to instrument the C code, since there are much more complicated crypto algos there, but as a first step just logging the inputs and outputs of the API-level functions would give us a ton of information about why, e.g. transaction signatures aren't passing.

We could require a high MSRV and std for this feature to work, so we could just have a static LOGFILE: Mutex<fs::File> as the log sink and not need any unsafe code or extra deps.

cc @rustyrussell who suggested this to me..

[tcharding]

If this is going to be feature gated and only used during debugging then is it ok to add dependencies and use a logging framework?

[apoelstra]

@tcharding yes, but I have no idea how to gate dependencies on cfg params

[tcharding]

Use an optional dependency and enable it by the feature, right?

[tcharding]

I hacked up a demo in #498

[apoelstra]

We definitely cannot use cargo features for this because it would make it too easy to turn on accidentally (or too easy for a malicious dependency to turn on). It will also make --all-features leak secret data.

[tcharding]

We should have a cfg flag,

What did you have in mind? A custom configuration conditional (like we do for bench) suffers the same easy attackability as a normal configuration conditional. I can't think of any other way to use cargo that does not suffer the same weakness?

[apoelstra]

suffers the same easy attackability

It doesn't. You can't set a cfg flag by accident, and it's not obvious to me that you can set it at all from a dependency. But merely avoiding the accidental cases is reason enough not to use cargo features.

[tcharding]

Ok, I'm convinced that if we use a custom config option "logging" a malicious dependency cannot turn it on. To convince myself I created crates app, mal, secp.

• secp: dummy crate that has a method add that includes logging guarded with #[cfg(logging)].
• mal: a crate that provides a function and trys to enable logging in secp
• app: an executable crate that depends on secp and mal

I turned on logging secp in the mal crate using .cargo/config, and I tried unsuccessfully to turn it on by setting env vars in a build script. Non of this turned it on in app.

[tcharding]

With that out of the way, can we use the log crate (as in #498) or do we want to log to a file by hand (as in #496)?

[tcharding]

Woke up last night thinking about this. I don't know exactly how cargo uses processes but if the build is done in a single process (or secp is built by the same process that builds mal or a child of that process) then isn't this attack still possible (continuing on from above example)

• mal sets RUSTFLAGS in its build.rs (ref: https://doc.rust-lang.org/cargo/reference/build-scripts.html#rustc-env)
• when app is being built, if mal is built before scep then logging will be turned on

I don't know how cargo selects the build order of dependencies or if it is possible to control/influence this from a dependency.

[apoelstra]

If this is possible it sounds like a bug in cargo that dependencies can have non-local effects on each other like this. But I don't know if it's possible.

With that out of the way, can we use the log crate

I don't think so. Can you make dependencies be gated on a RUSTFLAGS option?

[tcharding]

Oh sick, TIL! check this out:

[target.'cfg(demo)'.dependencies]
log = "0.4"

[apoelstra]

Nice! Let's do it.

[tcharding]

Can I clarify please, based on this comment being posted after the one above we are not going to use the log dependency but use stdlib only and write to a file? (Even though that target thing is a very sweet trick.)

[apoelstra]

@tcharding sorry to be inconsistent!

After seeing your two attempts, I prefer avoiding the log dependency, because the diff is basically the same length with or without the extra dep, and because cfg-gated deps still have a minor cost (to us, maintaining Cargo.toml, and to users of cargo vendor, which downloads every dep regardless of whether it might possibly be used or not).

[tcharding]

No sweat, cheers.