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

Jump to bottom

feat: allow key export in online mode #8113

Merged
merged 4 commits into from
May 12, 2021
Merged

feat: allow key export in online mode #8113

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a158036
feat: allow key export in online mode
lidel May 7, 2021
48bc573
test: confirm /api/v0/key/export is HTTP 404
lidel May 11, 2021
6b63de8
fix: check repo version before key export
lidel May 11, 2021
1d77f9d
refactor: key export requires exact repo ver
lidel May 11, 2021
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
20 changes: 16 additions & 4 deletions core/commands/keystore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@ import (

cmds "github.com/ipfs/go-ipfs-cmds"
config "github.com/ipfs/go-ipfs-config"
keystore "github.com/ipfs/go-ipfs-keystore"
oldcmds "github.com/ipfs/go-ipfs/commands"
cmdenv "github.com/ipfs/go-ipfs/core/commands/cmdenv"
"github.com/ipfs/go-ipfs/core/commands/e"
ke "github.com/ipfs/go-ipfs/core/commands/keyencode"
fsrepo "github.com/ipfs/go-ipfs/repo/fsrepo"
migrations "github.com/ipfs/go-ipfs/repo/fsrepo/migrations"
options "github.com/ipfs/interface-go-ipfs-core/options"
"github.com/libp2p/go-libp2p-core/crypto"
peer "github.com/libp2p/go-libp2p-core/peer"
Expand Down Expand Up @@ -150,7 +152,6 @@ path can be specified with '--output=<path>' or '-o=<path>'.
cmds.StringOption(outputOptionName, "o", "The path where the output should be stored."),
},
NoRemote: true,
PreRun: DaemonNotRunning,
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this check is probably fine, but mostly as a function of the keystore currently living on the filesystem and hoping that Go + the OS properly obey filesystem semantics cross platform. If we try and move keystore into some database this could start to fall apart since the database might require a single access point.

As for the keeping the keys on the filesystem, I'm not really a fan, especially since they're currently unencrypted (although that's fairly fixable), so even though it's low on the priority list it'd be a nice thing to deal with. This is especially so because we have no way of revoking IPNS keys (although setting the sequence number to the max value might do the job) or pushing people onto new keys if the old ones are compromised. Thus far IPNS has not had super high usage due to performance issues but as those resolve (IPNS over PubSub, faster DHT lookups, etc.) the idea of protecting the keys better may come into focus.

Are we ok either boxing ourselves into using a keystore that is always readable by multiple processes, or with having online key export potentially not be available in the future?

cc @Stebalien @lidel

Copy link
Member

Choose a reason for hiding this comment

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

So, I think we should be able to do this without boxing ourselves into a corner. We'd probably need something in the future that:

  1. Reads the config to find where the keys are actually stored.
  2. Possibly connects to the system keyring.

For now, the current code is likely fine. In the future, we'll likely need something to open a keystore without opening the rest of the repo (i.e., read the config to find where the keystore is, then read the keystore). Some keystores may not support this, but that's fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, it'll be a bit more complicated but it seems doable enough for us to go ahead with this now. Some things we may have to worry about are:

  • making sure to control how the config fine is modified if it has keystone info in there
  • checking the keystone type and for some of them returning something like "error cannot export keys from this keystone while the daemon is running"

Copy link
Member

Choose a reason for hiding this comment

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

Yep. That means checking the repo version is going to be important.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added version check in 6b63de8

Run: func(req *cmds.Request, res cmds.ResponseEmitter, env cmds.Environment) error {
name := req.Arguments[0]

Expand All @@ -163,13 +164,24 @@ path can be specified with '--output=<path>' or '-o=<path>'.
return err
}

r, err := fsrepo.Open(cfgRoot)
// Check repo version, and error out if not matching
ver, err := migrations.RepoVersion(cfgRoot)
if err != nil {
return err
}
if ver > fsrepo.RepoVersion {
Stebalien marked this conversation as resolved.
Show resolved Hide resolved
return fmt.Errorf("key export expects repo version (%d) but found (%d)", fsrepo.RepoVersion, ver)
}

// Export is read-only: safe to read it without acquiring repo lock
// (this makes export work when ipfs daemon is already running)
ksp := filepath.Join(cfgRoot, "keystore")
Stebalien marked this conversation as resolved.
Show resolved Hide resolved
ks, err := keystore.NewFSKeystore(ksp)
if err != nil {
return err
}
defer r.Close()

sk, err := r.Keystore().Get(name)
sk, err := ks.Get(name)
if err != nil {
return fmt.Errorf("key with name '%s' doesn't exist", name)
}
Expand Down
17 changes: 14 additions & 3 deletions test/sharness/t0165-keystore.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,14 +175,25 @@ ipfs key rm key_ed25519
test_cmp rsa_key_id roundtrip_rsa_key_id
'

test_expect_success "online export rsa key" '
test_must_fail ipfs key export generated_rsa_key
# export works directly on the keystore present in IPFS_PATH
test_expect_success "export and import ed25519 key while daemon is running" '
edhash=$(ipfs key gen exported_ed25519_key --type=ed25519)
lidel marked this conversation as resolved.
Show resolved Hide resolved
echo $edhash > ed25519_key_id
ipfs key export exported_ed25519_key &&
ipfs key rm exported_ed25519_key &&
ipfs key import exported_ed25519_key exported_ed25519_key.key > roundtrip_ed25519_key_id &&
test_cmp ed25519_key_id roundtrip_ed25519_key_id
'

test_expect_success "key export over HTTP /api/v0/key/export is not possible" '
ipfs key gen nohttpexporttest_key --type=ed25519 &&
curl -X POST -sI "http://$API_ADDR/api/v0/key/export&arg=nohttpexporttest_key" | grep -q "^HTTP/1.1 404 Not Found"
'

test_expect_success "online rotate rsa key" '
test_must_fail ipfs key rotate
'

test_kill_ipfs_daemon

}
Expand Down