Hashicorp Vault KMS implementation and kubernetes-example

[k-wall]

Type of change

Select the type of your PR

• Enhancement / new feature

Description

• Implementation of the KMS interface for HasiCorp Vault, with support integration tests.
• kubernetes-examples/topicencryption ready to illustrate the topic encryption. It deploys Hashicorp using the Helm Chart in a mode suitable for development (i.e. auto-unsealed).

Additional Context

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).

[k-wall]

There's a demo of this working on Kube https://asciinema.org/a/623785

[tombentley]

Thanks Keith!

[tombentley]

AFAICS, Vault does not include an immutable key id in the edek itself, but I'm inclined to include the key name here anyway. That's because I think it's better to push the bookkeeping to keep the kek id and edek together into the KMSes. AWS does this anyway, so why not make use of that? And for those KMSes which do provide an immutable key id alongside mutable key aliases it makes for a stronger contract for our Kms abstraction to force the Kms implementation to resolve it up front.

Coming back to Vault... it seems the only kind of key identifier it supports is a name. It also supports a delete key operation. So name is not immutable in time and space, and therefore the following is possible:

1. Create key foo.
2. Generate data key bar.
3. Use data key
4. Delete foo
5. You now cannot decrypt your data, but at least it's obvious why: You should get a 4xx on Vault's decrypt operation.
6. Create a new foo.
7. What happens if we try to decrypt the encrypted bar using foo now? Does it fail? Does the failure have an sensible error?

I think we need to:

• document that this is a known behaviour when using vault. We simply have to rely on users not deleting keys.
• document how it will appear to users at step 5 and step 7.

Another thing that vault lets you do is export and import keys. This means that the key with a given id could disappear for a time (export, delete, import). Or effectively get renamed (export, delete, import under a different name).

(One thing we could do to detect this situation is use the read-key operation to capture the keys dict. We expect the keys at encryption time to be a submap of the keys at decryption time. We could use that fact to be able to say with certainty "Your kek has been deleted and recreated". I don't think we actually want to do this though).

[k-wall]

I argee, the documentation is a good idea. I'll add it.

It looks like Vault associates a timestamp with each key. That would give an alternative way to detect the deleted/recreated case. I'll log the ideas I don't think we need to do them right away.

this is a key that's been rotated once

{
  "request_id": "5c5584fc-2f74-438b-7dfd-cc9f2eb66864",
  "lease_id": "",
  "lease_duration": 0,
  "renewable": false,
  "data": {
    "allow_plaintext_backup": false,
    "auto_rotate_period": 0,
    "deletion_allowed": false,
    "derived": false,
    "exportable": false,
    "imported_key": false,
    "keys": {
      "1": 1701799313,
      "2": 1701799318
    },
    "latest_version": 2,
    "min_available_version": 0,
    "min_decryption_version": 1,
    "min_encryption_version": 0,
    "name": "mykey",
    "supports_decryption": true,
    "supports_derivation": true,
    "supports_encryption": true,
    "supports_signing": false,
    "type": "aes256-gcm96"
  },
  "warnings": null
}

[k-wall]

I raised this as #784

[k-wall]

There's still some issues I want to address, which I'll get to tomorrow morning.
I also intend to open issues for the 'missing bits'. My intention is that we can get this merged soon and work together on the outstanding stuff that's a priority.

[SamBarker]

LGTM thanks @k-wall

We should probably add a Readme (a skelaton at least in this PR). To document how to configure Kroxylicious for use with Vault.

[SamBarker]

Seems like a sensible default path should we update the default (in a new PR)?

[k-wall]

I suppose we could update the Dockerfile to assume that path, but that would require users (Docker user or Kubernetes user) to mount the config file in the same place. I'm not sure that's too helpful.

[robobario]

LGTM!

[tombentley]

EDEKs get serialized in the encrypted record and may be deserialized by a different version of the Vault KMS provider. So I wonder if it's worth expending a byte on a version number, just so we've got wiggle room in the future to change the serialization schema.

You could also argue for a magic number to detect when we're trying to deserialize a EDEK that wasn't written by the vault KMS (e.g. some other KMS).

We don't have to decide either of these right now, but we should at least open an issue and have the discussion.

[k-wall]

I've added Javadoc to the VaultEdekSerde class and added a TODO to consider adding a version byte.

[tombentley]

What thread does this sendAsync complete on? Because IIUC that details leaks to the caller of generateDekPair, who might need to volatile/synchronized etc to make their code threadsafe.

[robobario]

It looks like the HTTP client by default would have been given a dedicated cached thread pool (another resource we should look at sharing across channels in future), so I imagine it will be completed from that pool, needs checking though.

The caller of generateDekPair is already working with a CompletionStage isn't the default assumption that any thread could be completing it and clients should code accordingly?

[robobario]

Could javadoc it and leave it up to the caller to handle switching to a different executor if required? Or I guess you could change the Kms apis so the client could optionally provide an executor they want the completion to happen on.

[k-wall]

What thread does this sendAsync complete on?

A HTTP pool thread will complete the future when the HTTP response is returned. IIUC that means that actions chained to that future will execute in that same thread, unless explicit provision is made. So in our case that means the thenApply action in InBandKeyManager#makeKeyContext will run on the HTTP thread. AFAICS that code looks thread safe (it uses only objects that are finals and returns a new created object via a completion stage).

If the apply in InBandKeyManager#makeKeyContext ever needed to rely on mutable state, then it would be up to it achieve thread safety. Using thenApplyAsync would be an option.

[tombentley]

Let's add a sentence to the Javadoc of the Kms to say that the thread on which stages complete is undefined. That way at least the contract is explicit.

[SamBarker]

I'm happy to merge this and address the TODO comments in a separate PRs

[SamBarker]

Interacting with the buffer both directly and through Serde feels a bit odd to me. I wonder if we should have Serde.putByteArray or Serder.copyArray

[k-wall]

That code now uses some Kafka Common utility method to do the unsigned varint work. I'm not sure that addresses your point. I suspect we'll want to refactor this further as we work on #767.
Hoping this is good enough for now.

[tombentley]

Eventually we might want a separate module just for the Serde abstraction (which is not really KMS specific) and utility methods for interacting with ByteBuffer, but like Keith I'm inclined to defer that for now.

[k-wall]

@tombentley @robobario @SamBarker I hope I've addressed the review comments to your satisfaction. In some cases where I think deferring would be appropriate, I've raise a separate issue.

I'm hoping this will be good enough to merge. Indeed, it you want to merge during your working day, go right ahead and squash/merge it.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

94.5% Coverage
0.0% Duplication