Skip to content

Comments

Rsa keygen#184

Merged
jking-aus merged 84 commits intosigp:unstablefrom
Zacholme7:rsa-keygen
Mar 18, 2025
Merged

Rsa keygen#184
jking-aus merged 84 commits intosigp:unstablefrom
Zacholme7:rsa-keygen

Conversation

@Zacholme7
Copy link
Member

Issue Addressed

#154

Proposed Changes

This pr introduces a new cli tool to generate rsa keys. It will generate a key.pem file to be used with the anchor nodes and a keys.json file that contains the base64 encoded keys to be used with the ssv ui webapp.

Note: this replaces the base64 headers in the key so that we can interop with the ssv-client for ssv-mini

@Zacholme7 Zacholme7 added enhancement New feature or request ready-for-review This PR is ready to be reviewed labels Mar 12, 2025
@Zacholme7 Zacholme7 requested review from dknopik and jking-aus March 12, 2025 13:35
Copy link
Member

@dknopik dknopik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

What do you think about changing the client to implicitly invoke this if it finds no key? Replacing its current key gen mechanism.

@Zacholme7 Zacholme7 requested a review from dknopik March 13, 2025 12:39
Copy link
Member

@dknopik dknopik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks! Just a nitpick:

@ThreeHrSleep ThreeHrSleep requested a review from dknopik March 14, 2025 06:35
@dknopik
Copy link
Member

dknopik commented Mar 14, 2025

Looks good! Another thing that came to my mind is zeroing the memory after use with zeroize. It's a bit complicated to do correctly (as moves might leave unzeroed memory, but clones don't) and it's difficult to reason about invoked library functions, but even doing it on a "best effort" basis would be better than nothing. Do you want to try it? If so, I recommend reading through the main docs page: https://docs.rs/zeroize/latest/zeroize/

@Zacholme7
Copy link
Member Author

Update: Need the ability to be able to force overwrites in ssv-mini so added cli flag to accomplish this

@Zacholme7
Copy link
Member Author

We are writing passwords out in plaintext right now, which kinda defeats the purpose of zeroing out the memory.

Adding an issue to enable password protection for the private key file.

@diegomrsantos diegomrsantos requested a review from Copilot March 17, 2025 15:18
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a new CLI tool for RSA key generation to support anchor nodes and the SSV UI webapp by generating a PEM file and a JSON file with base64 encoded keys.

  • Added a new Cargo package (keygen) with dependencies for key generation.
  • Implemented the key generation functionality in keygen’s library and integrated it into the existing anchor and client command flows.
  • Updated related Cargo.toml files and a keysplit utility to support the new keygen module.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
anchor/keygen/Cargo.toml Added package metadata and dependencies for the RSA keygen tool.
anchor/keygen/src/lib.rs Implemented RSA key generation with PEM and JSON output functionality.
Cargo.toml Added keygen to workspace members and dependencies.
anchor/src/main.rs Integrated the keygen subcommand into the main command interface.
anchor/client/src/lib.rs Replaced manual key generation with a call to the new run_keygen API.
anchor/keysplit/src/util.rs Updated error handling for UTF-8 conversion in key serialization.
anchor/Cargo.toml Added workspace configuration for the keygen package.
anchor/client/Cargo.toml Added workspace configuration for the keygen dependency.

Copy link
Member

@jking-aus jking-aus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice -- just left a few todo comments so we remember to encrypt

@jking-aus jking-aus merged commit fc9b939 into sigp:unstable Mar 18, 2025
10 checks passed
@Zacholme7 Zacholme7 deleted the rsa-keygen branch March 18, 2025 12:37
@dknopik dknopik removed the ready-for-review This PR is ready to be reviewed label Nov 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants