BlobCipherKeyCache::getLatestCipherKey return empty when there's no key#6908
BlobCipherKeyCache::getLatestCipherKey return empty when there's no key#6908sfc-gh-yiwu wants to merge 3 commits into
Conversation
sfc-gh-yiwu
force-pushed
the
encrypt_valgrind
branch
from
ba0a266 to
80830c6
Compare
Doxense CI Report for Windows 10
|
|
I intentionally added "encrypt_key_not_found" exception as there is no scenario where the code should hit this scenario; for instance:
|
This is in the BlobCipherKeyCache, and this is how I will use it. The caller will first query BlobCipherKeyCache for latest key, and initially when the cache is empty, the cache will return empty, and the caller will query the EncryptKeyProxy to obtain the latest key and insert into the cache. In this case the cache returning empty should not be considered an error. |
|
|
I see. |
I agree with the BlobCipherKeyCache usage, but, let's document this behavior in API contracts to avoid any confusion. |
|
|
||
| Reference<BlobCipherKey> BlobCipherKeyIdCache::getLatestCipherKey() { | ||
| if (latestBaseCipherKeyId == ENCRYPT_INVALID_CIPHER_KEY_ID) { | ||
| return Reference<BlobCipherKey>(); |
There was a problem hiding this comment.
we should be throwing an exception here; this should be a code logic bug?
There was a problem hiding this comment.
ENCRYPT_INVALID_CIPHER_KEY_ID exactly means the cache is empty.
There was a problem hiding this comment.
I think it is better not to conflate "invalid_cipher_id" with the "cache empty", a caller can pass "invalid_id" due to some bug even when cache is not-empty. We can add a helper method in the cache to return its size if needed.
There was a problem hiding this comment.
with getLatestCipherKey the caller is not passing in key id, so the ENCRYPT_INVALID_CIPHER_KEY_ID indicate internal state (no latest key). I agree for getCipherKey, it should come with invalid id check, which I'm not updating in this PR.
There was a problem hiding this comment.
If the cache is empty, then BlobCipherKeyIdCache should be invoked?
auto domainItr = domainCacheMap.find(domainId);
if (domainItr == domainCacheMap.end()) {
TraceEvent("GetLatestCipherKey_DomainNotFound").detail("DomainId", domainId);
throw encrypt_key_not_found();
}
The domainId check should return empty_reference based on your change? Am I missing anything here.
There was a problem hiding this comment.
Ah, sorry. I look at the valgrind stack again, and it is not EncryptionOps accessing BlobCipherKeyIdCache directly. It is BlobCipherKeyIdCache::insertBaseCipherKey calling getLatestCipherKey() before actually insert the key. In this case the BlobCipherKeyIdCache is created, but it is empty. In this case, do you think it make sense to keep the existing change?
Valgrind stack:
==46== Conditional jump or move depends on uninitialised value(s)
==46== at 0x6E09DAE: vfprintf (in /usr/lib64/libc-2.17.so)
==46== by 0x6E37178: vsnprintf (in /usr/lib64/libc-2.17.so)
==46== by 0x37DDB51: vsformat(std::string&, char const*, __va_list_tag*) (flow.cpp:229)
==46== by 0x37DE06C: format(char const*, ...) (flow.cpp:266)
==46== by 0xC00D07: toString (Trace.h:255)
==46== by 0xC00D07: std::enable_if<Traceable<unsigned long>::value, BaseTraceEvent&>::type BaseTraceEvent::detail<unsigned long>(char const*, unsigned long const&) (Trace.h:434)
==46== by 0x36D5BF6: BlobCipherKeyIdCache::getCipherByBaseCipherId(unsigned long const&, unsigned long const&) (BlobCipher.cpp:147)
==46== by 0x36D95A7: getLatestCipherKey (BlobCipher.cpp:137)
==46== by 0x36D95A7: BlobCipherKeyIdCache::insertBaseCipherKey(unsigned long const&, unsigned char const*, int) (BlobCipher.cpp:161)
==46== by 0x36D9E0B: BlobCipherKeyCache::insertCipherKey(long const&, unsigned long const&, unsigned char const*, int) (BlobCipher.cpp:253)
==46== by 0x23EC77E: EncryptionOpsWorkload::setupCipherEssentials() (EncryptionOps.actor.cpp:178)
==46== by 0x23ED0A5: EncryptionOpsWorkload::start(Database const&) (EncryptionOps.actor.cpp:305)
==46== by 0x1E5F2EA: CompoundWorkload::start(Database const&) (tester.actor.cpp:301)
==46== by 0x1E2E193: (anonymous namespace)::RunWorkloadAsyncActorState<(anonymous namespace)::RunWorkloadAsyncActor>::a_body1loopBody1when2(ReplyPromise<Void>&&, int) (tester.actor.cpp:592)
There was a problem hiding this comment.
A try-catch on caller side looks cumbersome. What do you think?
My initial thought that it is better for the caller to handle exceptions from a caching library, some exceptions can be 'acceptable' such as: key_not_found which could be satisfied by reaching out to KMS via EKPServer, however, I see below scheme could work as well:
- BlobCipherKeyCache throws key_not_found only for unexpected scenarios such as the one we discussed above. For a cache_miss it is OK to return an emptyReference. However, it is better to have unified behavior of empty-cache as well as cache-missing-a -specific scenario. If we choose not to throw exception, then I would say better not to distinguish these cases as technically they are same in some ways.
- As FDB EKs have 1:1 relation with KMS EKs, we can update EncryptKeyServer lookup interface to ensure that all cipher-keys requested are returned back to the client; if not, server throws "encrypt_key_not_found" (never to happen scenario and points to code logic bug/tampering etc.)
#2 I can take care as part of #6898.
Does this sounds reasonable?
Thanks
There was a problem hiding this comment.
by 0x36D95A7: BlobCipherKeyIdCache::insertBaseCipherKey(unsigned long const&, unsigned char const*, int) (BlobCipher.cpp:161)
if (cipherKey->getBaseCipherId() == baseCipherId) {
if (memcmp(cipherKey->rawBaseCipher(), baseCipher, baseCipherLen) == 0) {
TraceEvent("InsertBaseCipherKey_AlreadyPresent")
.detail("BaseCipherKeyId", baseCipherId)
.detail("DomainId", domainId); <---- line 161
// Key is already present; nothing more to do.
Something doesn't add up; the above scenario is ONLY possible when caller attempts to insert a "duplicate" baseCipherId with "exact CipherKey" match usecase (hence, no tampering or foul play).
I think the problem is the following line of code -
EncryptionOps.actor.cpp::setupCipherEssentials()
...
// insert the Encrypt Header cipherKey
generateRandomBaseCipher(AES_256_KEY_LENGTH, &buff[0], &cipherLen);
cipherKeyCache->insertCipherKey(
ENCRYPT_HEADER_DOMAIN_ID, headerBaseCipherId, buff, cipherLen, headerRandomSalt)
...
As expected inserting a cipher with API accepting 'salt' shouldn't update 'latestBaseCipher' details (as this entry should happen as part of reading encyrption header), however, the testcode is doing it as a 'hack' for "EncryptHeader authToken cipherKey".
Then, below code simulates "cache-miss" randomly for any {"domainId", "baseCipherId"} entry, and when done for domainId == -2 (EncryptHeader auth cipher key), the TraceEvent goes bonkers :)
Reference<BlobCipherKey> getEncryptionKey(const EncryptCipherDomainId& domainId,
const EncryptCipherBaseKeyId& baseCipherId,
const EncryptCipherRandomSalt& salt) {
const bool simCacheMiss = deterministicRandom()->randomInt(1, 100) < 15;
Reference<BlobCipherKeyCache> cipherKeyCache = BlobCipherKeyCache::getInstance();
Reference<BlobCipherKey> cipherKey = cipherKeyCache->getCipherKey(domainId, baseCipherId, salt);
if (simCacheMiss) {
TraceEvent("SimKeyCacheMiss").detail("EncyrptDomainId", domainId).detail("BaseCipherId", baseCipherId);
// simulate KeyCache miss that may happen during decryption; insert a CipherKey with known 'salt'
cipherKeyCache->insertCipherKey(domainId,
baseCipherId,
cipherKey->rawBaseCipher(),
cipherKey->getBaseCipherLen(),
cipherKey->getSalt());
// Ensure the update was a NOP
Reference<BlobCipherKey> cKey = cipherKeyCache->getCipherKey(domainId, baseCipherId, salt);
ASSERT(cKey->isEqual(cipherKey));
}
return cipherKey;
}
I think I know how to fix this issue. Please let me know if it OK to leave the Valgrind fix from this patch and I will fix it separately?
Thanks again for debugging this issue :).
There was a problem hiding this comment.
Sure. Go ahead with the fix. I will close the current PR and subsume it into my upcoming PR.
There was a problem hiding this comment.
A try-catch on caller side looks cumbersome. What do you think?
My initial thought that it is better for the caller to handle exceptions from a caching library, some exceptions can be 'acceptable' such as: key_not_found which could be satisfied by reaching out to KMS via EKPServer, however, I see below scheme could work as well:
- BlobCipherKeyCache throws key_not_found only for unexpected scenarios such as the one we discussed above. For a cache_miss it is OK to return an emptyReference. However, it is better to have unified behavior of empty-cache as well as cache-missing-a -specific scenario. If we choose not to throw exception, then I would say better not to distinguish these cases as technically they are same in some ways.
- As FDB EKs have 1:1 relation with KMS EKs, we can update EncryptKeyServer lookup interface to ensure that all cipher-keys requested are returned back to the client; if not, server throws "encrypt_key_not_found" (never to happen scenario and points to code logic bug/tampering etc.)
#2 I can take care as part of #6898.
Does this sounds reasonable? Thanks
Make sense to me. I will have #1 done in my upcoming PR.
let me add a check for it. |
Doxense CI Report for Windows 10
|
AWS CodeBuild CI Report for Linux CentOS 7
|
AWS CodeBuild CI Report for macOS BigSur 11.5.2
|
Doxense CI Report for Windows 10
|
AWS CodeBuild CI Report for macOS BigSur 11.5.2
|
AWS CodeBuild CI Report for Linux CentOS 7
|
AWS CodeBuild CI Report for Linux CentOS 7
|
|
Closing as discussed. |
AWS CodeBuild CI Report for Linux CentOS 7
|
4 participants
Instead of throwing error,
BlobCipherKeyCache::getLatestCipherKey()now returns empty key if the latest key doesn't exists.The PR also fixes valgrind complaining about
latestRandomSaltnot initialized whengetLatestCipherKey()try to print the salt if no key is found.Tested by running the EncryptionOps test with valgrind, and also updating the flow/BlobCipher unit test.
Code-Reviewer Section
The general guidelines can be found here.
Please check each of the following things and check all boxes before accepting a PR.
For Release-Branches
If this PR is made against a release-branch, please also check the following:
release-branchormainif this is the youngest branch)