feat: Add a deterministic (BIP-39) keygen #590

antondlr · 2025-09-08T09:53:41Z

Proposed Changes

Adds a deterministic BIP-39 based option to anchor keygen.
As a node operator, I highly prefer securing seed phrases over raw key material.

Additional Info

This is 100% Claude's work. I have no idea what I'm doing. Seems to work fine but may be ultra dangerous. etc.

cla-assistant · 2025-09-08T09:54:02Z

All committers have signed the CLA.

cla-assistant · 2025-09-08T09:54:08Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

dknopik · 2025-09-08T10:00:16Z

anchor/keygen/src/lib.rs

+fn generate_deterministic_rsa_key(mnemonic: &Mnemonic, index: u32) -> Result<Rsa<Private>, KeygenError> {
+    // Convert mnemonic to seed using BIP39 derivation
+    let seed = mnemonic.to_seed("");
+    let seed_bytes = &seed;
+
+    // Create info string for HKDF using the derivation index
+    let info = format!("anchor-rsa-key-{}", index);


The index does not seem to be actually used to create a derivation path for the seed?

hmm, I had generated an index 0 and an index 1 key and they were different. same index, same key luckily haha.

Ah I guess the "info" does that

dknopik · 2025-09-08T10:07:18Z

test_keygen_standalone/src/main.rs

+    let mut key_material = [0u8; 512];
+    hkdf.expand(info.as_bytes(), &mut key_material)
+        .map_err(|e| format!("HKDF expansion failed: {}", e))?;
+
+    // Use the derived key material to deterministically generate RSA parameters
+    generate_rsa_from_seed(&key_material)
+}
+
+fn generate_rsa_from_seed(seed: &[u8]) -> Result<Rsa<Private>, Box<dyn std::error::Error>> {
+    // Create a deterministic RNG from the seed
+    let mut rng = StdRng::from_seed({
+        let mut seed_array = [0u8; 32];
+        seed_array.copy_from_slice(&seed[..32]);
+        seed_array
+    });


We're expanding the seed to 512 bytes just to cut it down to 32 again.

I feel like it makes more sense to use the expanded key material directly instead of passing it through a RNG again

Copilot

Pull Request Overview

This PR adds deterministic BIP-39 based key generation to the anchor keygen command. The feature allows node operators to generate RSA keys from mnemonic seed phrases, providing a more familiar and manageable way to secure key material compared to raw private keys.

Implements BIP-39 mnemonic-based deterministic RSA key generation using HKDF for key derivation
Adds CLI options for mnemonic input (direct string, file, or auto-generation) with derivation index support
Includes comprehensive test coverage and documentation for the new deterministic key generation feature

Reviewed Changes

Copilot reviewed 6 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
test_keygen_standalone/src/main.rs	Standalone test implementation demonstrating deterministic RSA key generation from BIP-39 mnemonics
test_keygen_standalone/Cargo.toml	Dependencies for the standalone test including bip39, hkdf, openssl, rand, and sha2
anchor/keygen/src/lib.rs	Main implementation adding deterministic key generation with new CLI options, key derivation logic, and comprehensive tests
anchor/keygen/README.md	Updated documentation explaining deterministic key generation usage and security considerations
anchor/keygen/Cargo.toml	Added required dependencies for BIP-39 and cryptographic operations
.github/wordlist.txt	Added BIP-39 and cryptography-related terms to the spelling allowlist

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-08T13:30:11Z

anchor/keygen/src/lib.rs

+    for i in 0..128 {
+        p_bytes[i] ^= p_rng.next_u32() as u8;
+        q_bytes[i] ^= q_rng.next_u32() as u8;
+    }


Using only the lowest byte of next_u32() reduces entropy from 32 bits to 8 bits per iteration. Consider using rng.fill_bytes() or extracting all 4 bytes from each u32 to maintain full entropy.

Copilot · 2025-09-08T13:30:11Z

anchor/keygen/src/lib.rs

+    // XOR the first 128 bytes of extra entropy into p_bytes
+    for i in 0..128 {
+        p_bytes[i] ^= extra_entropy[i];
+    }
+    // XOR the remaining 64 bytes of extra entropy into q_bytes (cycling through)
+    for i in 0..64 {
+        q_bytes[i] ^= extra_entropy[128 + i];
+        q_bytes[i + 64] ^= extra_entropy[128 + i]; // Use each byte twice for full coverage


Reusing the same entropy byte for two different positions (q_bytes[i] and q_bytes[i + 64]) reduces the effective entropy. Consider using all 192 bytes of extra_entropy sequentially or with a different mixing strategy.

Suggested change

// XOR the first 128 bytes of extra entropy into p_bytes

for i in 0..128 {

p_bytes[i] ^= extra_entropy[i];

}

// XOR the remaining 64 bytes of extra entropy into q_bytes (cycling through)

for i in 0..64 {

q_bytes[i] ^= extra_entropy[128 + i];

q_bytes[i + 64] ^= extra_entropy[128 + i]; // Use each byte twice for full coverage

// XOR the first 96 bytes of extra entropy into p_bytes

for i in 0..96 {

p_bytes[i] ^= extra_entropy[i];

}

// XOR the next 96 bytes of extra entropy into q_bytes

for i in 0..96 {

q_bytes[i] ^= extra_entropy[96 + i];

diegomrsantos · 2025-09-08T21:52:20Z

Should the tests be inside the keygen dir?

AgeManning

I have had a small look through this.

Although I think this has great utility, I dont think its worth the security risk and we shouldn't promote it to the public. Here are my reasons:

It builds the RSA components itself, against the general rule of "Dont roll your own crypto"
RSA relies fairly heavily on generating random primes. It looks like this uses the seed for a PRNG but then sequentially looks for primes from this starting point. It not obvious to me that this method is as secure and randomly searching for primes, I suspect not.
This PR would require significant review before we could think of adding this to production.

One way of doing this that I'd personally feel a bit better about, is just straight using a rust-based RSA library, removing all component generation and just creating the RNG trait which is seeded by the mnemonic seed. i,e we use this function: https://docs.rs/rsa/latest/src/rsa/key.rs.html#217-220

The mnemonic seed bytes is only 64 bytes, although it uses pbkdf and can be modified to generate larger key sizes, which may be necessary.

EDIT: There exists this thing: https://docs.rs/deterministic-keygen/latest/deterministic_keygen/fn.derive_rsa_key_from_phrase.html

Which looks like it creates the PRNG from the seed.

AgeManning · 2025-09-15T06:54:19Z

anchor/keygen/src/lib.rs

+    #[clap(
+        long,
+        help = "BIP39 mnemonic phrase for deterministic key generation (12 or 24 words)",
+        requires = "deterministic"
+    )]
+    pub mnemonic: Option<String>,


I dont think we should allow mnemonics on the CLI. They will end up in the processes list and shell history.

antondlr added 2 commits September 8, 2025 11:49

claude take the wheel: add deterministic keygen

6e8b738

fix compilation issues

faf4495

dknopik reviewed Sep 8, 2025

View reviewed changes

fix fmt, cpellcheck

574449d

dknopik reviewed Sep 8, 2025

View reviewed changes

antondlr added 2 commits September 8, 2025 12:32

use all of the entropy.. i think?

ac6d91c

fix spellcheck, fmt, lint

defc9b6

diegomrsantos requested a review from Copilot September 8, 2025 13:29

Copilot AI reviewed Sep 8, 2025

View reviewed changes

AgeManning reviewed Sep 15, 2025

View reviewed changes

feat: Add a deterministic (BIP-39) keygen #590

Are you sure you want to change the base?

feat: Add a deterministic (BIP-39) keygen #590

Uh oh!

Conversation

antondlr commented Sep 8, 2025

Proposed Changes

Additional Info

Uh oh!

cla-assistant bot commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cla-assistant bot commented Sep 8, 2025

Uh oh!

dknopik Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

antondlr Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

dknopik Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

dknopik Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

diegomrsantos commented Sep 8, 2025

Uh oh!

AgeManning left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AgeManning Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cla-assistant bot commented Sep 8, 2025 •

edited

Loading

AgeManning left a comment •

edited

Loading