-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add ipfs cli 'rotate' command to rotate identity private keys #7515
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. |
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.
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 id
output to verify the ID has changed - Check
ipfs key self
output to make sure key has changed - Check
ipfs key list -l
to check if the rotated key has the correct ID - Do an
ipfs name publish --key=rotatedKey
, just to make sure it works
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_success
for these steps to make them easier to debug/notice.
419ebb2
to
7dda766
Compare
No description provided.