Add Secrets Database #1309

jordanschalm · 2021-09-20T15:11:58Z

This PR creates a separate database for confidential data. It implements part of the Identity-Agnostic Database Structure proposal.

Changes

Add an additional database instance in the scaffold
- ⚠️ Adds a new required flag to specify the location of this database
- Adds optional encryption support for the new secrets database, using a new private bootstrap file specifying the encryption key, stored alongside the other keys
- Updates localnet and integration tests to turn on encryption for the new database
Adds storage methods for initializing a database with a particular type (for the scaffold), and for ensuring that a database is marked as a particular type (for storage components to assert which database type they are being initialized against)
Adds the new flag to example systemd files

* provide Init functions to initialize a DB with a particular type * add methods to assert a DB is a particular type which can be used in constructors of storage modules which require a particular type

require --secretsdir flag

integration/testnet/network.go

engine/testutil/nodes.go

state/protocol/badger/mutator_test.go

zhangchiqing

Nice work, just had one question and a few suggestions

zhangchiqing · 2021-09-24T17:41:31Z

cmd/scaffold.go

+func loadSecretsEncryptionKey(dir string, myID flow.Identifier) ([]byte, error) {
+	data, err := io.ReadFile(filepath.Join(dir, fmt.Sprintf(bootstrap.PathSecretsEncryptionKey, myID)))
+	if err != nil {
+		return nil, fmt.Errorf("could not read key: %w", err)


better to print the path

zhangchiqing · 2021-09-24T17:43:19Z

cmd/scaffold.go

+	err := os.MkdirAll(fnb.BaseConfig.secretsdir, 0700)
+	fnb.MustNot(err).Str("dir", fnb.BaseConfig.secretsdir).Msg("could not create secrets db dir")


If the directory already exists, would running the second time still work?

It will - we do the same for the existing database

flow-go/cmd/scaffold.go

Line 437 in 27802a0

err := os.MkdirAll(fnb.BaseConfig.datadir, 0700)

zhangchiqing · 2021-09-24T17:45:45Z

cmd/scaffold.go

+	// attempt to read an encryption key for the secrets DB from the canonical path
+	encryptionKey, err := loadSecretsEncryptionKey(fnb.BootstrapDir, fnb.NodeID)
+	if errors.Is(err, os.ErrNotExist) {
+		fnb.Logger.Warn().Msg("starting with secrets database encryption disabled")


Why not crashing? shouldn't secret encryption key always exist?

Because creating the encryption key requires an operator action (see #1340 and onflow/flow#641), the plan is to not make it a hard requirement initially (discussion here)

OK, better to add a TODO for reminding us to remove it in next spork or something

👍 https://github.com/dapperlabs/flow-go/issues/5893

will add key derivation suggestions there

zhangchiqing · 2021-09-24T18:02:03Z

cmd/consensus/main.go

+			var flowClient *client.Client
+			if insecureAccessAPI {
+				flowClient, err = common.InsecureFlowClient(accessAddress)
+				if err != nil {
+					return nil, err


not sure if these change have anything to do with Secrets database

You're right, this is duplicating the setup that is already implemented here: https://github.com/onflow/flow-go/pull/1309/files#diff-7d960671cc56ce82783f5366bad31f7108cf00270c1bb39b0e00fa57ca96334dR354-R383. Not sure how that happened, thanks for catching

zhangchiqing · 2021-09-24T19:42:27Z

storage/badger/dkg_keys.go

-func NewDKGKeys(collector module.CacheMetrics, db *badger.DB) *DKGKeys {
+func NewDKGKeys(collector module.CacheMetrics, db *badger.DB) (*DKGKeys, error) {
+
+	err := operation.EnsureSecretDB(db)


Can't find my comment. Sorry if I asked this question again:

Can we let EnsureSecretDB check whether the encryption key is correct? Seems if the encryption key is incorrect, the db can still be initialized, and won't learn it's wrong until there is an query.

The database will fail to open if the encryption key isn't correct. Going to add a test case that validates this.

Added a test here: aab5d8e

codecov-commenter · 2021-09-24T19:53:19Z

Codecov Report

Merging #1309 (3e174db) into master (28a6922) will decrease coverage by 1.01%.
The diff coverage is 38.77%.

@@            Coverage Diff             @@
##           master    #1309      +/-   ##
==========================================
- Coverage   56.25%   55.24%   -1.02%     
==========================================
  Files         500      506       +6     
  Lines       31162    31798     +636     
==========================================
+ Hits        17531    17567      +36     
- Misses      11262    11856     +594     
- Partials     2369     2375       +6

Flag	Coverage Δ
unittests	`55.24% <38.77%> (-1.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/bootstrap/utils/key_generation.go	`39.32% <0.00%> (-7.35%)`	⬇️
cmd/node_builder.go	`0.00% <0.00%> (ø)`
storage/badger/operation/prefix.go	`76.00% <ø> (ø)`
cmd/scaffold.go	`0.92% <13.51%> (ø)`
storage/badger/init.go	`42.85% <42.85%> (ø)`
storage/badger/operation/init.go	`84.61% <84.61%> (ø)`
storage/badger/dkg_keys.go	`100.00% <100.00%> (ø)`
cmd/flags.go	`0.00% <0.00%> (ø)`
cmd/machine_account.go	`0.00% <0.00%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28a6922...3e174db. Read the comment docs.

storage/badger/operation/init_test.go

zhangchiqing

Nice work 💯

huitseeker

LGTM, under heavy caveats that we discussed offline and that I'll re-state here for completeness:

since the DKG secrets can lead to consensus votes (in consensus v2),
those secrets being read by an out-of-process intruder on the node operator's machine can lead to a compromise of chain safety,
hence the DKG secrets DB being left unencrypted is unsafe (though perhaps just as bad as we deal with staking key persistence and custody)
so this should only be released in mainnet after a documentation PR that outlines all on-disk locations that the operator should secure with spectacular clarity,
this doc pr should let operators use infra / sre means to secure those storage locations using one of the myriad of solutions available (some FUSE-based overlay encryption, Docker, K8s, GCP secrets, etc ...)

huitseeker · 2021-09-29T17:54:43Z

cmd/scaffold.go

+	// attempt to read an encryption key for the secrets DB from the canonical path
+	encryptionKey, err := loadSecretsEncryptionKey(fnb.BootstrapDir, fnb.NodeID)
+	if errors.Is(err, os.ErrNotExist) {
+		fnb.Logger.Warn().Msg("starting with secrets database encryption disabled")


will add key derivation suggestions there

jordanschalm · 2021-09-29T18:47:01Z

bors merge

bors · 2021-09-29T19:05:33Z

Build succeeded:

jordanschalm added 16 commits September 20, 2021 10:22

add db type to storage layer

cca2e8a

* provide Init functions to initialize a DB with a particular type * add methods to assert a DB is a particular type which can be used in constructors of storage modules which require a particular type

add secrets db to scaffold

99ec6d6

instantiate dkg key storage in SN node only

6cdebd5

add secrets dir flag

89517c1

revert change to default datadir

1fb3d6d

require --secretsdir flag

read encryption key for secrets db

76e7598

generate encryption keys for localnet & integration tests

3c244c7

Merge branch 'master' into jordan/5856-secrets-db

9bde6f6

fix secrets db init & keyfile write func

c3e474b

capture errors in deferred unittest helpers

fa09563

update signer store test

28cec3e

add db type marker requirement test

c6684cb

refactor to address a circular dependency

d44d55e

remove snowflake type, update comments

45a750a

tests for lower-level marker db methods

3195d82

extend comments

af1106d

jordanschalm changed the title ~~Identity-Agnostic Database Structure~~ Add Secrets Database Sep 22, 2021

jordanschalm added 3 commits September 22, 2021 17:08

Merge branch 'master' into jordan/5856-secrets-db

3b74c4f

update volume bindings in int. tests to support secrets db

210a50c

use mkdirall to account for deeper dir structure

6388c38

jordanschalm marked this pull request as ready for review September 22, 2021 21:34

jordanschalm requested review from AlexHentschel, vishalchangrani and zhangchiqing as code owners September 22, 2021 21:34

jordanschalm requested review from huitseeker and synzhu and removed request for vishalchangrani September 22, 2021 22:00

jordanschalm added 3 commits September 22, 2021 18:02

fix assignment in dkg tests

5decea8

disable secrets db for consensus follower

27802a0

Merge branch 'master' into jordan/5856-secrets-db

49751e5

jordanschalm requested a review from tarakby September 23, 2021 12:44

add secrets db to in-mem mock nodes

f99d5e7

jordanschalm removed the request for review from synzhu September 23, 2021 13:41

update example systemd files

3731576

jordanschalm commented Sep 24, 2021

View reviewed changes

integration/testnet/network.go Outdated Show resolved Hide resolved

engine/testutil/nodes.go Outdated Show resolved Hide resolved

state/protocol/badger/mutator_test.go Outdated Show resolved Hide resolved

jordanschalm requested review from vishalchangrani, synzhu and peterargue and removed request for AlexHentschel September 24, 2021 14:07

jordanschalm added 2 commits September 24, 2021 14:54

naming and comments updates

dc2bef9

Merge branch 'master' into jordan/5856-secrets-db

3e43c99

zhangchiqing reviewed Sep 24, 2021

View reviewed changes

jordanschalm added 2 commits September 24, 2021 15:34

include path in errors when reading bootstrap files

8858d99

remove duplicate initialization of flow client

8e7c481

zhangchiqing reviewed Sep 24, 2021

View reviewed changes

kc1116 reviewed Sep 24, 2021

View reviewed changes

storage/badger/operation/init_test.go Outdated Show resolved Hide resolved

kc1116 approved these changes Sep 24, 2021

View reviewed changes

storage/badger/operation/init_test.go Outdated Show resolved Hide resolved

jordanschalm added 4 commits September 24, 2021 18:14

opening db with wrong key should fail

aab5d8e

fix nesting of subtests

3e174db

Merge branch 'master' into jordan/5856-secrets-db

9a20eb9

add todo to enforce encryption

959b1c2

zhangchiqing approved these changes Sep 29, 2021

View reviewed changes

jordanschalm mentioned this pull request Sep 29, 2021

[v0.22] Add Secrets Database #1381

Closed

huitseeker approved these changes Sep 29, 2021

View reviewed changes

bors bot merged commit 403772e into master Sep 29, 2021

bors bot deleted the jordan/5856-secrets-db branch September 29, 2021 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Secrets Database #1309

Add Secrets Database #1309

jordanschalm commented Sep 20, 2021 •

edited

zhangchiqing left a comment

zhangchiqing Sep 24, 2021

jordanschalm Sep 24, 2021

zhangchiqing Sep 24, 2021

jordanschalm Sep 24, 2021

zhangchiqing Sep 24, 2021

jordanschalm Sep 24, 2021

zhangchiqing Sep 24, 2021

jordanschalm Sep 29, 2021

huitseeker Sep 29, 2021

zhangchiqing Sep 24, 2021

jordanschalm Sep 24, 2021

jordanschalm Sep 24, 2021

zhangchiqing Sep 24, 2021

jordanschalm Sep 24, 2021

jordanschalm Sep 24, 2021

codecov-commenter commented Sep 24, 2021 •

edited

zhangchiqing left a comment

huitseeker left a comment

huitseeker Sep 29, 2021

jordanschalm commented Sep 29, 2021

bors bot commented Sep 29, 2021

		err := os.MkdirAll(fnb.BaseConfig.secretsdir, 0700)
		fnb.MustNot(err).Str("dir", fnb.BaseConfig.secretsdir).Msg("could not create secrets db dir")

Add Secrets Database #1309

Add Secrets Database #1309

Conversation

jordanschalm commented Sep 20, 2021 • edited

Changes

zhangchiqing 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

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

codecov-commenter commented Sep 24, 2021 • edited

Codecov Report

zhangchiqing left a comment

Choose a reason for hiding this comment

huitseeker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanschalm commented Sep 29, 2021

bors bot commented Sep 29, 2021

jordanschalm commented Sep 20, 2021 •

edited

codecov-commenter commented Sep 24, 2021 •

edited