kroxylicious icon indicating copy to clipboard operation
kroxylicious copied to clipboard

AWS KMS implementation

Open k-wall opened this issue 1 year ago • 4 comments

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

k-wall avatar Apr 17 '24 13:04 k-wall

Note to self: work out why Localstack is so slow to startup.

Edit: Fixed - upgrading knocked 10seconds of the startup time!!

k-wall avatar Apr 25 '24 21:04 k-wall

Successfully tested end to end against real AWS.

Todo:

  • ~~get rid of the if/then/else ladders getTarget nonsense~~
  • ~~refactor the AWS request signing to be nice~~
  • ~~work out why localstack is so slow to startup!~~

k-wall avatar Apr 26 '24 15:04 k-wall

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:

  1. Documentation for the AWS KMS.
  2. The TLS classes in the AWS module are straight copies of the Vault counterparts. It'd be nice to share this implementations.
  3. The system tests will need refactoring to test Record Encryption with AWS KMS too.
  4. 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).

k-wall avatar May 09 '24 18:05 k-wall

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

k-wall avatar May 17 '24 14:05 k-wall

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

k-wall avatar May 17 '24 14:05 k-wall

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.'}

k-wall avatar May 17 '24 15:05 k-wall

@SamBarker can I get another re-review? hopefully this is close now.

k-wall avatar May 17 '24 15:05 k-wall

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

SamBarker avatar May 19 '24 23:05 SamBarker

Quality Gate Failed Quality Gate failed

Failed conditions
6.3% Duplication on New Code (required ≤ 5%)

See analysis details on SonarCloud

sonarqubecloud[bot] avatar May 21 '24 08:05 sonarqubecloud[bot]