small BlobCipher and SimKmsConnector fixes and changes by sfc-gh-yiwu · Pull Request #6936 · apple/foundationdb

sfc-gh-yiwu · 2022-04-22T17:59:40Z

SimKmsConnector fix domain id being unsigned
SimKmsConnector fix returning cipher id 0 as latest key, which is invalid
SimKmsConnector fix keys initialized as c-style strings with incorrect length and uninitialized bytes
SimKmsConnector fix returning different keys for the same id after restart
BlobCipher change APIs to return null reference when key not found
BlobCipher insertCipherKey to return the inserted key

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.

The PR has a description, explaining both the problem and the solution.
The description mentions which forms of testing were done and the testing seems reasonable.
Every function/class/actor that was touched is reasonably well documented.

For Release-Branches

If this PR is made against a release-branch, please also check the following:

This change/bugfix is a cherry-pick from the next younger branch (younger release-branch or main if this is the youngest branch)
There is a good reason why this PR needs to go into a release branch and this reason is documented (either in the description above or in a linked GitHub issue)

fdb-windows-ci · 2022-04-22T18:29:05Z

Doxense CI Report for Windows 10

Commit ID: b4ff784
Result: ✔️ SUCCEEDED
Build Logs (available for 30 days)

foundationdb-ci · 2022-04-22T19:14:01Z

AWS CodeBuild CI Report for Linux CentOS 7

CodeBuild project: foundationdb-pr
Commit ID: b4ff784
Result: SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

sfc-gh-yiwu · 2022-04-22T19:29:31Z

-		throw encrypt_key_not_found();
+	// ENCRYPT_INVALID_CIPHER_KEY_ID indicate there was no key inserted. In this case we return empty key instead of
+	// throwing encrypt_invalid_id.
+	if (latestBaseCipherKeyId == ENCRYPT_INVALID_CIPHER_KEY_ID) {


@sfc-gh-ahusain I'm still making this change since I want to make it consistent that things isn't actually an error don't throw an exception. Or if you prefer to, I can make latestBaseCipherKeyId an Optional, and testing against Optional::present() rather than whether the id is invalid to tell if the cache is empty.

I think you need to rebase the branch with the latest main, I updated this code as part of re-merging the auth header changes. API throws encyrpt_key_not_found if cache is empty; otherwise, it asserts that member values are valid.

yeah I saw that change. I'm just so tempting to do without exception :/

My suggestion was to pull the change and replace exception with "empty reference"; I thought we agreed that "invalid_id" will be thrown and if key is not present due to: empty cache and/or key_not_found, then "empty reference" will be returned ?

updated to use Optional. does it look a little bit better?

sfc-gh-ahusain

LGTM. Thanks for doing it

fdb-windows-ci · 2022-04-22T21:16:16Z

Doxense CI Report for Windows 10

Commit ID: 428bd84
Result: ✔️ SUCCEEDED
Build Logs (available for 30 days)

foundationdb-ci · 2022-04-22T21:38:01Z

AWS CodeBuild CI Report for Linux CentOS 7

CodeBuild project: foundationdb-pr
Commit ID: d7eaeeb
Result: FAILED
Error: Error while executing command: ctest -j ${NPROC} --no-compress-output -T test --output-on-failure. Reason: exit status 8
Build Logs (available for 30 days)

foundationdb-ci · 2022-04-22T21:46:08Z

AWS CodeBuild CI Report for macOS BigSur 11.5.2

CodeBuild project: foundationdb-pr-macos
Commit ID: 428bd84
Result: SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

fdb-windows-ci · 2022-04-22T21:57:05Z

Doxense CI Report for Windows 10

Commit ID: c926d2b
Result: ✔️ SUCCEEDED
Build Logs (available for 30 days)

foundationdb-ci · 2022-04-22T22:12:59Z

AWS CodeBuild CI Report for Linux CentOS 7

CodeBuild project: foundationdb-pr
Commit ID: c926d2b
Result: FAILED
Error: Error while executing command: ctest -j ${NPROC} --no-compress-output -T test --output-on-failure. Reason: exit status 8
Build Logs (available for 30 days)

foundationdb-ci · 2022-04-22T22:16:50Z

AWS CodeBuild CI Report for Linux CentOS 7

CodeBuild project: foundationdb-pr
Commit ID: 428bd84
Result: SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

sfc-gh-ahusain · 2022-04-23T00:45:27Z

 BlobCipherKeyIdCacheKey BlobCipherKeyIdCache::getCacheKey(const EncryptCipherBaseKeyId& baseCipherKeyId,
                                                          const EncryptCipherRandomSalt& salt) {
-	if (baseCipherKeyId == ENCRYPT_INVALID_CIPHER_KEY_ID || salt == ENCRYPT_INVALID_RANDOM_SALT) {
+	if (baseCipherKeyId == ENCRYPT_INVALID_CIPHER_KEY_ID) {


any reason we removed the 'salt' validity check? We can force the invariant that salt > 0?

I changed to use Optional for latest salt so there's no need to add an invalid salt value as a placeholder. plus... I'm a bit paranoid as when we create the random salt in theory it can be 0, so 0 is still a valid salt. True we can control how the salt is generated, but there's probably no need to do so.

as when we create the random salt in theory it can be 0

RandomSalt generation is controlled by BlobCipher and happens as part of BaseCipher KeyDerivation (https://github.com/apple/foundationdb/blob/main/flow/BlobCipher.cpp#L48); if it fair that std::mt19937 random; can generate 0 as the output, we could code to handle '0' random-num generated (it is super rate to happen, but, could) to the BlobCipherKey::BlobCipherKey() to distinguish salt from its default/invalid value i.e. 0

However, once we make above changes, the library controls the salt generation and I don't think it can be modified other than tampering with plaintext EncryptHeader (brute force attack..)

changed to use Optional for latest salt

Am not sure if Optional is the right choice, the values will be always present after a first latestKey gets inserted; to me they is a "default" value at the time of object construction and then value keeps updating with time.

It is not a biggy either ways and we have discussed this more than needed, if you prefer Optional, please proceed with it; no strong opinions from my side.
Thanks .

The "default" value effectively make the salt an Optional. To me, Optional is just an explicit way to represent nullable value.

And I put back the invalid random salt check, just in case you think it is handy somewhere else.

sfc-gh-yiwu · 2022-04-23T04:28:57Z

Adding another fix to make SimKmsConnector return fixed keys after restart.

fdb-windows-ci · 2022-04-23T04:32:09Z

Doxense CI Report for Windows 10

Commit ID: 5de271f
Result: ✔️ SUCCEEDED
Build Logs (available for 30 days)

fdb-windows-ci · 2022-04-23T04:48:36Z

Doxense CI Report for Windows 10

Commit ID: 5e9f6ab
Result: ✔️ SUCCEEDED
Build Logs (available for 30 days)

foundationdb-ci · 2022-04-23T04:55:07Z

AWS CodeBuild CI Report for macOS BigSur 11.5.2

CodeBuild project: foundationdb-pr-macos
Commit ID: 5de271f
Result: SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

foundationdb-ci · 2022-04-23T05:42:25Z

AWS CodeBuild CI Report for Linux CentOS 7

CodeBuild project: foundationdb-pr
Commit ID: 5de271f
Result: SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

foundationdb-ci · 2022-04-23T05:42:41Z

AWS CodeBuild CI Report for Linux CentOS 7

CodeBuild project: foundationdb-pr
Commit ID: 5e9f6ab
Result: SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

foundationdb-ci · 2022-04-23T06:33:30Z

AWS CodeBuild CI Report for macOS BigSur 11.5.2

CodeBuild project: foundationdb-pr-macos
Commit ID: 5de271f
Result: SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

foundationdb-ci · 2022-04-23T07:15:12Z

AWS CodeBuild CI Report for Linux CentOS 7

CodeBuild project: foundationdb-pr
Commit ID: 5de271f
Result: SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

foundationdb-ci · 2022-05-03T21:23:08Z

AWS CodeBuild CI Report for macOS BigSur 11.5.2

CodeBuild project: foundationdb-pr-macos
Commit ID: 97ef77b
Result: FAILED
Error: Error while executing command: ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i ${HOME}/.ssh_key ec2-user@${MAC_EC2_HOST} /usr/local/bin/bash --login -c ./build_pr_macos.sh. Reason: exit status 1
Build Logs (available for 30 days)

fdb-windows-ci · 2022-05-03T21:34:48Z

Doxense CI Report for Windows 10

Commit ID: 97ef77b
Result: ✔️ SUCCEEDED
Build Logs (available for 30 days)

foundationdb-ci · 2022-05-03T22:40:10Z

AWS CodeBuild CI Report for Linux CentOS 7

CodeBuild project: foundationdb-pr
Commit ID: 97ef77b
Result: SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

…start

foundationdb-ci · 2022-05-04T00:34:28Z

AWS CodeBuild CI Report for Linux CentOS 7

CodeBuild project: foundationdb-pr
Commit ID: 4834d05
Result: SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

fdb-windows-ci · 2022-05-04T01:10:41Z

Doxense CI Report for Windows 10

Commit ID: 4834d05
Result: ✔️ SUCCEEDED
Build Logs (available for 30 days)

sfc-gh-yiwu · 2022-05-04T18:11:28Z

@sfc-gh-ahusain Do you have more comments on this one? I'm wanting to merge this one so I can rebase #6942 onto it. Thanks.

sfc-gh-ahusain · 2022-05-04T18:15:25Z


 	explicit SimKmsConnectorContext(uint32_t keyCount) : maxEncryptionKeys(keyCount) {
-		uint8_t buffer[AES_256_KEY_LENGTH];
+		const unsigned char SHA_KEY[] = "0c39e7906db6d51ac0573d328ce1b6be";


Do we intend to checkin this code?

This is not debug code. It is intent to make the encrypt keys deterministic after restart, otherwise SimKmsConnector will return different set of cipher keys after a crash in simulation.

sfc-gh-ahusain · 2022-05-04T18:18:03Z

@sfc-gh-ahusain Do you have more comments on this one? I'm wanting to merge this one so I can rebase #6942 onto it. Thanks.

Looks great. One minor comment, I think some debugging changes are still in the code that you might want to get rid off before checking in.

Thanks for doing this change.

sfc-gh-ahusain

LGTM

sfc-gh-yiwu requested a review from sfc-gh-ahusain April 22, 2022 17:59

sfc-gh-yiwu commented Apr 22, 2022

View reviewed changes

sfc-gh-ahusain reviewed Apr 22, 2022

View reviewed changes

Comment thread flow/BlobCipher.cpp Outdated

sfc-gh-ahusain previously approved these changes Apr 22, 2022

View reviewed changes

sfc-gh-ahusain reviewed Apr 22, 2022

View reviewed changes

Comment thread fdbserver/SimEncryptKmsProxy.actor.h Outdated

sfc-gh-yiwu dismissed sfc-gh-ahusain’s stale review via d7eaeeb April 22, 2022 21:00

sfc-gh-yiwu force-pushed the encrypt_small branch from d7eaeeb to 428bd84 Compare April 22, 2022 21:08

sfc-gh-ahusain reviewed Apr 23, 2022

View reviewed changes

Comment thread fdbserver/EncryptKeyProxy.actor.cpp Outdated

sfc-gh-ahusain reviewed Apr 23, 2022

View reviewed changes

Comment thread flow/BlobCipher.cpp

sfc-gh-ahusain reviewed Apr 23, 2022

View reviewed changes

Comment thread flow/BlobCipher.cpp Outdated

sfc-gh-ahusain reviewed Apr 23, 2022

View reviewed changes

Comment thread flow/BlobCipher.cpp

sfc-gh-yiwu force-pushed the encrypt_small branch from c926d2b to 5de271f Compare April 23, 2022 04:17

sfc-gh-yiwu force-pushed the encrypt_small branch from 5e9f6ab to 5de271f Compare April 23, 2022 05:55

sfc-gh-yiwu mentioned this pull request Apr 23, 2022

Support TLog encryption in commit proxy #6942

Merged

5 tasks

jzhou77 assigned sfc-gh-ahusain Apr 25, 2022

sfc-gh-yiwu added 6 commits May 3, 2022 16:09

small BlobCipher and SimEncryptKmsProxy fixes and changes

e7162d7

address comments

0584a43

change latestBaseCipherKeyId to be Optional

b9a4e5d

Fix SimKmsConnector returning different keys for the same id after re…

365dab6

…start

2nd attempt to fix SimKmsConnector return different keys after restart

af360ae

Revert the change to invalid random salt check

4834d05

sfc-gh-yiwu force-pushed the encrypt_small branch from 97ef77b to 4834d05 Compare May 3, 2022 23:10

sfc-gh-yiwu changed the title ~~small BlobCipher and SimEncryptKmsProxy fixes and changes~~ small BlobCipher and SimKmsConnector fixes and changes May 4, 2022

sfc-gh-ahusain reviewed May 4, 2022

View reviewed changes

sfc-gh-ahusain self-requested a review May 4, 2022 19:36

sfc-gh-ahusain approved these changes May 4, 2022

View reviewed changes

sfc-gh-yiwu merged commit 66f1c5c into apple:main May 4, 2022

sfc-gh-yiwu deleted the encrypt_small branch May 4, 2022 21:09

Uh oh!

Conversation

sfc-gh-yiwu commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code-Reviewer Section

For Release-Branches

Uh oh!

fdb-windows-ci commented Apr 22, 2022

Doxense CI Report for Windows 10

Uh oh!

foundationdb-ci commented Apr 22, 2022

AWS CodeBuild CI Report for Linux CentOS 7

Uh oh!

sfc-gh-yiwu Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sfc-gh-ahusain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fdb-windows-ci commented Apr 22, 2022

Doxense CI Report for Windows 10

Uh oh!

foundationdb-ci commented Apr 22, 2022

AWS CodeBuild CI Report for Linux CentOS 7

Uh oh!

foundationdb-ci commented Apr 22, 2022

AWS CodeBuild CI Report for macOS BigSur 11.5.2

Uh oh!

fdb-windows-ci commented Apr 22, 2022

Doxense CI Report for Windows 10

Uh oh!

foundationdb-ci commented Apr 22, 2022

AWS CodeBuild CI Report for Linux CentOS 7

Uh oh!

foundationdb-ci commented Apr 22, 2022

AWS CodeBuild CI Report for Linux CentOS 7

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfc-gh-yiwu Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfc-gh-ahusain Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sfc-gh-yiwu commented Apr 23, 2022

Uh oh!

fdb-windows-ci commented Apr 23, 2022

Doxense CI Report for Windows 10

Uh oh!

fdb-windows-ci commented Apr 23, 2022

Doxense CI Report for Windows 10

Uh oh!

foundationdb-ci commented Apr 23, 2022

AWS CodeBuild CI Report for macOS BigSur 11.5.2

Uh oh!

foundationdb-ci commented Apr 23, 2022

AWS CodeBuild CI Report for Linux CentOS 7

Uh oh!

sfc-gh-yiwu commented Apr 22, 2022 •

edited

Loading

sfc-gh-yiwu Apr 22, 2022 •

edited

Loading

sfc-gh-yiwu Apr 23, 2022 •

edited

Loading

sfc-gh-ahusain Apr 23, 2022 •

edited

Loading