-
Notifications
You must be signed in to change notification settings - Fork 53
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
feat: Keyring #2557
feat: Keyring #2557
Changes from 17 commits
5f843d9
f7b1295
adadfb2
e7f698e
2a56209
fc74058
3c38648
c514789
35d9a14
1438760
e112134
ebe4152
4ded5fc
e425069
705cd35
ce8a2e5
7983fb5
f4584b1
643ae14
f7eb930
f6b0ed1
ce5e693
a3e0629
0798712
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: I wonder if all this logic with generating hashes should be scoped to this cli package. Maybe it makes sense to extract it in another place so other that other packages can use it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that would be nice. Would it make sense to have a |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
// Copyright 2024 Democratized Data Foundation | ||
// | ||
// Use of this software is governed by the Business Source License | ||
// included in the file licenses/BSL.txt. | ||
// | ||
// As of the Change Date specified in that file, in accordance with | ||
// the Business Source License, use of this software will be governed | ||
// by the Apache License, Version 2.0, included in the file | ||
// licenses/APL.txt. | ||
|
||
package cli | ||
|
||
import ( | ||
"github.com/spf13/cobra" | ||
) | ||
|
||
func MakeKeyringCommand() *cobra.Command { | ||
var cmd = &cobra.Command{ | ||
Use: "keyring", | ||
Short: "Manage DefraDB private keys", | ||
Long: `Manage DefraDB private keys. | ||
Generate, import, and export private keys.`, | ||
} | ||
return cmd | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
// Copyright 2024 Democratized Data Foundation | ||
// | ||
// Use of this software is governed by the Business Source License | ||
// included in the file licenses/BSL.txt. | ||
// | ||
// As of the Change Date specified in that file, in accordance with | ||
// the Business Source License, use of this software will be governed | ||
// by the Apache License, Version 2.0, included in the file | ||
// licenses/APL.txt. | ||
|
||
package cli | ||
|
||
import ( | ||
"github.com/spf13/cobra" | ||
) | ||
|
||
func MakeKeyringExportCommand() *cobra.Command { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: What are your plans for testing all of this? Are you looking to just rely on manual testing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the file and system keyrings require user input, I don't think they are worth testing. I could add an option for mock (in memory) keyring and add tests for that if you or others think they would be useful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't need to mock to test it I don't think - tests can handle things like password prompts without to much hassle. I'm a bit nervous about not pushing for this (either here or in a follow up PR), as apart from the openAPI stuff it would be the only thing in the whole repo that really requires manual testing (and thus is unexpected/surprising, as well as being generally more expensive and unreliable compared to automated testing). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll work on adding some tests. I think I have an idea on how to avoid the user input. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added unit tests for all keyring commands. |
||
var cmd = &cobra.Command{ | ||
Use: "export <name>", | ||
Short: "Export a private key", | ||
Long: `Export a private key. | ||
Prints the hexadecimal representation of a private key. | ||
|
||
Example: | ||
defradb keyring export encryption-key`, | ||
Args: cobra.ExactArgs(1), | ||
RunE: func(cmd *cobra.Command, args []string) error { | ||
keyring, err := openKeyring(cmd) | ||
if err != nil { | ||
return err | ||
} | ||
keyBytes, err := keyring.Get(args[0]) | ||
if err != nil { | ||
return err | ||
} | ||
cmd.Printf("%x\n", keyBytes) | ||
return nil | ||
}, | ||
} | ||
return cmd | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
// Copyright 2024 Democratized Data Foundation | ||
// | ||
// Use of this software is governed by the Business Source License | ||
// included in the file licenses/BSL.txt. | ||
// | ||
// As of the Change Date specified in that file, in accordance with | ||
// the Business Source License, use of this software will be governed | ||
// by the Apache License, Version 2.0, included in the file | ||
// licenses/APL.txt. | ||
|
||
package cli | ||
|
||
import ( | ||
"github.com/spf13/cobra" | ||
|
||
"github.com/sourcenetwork/defradb/crypto" | ||
) | ||
|
||
func MakeKeyringGenerateCommand() *cobra.Command { | ||
var noEncryption bool | ||
var cmd = &cobra.Command{ | ||
Use: "generate", | ||
Short: "Generate private keys", | ||
Long: `Generate private keys. | ||
Randomly generate and store private keys in the keyring. | ||
|
||
WARNING: This will overwrite existing keys in the keyring. | ||
|
||
Example: | ||
defradb keyring generate | ||
|
||
Example: with no encryption key | ||
defradb keyring generate --no-encryption-key | ||
|
||
Example: with system keyring | ||
defradb keyring generate --keyring-backend system`, | ||
RunE: func(cmd *cobra.Command, args []string) error { | ||
keyring, err := openKeyring(cmd) | ||
if err != nil { | ||
return err | ||
} | ||
if !noEncryption { | ||
// generate optional encryption key | ||
encryptionKey, err := crypto.GenerateAES256() | ||
if err != nil { | ||
return err | ||
} | ||
err = keyring.Set(encryptionKeyName, encryptionKey) | ||
if err != nil { | ||
return err | ||
} | ||
} | ||
peerKey, err := crypto.GenerateEd25519() | ||
if err != nil { | ||
return err | ||
} | ||
return keyring.Set(peerKeyName, peerKey) | ||
}, | ||
} | ||
cmd.Flags().BoolVar(&noEncryption, "no-encryption-key", false, "Skip generating an encryption key") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: I think it might be worth highlighting that this would disable doc-encryption in the description text. |
||
return cmd | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
// Copyright 2024 Democratized Data Foundation | ||
// | ||
// Use of this software is governed by the Business Source License | ||
// included in the file licenses/BSL.txt. | ||
// | ||
// As of the Change Date specified in that file, in accordance with | ||
// the Business Source License, use of this software will be governed | ||
// by the Apache License, Version 2.0, included in the file | ||
// licenses/APL.txt. | ||
|
||
package cli | ||
|
||
import ( | ||
"encoding/hex" | ||
|
||
"github.com/spf13/cobra" | ||
) | ||
|
||
func MakeKeyringImportCommand() *cobra.Command { | ||
var cmd = &cobra.Command{ | ||
Use: "import <name> <private-key-hex>", | ||
Short: "Import a private key", | ||
Long: `Import a private key. | ||
Store an externally generated key in the keyring. | ||
|
||
Example: | ||
defradb keyring import encryption-key 0000000000000000`, | ||
Args: cobra.ExactArgs(2), | ||
RunE: func(cmd *cobra.Command, args []string) error { | ||
keyring, err := openKeyring(cmd) | ||
if err != nil { | ||
return err | ||
} | ||
keyBytes, err := hex.DecodeString(args[1]) | ||
if err != nil { | ||
return err | ||
} | ||
return keyring.Set(args[0], keyBytes) | ||
}, | ||
} | ||
return cmd | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -139,5 +139,29 @@ Start a DefraDB node, interact with a local or remote node, and much more. | |
"Path to the private key for tls", | ||
) | ||
|
||
cmd.PersistentFlags().String( | ||
"keyring-namespace", | ||
"defradb", | ||
"Service name to use when using the system backend", | ||
) | ||
|
||
cmd.PersistentFlags().String( | ||
"keyring-backend", | ||
"file", | ||
"Keyring backend to use. Options are file or system", | ||
) | ||
|
||
cmd.PersistentFlags().String( | ||
"keyring-path", | ||
"keys", | ||
"Path to store encrypted keys when using the file backend", | ||
) | ||
|
||
cmd.PersistentFlags().Bool( | ||
"no-keyring", | ||
false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: By default we want the keyring? question: For my understanding, what happens to the flow for using keys when there is no keyring? Does it make sense to make keyring functionality (disable or active) dependent on weather acp is on or off? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think enabled by default is a sensible option. The keyring also stores the peer key and encryption key. If you disable the keyring the peer key will be ephemeral and encryption disabled. I don't think the keyring should be dependent on ACP because of the other keys. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, would be nice to document this with my other |
||
"Disable the keyring and generate ephemeral keys", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Would this disable encryption at rest? If so, I suggest noting that in this description. question: What happens if keyring commands are called if the keyring is disabled? question: Is this flag available for all commands? Not just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is what happens when you use
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Calling it in
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, I'll update the description.
You can still use the keyring commands to manage keys.
This currently only applies to the start command. I'm planning on moving the flags that only apply to the start command in a future PR. If you disable the keyring after using it, the peer key will be randomly generated, and at rest encryption will be disabled. If at rest encryption was previously enabled, the datastore will fail to open due to the encryption key not matching. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It is a no-op? Might it be worth excluding it as a flag from those commands then? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Okay nice, I'm happy with that - thanks Keenan :) |
||
) | ||
|
||
return cmd | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,6 @@ import ( | |
"fmt" | ||
"os" | ||
"os/signal" | ||
"path/filepath" | ||
"syscall" | ||
|
||
"github.com/libp2p/go-libp2p/core/peer" | ||
|
@@ -23,6 +22,7 @@ import ( | |
"github.com/sourcenetwork/defradb/db" | ||
"github.com/sourcenetwork/defradb/errors" | ||
"github.com/sourcenetwork/defradb/http" | ||
"github.com/sourcenetwork/defradb/keyring" | ||
"github.com/sourcenetwork/defradb/net" | ||
netutils "github.com/sourcenetwork/defradb/net/utils" | ||
"github.com/sourcenetwork/defradb/node" | ||
|
@@ -84,23 +84,32 @@ func MakeStartCommand() *cobra.Command { | |
} | ||
|
||
if cfg.GetString("datastore.store") != configStoreMemory { | ||
// It would be ideal to not have the key path tied to the datastore. | ||
// Running with memory store mode will always generate a random key. | ||
// Adding support for an ephemeral mode and moving the key to the | ||
// config would solve both of these issues. | ||
rootDir := mustGetContextRootDir(cmd) | ||
key, err := loadOrGeneratePrivateKey(filepath.Join(rootDir, "data", "key")) | ||
if err != nil { | ||
return err | ||
} | ||
netOpts = append(netOpts, net.WithPrivateKey(key)) | ||
|
||
// TODO-ACP: Infuture when we add support for the --no-acp flag when admin signatures are in, | ||
// we can allow starting of db without acp. Currently that can only be done programmatically. | ||
// https://github.com/sourcenetwork/defradb/issues/2271 | ||
dbOpts = append(dbOpts, db.WithACP(rootDir)) | ||
} | ||
|
||
if !cfg.GetBool("keyring.disabled") { | ||
kr, err := openKeyring(cmd) | ||
if err != nil { | ||
return NewErrKeyringHelp(err) | ||
} | ||
// load the required peer key | ||
peerKey, err := kr.Get(peerKeyName) | ||
if err != nil { | ||
return NewErrKeyringHelp(err) | ||
} | ||
netOpts = append(netOpts, net.WithPrivateKey(peerKey)) | ||
// load the optional encryption key | ||
encryptionKey, err := kr.Get(encryptionKeyName) | ||
if err != nil && !errors.Is(err, keyring.ErrNotFound) { | ||
return err | ||
} | ||
storeOpts = append(storeOpts, node.WithEncryptionKey(encryptionKey)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: This implies that if the keyring is enabled, then we also use use at-rest encryption. Should these technically be separate config options? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! I've added a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update: I've made the key optional instead of the config option. |
||
} | ||
|
||
opts := []node.NodeOpt{ | ||
node.WithPeers(peers...), | ||
node.WithStoreOpts(storeOpts...), | ||
|
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.
question: Why do we need to start with this flag?
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.
Otherwise it will prompt for a password before starting.
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.
todo: Might be worth adding instructions for this extra configuration step in the root
README.md
(perhaps in the configuration section?) where as that is the default, and mention this flag to skip the keyring configuration.