AWS KMS implementation
Type of change
Select the type of your PR
- Enhancement
Description
AWS KMS KMS implementation
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 Sonarcloud 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).
Note to self: work out why Localstack is so slow to startup.
Edit: Fixed - upgrading knocked 10seconds of the startup time!!
Successfully tested end to end against real AWS.
Todo:
- ~~get rid of the if/then/else ladders
getTargetnonsense~~ - ~~refactor the AWS request signing to be nice~~
- ~~work out why localstack is so slow to startup!~~
I think this PR is now in a reviewable state. All feedback welcome. The existing integration tests (KmsIT, RecordEncryptionIT) test against Localstack.
I'm planning to leave the following for separate PRs:
- Documentation for the AWS KMS.
- The TLS classes in the AWS module are straight copies of the Vault counterparts. It'd be nice to share this implementations.
- The system tests will need refactoring to test Record Encryption with AWS KMS too.
- It'd be nice to be able to test AWS KMS against a Cloud instance rather than Localstack to get a good degree of confidence that it really works! The same is true for Vault (to a lesser extent).
Follow on tasks.
#1216 - Documentation for AWS KMS integration #1217 - AWS KMS serde improvements #1218 - Allow JdkTls class to be shared between KMSs (or Filters) https://github.com/kroxylicious/kroxylicious/issues/1219 - RecordEncryptionST should execute tests against AWS (LocalStack) too
@tombentley
If we're not handling it/can't test it then failing with an exception seems like the right thing to do.
I changed the code in AwsV4SigningHttpRequestBuilder#signRequest so that only headers with a single value are used in the signature calculation. If you were to specify a header with >1 value or no value, then the header is excluded from the signature (but is still part of the request).
AWS's specification is really weak in this area (FWIW I raised an issue against their docs).
We don't need such headers right now anyway.
I gave the implementation as it stands now another spin against real AWS. All seems to be working just fine. I even tried a key rotation. All good.
I was curious to check the behaviour when an API call fails end to end. Here's what you see if you disable the key in AWS in the Kroxylicious logs.
2024-05-17 16:00:45 <KQueueEventLoopGroup-5-8> WARN io.kroxylicious.proxy.internal.FilterHandler:294 - [id: 0x0226cc5f, L:/127.0.0.1:55358 - R:localhost/127.0.0.1:9092]: Filterresponse for RecordEncryptionFilter@1186303788 FETCH ended exceptionally - closing connection. Cause message io.kroxylicious.kms.service.KmsException: decryptEdek failed after 3 attempts, last failure message: io.kroxylicious.kms.service.KmsException: Operation failed, key cc4ef56c-9b33-490c-9709-afe0e80cd449, HTTP status code 400, AWS error: ErrorResponse{type='DisabledException', message='arn:aws:kms:us-east-1:891377379385:key/cc4ef56c-9b33-490c-9709-afe0e80cd449 is disabled.'}
@SamBarker can I get another re-review? hopefully this is close now.
For the record, I think we should merge despite the failing sonar gate as we have https://github.com/kroxylicious/kroxylicious/issues/1218 to address the duplication
