Repository: cossacklabs/acra (language: python)

Issue:
Cache fetching rotated key names
* added caching results from `store.getCachedHistoricalPrivateKeyFilenames` call that reads folder every time when decryption keys are requested.
* due to `[]byte` type used in our in-memory cache with interface `Get(id[]byte)[]byte`/`Set([]byte, []byte)`, to cache results used msgpack as [more efficient](https://github.com/alecthomas/go_serialization_benchmarks) way to serialize/deserialize than `gob` or `split/join` strings.
* struct for msgpack placed in `internal` package because msgpack can serialize only public structs. And this struct shouldn't be public and exported from `keystore/filesystem` package. So to hide and encapsulate this struct, it placed to `internal` that accessible from `keystore/filesystem`, but not accessed from top-level packages.
* added benchmarks that prove efficient serialization in comparison with simple strings operations
* because of checked profiling data that this function removed from call graph, also updated docker-compose and added tmpfs usage for postgresql that decreased time of tests and works more predictable. It cannot be used for keystore because docker doesn't support tmpfs volumes shared between containers.
* also in prifling data found that we have extra allocations in `getSymmetricKeys` due to small start allocation `keys := make([][]byte, 0, 1)` when usually may be at least 2 keys (current and 1 rotated). So increased to 4. It is not increase memory usage because it's allocates +3 structs with 3 `int` fields (SliceHeader with ptr + length + capacity), so it's just 3 * 24 bytes (72) instead of 24.

## 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) 

[coding guidelines]: https://golang.org/doc/effective_go
[Example projects and code samples]: https://github.com/cossacklabs/acra-engineering-demo
[Acra documentation]: https://docs.cossacklabs.com/
[documentation repository]: https://github.com/cossacklabs/product-docs


Discussion / hints:
We probably _could_ make keystore cache interface to be `[]byte <-> interface{}` instead of `[]byte <-> []byte`, but I guess existing scenario (caching bytes array) is more common in practice, and using interface would need type casting on each access, so probably some little overhead will be involved
> We probably could make keystore cache interface to be []byte <-> interface{} instead of []byte <-> []byte, but I guess existing scenario (caching bytes array) is more common in practice, and using interface would need type casting on each access, so probably some little overhead will be involved

I though about it... but I don't want to use reflection here because it more type checks, more code like:
```
value, ok := cache.Get(key)
if !ok{...}
typedValue, ok := value.([]string}
if !ok {...}
```

for now, it is enough to convert everything to bytes, imho. you can add more arguments why we should change this approach.

Fix patch:
diff --git a/keystore/filesystem/server_keystore.go b/keystore/filesystem/server_keystore.go
index 607926170..2d155d1a8 100644
--- a/keystore/filesystem/server_keystore.go
+++ b/keystore/filesystem/server_keystore.go
@@ -924,10 +924,33 @@ func (store *KeyStore) GetPoisonKeyPair() (*keys.Keypair, error) {
 	privateKey, privateOk := store.cache.Get(PoisonKeyFilename)
 	publicKey, publicOk := store.cache.Get(poisonKeyFilenamePublic)
 	if privateOk && publicOk {
-		return &keys.Keypair{Public: &keys.PublicKey{Value: publicKey}, Private: &keys.PrivateKey{Value: privateKey}}, nil
+		decryptedPrivate, err := store.encryptor.Decrypt(privateKey, []byte(PoisonKeyFilename))
+		if err != nil {
+			return nil, err
+		}
+		return &keys.Keypair{Public: &keys.PublicKey{Value: publicKey}, Private: &keys.PrivateKey{Value: decryptedPrivate}}, nil
+	}
+	privatePath := store.GetPrivateKeyFilePath(PoisonKeyFilename)
+	private, err := store.loadPrivateKey(privatePath)
+	if err != nil {
+		if IsKeyReadError(err) {
+			log.Debug("Generate poison key pair")
+			return store.generateKeyPair(PoisonKeyFilename, []byte(PoisonKeyFilename))
+		}
+		return nil, err
+	}
+	encryptedPrivate := private.Value
+	if private.Value, err = store.encryptor.Decrypt(private.Value, []byte(PoisonKeyFilename)); err != nil {
+		return nil, err
+	}
+	publicPath := store.GetPublicKeyFilePath(poisonKeyFilenamePublic)
+	public, err := store.loadPublicKey(publicPath)
+	if err != nil {
+		return nil, err
 	}
-	log.Debug("Generate poison key pair")
-	return store.generateKeyPair(PoisonKeyFilename, []byte(PoisonKeyFilename))
+	store.cache.Add(PoisonKeyFilename, encryptedPrivate)
+	store.cache.Add(poisonKeyFilenamePublic, public.Value)
+	return &keys.Keypair{Public: public, Private: private}, nil
 }
 
 // GetPoisonPrivateKeys returns all private keys used to decrypt poison records, from newest to oldest.


Test patch:
diff --git a/keystore/filesystem/server_keystore_test.go b/keystore/filesystem/server_keystore_test.go
index e7bb5e219..1a714611f 100644
--- a/keystore/filesystem/server_keystore_test.go
+++ b/keystore/filesystem/server_keystore_test.go
@@ -1101,3 +1101,64 @@ func BenchmarkHistoricalPathsSerialization(b *testing.B) {
 		reflect.DeepEqual(paths, values)
 	})
 }
+
+func TestKeyStore_GetPoisonKeyPair(t *testing.T) {
+	tmpDir, err := ioutil.TempDir("", "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	encryptor, err := keystore.NewSCellKeyEncryptor([]byte(`some key`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	keyStore, err := NewFilesystemKeyStore(tmpDir, encryptor)
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Run("Check empty cache before generation", func(t *testing.T) {
+		_, ok := keyStore.cache.Get(PoisonKeyFilename)
+		if ok {
+			t.Fatal("Unexpected cached poison private key")
+		}
+		_, ok = keyStore.cache.Get(poisonKeyFilenamePublic)
+		if ok {
+			t.Fatal("Unexpected cached poison public key")
+		}
+	})
+	// we don't have api to generate keypair because keystore generates it automatically on first request
+	keyPair, err := keyStore.GetPoisonKeyPair()
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Run("Check caching keys after generation and property that private key encrypted", func(t *testing.T) {
+		privateKey, ok := keyStore.cache.Get(PoisonKeyFilename)
+		if !ok {
+			t.Fatal("Private key wasn't cached")
+		}
+		if bytes.Equal(privateKey, keyPair.Private.Value) {
+			t.Fatal("Cached private key wasn't encrypted and equal to generated raw keypair")
+		}
+		publicKey, ok := keyStore.cache.Get(poisonKeyFilenamePublic)
+		if !ok {
+			t.Fatal("Public key wasn't cached")
+		}
+		if !bytes.Equal(keyPair.Public.Value, publicKey) {
+			t.Fatal("Cached public key not equal to raw key")
+		}
+	})
+	t.Run("Check reading keys from the cache after purging keystore folder", func(t *testing.T) {
+		if err := os.RemoveAll(tmpDir); err != nil {
+			t.Fatal(err)
+		}
+		keyPair2, err := keyStore.GetPoisonKeyPair()
+		if err != nil {
+			t.Fatal(err)
+		}
+		if !bytes.Equal(keyPair2.Public.Value, keyPair.Public.Value) {
+			t.Fatal("Took unexpected new keypair's public key")
+		}
+		if !bytes.Equal(keyPair2.Private.Value, keyPair.Private.Value) {
+			t.Fatal("Took unexpected new keypair's private key")
+		}
+	})
+}