Use kms per client #556

Zhaars · 2022-08-01T09:51:34Z

Initial Implementation of KMS per client.

This PR contains several significant changes:

change keystore.Encryptor interface - now Encrypt/Decrypt method expect to pass KeyContext.
SCellEncryptor implementation was changed based on KeyContext.Context
Implemented kms.Encryptor and kms.KeyMakingWrapper which is simple Decorator with kms key creation

type KeyContext struct {
  ClientID []byte
  ZoneID []byte
  Context []byte
  Purpose []byte/string/etc
}

Left some TODOs in the code with some open questions to discuss.

Checklist

Change is covered by automated tests
The coding guidelines are followed
Public API has proper documentation in the Acra documentation site or has PR on documentation repository
with new changes
CHANGELOG.md is updated (in case of notable or breaking changes)
CHANGELOG_DEV.md is updated
Benchmark results are attached (if applicable)
Example projects and code samples are up-to-date (in case of API changes)

Implementation of KMS pre client

# Conflicts: # keystore/filesystem/server_keystore.go # keystore/filesystem/server_keystore_test.go

Fixed conflicts with master

keystore/filesystem/filesystem_backup.go

Lagovas · 2022-08-01T13:26:35Z

keystore/kms/key_encryptor.go

+	keyID, err := getKeyIDFromContext(keyContext)
+	if err != nil {
+		// TODO: add logging
+		return nil, err


we can try to get logger from the context via logging.GetLoggerFromContext function

keystore/kms/key_encryptor.go

keystore/kms/key_making_wrapper.go

keystore/filesystem/filesystem_backup.go

keystore/keystore.go

Fixed after review

Fixed imports

keystore/filesystem/filesystem_backup.go

G1gg1L3s · 2022-08-01T19:39:49Z

keystore/filesystem/server_keystore.go

@@ -239,8 +242,10 @@ func newFilesystemKeyStore(privateKeyFolder, publicKeyFolder string, storage Sto
 			return nil, err
 		}
 	}
+
+	ctx, _ := context.WithTimeout(context.Background(), network.DefaultNetworkTimeout)


Can you please tell, why there is a timeout in context?

Sure. It is a good practice to have a timeout for any network operation. We can ignore it for our SCellEncrytor as we don't do any network calls, but for KMS-related stuff, we should have some timeout checking. In the case of KMS unavailability or some network issue, the request will exit (in 60 secs) with an error TimoutExceeded

And context is a commonly used thing for this case in Go. A great practice to pass context as the first param to the functions

Fixed after review/added integration tests/extend addzone with KMS KEK creating

Added KMS keystore encryptor initialization to acra-translator

Restructure configs

Added KMS keystore encryptor for all keystore use acra tools

Lagovas · 2022-08-03T16:05:44Z

cmd/acra-keys/keys/migrate-keys.go

@@ -226,7 +226,7 @@ func MigrateV1toV2(srcV1 filesystem.KeyExport, dstV2 keystoreV2.KeyFileImportV1)

 	log.Tracef("Importing %d keys from keystore v1", expected)
 	for _, key := range keys {
-		log := log.WithField("purpose", key.Purpose).WithField("id", key.ID)
+		log := log.WithField("purpose", key.KeyContext.Purpose).WithField("id", keystore.GetKeyContextFromContext(key.KeyContext))


how key.KeyContext will be logged?

But we are logging key.KeyContext.Purpose here or what do you mean?

.WithField("id", keystore.GetKeyContextFromContext(key.KeyContext))
How it will look in log entry? Something like &{ClientID: nil, ZoneID: nil, Context: []byte{0, 12, 42, 321}} or something else?

Not precisely, previously we had

log := log.WithField("purpose", key.Purpose).WithField("id", key.ID)

where key.ID is simple bytes slice which is strange as it would print something like []byte{0, 12, 42, 321}

So I just repeat the functionality. Here key.KeyContext is not bytes slice but keystore.KeyContext struct. So now it will print some bytes slice because of the function.

func GetKeyContextFromContext(keyContext KeyContext) []byte { if keyContext.ZoneID != nil { return keyContext.ZoneID } if keyContext.ClientID != nil { return keyContext.ClientID } if keyContext.Context != nil { return keyContext.Context } return nil }

I think it makes sense to wrap with string to print string data instead of bytes @Lagovas

+1. maybe lets implement String() string to allow logger use this function?

keystore/filesystem/translator_keystore.go

keystore/keystore.go

keystore/kms/key_encryptor.go

tests/test.py

Fixed after review/add integration tests

keystore/kms/aws/client.go

Lagovas · 2022-08-05T01:42:36Z

tests/test.py

+    for new records"""
+    args = [os.path.join(BINARY_OUTPUT_FOLDER, 'acra-poisonrecordmaker')]
+    if keys_dir:
+        args.append('--keys_dir={}'.format(keys_dir))


should we pass here keystore version too?

No, its is required for keymaker but not for poisonrecordmaker.

Added keystore_encryption_type flag

Fixed unit tests

Fixed tests/updated CHANGELOG file

Fixed after review

Lagovas

great job

zhars/use_kms_per_client

e63491d

Implementation of KMS pre client

Zhaars requested review from Lagovas and G1gg1L3s August 1, 2022 09:51

Zhaars added 2 commits August 1, 2022 14:38

Merge branch 'master' into zhars/use_kms_per_client

a3d9870

# Conflicts: # keystore/filesystem/server_keystore.go # keystore/filesystem/server_keystore_test.go

zhars/use_kms_per_client

712f7dd

Fixed conflicts with master

Lagovas reviewed Aug 1, 2022

View reviewed changes

Zhaars added 2 commits August 1, 2022 17:44

zhars/use_kms_per_client

c053fa5

Fixed after review

zhars/use_kms_per_client

2ad118d

Fixed imports

G1gg1L3s reviewed Aug 1, 2022

View reviewed changes

zhars/use_kms_per_client

53e5ac3

Fixed after review/added integration tests/extend addzone with KMS KEK creating

Zhaars requested a review from Lagovas August 3, 2022 15:31

Zhaars marked this pull request as ready for review August 3, 2022 15:32

Zhaars added 3 commits August 3, 2022 17:45

zhars/use_kms_per_client

906cc1c

Added KMS keystore encryptor initialization to acra-translator

zhars/use_kms_per_client

24ef4ec

Restructure configs

zhars/use_kms_per_client

184b5f7

Added KMS keystore encryptor for all keystore use acra tools

Zhaars added acra-server tests cloud acra-rollback acra-poison_records_maker acra-translator acra-rotate acra-keymaker labels Aug 3, 2022

Lagovas reviewed Aug 3, 2022

View reviewed changes

zhars/use_kms_per_client

2ec2dc8

Fixed after review/add integration tests

Zhaars requested review from Lagovas and G1gg1L3s August 4, 2022 16:50

Zhaars self-assigned this Aug 4, 2022

Lagovas reviewed Aug 5, 2022

View reviewed changes

zhars/use_kms_per_client

a4209e3

Added keystore_encryption_type flag

Zhaars added 3 commits August 5, 2022 16:28

zhars/use_kms_per_client

6bd313b

Fixed unit tests

zhars/use_kms_per_client

e33262e

Fixed tests/updated CHANGELOG file

zhars/use_kms_per_client

d40c065

Fixed after review

Lagovas approved these changes Aug 8, 2022

View reviewed changes

Zhaars merged commit 0404a68 into master Aug 8, 2022

Lagovas deleted the zhars/use_kms_per_client branch August 22, 2022 21:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use kms per client #556

Use kms per client #556

Zhaars commented Aug 1, 2022 •

edited

Loading

Lagovas Aug 1, 2022

G1gg1L3s Aug 1, 2022

Zhaars Aug 3, 2022

Zhaars Aug 3, 2022

Lagovas Aug 3, 2022

Zhaars Aug 4, 2022

Lagovas Aug 4, 2022

Zhaars Aug 4, 2022

Lagovas Aug 8, 2022 •

edited

Loading

Lagovas Aug 5, 2022

Zhaars Aug 5, 2022

Lagovas left a comment

Use kms per client #556

Use kms per client #556

Conversation

Zhaars commented Aug 1, 2022 • edited Loading

Checklist

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

Lagovas Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lagovas left a comment

Choose a reason for hiding this comment

Zhaars commented Aug 1, 2022 •

edited

Loading

Lagovas Aug 8, 2022 •

edited

Loading