sch-UID2-5851 migration from salt to key rotation - #567
Conversation
sophia-chen-ttd
changed the title
| this.lastActiveKeyId = new AtomicInteger(getLastActiveKeyId(salts)); | ||
| } | ||
|
|
||
| private int getLastActiveKeyId(SaltEntry[] salts) { |
| import java.util.Arrays; | ||
| import java.util.concurrent.atomic.AtomicInteger; | ||
|
|
||
| public class KeyIdGenerator { |
There was a problem hiding this comment.
I think it's worthwhile to add a comment somewhere in this class about the assumptions of key IDs in the latest rotated buckets and the intended outcomes of this logic:
- Key IDs from the latest buckets were the last allocated key IDs
- These key IDs should always be consecutive
- If the last allocated key ID contains 16777215, we should wrap around
- Continuing to increment from the last "highest" key ID will result in monotonic incrementation of key IDs for all newly rotated buckets
Hopefully my understanding is correct, it took me a while to grasp all of these assumptions and outcomes so a comment would help future readers of this code
| var refreshFrom = calculateRefreshFrom(oldSalt, targetDate); | ||
| var currentSalt = calculateCurrentSalt(oldSalt, shouldRotate); | ||
| var previousSalt = calculatePreviousSalt(oldSalt, shouldRotate, targetDate); | ||
| var currentKey = calculateCurrentKey(oldSalt, shouldRotate, keyIdGenerator); |
There was a problem hiding this comment.
Can we rename these and other related vars to something like "keySalt"? I keep forgetting this includes both the encryption key and encryption salt
| } | ||
|
|
||
| private String calculateCurrentSalt(SaltEntry salt, boolean shouldRotate) throws Exception { | ||
| return shouldRotate ? |
There was a problem hiding this comment.
nit: nested ternaries are hard to read, just use if statements
| } | ||
|
|
||
| @Test | ||
| void testKeyRotationKeyIdIncrementation() throws Exception { |
There was a problem hiding this comment.
Can we consolidate testKeyRotationKeyIdIncrementation, testKeyRotationKeyIdWrap, testKeyRotationKeyIdWrapManyBuckets?
| } | ||
|
|
||
| @Test | ||
| void testRotateFromSaltToKey() throws Exception { |
There was a problem hiding this comment.
Can we consolidate testRotateFromSaltToKey and testKeyRotationInitialKeyIdPopulation?
| private final AtomicInteger lastActiveKeyId; | ||
|
|
||
| public KeyIdGenerator(SaltEntry[] buckets) { | ||
| this.lastActiveKeyId = new AtomicInteger(getLastActiveKeyId(buckets)); |
There was a problem hiding this comment.
Maybe store next active key ID
|
|
||
| /** | ||
| * Intended outcomes of KeyIdGenerator: | ||
| * - Key ids are always consecutive, starting from 0 |
There was a problem hiding this comment.
Key IDs are always monotonically increasing
| * - When the last allocated key id reaches 16777215, the next key id will wrap around to 0 | ||
| * - Continuing to increment from the highest key id will result in monotonic incrementation of key ids for all newly rotated buckets | ||
| * | ||
| * Assumptions: |
| private static int getLastActiveKeyId(SaltEntry[] buckets) { | ||
| long maxLastUpdated = Arrays.stream(buckets).mapToLong(SaltEntry::lastUpdated).max().orElse(0); | ||
| int[] lastActiveKeyIdsSorted = Arrays.stream(buckets) | ||
| .filter(s -> s.lastUpdated() == maxLastUpdated && s.currentKey() != null) |
There was a problem hiding this comment.
Make this more robust to turning off the feature switch
…5853-metrics-and-dashboard-for-key-rotation
…5853-metrics-and-dashboard-for-key-rotation
…5853-metrics-and-dashboard-for-key-rotation
…hboard-for-key-rotation sch-UID2-5853 Added logs for key bucket count in salt rotation
2 participants
Implemented key rotation set behind a feature flag.
When flag is enabled:
When flag is disabled:
--
TESTS:
Other: