Add option to specify key type (RSA or Ed25519) #4076
Conversation
License: MIT Signed-off-by: Justin Drake <drakefjustin@gmail.com>
whyrusleeping
requested review from
magik6k,
Kubuxu and
Stebalien
and removed request for
magik6k
There was a problem hiding this comment.
Would be nice to get some sharness tests for this, something like https://github.com/ipfs/go-ipfs/pull/4001/files#diff-362467109e2261605f06379a54ce9703
Other than the lack of tests and the option nitpick LGTM
cmd/ipfs/init.go
Outdated
| @@ -90,6 +94,12 @@ environment variable: | |||
| return | |||
| } | |||
|
|
|||
| keyType, _, err := req.Option("k").Int() | |||
There was a problem hiding this comment.
Use the longer option name here
There was a problem hiding this comment.
I used the short option name for consistency. See empty, _, err := req.Option("e").Bool() and nBitsForKeypair, _, err := req.Option("b").Int() right above.
There was a problem hiding this comment.
Most other commands use long option names. Looks this one was not migrated.
There was a problem hiding this comment.
I've used the long name for all the options that were using the short name in this file. 👍
cmd/ipfs/init.go
Outdated
| @@ -49,6 +52,7 @@ environment variable: | |||
| }, | |||
| Options: []cmds.Option{ | |||
| cmds.IntOption("bits", "b", "Number of bits to use in the generated RSA private key.").Default(nBitsForKeypairDefault), | |||
| cmds.IntOption("key-type", "k", "Key type (RSA or Ed25519-id").Default(ci.RSA), | |||
There was a problem hiding this comment.
Use the default you defined in line 25.
cmd/ipfs/init.go
Outdated
| @@ -16,10 +16,13 @@ import ( | |||
| namesys "github.com/ipfs/go-ipfs/namesys" | |||
| config "github.com/ipfs/go-ipfs/repo/config" | |||
| fsrepo "github.com/ipfs/go-ipfs/repo/fsrepo" | |||
| ci "gx/ipfs/QmaPbCnUMBohSGo3KnxEa2bHqyJVVeEEcwtqJAYxerieBo/go-libp2p-crypto" | |||
There was a problem hiding this comment.
Separate local and gx deps with a new line.
License: MIT Signed-off-by: Justin Drake <drakefjustin@gmail.com>
JustinDrake
force-pushed
the
ed25519option
branch
from
1c1d6ba to
d2ba362
Compare
cmd/ipfs/init.go
Outdated
| @@ -49,6 +53,7 @@ environment variable: | |||
| }, | |||
| Options: []cmds.Option{ | |||
| cmds.IntOption("bits", "b", "Number of bits to use in the generated RSA private key.").Default(nBitsForKeypairDefault), | |||
| cmds.IntOption("key-type", "k", "Key type (RSA or Ed25519-id").Default(keypairTypeDefault), | |||
There was a problem hiding this comment.
appears to be a typo here, missing paren
|
One comment, then would be great to have a test that tests ipfs init with one of these keys, and verifies |
License: MIT Signed-off-by: Justin Drake <drakefjustin@gmail.com>
JustinDrake
force-pushed
the
ed25519option
branch
from
4ea6c62 to
a881e02
Compare
| if nbits < 1024 { | ||
| return ident, errors.New("Bitsize less than 1024 is considered unsafe.") | ||
|
|
||
| switch keyType { |
There was a problem hiding this comment.
So what happens here if the user passed an incorrect key type?
I don't see the user option validated anywhere, it's just an int.
There was a problem hiding this comment.
Basically you need a default handler that errors out with something helpful for the user.
There was a problem hiding this comment.
Incorrect key type options will error out on line 113 with sk, pk, err := ci.GenerateKeyPair(keyType, nbits).
There was a problem hiding this comment.
It's a little inconsistent with the intent of the switch, and, more importantly, there are other valid key types supported by libp2p-crypto that we don't use yet in ipfs (eg secp256k1 keys).
So I think it's better to tell the user we are not ready to handle this type of key rather than fallthrough and possibly succeed with surprising results.
|
This has laying around for 1.5 years. Is it ever going to implemented? |
|
(this was implemented a while ago) |
See #3896 for context
License: MIT
Signed-off-by: Justin Drake drakefjustin@gmail.com