Add ipfs cli 'rotate' command to rotate identity private keys#7515
Add ipfs cli 'rotate' command to rotate identity private keys#7515aschmahmann merged 2 commits intomasterfrom
Conversation
|
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
aschmahmann
left a comment
There was a problem hiding this comment.
Added a few small suggestions. We should also add some tests for this. The general way we do this is by adding additional sharness tests (https://github.com/ipfs/go-ipfs/tree/master/test/sharness).
At least one test we should probably do is:
- Create repo + run daemon
- Kill daemon
- Rotate keys
- Rerun the daemon
- Check
ipfs idoutput to verify the ID has changed - Check
ipfs key selfoutput to make sure key has changed - Check
ipfs key list -lto check if the rotated key has the correct ID - Do an
ipfs name publish --key=rotatedKey, just to make sure it works
aschmahmann
left a comment
There was a problem hiding this comment.
This PR is good to go. If we switch the default rotation to RSA and maybe use functions in the test to test RSA + Ed25519 + defaults.
test/sharness/t0027-rotate.sh
Outdated
| test_expect_success "rotating keys" ' | ||
| ipfs rotate --oldkey=oldkey | ||
| ' |
There was a problem hiding this comment.
Maybe we should put this in a function like you did in the init PR, so we can verify RSA, Ed25519 and the default all work.
Default RSA will work awkwardly with the new strict rule that requires not specifying bits when using ed25519: If you set the defaults to RSA+2048, then whenever you want to use --algorithm=ed25519 you don't have a way of unsetting the number of bits flag. Maybe best is if rotation has NO default? |
|
Having a default is fine. Can't we just have the defaults to mimic what we have in |
Sure. This works too. |
2bc1ff0 to
b8c649f
Compare
| FROM_ALG=$1 | ||
| TO_ALG=$2 |
There was a problem hiding this comment.
This is fine, although I think we can simplify it so that we:
- first test basic rotation on default, RSA, Ed25519 using the test_rotate function which indicates that rotation basically works
- test rotating Ed25519 -> RSA -> Ed25519 which indicates that we can rotate to/from each key type. We can even use
test_expect_successfor these steps to make them easier to debug/notice.
419ebb2 to
7dda766
Compare
No description provided.