SeedSigner / seedsigner

Use an air-gapped Raspberry Pi Zero to sign for Bitcoin transactions! (and do other cool stuff)
MIT License
731 stars 171 forks source link

[RFC] CI for Ensuring Deterministic Transaction Generation and Stability #590

Open OliverOffing opened 3 months ago

OliverOffing commented 3 months ago

Following the recent chat on X.com about 'Dark Skippy' - the sneaky exfiltration of master secret seeds by dodgy signing devices - I'm wondering if there's any interest in a Continuous Integration (CI) job. This job would use seedsigner's code to sign a transaction, and check that the signature doesn't change when a Pull Request is merged.

The idea here is to help prevent attacks where malware might be snuck into the codebase through a series of seemingly harmless pull requests. These could look like small refactors or new features, but once they're all merged, they might do something we don't want.

I'd be more than willing to assist in the implementation of this feature, but I wanted to gauge interest and ensure I'm not overlooking any potential concerns or complications.

What do you think?

kdmukai commented 3 months ago

Thanks to @dbast we already have CI running our test suite on each PR update. And we have this test:

https://github.com/SeedSigner/seedsigner/blob/dev/tests/test_decodepsbtqr.py#L216

That test wasn't really meant to be a rigorous signature checker, but since the expected result is hard-coded, it's effectively serving the purpose you describe above.

However, it would be better to write an explicit standalone test (in tests/test_seed.py?). Even better if the test data uses a test vector from, say, Bitcoin Core's test suite.

jdlcdl commented 3 months ago

Could even call it something like "test_deterministic_signatures_never_change.py" so that we'd all be up-in-arms on any pr that deletes lines from the test.

kdmukai commented 3 months ago

Also note that such a test also belongs in embit which relays the actual signing/nonce generation to secp256k1. I took a quick look through the embit test suite and didn't find a relevant test, though I easily could have missed it.

I think it makes sense to have redundant tests in BOTH SeedSigner and embit.

So if no such test exists in embit, whoever takes this on for SeedSigner should also PR a test into embit.

dbast commented 3 months ago

this also makes the CI results on Github more trustable https://github.com/SeedSigner/seedsigner/pull/568 as plain action versions are git tags and thus are mutable = can influence the workflow outcome if manipulated....

(another step on top would be installing dependencies during CI via poetry lock files instead of requirement*.txt files, to validate the sha256 of all installed dependencies during CI runs)

kdmukai commented 3 months ago

(another step on top would be installing dependencies during CI via poetry lock files instead of requirement*.txt files, to validate the sha256 of all installed dependencies during CI runs)

I don't have any experience with Poetry but am interested to learn more.

Its (optional) compatibility with our existing requirements.txt seems, at a minimum, an easy, obvious win. Dunno how much it makes sense to keep a toe in both worlds or to just rip the bandaid off and do a full switch. But @dbast let's pull this out into its own convo / PR.

https://python-poetry.org/docs/cli/#export

OliverOffing commented 3 months ago

Alternatively, there's uv. I've been experimenting with it and having good results. It generates a requirements.txt file with fixed versions for all dependencies, which personally seems to be the solution with the most compatibility.