Added app level encryption feature. #599

andresuribe87 · 2023-07-25T23:54:50Z

Overview

Allow for operators of ssi-service to choose whether to do app level encryption.

Description

At a high-level, this PR is adding a Data Encryption Key. The DEK can be stored either in the configured storage, or in a KMS. The DEK is used for every read/write that happens in the application.

Specifically, this PR does the following:

Removed some methods that weren't necessary in the storage interface.
Implemented and EncryptedWrapper, which takes in any implementation of a storage, and adds an encryption layer.
Refactored the existing Key Encryption Mechanism so it could be reused with the data encryption layer.
Added documentation.

How Has This Been Tested?

All unit tests and integration tests pass when encryption is enabled (see the dev.toml file).

codecov-commenter · 2023-07-26T00:01:01Z

Codecov Report

Merging #599 (e7c94e6) into main (31ea6fd) will increase coverage by 1.27%.
The diff coverage is 50.51%.

@@            Coverage Diff             @@
##             main     #599      +/-   ##
==========================================
+ Coverage   25.70%   26.97%   +1.27%     
==========================================
  Files          46       48       +2     
  Lines        5699     5860     +161     
==========================================
+ Hits         1465     1581     +116     
- Misses       3951     3995      +44     
- Partials      283      284       +1

Files Changed	Coverage Δ
pkg/service/did/web.go	`0.00% <0.00%> (ø)`
pkg/storage/bolt.go	`70.22% <ø> (+4.18%)`	⬆️
pkg/encryption/encryption.go	`16.17% <16.17%> (ø)`
config/config.go	`43.62% <42.85%> (+16.26%)`	⬆️
pkg/service/keystore/storage.go	`38.28% <42.85%> (+3.54%)`	⬆️
pkg/service/operation/storage.go	`25.25% <50.00%> (+0.51%)`	⬆️
pkg/storage/encrypt.go	`57.77% <57.77%> (ø)`
pkg/service/keystore/service.go	`42.75% <60.00%> (+0.64%)`	⬆️
pkg/storage/storage.go	`70.00% <85.48%> (+25.26%)`	⬆️

... and 2 files with indirect coverage changes

decentralgabe

it would be great to have tests that exercise different confidence levels (no encryption, local encryption, hsm encryption)

for hsm we would need a reliable cloud test env, so maybe that's out of scope for now but would be good to track in a future issue.

similarly, we should track issues for managing key rotation.

if you didn't (I may have missed) it would be useful to highlight that we've specifically chosen an authenticated encryption scheme which gives us the added benefit of tamper evidence, and may also prove useful for decrypting/re-encrypting data during key rotations since we'll be able to see which key encrypted the data

another thing worth covering in the doc is the data that we need to keep in plaintext, and the considerations around that, for keys. these keys are often just UUIDs but sometimes contain other information to make prefix queries viable. that means they could contain a DID, which by someones definition may be PII. <-- not sure here but we should verify

decentralgabe · 2023-07-26T00:05:39Z

config/config.go

+}
+
+type EncryptionConfig struct {
+	DisableEncryption bool `toml:"disable_encryption"`


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?

I added some in a061870

decentralgabe · 2023-07-26T00:06:34Z

config/config.go

@@ -72,6 +72,10 @@ type ServicesConfig struct {
 	StorageOptions  []storage.Option `toml:"storage_option"`


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)

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

decentralgabe · 2023-07-26T00:08:26Z

doc/STORAGE.md

+# encryption
+disable_encryption = true
+```


this is great to have

decentralgabe · 2023-07-26T00:08:35Z

pkg/encryption/encryption.go

+	"context"
+	"strings"
+
+	util2 "github.com/TBD54566975/ssi-sdk/util"


Suggested change

util2 "github.com/TBD54566975/ssi-sdk/util"

sdkutil "github.com/TBD54566975/ssi-sdk/util"

decentralgabe · 2023-07-26T00:09:20Z

pkg/encryption/encryption.go

+
+type KeyResolver func(ctx context.Context) ([]byte, error)
+
+type XChaCha20Poly1305Encrypter struct {


is this only used locally since the doc above mentions only AES GCM is supported by HSMs?

It's only used when the encryption keys are stored locally (either KEK or DEK).

decentralgabe · 2023-07-26T00:13:03Z

pkg/service/keystore/service.go

@@ -50,20 +49,17 @@ func (s Service) Config() config.KeyStoreServiceConfig {
 }

 func NewKeyStoreService(config config.KeyStoreServiceConfig, s storage.ServiceStorage) (*Service, error) {
-	if err := EnsureServiceKeyExists(config, s); err != nil {
-		return nil, sdkutil.LoggingErrorMsg(err, "initializing keystore")
+	encrypter, decrypter, err := NewServiceEncryption(s, config.EncryptionConfig, ServiceKeyEncryptionKey)


I would prefer, that instead of a noopEncrypter and noopDecrypter we check config here, and if the flag is off we pass in nil values below, this avoids the overhead of a no-op call for each read/write

IIUC, I'm not quite sure there is any overhead.

A conditional statement evaluating whether an encrypter is nil is the same overhead as making a call that does a noop on a non-nil object. In fact, the latter might end up being optimized away by the compiler. Not having if statements typically helps with branch prediction.

I changed this internally so nil values can be returned, but I'm not convinced it's easier to read.

decentralgabe · 2023-07-26T00:13:25Z

pkg/service/keystore/service_test.go

-			kt: crypto.RSA,
-		},
-	}
-	for _, test := range tests {


why delete this?

The {En,De}cryptKey methods weren't being used, so I removed that test. After evaluating, the better thing to do is to make these tests acts on XChaCha20Poly1305Encrypter. I've moved this to the relevant file.

decentralgabe · 2023-07-26T00:14:07Z

pkg/service/keystore/storage.go

 		return nil
 	}

 	watchKeys := []storage.WatchKey{{
 		Namespace: namespace,
-		Key:       skKey,
+		Key:       encryptionKeyKey,


maybe encryptionKeyID?

Settled for encryptionMaterialKey.

decentralgabe · 2023-07-26T00:14:56Z

pkg/service/service.go

+	if err != nil {
+		return nil, errors.Wrap(err, "creating app level encrypter")
+	}
+	storageProvider := storage.NewEncryptedWrapper(unencryptedStorageProvider, storageEncrypter, storageDecrypter)


if this is a no-op the encrypted wrapper is unnecessary

decentralgabe · 2023-07-26T00:15:15Z

pkg/service/service.go

 	}
-	keyStoreServiceFactory := keystore.NewKeyStoreServiceFactory(config.KeyStoreConfig, storageProvider)
+	keyStoreServiceFactory := keystore.NewKeyStoreServiceFactory(config.KeyStoreConfig, storageProvider, keyEncrypter, keyDecrypter)


same comment here

Changed internally to handle possible nil key{En,De}crypter values.

andresuribe87 · 2023-07-26T09:02:44Z

it would be great to have tests that exercise different confidence levels (no encryption, local encryption, hsm encryption)

for hsm we would need a reliable cloud test env, so maybe that's out of scope for now but would be good to track in a future issue.

no encryption is covered in unit tests. Local encryption is covered in integration tests. HSM will be tracked in #600

similarly, we should track issues for managing key rotation.

#601

if you didn't (I may have missed) it would be useful to highlight that we've specifically chosen an authenticated encryption scheme which gives us the added benefit of tamper evidence, and may also prove useful for decrypting/re-encrypting data during key rotations since we'll be able to see which key encrypted the data

I'm going to leave that for the same issue as above (i.e. #601 )

another thing worth covering in the doc is the data that we need to keep in plaintext, and the considerations around that, for keys. these keys are often just UUIDs but sometimes contain other information to make prefix queries viable. that means they could contain a DID, which by someones definition may be PII. <-- not sure here but we should verify

We could encrypt keys as long as we preserve the key prefix structure. That said, it doesn't seem like it's worth the effort and additional complexity. I added some more info to the docs, and created another issue to track #603

andresuribe87

Seems like these never went through :O

andresuribe87 · 2023-07-26T07:29:18Z

config/config.go

+}
+
+type EncryptionConfig struct {
+	DisableEncryption bool `toml:"disable_encryption"`


I added some in a061870

andresuribe87 · 2023-07-26T07:30:28Z

pkg/encryption/encryption.go

+
+type KeyResolver func(ctx context.Context) ([]byte, error)
+
+type XChaCha20Poly1305Encrypter struct {


It's only used when the encryption keys are stored locally (either KEK or DEK).

andresuribe87 · 2023-07-26T07:36:24Z

pkg/service/keystore/service.go

@@ -50,20 +49,17 @@ func (s Service) Config() config.KeyStoreServiceConfig {
 }

 func NewKeyStoreService(config config.KeyStoreServiceConfig, s storage.ServiceStorage) (*Service, error) {
-	if err := EnsureServiceKeyExists(config, s); err != nil {
-		return nil, sdkutil.LoggingErrorMsg(err, "initializing keystore")
+	encrypter, decrypter, err := NewServiceEncryption(s, config.EncryptionConfig, ServiceKeyEncryptionKey)


IIUC, I'm not quite sure there is any overhead.

A conditional statement evaluating whether an encrypter is nil is the same overhead as making a call that does a noop on a non-nil object. In fact, the latter might end up being optimized away by the compiler. Not having if statements typically helps with branch prediction.

I changed this internally so nil values can be returned, but I'm not convinced it's easier to read.

andresuribe87 · 2023-07-26T07:38:08Z

pkg/service/keystore/service_test.go

-			kt: crypto.RSA,
-		},
-	}
-	for _, test := range tests {


The {En,De}cryptKey methods weren't being used, so I removed that test. After evaluating, the better thing to do is to make these tests acts on XChaCha20Poly1305Encrypter. I've moved this to the relevant file.

andresuribe87 · 2023-07-26T07:45:52Z

pkg/service/keystore/storage.go

 		return nil
 	}

 	watchKeys := []storage.WatchKey{{
 		Namespace: namespace,
-		Key:       skKey,
+		Key:       encryptionKeyKey,


Settled for encryptionMaterialKey.

andresuribe87 · 2023-07-26T07:50:28Z

pkg/service/service.go

+	if err != nil {
+		return nil, errors.Wrap(err, "creating app level encrypter")
+	}
+	storageProvider := storage.NewEncryptedWrapper(unencryptedStorageProvider, storageEncrypter, storageDecrypter)


andresuribe87 · 2023-07-26T08:12:05Z

pkg/service/service.go

 	}
-	keyStoreServiceFactory := keystore.NewKeyStoreServiceFactory(config.KeyStoreConfig, storageProvider)
+	keyStoreServiceFactory := keystore.NewKeyStoreServiceFactory(config.KeyStoreConfig, storageProvider, keyEncrypter, keyDecrypter)


Changed internally to handle possible nil key{En,De}crypter values.

andresuribe87 · 2023-07-26T08:15:26Z

pkg/encryption/encryption.go

+	"context"
+	"strings"
+
+	util2 "github.com/TBD54566975/ssi-sdk/util"


andresuribe87 · 2023-07-26T08:17:23Z

config/config.go

@@ -72,6 +72,10 @@ type ServicesConfig struct {
 	StorageOptions  []storage.Option `toml:"storage_option"`


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

Added app level encryption feature.

7fdd5bf

andresuribe87 requested review from decentralgabe and nitro-neal as code owners July 25, 2023 23:54

Merge branch 'main' into encryption_for_all

7c03551

decentralgabe suggested changes Jul 26, 2023

View reviewed changes

andresuribe87 added 5 commits July 26, 2023 03:28

Add validation of config parameters.

a061870

test

20c1023

better name

5c6a600

naming

bbe0354

nil encrypter for kek

bf0c2ba

This was referenced Jul 26, 2023

Application level encryption SIP #602

Open

App Level Encryption Key Encryption #603

Open

andresuribe87 added 2 commits July 26, 2023 04:53

docs.

df7c216

integration fix

e7c94e6

decentralgabe approved these changes Jul 26, 2023

View reviewed changes

andresuribe87 commented Jul 26, 2023

View reviewed changes

andresuribe87 merged commit f836a81 into TBD54566975:main Jul 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added app level encryption feature. #599

Added app level encryption feature. #599

andresuribe87 commented Jul 25, 2023

codecov-commenter commented Jul 26, 2023 •

edited

Loading

decentralgabe left a comment

decentralgabe Jul 26, 2023

andresuribe87 Jul 26, 2023

decentralgabe Jul 26, 2023

andresuribe87 Jul 26, 2023

decentralgabe Jul 26, 2023

decentralgabe Jul 26, 2023

andresuribe87 Jul 26, 2023

decentralgabe Jul 26, 2023

andresuribe87 Jul 26, 2023

decentralgabe Jul 26, 2023

andresuribe87 Jul 26, 2023

decentralgabe Jul 26, 2023

andresuribe87 Jul 26, 2023

decentralgabe Jul 26, 2023

andresuribe87 Jul 26, 2023

decentralgabe Jul 26, 2023

andresuribe87 Jul 26, 2023

decentralgabe Jul 26, 2023

andresuribe87 Jul 26, 2023

andresuribe87 commented Jul 26, 2023

andresuribe87 left a comment

andresuribe87 Jul 26, 2023

andresuribe87 Jul 26, 2023

andresuribe87 Jul 26, 2023

andresuribe87 Jul 26, 2023

andresuribe87 Jul 26, 2023

andresuribe87 Jul 26, 2023

andresuribe87 Jul 26, 2023

andresuribe87 Jul 26, 2023

andresuribe87 Jul 26, 2023

		@@ -72,6 +72,10 @@ type ServicesConfig struct {
		StorageOptions []storage.Option `toml:"storage_option"`

	util2 "github.com/TBD54566975/ssi-sdk/util"
	sdkutil "github.com/TBD54566975/ssi-sdk/util"


		type KeyResolver func(ctx context.Context) ([]byte, error)

		type XChaCha20Poly1305Encrypter struct {

Added app level encryption feature. #599

Added app level encryption feature. #599

Conversation

andresuribe87 commented Jul 25, 2023

Overview

Description

How Has This Been Tested?

codecov-commenter commented Jul 26, 2023 • edited Loading

Codecov Report

decentralgabe 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

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

andresuribe87 commented Jul 26, 2023

andresuribe87 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

codecov-commenter commented Jul 26, 2023 •

edited

Loading