-
Notifications
You must be signed in to change notification settings - Fork 166
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
Add Secrets Database #1309
Add Secrets Database #1309
Changes from 24 commits
cca2e8a
99ec6d6
6cdebd5
89517c1
1fb3d6d
76e7598
3c244c7
9bde6f6
c3e474b
fa09563
28cec3e
c6684cb
d44d55e
45a750a
3195d82
af1106d
3b74c4f
210a50c
6388c38
5decea8
27802a0
49751e5
f99d5e7
3731576
dc2bef9
3e43c99
8858d99
8e7c481
aab5d8e
3e174db
9a20eb9
959b1c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -5,6 +5,7 @@ import ( | |||
"crypto/tls" | ||||
"crypto/x509" | ||||
"encoding/json" | ||||
"errors" | ||||
"fmt" | ||||
"io/ioutil" | ||||
"math/rand" | ||||
|
@@ -73,7 +74,6 @@ type Storage struct { | |||
Setups storage.EpochSetups | ||||
Commits storage.EpochCommits | ||||
Statuses storage.EpochStatuses | ||||
DKGKeys storage.DKGKeys | ||||
} | ||||
|
||||
type namedModuleFunc struct { | ||||
|
@@ -122,7 +122,8 @@ func (fnb *FlowNodeBuilder) BaseFlags() { | |||
fnb.flags.StringVar(&fnb.BaseConfig.BindAddr, "bind", defaultConfig.BindAddr, "address to bind on") | ||||
fnb.flags.StringVarP(&fnb.BaseConfig.BootstrapDir, "bootstrapdir", "b", defaultConfig.BootstrapDir, "path to the bootstrap directory") | ||||
fnb.flags.DurationVarP(&fnb.BaseConfig.timeout, "timeout", "t", defaultConfig.timeout, "node startup / shutdown timeout") | ||||
fnb.flags.StringVarP(&fnb.BaseConfig.datadir, "datadir", "d", defaultConfig.datadir, "directory to store the protocol state") | ||||
fnb.flags.StringVarP(&fnb.BaseConfig.datadir, "datadir", "d", defaultConfig.datadir, "directory to store the public database (protocol state)") | ||||
fnb.flags.StringVar(&fnb.BaseConfig.secretsdir, "secretsdir", defaultConfig.secretsdir, "directory to store private database (secrets)") | ||||
fnb.flags.StringVarP(&fnb.BaseConfig.level, "loglevel", "l", defaultConfig.level, "level for logging output") | ||||
fnb.flags.DurationVar(&fnb.BaseConfig.PeerUpdateInterval, "peerupdate-interval", defaultConfig.PeerUpdateInterval, "how often to refresh the peer connections for the node") | ||||
fnb.flags.DurationVar(&fnb.BaseConfig.UnicastMessageTimeout, "unicast-timeout", defaultConfig.UnicastMessageTimeout, "how long a unicast transmission can take to complete") | ||||
|
@@ -456,9 +457,42 @@ func (fnb *FlowNodeBuilder) initDB() { | |||
WithValueLogFileSize(128 << 23). | ||||
WithValueLogMaxEntries(100000) // Default is 1000000 | ||||
|
||||
db, err := badger.Open(opts) | ||||
fnb.MustNot(err).Msg("could not open key-value store") | ||||
fnb.DB = db | ||||
publicDB, err := bstorage.InitPublic(opts) | ||||
fnb.MustNot(err).Msg("could not open public db") | ||||
fnb.DB = publicDB | ||||
} | ||||
|
||||
func (fnb *FlowNodeBuilder) initSecretsDB() { | ||||
|
||||
// if the secrets DB is disabled (only applicable for Consensus Follower, | ||||
// which makes use of this same logic), skip this initialization | ||||
if !fnb.BaseConfig.secretsDBEnabled { | ||||
return | ||||
} | ||||
|
||||
if fnb.BaseConfig.secretsdir == NotSet { | ||||
fnb.Logger.Fatal().Msgf("missing required flag '--secretsdir'") | ||||
} | ||||
|
||||
err := os.MkdirAll(fnb.BaseConfig.secretsdir, 0700) | ||||
fnb.MustNot(err).Str("dir", fnb.BaseConfig.secretsdir).Msg("could not create secrets db dir") | ||||
Comment on lines
+480
to
+481
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. If the directory already exists, would running the second time still work? 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 will - we do the same for the existing database Line 437 in 27802a0
|
||||
|
||||
log := sutil.NewLogger(fnb.Logger) | ||||
|
||||
opts := badger.DefaultOptions(fnb.BaseConfig.secretsdir).WithLogger(log) | ||||
// 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") | ||||
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. Why not crashing? shouldn't secret encryption key always exist? 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. 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) 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. OK, better to add a TODO for reminding us to remove it in next spork or something 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. will add key derivation suggestions there |
||||
} else if err != nil { | ||||
fnb.Logger.Fatal().Err(err).Msg("failed to read secrets db encryption key") | ||||
} else { | ||||
opts = opts.WithEncryptionKey(encryptionKey) | ||||
} | ||||
|
||||
secretsDB, err := bstorage.InitSecret(opts) | ||||
fnb.MustNot(err).Msg("could not open secrets db") | ||||
fnb.SecretsDB = secretsDB | ||||
} | ||||
|
||||
func (fnb *FlowNodeBuilder) initStorage() { | ||||
|
@@ -484,7 +518,6 @@ func (fnb *FlowNodeBuilder) initStorage() { | |||
setups := bstorage.NewEpochSetups(fnb.Metrics.Cache, fnb.DB) | ||||
commits := bstorage.NewEpochCommits(fnb.Metrics.Cache, fnb.DB) | ||||
statuses := bstorage.NewEpochStatuses(fnb.Metrics.Cache, fnb.DB) | ||||
dkgKeys := bstorage.NewDKGKeys(fnb.Metrics.Cache, fnb.DB) | ||||
|
||||
fnb.Storage = Storage{ | ||||
Headers: headers, | ||||
|
@@ -500,7 +533,6 @@ func (fnb *FlowNodeBuilder) initStorage() { | |||
Setups: setups, | ||||
Commits: commits, | ||||
Statuses: statuses, | ||||
DKGKeys: dkgKeys, | ||||
} | ||||
} | ||||
|
||||
|
@@ -805,6 +837,12 @@ func WithDataDir(dataDir string) Option { | |||
} | ||||
} | ||||
|
||||
func WithSecretsDBEnabled(enabled bool) Option { | ||||
return func(config *BaseConfig) { | ||||
config.secretsDBEnabled = enabled | ||||
} | ||||
} | ||||
|
||||
func WithMetricsEnabled(enabled bool) Option { | ||||
return func(config *BaseConfig) { | ||||
config.metricsEnabled = enabled | ||||
|
@@ -939,6 +977,7 @@ func (fnb *FlowNodeBuilder) Ready() <-chan struct{} { | |||
fnb.initProfiler() | ||||
|
||||
fnb.initDB() | ||||
fnb.initSecretsDB() | ||||
|
||||
fnb.initMetrics() | ||||
|
||||
|
@@ -1040,3 +1079,13 @@ func loadPrivateNodeInfo(dir string, myID flow.Identifier) (*bootstrap.NodeInfoP | |||
err = json.Unmarshal(data, &info) | ||||
return &info, err | ||||
} | ||||
|
||||
// loadSecretsEncryptionKey loads the encryption key for the secrets database. | ||||
// If the file does not exist, returns os.ErrNotExist. | ||||
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) | ||||
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. better to print the path 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. |
||||
} | ||||
return data, nil | ||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
package cmd | ||
|
||
import ( | ||
"fmt" | ||
"io/ioutil" | ||
"os" | ||
"path/filepath" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
|
||
"github.com/onflow/flow-go/cmd/bootstrap/utils" | ||
"github.com/onflow/flow-go/fvm/errors" | ||
"github.com/onflow/flow-go/model/bootstrap" | ||
"github.com/onflow/flow-go/utils/unittest" | ||
) | ||
|
||
// TestLoadSecretsEncryptionKey checks that the key file is read correctly if it exists | ||
// and returns the expected sentinel error if it does not exist. | ||
func TestLoadSecretsEncryptionKey(t *testing.T) { | ||
myID := unittest.IdentifierFixture() | ||
|
||
unittest.RunWithTempDir(t, func(dir string) { | ||
path := filepath.Join(dir, fmt.Sprintf(bootstrap.PathSecretsEncryptionKey, myID)) | ||
|
||
t.Run("should return ErrNotExist if file doesn't exist", func(t *testing.T) { | ||
require.NoFileExists(t, path) | ||
_, err := loadSecretsEncryptionKey(dir, myID) | ||
assert.Error(t, err) | ||
assert.True(t, errors.Is(err, os.ErrNotExist)) | ||
}) | ||
|
||
t.Run("should return key and no error if file exists", func(t *testing.T) { | ||
err := os.MkdirAll(filepath.Join(dir, bootstrap.DirPrivateRoot, fmt.Sprintf("private-node-info_%v", myID)), 0700) | ||
require.NoError(t, err) | ||
key, err := utils.GenerateSecretsDBEncryptionKey() | ||
require.NoError(t, err) | ||
err = ioutil.WriteFile(path, key, 0700) | ||
require.NoError(t, err) | ||
|
||
data, err := loadSecretsEncryptionKey(dir, myID) | ||
assert.NoError(t, err) | ||
assert.Equal(t, key, data) | ||
}) | ||
}) | ||
} |
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.
not sure if these change have anything to do with Secrets database
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.
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
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.
8e7c481