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.

Sign up for GitHub

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 option to specify key type (RSA or Ed25519) #4076

Closed
wants to merge 3 commits into from
Closed

Add option to specify key type (RSA or Ed25519) #4076

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be8e8b2
Add option to specify key type (RSA or Ed25519)
JustinDrake Jul 17, 2017
d2ba362
Incorporate review from @Kubuxu
JustinDrake Jul 31, 2017
a881e02
Fix typo spotted by @whyursleeping
JustinDrake Aug 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions cmd/ipfs/init.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

Separate local and gx deps with a new line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

)

// The default keypair is 2048-bit RSA
const (
nBitsForKeypairDefault = 2048
keypairTypeDefault = ci.RSA
)

var initCmd = &cmds.Command{
Expand Down Expand Up @@ -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),
Copy link
Member

Choose a reason for hiding this comment

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

Use the default you defined in line 25.

cmds.BoolOption("empty-repo", "e", "Don't add and pin help files to the local storage.").Default(false),
cmds.StringOption("profile", "p", "Apply profile settings to config. Multiple profiles can be separated by ','"),

Expand Down Expand Up @@ -90,6 +94,12 @@ environment variable:
return
}

keyType, _, err := req.Option("k").Int()
Copy link
Member

Choose a reason for hiding this comment

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

Use the longer option name here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the short option name for consistency. See empty, _, err := req.Option("e").Bool() and nBitsForKeypair, _, err := req.Option("b").Int() right above.

Copy link
Member

Choose a reason for hiding this comment

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

Most other commands use long option names. Looks this one was not migrated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've used the long name for all the options that were using the short name in this file. 👍

if err != nil {
res.SetError(err, cmds.ErrNormal)
return
}

var conf *config.Config

f := req.Files()
Expand Down Expand Up @@ -118,7 +128,7 @@ environment variable:
profiles = strings.Split(profile, ",")
}

if err := doInit(os.Stdout, req.InvocContext().ConfigRoot, empty, nBitsForKeypair, profiles, conf); err != nil {
if err := doInit(os.Stdout, req.InvocContext().ConfigRoot, empty, nBitsForKeypair, keyType, profiles, conf); err != nil {
res.SetError(err, cmds.ErrNormal)
return
}
Expand All @@ -130,10 +140,10 @@ Reinitializing would overwrite your keys.
`)

func initWithDefaults(out io.Writer, repoRoot string) error {
return doInit(out, repoRoot, false, nBitsForKeypairDefault, nil, nil)
return doInit(out, repoRoot, false, nBitsForKeypairDefault, keypairTypeDefault, nil, nil)
}

func doInit(out io.Writer, repoRoot string, empty bool, nBitsForKeypair int, confProfiles []string, conf *config.Config) error {
func doInit(out io.Writer, repoRoot string, empty bool, nBitsForKeypair, keyType int, confProfiles []string, conf *config.Config) error {
if _, err := fmt.Fprintf(out, "initializing IPFS node at %s\n", repoRoot); err != nil {
return err
}
Expand All @@ -148,7 +158,7 @@ func doInit(out io.Writer, repoRoot string, empty bool, nBitsForKeypair int, con

if conf == nil {
var err error
conf, err = config.Init(out, nBitsForKeypair)
conf, err = config.Init(out, nBitsForKeypair, keyType)
if err != nil {
return err
}
Expand Down
21 changes: 14 additions & 7 deletions repo/config/init.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import (
ci "gx/ipfs/QmaPbCnUMBohSGo3KnxEa2bHqyJVVeEEcwtqJAYxerieBo/go-libp2p-crypto"
)

func Init(out io.Writer, nBitsForKeypair int) (*Config, error) {
identity, err := identityConfig(out, nBitsForKeypair)
func Init(out io.Writer, nBitsForKeypair, keyType int) (*Config, error) {
identity, err := identityConfig(out, nBitsForKeypair, keyType)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -93,15 +93,22 @@ func datastoreConfig() (Datastore, error) {
}

// identityConfig initializes a new identity.
func identityConfig(out io.Writer, nbits int) (Identity, error) {
func identityConfig(out io.Writer, nbits, keyType int) (Identity, error) {
// TODO guard higher up
ident := Identity{}
if nbits < 1024 {
return ident, errors.New("Bitsize less than 1024 is considered unsafe.")

switch keyType {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Basically you need a default handler that errors out with something helpful for the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Incorrect key type options will error out on line 113 with sk, pk, err := ci.GenerateKeyPair(keyType, nbits).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

case ci.RSA:
if nbits < 1024 {
return ident, errors.New("Bitsize less than 1024 is considered unsafe for RSA.")
}

fmt.Fprintf(out, "generating %v-bit RSA keypair...", nbits)
case ci.Ed25519:
fmt.Fprintf(out, "generating Ed25519 keypair...")
}

fmt.Fprintf(out, "generating %v-bit RSA keypair...", nbits)
sk, pk, err := ci.GenerateKeyPair(ci.RSA, nbits)
sk, pk, err := ci.GenerateKeyPair(keyType, nbits)
if err != nil {
return ident, err
}
Expand Down