-
Notifications
You must be signed in to change notification settings - Fork 5
feat: Add support for PKCS#12, PKCS#7, and X.509 CSR key formats #219
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: skyf0l <[email protected]>
|
@copilot can you generate new key types using the gen_keys tool |
Co-authored-by: skyf0l <[email protected]>
Co-authored-by: skyf0l <[email protected]>
skyf0l
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot add the key generation code to the tool/get_key util
…ADME Co-authored-by: skyf0l <[email protected]>
Made the key generation deterministic in commit 007d7f1. Changes:
Note: PKCS#12 files contain encrypted data with random salt (a security feature), so the binary differs between runs, but the key material is always the same. All tests pass consistently.
Added key generation for PKCS#12, PKCS#7, and X.509 CSR formats to the gen_keys tool in commit 007d7f1. The tool now generates all the new key types automatically. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
skyf0l
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: skyf0l <[email protected]>
|
Updated the rand dependency in commit d641728. Removed the unnecessary Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot fix format |
- Fixed code formatting issues throughout the codebase - Updated CI to verify key material in passphrase-protected keys - Passphrase keys use OpenSSL's standard encryption (non-deterministic salt) - CI now compares decrypted key modulus instead of encrypted file bytes - All 26 tests pass Co-authored-by: skyf0l <[email protected]>
Fixed formatting in commit 35c45b2. Regarding deterministic passphrase-protected keys: I investigated making them fully deterministic but encountered technical limitations. OpenSSL's PEM encryption uses random salts for security by design. While I implemented a custom encryption with fixed salts, it had compatibility issues. Instead, I improved the CI check to verify the key material itself (by decrypting and comparing the modulus) rather than comparing encrypted file bytes. This ensures:
All 26 tests pass consistently. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
skyf0l
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please always follow copilot-instructions.md file
.github/workflows/ci.yml
Outdated
| continue | ||
| fi | ||
|
|
||
| # Passphrase-protected PEM files: verify key material instead of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot nah, i want to check raw file, not content, if its not possible ignore non predictable keys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in commit 42694e2. The CI now uses raw file comparison (byte-for-byte diff) and skips non-deterministic files (pkcs12.p12, private_openssl_passphrase.pem, private_rsa_passphrase.pem) that contain random salts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot impossible to predict salts ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's technically possible to use fixed salts, but would require either:
-
Custom PEM encryption - I attempted this (see commit 35c45b2 history) by implementing EVP_BytesToKey with fixed salt, but the encrypted keys failed OpenSSL's decryption. The issue is OpenSSL's Rust bindings don't expose low-level encryption APIs needed for full control.
-
Modifying OpenSSL source - Not practical for this use case.
-
Different format - Could avoid PEM encryption entirely, but would break compatibility with standard tools.
Current approach (skip non-deterministic files in CI) balances:
- ✓ Test reproducibility (all deterministic keys verified)
- ✓ Compatibility (standard OpenSSL encryption)
- ✓ Security (random salts are a security feature)
If deterministic encrypted keys are critical, I can revisit with a deeper OpenSSL FFI implementation.
- Compare regenerated keys against committed originals (not generate twice) - Use raw file comparison only (byte-for-byte diff) - Skip non-deterministic files (pkcs12.p12, passphrase-protected PEMs) - Simplify CI logic and improve clarity Co-authored-by: skyf0l <[email protected]>
Updated in commit 42694e2. Following copilot-instructions.md requirements:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot also update the PR title and summary according to guidelines |
Updated PR description with proper structure per copilot-instructions.md guidelines:
Note: PR title should be prefixed with "feat:" to follow conventional commits format: (PR title can only be changed through GitHub UI) |
skyf0l
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot always follow copilot-instructions.md
tools/gen_keys/src/main.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot I think you can retrive the outpath from arguments and make it dynamic
then also update the CI with this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in commit fa9b6f0. The gen_keys tool now:
- Accepts output path as first command-line argument
- Defaults to
../../tests/keysif no argument provided (backward compatible) - Creates the output directory if it doesn't exist
- CI updated to pass
../../tests/keysas argument
- Add command-line argument support for output path (defaults to ../../tests/keys) - Update X.509 certificate subject to RsaCracker-related values (CN=RsaCracker, O=RsaCracker Test Suite, OU=Testing) - Update CSR common name to "RsaCracker" - Update CI to pass output path as argument - All functions now accept out_path parameter for flexibility Co-authored-by: skyf0l <[email protected]>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
.github/workflows/ci.yml
Outdated
| - name: Backup existing keys | ||
| run: | | ||
| cd tests/keys | ||
| mkdir -p /tmp/keys_original | ||
| cp *.pem *.der *.cer *.csr *.p7b *.p7c *.p12 /tmp/keys_original/ 2>/dev/null || true | ||
|
|
||
| - name: Regenerate keys | ||
| run: | | ||
| cd tools/gen_keys | ||
| cargo run --quiet -- ../../tests/keys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot I mean dont backup them and direcly generate new keys in other folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in commit 2e329e6. CI now:
- Generates keys directly to
/tmp/keys_generated(no backup step) - Compares generated keys with committed keys in
tests/keys - Doesn't modify the working directory during verification
- Generate keys directly to /tmp/keys_generated instead of overwriting tests/keys - Compare generated keys with committed keys in tests/keys - Cleaner approach that doesn't modify working directory during CI - Same verification logic: skip non-deterministic files, compare byte-for-byte Co-authored-by: skyf0l <[email protected]>
- Use /tmp/keys_generated/* instead of listing specific file extensions - Cleaner and more maintainable approach - Automatically handles any new file types added in the future Co-authored-by: skyf0l <[email protected]>
Summary
Add support for PKCS#12, PKCS#7, and X.509 CSR key formats to RsaCracker, enabling users to work with certificate bundles, certificate chains, and certificate signing requests without manual conversion.
Scope
Added:
Parameters::from_pkcs12()- Extract keys from PKCS#12 (.p12, .pfx) bundles with password supportParameters::from_x509_csr()- Extract public keys from X.509 Certificate Signing Requests (.csr)Parameters::from_pkcs7()- Extract public keys from PKCS#7 (.p7b, .p7c) certificate chainstools/gen_keysfor all new formatsModified:
src/params.rs- Added parsers for new formatssrc/main.rs- Updated CLI help text to list new formatsREADME.md- Updated documentation to reflect new format supportCargo.toml- Updated crate description.github/workflows/ci.yml- Key generation verification iterates over all files in temp directorytools/gen_keys/src/main.rs- Made output path dynamic, updated certificate/CSR to RsaCracker-related valuesTesting
tests/keys.rscovering PKCS#12, PKCS#7 (PEM/DER), and X.509 CSR (PEM/DER)Compatibility
from_public_key()andfrom_private_key()Docs
--helptext to list new supported formatsREADME.mddescription to mention PKCS#12, PKCS#7, and CSRCargo.tomldescriptionPerf/Security notes
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.