generated from TBD54566975/tbd-project-template
-
Notifications
You must be signed in to change notification settings - Fork 55
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
Added app level encryption feature. #599
Merged
andresuribe87
merged 9 commits into
TBD54566975:main
from
andresuribe87:encryption_for_all
Jul 26, 2023
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
7fdd5bf
Added app level encryption feature.
andresuribe87 7c03551
Merge branch 'main' into encryption_for_all
andresuribe87 a061870
Add validation of config parameters.
andresuribe87 20c1023
test
andresuribe87 5c6a600
better name
andresuribe87 bbe0354
naming
andresuribe87 bf0c2ba
nil encrypter for kek
andresuribe87 df7c216
docs.
andresuribe87 e7c94e6
integration fix
andresuribe87 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ package config | |
|
||
import ( | ||
"fmt" | ||
"io/fs" | ||
"os" | ||
"path/filepath" | ||
"reflect" | ||
|
@@ -72,6 +73,10 @@ type ServicesConfig struct { | |
StorageOptions []storage.Option `toml:"storage_option"` | ||
ServiceEndpoint string `toml:"service_endpoint"` | ||
|
||
// Application level encryption configuration. Defines how values are encrypted before they are stored in the | ||
// configured KV store. | ||
AppLevelEncryptionConfiguration EncryptionConfig `toml:"storage_encryption,omitempty"` | ||
|
||
// Embed all service-specific configs here. The order matters: from which should be instantiated first, to last | ||
KeyStoreConfig KeyStoreServiceConfig `toml:"keystore,omitempty"` | ||
DIDConfig DIDServiceConfig `toml:"did,omitempty"` | ||
|
@@ -94,21 +99,53 @@ type BaseServiceConfig struct { | |
type KeyStoreServiceConfig struct { | ||
*BaseServiceConfig | ||
|
||
// The URI for the master key. We use tink for envelope encryption as described in https://github.com/google/tink/blob/9bc2667963e20eb42611b7581e570f0dddf65a2b/docs/KEY-MANAGEMENT.md#key-management-with-tink | ||
// When left empty, then a random key is generated and used. | ||
// Configuration describing the encryption of the private keys that are under ssi-service's custody. | ||
EncryptionConfig | ||
} | ||
|
||
type EncryptionConfig struct { | ||
DisableEncryption bool `toml:"disable_encryption"` | ||
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. do we have some circuit-breaker logic that doesn't allow this to be a set, or at least throws a warning if env == prod? 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 added some in a061870 |
||
|
||
// The URI for a master key. We use tink for envelope encryption as described in https://github.com/google/tink/blob/9bc2667963e20eb42611b7581e570f0dddf65a2b/docs/KEY-MANAGEMENT.md#key-management-with-tink | ||
// When left empty and DisableEncryption is off, then a random key is generated and used. This random key is persisted unencrypted in the | ||
// configured storage. Production deployments should never leave this field empty. | ||
MasterKeyURI string `toml:"master_key_uri"` | ||
|
||
// Path for credentials. Required when using an external KMS. More info at https://github.com/google/tink/blob/9bc2667963e20eb42611b7581e570f0dddf65a2b/docs/KEY-MANAGEMENT.md#credentials | ||
// Path for credentials. Required when MasterKeyURI is set. More info at https://github.com/google/tink/blob/9bc2667963e20eb42611b7581e570f0dddf65a2b/docs/KEY-MANAGEMENT.md#credentials | ||
KMSCredentialsPath string `toml:"kms_credentials_path"` | ||
} | ||
|
||
func (e EncryptionConfig) GetMasterKeyURI() string { | ||
return e.MasterKeyURI | ||
} | ||
|
||
func (e EncryptionConfig) GetKMSCredentialsPath() string { | ||
return e.KMSCredentialsPath | ||
} | ||
|
||
func (e EncryptionConfig) EncryptionEnabled() bool { | ||
return !e.DisableEncryption | ||
} | ||
|
||
func (k *KeyStoreServiceConfig) IsEmpty() bool { | ||
if k == nil { | ||
return true | ||
} | ||
return reflect.DeepEqual(k, &KeyStoreServiceConfig{}) | ||
} | ||
|
||
func (k *KeyStoreServiceConfig) GetMasterKeyURI() string { | ||
return k.MasterKeyURI | ||
} | ||
|
||
func (k *KeyStoreServiceConfig) GetKMSCredentialsPath() string { | ||
return k.KMSCredentialsPath | ||
} | ||
|
||
func (k *KeyStoreServiceConfig) EncryptionEnabled() bool { | ||
return !k.DisableEncryption | ||
} | ||
|
||
type DIDServiceConfig struct { | ||
*BaseServiceConfig | ||
Methods []string `toml:"methods"` | ||
|
@@ -211,7 +248,10 @@ func (p *WebhookServiceConfig) IsEmpty() bool { | |
|
||
// LoadConfig attempts to load a TOML config file from the given path, and coerce it into our object model. | ||
// Before loading, defaults are applied on certain properties, which are overwritten if specified in the TOML file. | ||
func LoadConfig(path string) (*SSIServiceConfig, error) { | ||
func LoadConfig(path string, fs fs.FS) (*SSIServiceConfig, error) { | ||
if fs == nil { | ||
fs = os.DirFS(".") | ||
} | ||
loadDefaultConfig, err := checkValidConfigPath(path) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "validate config path") | ||
|
@@ -226,17 +266,33 @@ func LoadConfig(path string) (*SSIServiceConfig, error) { | |
if loadDefaultConfig { | ||
defaultServicesConfig := getDefaultServicesConfig() | ||
config.Services = defaultServicesConfig | ||
} else if err = loadTOMLConfig(path, &config); err != nil { | ||
} else if err = loadTOMLConfig(path, &config, fs); err != nil { | ||
return nil, errors.Wrap(err, "load toml config") | ||
} | ||
|
||
if err = applyEnvVariables(&config); err != nil { | ||
return nil, errors.Wrap(err, "apply env variables") | ||
} | ||
|
||
if err = validateConfig(&config); err != nil { | ||
return nil, errors.Wrap(err, "validating config values") | ||
} | ||
|
||
return &config, nil | ||
} | ||
|
||
func validateConfig(s *SSIServiceConfig) error { | ||
if s.Server.Environment == EnvironmentProd { | ||
if s.Services.KeyStoreConfig.DisableEncryption { | ||
return errors.New("prod environment cannot disable key encryption") | ||
} | ||
if s.Services.AppLevelEncryptionConfiguration.DisableEncryption { | ||
logrus.Warn("prod environment detected without app level encryption. This is strongly discouraged.") | ||
} | ||
} | ||
return nil | ||
} | ||
|
||
func checkValidConfigPath(path string) (bool, error) { | ||
// no path, load default config | ||
defaultConfig := false | ||
|
@@ -314,9 +370,13 @@ func getDefaultServicesConfig() ServicesConfig { | |
} | ||
} | ||
|
||
func loadTOMLConfig(path string, config *SSIServiceConfig) error { | ||
func loadTOMLConfig(path string, config *SSIServiceConfig, fs fs.FS) error { | ||
// load from TOML file | ||
if _, err := toml.DecodeFile(path, &config); err != nil { | ||
file, err := fs.Open(path) | ||
if err != nil { | ||
return errors.Wrapf(err, "opening path %s", path) | ||
} | ||
if _, err := toml.NewDecoder(file).Decode(&config); err != nil { | ||
return errors.Wrapf(err, "could not load config: %s", path) | ||
} | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,32 @@ | ||
package config | ||
|
||
import ( | ||
"embed" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestConfig(t *testing.T) { | ||
config, err := LoadConfig(Filename) | ||
assert.NoError(t, err) | ||
assert.NotEmpty(t, config) | ||
//go:embed testdata | ||
var testdata embed.FS | ||
|
||
assert.False(t, config.Server.ReadTimeout.String() == "") | ||
assert.False(t, config.Server.WriteTimeout.String() == "") | ||
assert.False(t, config.Server.ShutdownTimeout.String() == "") | ||
assert.False(t, config.Server.APIHost == "") | ||
func TestLoadConfig(t *testing.T) { | ||
t.Run("returns no errors when passed in file", func(t *testing.T) { | ||
config, err := LoadConfig(Filename, nil) | ||
assert.NoError(t, err) | ||
assert.NotEmpty(t, config) | ||
|
||
assert.NotEmpty(t, config.Services.StorageProvider) | ||
assert.False(t, config.Server.ReadTimeout.String() == "") | ||
assert.False(t, config.Server.WriteTimeout.String() == "") | ||
assert.False(t, config.Server.ShutdownTimeout.String() == "") | ||
assert.False(t, config.Server.APIHost == "") | ||
|
||
assert.NotEmpty(t, config.Services.StorageProvider) | ||
}) | ||
|
||
t.Run("returns errors when prod disables encryption", func(t *testing.T) { | ||
_, err := LoadConfig("testdata/test1.toml", testdata) | ||
assert.Error(t, err) | ||
assert.ErrorContains(t, err, "prod environment cannot disable key encryption") | ||
}) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
[server] | ||
env = "prod" # either 'dev', 'test', or 'prod' | ||
|
||
[services.keystore] | ||
name = "keystore" | ||
disable_encryption = true |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
general comment: it would be great to have a light design doc around this kind of thing before going for an implementation. there are a number of hairy details that would be good to discuss and get feedback on. not only that it would serve as a record for how the feature was thought through which will be useful in the future (as we have already seen with existing SIPs)
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.
Didn't deem it worth making a doc at the time I started this. Seemed pretty straightforward. I'll do one in a separate PR. Tracked in #602