Skip to content

Introduce a wrapper around KekId to provide semantic value#774

Closed
SamBarker wants to merge 15 commits intokroxylicious:mainfrom
SamBarker:SemanticKekIdType
Closed

Introduce a wrapper around KekId to provide semantic value#774
SamBarker wants to merge 15 commits intokroxylicious:mainfrom
SamBarker:SemanticKekIdType

Conversation

@SamBarker
Copy link
Copy Markdown
Member

@SamBarker SamBarker commented Dec 4, 2023 •
edited
Loading

Type of change

  • Enhancement / new feature

Description

Adding a type to wrap the KekId so that we can encasulate behaviour related to ID's better. This replaces a lot of generics with an actual KekId type and pushes the need to get at the raw kek ID down to the serialisation layer.

Additional Context

This is an alternative solution to the same problem as #766.
If we are to merge this PR we should follow up with a second one replacing the generics for the DEK, I think but haven't tried to do so, with a Dek type as well.

Why are you making this pull request?

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • Write tests
  • Make sure all tests pass
  • Review performance test results. Ensure that any degradations to performance numbers are understood and justified.
  • Make sure all Sonar-Lint warnings are addressed or are justifiably ignored.
  • Update documentation
  • Reference relevant issue(s) and close them after merging
  • For user facing changes, update CHANGELOG.md (remember to include changes affecting the API of the test artefacts too).

SamBarker
SamBarker commented Dec 4, 2023
View reviewed changes
Comment thread kroxylicious-kms/src/main/java/io/kroxylicious/kms/service/Kms.java
Comment thread kroxylicious-kms/src/main/java/io/kroxylicious/kms/service/KekId.java
Comment thread kroxylicious-kms/src/main/java/io/kroxylicious/kms/service/Kms.java
*/
@NonNull
CompletionStage<DekPair<E>> generateDekPair(@NonNull K kekRef);
CompletionStage<DekPair<E>> generateDekPair(@NonNull KekId kekRef);
Copy link
Copy Markdown
Member Author

@SamBarker SamBarker Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably do

Suggested change
CompletionStage<DekPair<E>> generateDekPair(@NonNull KekId kekRef);
CompletionStage<DekPair<E>> generateDekPair(@NonNull KekId kekId);

However even if we don't proceed with this PR its highlighting that we are inconsistent in our usage of kekId, kekRef and kek and we should standardise on one.

Copy link
Copy Markdown
Member

@robobario robobario Dec 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth updating in this PR

@SamBarker SamBarker marked this pull request as ready for review December 10, 2023 22:41
robobario
robobario reviewed Dec 10, 2023
View reviewed changes
Comment thread kroxylicious-kms/src/main/java/io/kroxylicious/kms/service/KekId.java
/**
* A semantic wrapper around KMS specific key id type
*/
public interface KekId {
Copy link
Copy Markdown
Member

@robobario robobario Dec 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see adding in KekId makes the InBandManager more readable and easier to relate to the concepts being implemented, but does switching to <K> K getId(Class<K> keyType) make it harder to verify that we've strung together a KMS with the right combination of keyIdSerde, resolveAlias and unencryptedKeyId? It seems funky to me that the serde has to know "this KMS's implementation supports getting UUID".

Copy link
Copy Markdown
Member Author

@SamBarker SamBarker Dec 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I hear you. My rationale was the serde has to know the concrete type anyway because its sole job is to translate the concrete type to bytes.

robobario
robobario reviewed Dec 10, 2023
View reviewed changes
Comment thread kroxylicious-kms/src/main/java/io/kroxylicious/kms/service/KekId.java Outdated
@robobario
Copy link
Copy Markdown
Member

robobario commented Dec 11, 2023 •
edited
Loading

not related to this PR but it feels like it could use an additional layer of protection to prevent a misnamed key or incorrect template from forwarding unencrypted data. Maybe we could add an additional, optional configuration like prefixes/literals/regexes/whitelist/blacklist/etc of topics so there is some secondary bit of information about which topics should have a key. edit: issue created #811

@robobario
Copy link
Copy Markdown
Member

robobario commented Dec 11, 2023 •
edited
Loading

Another unrelated thing to figure out, I wonder if we will want a configurable policy for kekId resolution/caching. Should we cache non-existence aggressively if we are taking it to mean do-not-encrypt. You could imagine the proxy hammering the KMS if they are only targeting a small subset of their topics. edit: issue created #816

robobario
robobario reviewed Dec 11, 2023
View reviewed changes
Comment thread kroxylicious-kms/src/main/java/io/kroxylicious/kms/service/Kms.java Outdated
@robobario robobario mentioned this pull request Dec 11, 2023
Open
@SamBarker SamBarker force-pushed the SemanticKekIdType branch 2 times, most recently from 649fcf1 to 1723516 Compare December 11, 2023 23:30
robobario
robobario approved these changes Dec 12, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@robobario robobario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, should make an issue to do the same for DEKs

@SamBarker SamBarker mentioned this pull request Dec 12, 2023
Closed
7 tasks
k-wall
k-wall approved these changes Dec 12, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@k-wall k-wall left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

several nits, but the code looks okay to me.

I'm not quite grasping the motivation for the change. Can you give an example what is actually improved by the change? Haven't we just now got an extra step of indirection?

Separately, when are you planning to land this change? Is this a 0.4.0?

@SamBarker
Copy link
Copy Markdown
Member Author

SamBarker commented Dec 12, 2023

No it's a 0.5.0 change.

The original motivation was the same as the encryption scheme change to remove the semantic null when when catching an unknown alias exception. Which I had in this Pr. However the consensus in NZ was that wasn't the right solution to the unknown alias problem but the rest of the change improved readability.

Is that a fair summary @robobario @gracegrimwood ?

As well as improving readability I think introducing a wrapper is helpful. As it will allow us to attach behaviour To the keys themselves (my original idea was an isUnencrypted).

@SamBarker SamBarker force-pushed the SemanticKekIdType branch 2 times, most recently from 5a11ea4 to 575dae9 Compare December 12, 2023 23:21
k-wall
k-wall reviewed Dec 13, 2023
View reviewed changes
Comment thread ...-hashicorp-vault/src/main/java/io/kroxylicious/kms/provider/hashicorp/vault/StringKekid.java Outdated
import io.kroxylicious.kms.service.KmsException;

public record StringKekid(@NonNull String keyId) implements KekId {
public StringKekid(String keyId) {
Copy link
Copy Markdown
Member

@k-wall k-wall Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use compact ctor

Suggested change
public StringKekid(String keyId) {
public StringKekid() {

@SamBarker SamBarker added this to the 0.5.0 milestone Dec 13, 2023
SamBarker
SamBarker commented Dec 13, 2023
View reviewed changes
Comment thread ...-hashicorp-vault/src/main/java/io/kroxylicious/kms/provider/hashicorp/vault/StringKekid.java

public record StringKekid(@NonNull String keyId) implements KekId {

public StringKekid {
Copy link
Copy Markdown
Member Author

@SamBarker SamBarker Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@k-wall @robobario would this be better called VaultKekId. I know it is a string but its the vault specific implementation of an ID.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Dec 13, 2023

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
94.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

SamBarker and others added 15 commits December 15, 2023 11:38
@SamBarker
Introduce a wrapper around KekId to provide semantic value
fee5e45 
Signed-off-by: Sam Barker <[email protected]>

rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
Remove the generics from KekId.
8161a9e 
Turns out its viral, and thus we stop needing a lot of other generics.

Signed-off-by: Sam Barker <[email protected]>

rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
Specify the expected keyType when interacting with the underlying key
1d5c312 
Signed-off-by: Sam Barker <[email protected]>

rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
Clean up warnings in EnvelopeEncryptionFilterTest
71ad195 
rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
PR review: Convert MyKekId from class to record.
0a83c96 
rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
Add KekId to Vault KMS
43a6167 
rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
Make the ID's records
02a5e20 
rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
PR Feedback: consistent capitalisation of UUID and remove pointless o…
dc05020 
…verrides

rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
PR Feedback: apply nonnull annotations and validation
1b8d6fa 
rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
PR feedback: Raw completionStage to var
688847b 
rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker @k-wall
PR Feedback: Update TopicNameBasedKekSelector javadoc
01cc221 
Co-authored-by: Keith Wall <[email protected]>
Signed-off-by: Sam Barker <[email protected]>
@SamBarker
PR feedback: Remove Generics from TemplateKekSelector.Pair
84f4521 
rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
PR Feedback: Using compact ctor
a10204f 
rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
Fix import sort
9dfd73d 
Why are the Intellij and impsort plug-ins out of sync again :/

rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@SamBarker
Push the KekId down itno the edek serde
e798074 
rh-pre-commit.version: 2.0.1
rh-pre-commit.check-secrets: ENABLED
@k-wall k-wall mentioned this pull request Dec 22, 2023
Merged
7 tasks
@SamBarker
Copy link
Copy Markdown
Member Author

SamBarker commented Jan 22, 2024

Going to close this for now as so much has moved underneath it. I'm still in favour of shifting from K to KekId but need to revisit that in light of other changes.

@SamBarker SamBarker closed this Jan 22, 2024
@SamBarker SamBarker deleted the SemanticKekIdType branch August 16, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@robobario robobario robobario approved these changes

@k-wall k-wall k-wall approved these changes

Assignees

No one assigned

Labels

filter/record-encryption

Projects

None yet

Milestone

0.5.0

Development

Successfully merging this pull request may close these issues.

3 participants

@SamBarker @robobario @k-wall