Support an unencrypted encryption scheme#766
Support an unencrypted encryption scheme#766SamBarker wants to merge 3 commits intokroxylicious:mainfrom
Conversation
Signed-off-by: Sam Barker <[email protected]> rh-pre-commit.version: 2.0.1 rh-pre-commit.check-secrets: ENABLED
Signed-off-by: Sam Barker <[email protected]> rh-pre-commit.version: 2.0.1 rh-pre-commit.check-secrets: ENABLED
| }) | ||
| .thenApply(kekId -> new Pair<>(topicName, kekId))) | ||
| .toList(); | ||
| return EnvelopeEncryptionFilter.join(collect).thenApply(list -> { |
There was a problem hiding this comment.
This change means we stop having a meaningful null value.
| */ | ||
| public abstract @NonNull CompletionStage<Map<String, K>> selectKek(@NonNull Set<String> topicNames); | ||
| } | ||
| public abstract @NonNull CompletionStage<Map<String, EncryptionScheme<K>>> selectKek(@NonNull Set<String> topicNames); |
There was a problem hiding this comment.
I think this really means the class/interface should be renamed from KekSelector to EncryptionSchemeSelector or something else which implies more than just selecting Kek's.
| Objects.requireNonNull(recordFields); | ||
| } | ||
|
|
||
| public static <K> EncryptionScheme<K> unencryptedScheme(K kekId) { |
There was a problem hiding this comment.
The existence of this factory implies we should also have
RecordScheme(K kekId) and RecordAndHeadersScheme(K kekId)
Which feels strange to say the least.
So maybe instead of/as well as giving meaning to the empty set we should expand RecordField it include None?
cc: @tombentley
There was a problem hiding this comment.
So maybe instead of/as well as giving meaning to the empty set we should expand RecordField it include None?
How would you disallow meaningless combinations Set.of(NONE,RECORD_VALUE)?
There was a problem hiding this comment.
How would you disallow meaningless combinations Set.of(NONE,RECORD_VALUE)?
On the one hand I ask why do we need to disallow it? Just because it doesn't have obvious value to us doesn't mean it should be banned. Maybe there is a some future usage where that could be useful. Also looking at our current usage there is no reason to ban the combination as we pick specific types to operate on.
On the other hand your right as an API it does invite people to setup strange and potentially contradictory instructions.
| public CompletionStage<Map<String, EncryptionScheme<K>>> selectKek(@NonNull Set<String> topicNames) { | ||
| return CompletableFuture.completedFuture(encryptionSchemesByTopicName.entrySet() | ||
| .stream() | ||
| .filter(entry -> topicNames.contains(entry.getKey())) |
There was a problem hiding this comment.
The lack of this filer was leading to my original issue however it was much harder to fix with a semantically valid null.
| @NonNull | ||
| CompletionStage<K> resolveAlias(@NonNull String alias); | ||
|
|
||
| K unEncryptedKekId(); |
There was a problem hiding this comment.
Please add Javadoc. @nullable? The camelisation of the method name is unusual too.
Is this a sentinel value that represents the the unencrypted case? I think that might be awkward for KMSs that use string values for key names. An empty string doesn't seem like a great choice.
There was a problem hiding this comment.
Yeah, that capitalisation is definitely odd, I'll update.
Is this a sentinel value that represents the the unencrypted case?
Yes, its a function of having the KekId as a generic type rather than an interface that the implementations provide.
I think that might be awkward for KMSs that use string values for key names. An empty string doesn't seem like a great choice.
Yes it is awkward for String keys, but they are free to pick anything including __UNENCRYPTERD__ or **PLAINTESXT** which would probably work quite well if it showed up in logs.
Signed-off-by: Sam Barker <[email protected]> rh-pre-commit.version: 2.0.1 rh-pre-commit.check-secrets: ENABLED
|
Kudos, SonarCloud Quality Gate passed! |
|
I think the output of the conversations around #774 is that 774 represents a better starting point for fixing the null handling but a version of this idea could be the final solution to conveying the state. |
3 participants
Type of change
Select the type of your PR
Description
Changes the kekSelection contract so that the KekSelector can mark a record as un-encrypted.
Additional Context
Checklist
Please go through this checklist and make sure all applicable tasks have been done