feat: Keyring #2557

nasdf · 2024-04-26T00:25:19Z

Relevant issue(s)

Resolves #2556

Description

This PR adds a keyring to the cli.

Notable changes:

created keyring package
- file backed keystore uses a password based key generation to encrypt
- system backed keystore uses the OS managed keyring
added keyring root command
- --no-keyring disables the keyring and generates ephemeral keys
- --keyring-backend flag sets the keyring backend (file or system)
- --keyring-path flag sets the keyring directory when using file backend
- --keyring-namespace flag sets the service name when using system backend
added keyring generate generates required keys
- --no-encryption-key skips generating encryption key (disables datastore encryption)
added keyring import import external keys (hexadecimal for now)
added keyring export export existing key (hexadecimal for now)
moved key generation to crypto package

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Manually tested on MacOS.

Specify the platform(s) on which this was tested:

MacOS

AndrewSisley · 2024-04-26T17:26:08Z

cli/keyring.go

+	"errors"
+	"syscall"
+
+	"github.com/99designs/keyring"


question: How confident are we in this dependency? The repo hasn't been updated in more than two years, and it is managed by a company that looks to have a focus way away from the security world and may lack the technical expertise or attention to keep it secure (assuming it and its dependencies were secure 2 years ago).

Some of its' dependencies are very out of date, and a couple are even more stale and suspicious looking than this package (e.g. https://github.com/gsterjov/go-libsecret with 14 stars, presumably responsible for the very old version of https://github.com/godbus/dbus)

Different websites say different things RE its' size, one says it only had 16 employees in 2022 (when the repo was last updated), other top search engine hits estimate 300, and 3777 employees.

99Designs:

Global creative platform for designers and clients

@jsimnz @nasdf /All

I've replaced this dependency with a custom implementation using a newer library.

Dependencies and code of the new one look much healthier, thanks Keenan :)

islamaliev · 2024-04-26T19:43:08Z

cli/keyring.go

+		FilePasswordFunc:        prompt,
+		FileDir:                 dir,
+		KeyCtlScope:             "user",
+		KeyCtlPerm:              0, // TODO


todo: Please create a task if this is something that needs to be handled later. Otherwise it will linger.

I will fix before merging.

I ended up replacing this dependency.

islamaliev · 2024-04-26T19:52:01Z

cli/keyring.go

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.

I think that would be nice. Would it make sense to have a crypto package?

islamaliev · 2024-04-26T20:28:22Z

node/store.go

@@ -74,5 +74,12 @@ func NewStore(opts ...StoreOpt) (datastore.RootStore, error) {
 	badgerOpts.ValueLogFileSize = options.valueLogFileSize
 	badgerOpts.EncryptionKey = options.encryptionKey

+	if len(options.encryptionKey) > 0 {


suggestion: I prefer if options.encryptionKey != "" as it clearly shows that the type of the field is string, not a slice

I think you may have misread the type. encryptionKey is a []byte

I see, my bad

AndrewSisley

Looks good to me, I think I only have documentation requests for you :)

Thanks a bunch Keenan :)

AndrewSisley · 2024-04-29T15:05:03Z

keyring/file.go

+
+type fileKeyring struct {
+	dir string
+	key []byte


todo: A comment on this prop explaining that this is the key to the keyring file, and not some cached content from within it would be handy please, it threw me a little bit at first :)

AndrewSisley · 2024-04-29T15:08:02Z

keyring/keyring.go

+)
+
+// ErrNotFound is returned when a keyring item is not found.
+var ErrNotFound = keyring.ErrNotFound


todo: I think this would be much easier to find/notice if declared in an errors.go file, than here. Is a little inconsistent with the rest of our codebase, and it will encourage bad behaviour if this package needs a second, third, etc error added later.

AndrewSisley · 2024-04-29T15:10:46Z

keyring/system.go

+
+type systemKeyring struct{}
+
+func newSystemKeyring() *systemKeyring {


todo: A comment documenting that this keying implementation defers keying responsibility to the machine executing the code could be handy. Otherwise readers will need to guess/read the implementation.

jsimnz · 2024-04-29T18:31:38Z

cli/start.go

+				if err != nil {
+					return err
+				}
+				storeOpts = append(storeOpts, node.WithEncryptionKey(encryptionKey))


question: This implies that if the keyring is enabled, then we also use use at-rest encryption. Should these technically be separate config options?

Good catch! I've added a datastore.encryptionDisabled config item.

Update: I've made the key optional instead of the config option.

jsimnz

Heading out, but wanted to get this TODO on your radar before this got merged. May have more comments soon.

jsimnz · 2024-04-29T19:07:24Z

keyring/file.go

+	if err := os.MkdirAll(dir, 0755); err != nil {
+		return nil, err
+	}
+	key := pbkdf2.Key(password, []byte("defradb"), 4096, 32, sha1.New)


todo: Shouldn't be using a static salt here. I believe if you use the jwa.PBES2_HS512_A256KW instead of jwa.A256KW here then the Encrypt and Decrypt calls will handle the KDF and salt for you, and the salt then gets included in the JWE headers.

then the key is just the password defined by the user, and you don't have to do the KDF manually.

and the added benefit that it will use sha256 instead of sha1

I was hoping there was something like this in the library. I've replaced it as suggested.

jsimnz

one more, kk now im really heading out, will be AFK

jsimnz · 2024-04-29T19:30:48Z

keyring/system.go

+
+func (systemKeyring) Set(name string, key []byte) error {
+	enc := base64.StdEncoding.EncodeToString(key)
+	return keyring.Set(service, name, enc)


todo: Adding comment here, but it applies to all the system Get/Set calls.

By using a static service name here, then if you have two nodes running on the same machine, that both want to use a the system store, their keys will collide.

Can either
A) Use a paramaterized service name via the config
B) Use the single service name as you have here, but add a namespace on the item names during the Get/Set, which is set via the config.
C) Anything else you think of ;)

Decided to go with option A.

jsimnz · 2024-04-29T23:22:19Z

cli/config.go

@@ -75,6 +75,7 @@ func defaultConfig() *viper.Viper {

 	cfg.SetDefault("datastore.badger.path", "data")
 	cfg.SetDefault("datastore.encryptionDisabled", false)
+	cfg.SetDefault("keyring.service", "defradb")


suggestion: "namespace" might be a more approachable/understandable name from an external UX POV

shahzadlone · 2024-04-30T07:22:51Z

.github/workflows/start-binary.yml

@@ -48,7 +48,7 @@ jobs:

      - name: Attempt to start binary
        run: |
-          ./build/defradb start &
+          ./build/defradb start --no-keyring &


question: Why do we need to start with this flag?

Otherwise it will prompt for a password before starting.

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.

shahzadlone · 2024-04-30T07:26:23Z

cli/root.go

+
+	cmd.PersistentFlags().Bool(
+		"no-keyring",
+		false,


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?

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.

Makes sense, would be nice to document this with my other todo in the README.md

fredcarle · 2024-05-07T18:12:56Z

cli/keyring_generate.go

+  defradb keyring generate --no-encryption-key
+
+Example: with system keyring
+  defradb keyring generate --backend system`,


todo: This example is wrong Error: unknown flag: --backend.

I think it's the global flag --keyring-backend that is correct here.

good catch!

AndrewSisley · 2024-05-07T18:13:12Z

cli/keyring_export.go

+	"github.com/spf13/cobra"
+)
+
+func MakeKeyringExportCommand() *cobra.Command {


question: What are your plans for testing all of this? Are you looking to just rely on manual testing?

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.

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).

I'll work on adding some tests. I think I have an idea on how to avoid the user input.

I've added unit tests for all keyring commands.

AndrewSisley · 2024-05-07T18:16:40Z

keyring/keyring.go

+
+// Keyring provides a simple set/get interface for a keyring service.
+type Keyring interface {
+	// Set stores the given key in the keystore under the given name.


suggestion: I think it is worth explicitly defining what should happen when calling Set and a key of that name already exists, in the documentation on this interface.

suggestion: I think it is worth explicitly defining what should happen when calling Get and Delete and a name of a key that doesn't exist, in the documentation on this interface.

AndrewSisley · 2024-05-07T18:32:39Z

cli/keyring_generate.go

+			return keyring.Set(peerKeyName, peerKey)
+		},
+	}
+	cmd.Flags().BoolVar(&noEncryption, "no-encryption-key", false, "Skip generating an encryption key")


suggestion: I think it might be worth highlighting that this would disable doc-encryption in the description text.

AndrewSisley · 2024-05-07T18:36:00Z

tests/integration/utils2.go

@@ -22,14 +22,14 @@ import (

 	"github.com/bxcodec/faker/support/slice"


todo: Can you please add some automated testing of encryption at rest? Perhaps a new test environment variable, and a CI job that runs with it enabled?

Maybe two env vars and CI jobs, one for each keyring type.

Added a new variant for encrypted datastore to the test workflow

AndrewSisley · 2024-05-07T18:43:18Z

cli/root.go

+	cmd.PersistentFlags().Bool(
+		"no-keyring",
+		false,
+		"Disable the keyring and generate ephemeral keys",


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 start/init? If so what happens if you disable it whilst/after it has been in use (do you lose access to all data in the node?)?

This is what happens when you use --no-keyring after it has been in use:

defradb start --no-keyring 2024-05-07T14:44:30.085-0400 ERROR badger v4@v4.2.1-0.20231113215945-a63444ca5276/db.go:263 Received err: Encryption key mismatch. Cleaning up... Error: Encryption key mismatch

question: What happens if keyring commands are called if the keyring is disabled?

--no-keyring is unused for keyring commands. It won't affect the outcome.

Calling it in start is of less a worry to me than if someone uses it in an active instance, for example:

defradb client collection get --name User bae-123 --no-keyring // or defradb client collection create --name User '{ "name": "Bob" }' --no-keyring

question: Would this disable encryption at rest? If so, I suggest noting that in this description.

Yes, I'll update the description.

question: What happens if keyring commands are called if the keyring is disabled?

You can still use the keyring commands to manage keys.

question: Is this flag available for all commands? Not just start/init? If so what happens if you disable it whilst/after it has been in use?

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.

--no-keyring is unused for keyring commands. It won't affect the outcome.

It is a no-op? Might it be worth excluding it as a flag from those commands then?

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.

Okay nice, I'm happy with that - thanks Keenan :)

AndrewSisley

Thanks Keenan - code is really readable, and I feel like we are getting good value per line out of this :)

Thanks for answering my questions :)

I have a couple of todos around testing, but perhaps we can probably get away with them being done in follow-up PRs if not here (and if no push back :)). If we want them to be done in new PRs, please create tickets for them before merge.

finished changes

## Relevant issue(s) Resolves #2556 ## Description This PR adds a keyring to the cli. Notable changes: - created `keyring` package - file backed keystore uses a password based key generation to encrypt - system backed keystore uses the OS managed keyring - added `keyring` root command - `--no-keyring` disables the keyring and generates ephemeral keys - `--keyring-backend` flag sets the keyring backend (file or system) - `--keyring-path` flag sets the keyring directory when using file backend - `--keyring-namespace` flag sets the service name when using system backend - added `keyring generate` generates required keys - `--no-encryption-key` skips generating encryption key (disables datastore encryption) - added `keyring import` import external keys (hexadecimal for now) - added `keyring export` export existing key (hexadecimal for now) - moved key generation to `crypto` package ## Tasks - [x] I made sure the code is well commented, particularly hard-to-understand areas. - [x] I made sure the repository-held documentation is changed accordingly. - [x] I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in [tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)). - [x] I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ... ## How has this been tested? Manually tested on MacOS. Specify the platform(s) on which this was tested: - MacOS

add basic keyring

5f843d9

nasdf added feature New feature or request area/cli Related to the CLI binary labels Apr 26, 2024

nasdf added this to the DefraDB v0.11 milestone Apr 26, 2024

nasdf self-assigned this Apr 26, 2024

nasdf added 2 commits April 25, 2024 17:25

go mod tidy

f7b1295

Merge branch 'develop' into nasdf/feat/keyring

adadfb2

nasdf requested a review from a team April 26, 2024 00:27

go mod tidy

e7f698e

AndrewSisley reviewed Apr 26, 2024

View reviewed changes

islamaliev reviewed Apr 26, 2024

View reviewed changes

add keystore package

2a56209

AndrewSisley approved these changes Apr 29, 2024

View reviewed changes

nasdf added 3 commits April 29, 2024 10:10

add documentation. move errors to errors.go

fc74058

add no-keyring flag

3c38648

add no-keyring flag to start-binary workflow

c514789

jsimnz reviewed Apr 29, 2024

View reviewed changes

add datastore.encryptionDisabled config item

35d9a14

jsimnz requested changes Apr 29, 2024

View reviewed changes

replace pbkdf2 with pbes2

1438760

jsimnz previously requested changes Apr 29, 2024

View reviewed changes

add keyring.service config

e112134

jsimnz reviewed Apr 29, 2024

View reviewed changes

change keyring.service to keyring.namespace

ebe4152

shahzadlone reviewed Apr 30, 2024

View reviewed changes

nasdf added 2 commits May 6, 2024 17:00

add keyring cli commands

4ded5fc

make docs

e425069

nasdf changed the title ~~feat: CLI only keyring~~ feat: keyring cli May 7, 2024

Merge branch 'develop' into nasdf/feat/keyring

705cd35

nasdf changed the title ~~feat: keyring cli~~ feat: Keyring May 7, 2024

nasdf requested a review from a team May 7, 2024 00:23

fredcarle modified the milestones: DefraDB v0.11, DefraDB v0.12 May 7, 2024

fredcarle reviewed May 7, 2024

View reviewed changes

AndrewSisley reviewed May 7, 2024

View reviewed changes

fix keyring generate example

ce8a2e5

AndrewSisley reviewed May 7, 2024

View reviewed changes

update keyring interface docs

7983fb5

AndrewSisley reviewed May 7, 2024

View reviewed changes

AndrewSisley approved these changes May 7, 2024

View reviewed changes

nasdf and others added 6 commits May 7, 2024 12:39

add keyring cli tests

f4584b1

add DEFRA_BADGER_ENCRYPTION test env variable

643ae14

make docs

f7eb930

add database-encryption option to test matrix

f6b0ed1

fix test matrix database-encryption

ce5e693

Merge branch 'develop' into nasdf/feat/keyring

a3e0629

nasdf requested a review from jsimnz May 8, 2024 16:18

Merge branch 'develop' into nasdf/feat/keyring

0798712

nasdf merged commit 01e1b66 into sourcenetwork:develop May 10, 2024
28 of 29 checks passed

nasdf mentioned this pull request Sep 6, 2024

Encryption at rest #1987

Closed


		type systemKeyring struct{}

		func newSystemKeyring() *systemKeyring {

		@@ -22,14 +22,14 @@ import (

		"github.com/bxcodec/faker/support/slice"

feat: Keyring #2557

feat: Keyring #2557

Conversation

nasdf commented Apr 26, 2024 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

AndrewSisley Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsimnz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsimnz left a comment

Choose a reason for hiding this comment

jsimnz Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

nasdf commented Apr 26, 2024 •

edited

Loading

AndrewSisley Apr 26, 2024 •

edited

Loading

jsimnz Apr 29, 2024 •

edited

Loading

shahzadlone Apr 30, 2024 •

edited

Loading

AndrewSisley May 7, 2024 •

edited

Loading

AndrewSisley May 7, 2024 •

edited

Loading

AndrewSisley May 7, 2024 •

edited

Loading

AndrewSisley May 7, 2024 •

edited

Loading