Skip to content
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

feat: adds secp256k1 keypair type to ipfs key gen command #9830

Merged
merged 1 commit into from
May 31, 2023

Conversation

imthe-1
Copy link

@imthe-1 imthe-1 commented Apr 20, 2023

As per discussions with @2color we found out that we can not create secp256k1 IPNS keypairs as of now and since we can create k1 keypairs using js-ipfs/ipfs-core, we have add the ability here as well.

This might be helpful for projects, like ours, that want to derive their IPNS keypairs from a seed phrase.

@imthe-1 imthe-1 requested a review from a team as a code owner April 20, 2023 15:31
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

@lidel
Copy link
Member

lidel commented May 15, 2023

@imthe-1 are you still planning to work on this and add tests? If so, please rebase on the latest master. 🙏

If you are not fan of bash, its ok to migrate/add relevant tests to https://github.com/ipfs/kubo/tree/master/test/cli (GO instead Bash).

@imthe-1
Copy link
Author

imthe-1 commented May 15, 2023

yes @lidel. will rebase and update the PR for sure. planning to close this by tomorrow.

got it. thanks!

@imthe-1 imthe-1 force-pushed the feat/secp256k1-support branch from c8a331b to 7312920 Compare May 19, 2023 12:19
@imthe-1
Copy link
Author

imthe-1 commented May 19, 2023

@Jorropo @lidel guys I have added the test cases, rebased and updated the PR.

168 test cases got added in https://github.com/ipfs/kubo/blob/master/test/sharness/t0027-rotate.sh by only making some minor additions.
4 test cases added to https://github.com/ipfs/kubo/blob/master/test/sharness/t0165-keystore.sh with a bit of struggle on the key import and export test cases. and this is where I need help from you guys.

Key import and export test cases are still missing since I ran into the following error when running the export command:

./ipfs key export -f pem-pkcs8-cleartext key2
Error: marshalling key to PKCS8 format: x509: unknown key type while marshaling PKCS#8: *crypto.Secp256k1PrivateKey

seems like the crypto/x509 library couldn't export secp256k1 private key in PKCS8 format. Please have a look and let me know if any further changes are required from my side🙏

Also, I am curious why do we have a openssl_secp384r1.pem file here(https://github.com/ipfs/kubo/blob/master/test/sharness/t0165-keystore-data/openssl_secp384r1.pem) and is the info mentioned in the readme(https://github.com/ipfs/kubo/blob/master/test/sharness/t0165-keystore-data/README.md) still valid?

The t0165-keystore.sh file treats openssl_secp384r1.pem as an INVALID KEY. can you please help me understand its purpose please.

@imthe-1 imthe-1 requested a review from Jorropo May 19, 2023 22:03
@imthe-1 imthe-1 force-pushed the feat/secp256k1-support branch from 7312920 to ba64b99 Compare May 24, 2023 17:36
@imthe-1 imthe-1 force-pushed the feat/secp256k1-support branch from ba64b99 to c68bb88 Compare May 31, 2023 16:44
@imthe-1
Copy link
Author

imthe-1 commented May 31, 2023

@Jorropo @lidel guys are we good to merge this?

Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

Looks fine to me, I havn't checked this is actually working myself but I belive I'll hear about it if it doesn't.

Thx for your patch ❤️ 🎉

@Jorropo Jorropo merged commit 67e1a17 into ipfs:master May 31, 2023
@BigLep
Copy link
Contributor

BigLep commented May 31, 2023

@Jorropo : do we need a changelog update here?

@Jorropo
Copy link
Contributor

Jorropo commented May 31, 2023

@BigLep it's a minor QOL, Ed25519 is still our recommended key type I don't want to push anyone to start using secp256k1 just because it's on a changelog.
On the other hand if you wanted to use your bitcoin address as your IPFS peerid as your IPNS name, I guess you would want to know about it.

So yes, saying you now have the option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants