Repository navigation
fix(kafka): apply sts_header_overrides to MSK IAM SASL auth - #7187
Conversation
PR Reviewer Guide 🔍(Review updated until commit f33698e)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to f33698e Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit 1e06231
Suggestions up to commit 9f917fb
Suggestions up to commit a5c5abe
Suggestions up to commit 89e429d
Suggestions up to commit 5ae10b5
|
|
Persistent review updated to latest commit 5ae10b5 |
5ae10b5 to
89e429d
Compare
|
Persistent review updated to latest commit 89e429d |
89e429d to
a5c5abe
Compare
|
Persistent review updated to latest commit a5c5abe |
| * {@code aws.sts_header_overrides} are applied on the data-plane {@code sts:AssumeRole} used for | ||
| * broker authentication. | ||
| */ | ||
| public class MskIamAuthCredentialsCallbackHandler implements AuthenticateCallbackHandler { |
There was a problem hiding this comment.
Can you move this to the util package and make it package private? If Kafka accept this it would be better to keep it isolated.
There was a problem hiding this comment.
Done — moved the handler into the util package and made the class package-private. Unit tests still green (7/7).
| private static AwsCredentialsProvider awsGlueCredentialsProvider; | ||
| private static GlueSchemaRegistryKafkaDeserializer glueDeserializer; | ||
|
|
||
| public static AwsCredentialsProvider getMskCredentialsProvider() { |
There was a problem hiding this comment.
This also could be package private if you move the other class.
There was a problem hiding this comment.
Done — getMskCredentialsProvider() is now package-private since both classes live in util. (35/35 passing.)
| /** | ||
| * SASL client callback handler for the {@code AWS_MSK_IAM} mechanism that sources credentials | ||
| * exclusively from Data Prepper's own header-aware STS provider, so the configured | ||
| * {@code aws.sts_header_overrides} are applied on the data-plane {@code sts:AssumeRole} used for |
There was a problem hiding this comment.
I was curious how this even worked since I don't see this here.
We already have code to use the STS headers. They are just not used on communicating with the Kafka client directly.
There was a problem hiding this comment.
Yep, that STS-header code is only used for the initial GetBootstrapBrokers call. The Kafka client's own SASL handshake goes through the vendored IAMClientCallbackHandler, which builds its own provider from the JAAS role with no header overrides — so the data-plane AssumeRole was missing the headers. This callback handler is what routes that handshake through the header-aware provider too.
a5c5abe to
9f917fb
Compare
|
Persistent review updated to latest commit 9f917fb |
✅ License Header Check PassedAll newly added files have proper license headers. Great work! 🎉 |
9f917fb to
1e06231
Compare
|
Persistent review updated to latest commit 1e06231 |
dlvenable
left a comment
There was a problem hiding this comment.
Thank you @hozkaya2000 for this contribution!
dinujoh
left a comment
There was a problem hiding this comment.
Kafka plugin integ test are failing. Can you take a look ?
Route role-based AWS_MSK_IAM SASL (data-plane) authentication through a dedicated callback handler that sources credentials exclusively from Data Prepper's header-aware STS provider, so aws.sts_header_overrides (x-amz-source-arn / x-amz-source-account) are applied on the data-plane sts:AssumeRole used for broker auth. The handler implements AuthenticateCallbackHandler directly (never extends IAMClientCallbackHandler), so the vendored library never assembles its own header-less MSKCredentialProvider. The ROLE path is now unconditional: credentials always flow through our provider regardless of whether overrides are configured. Resolves opensearch-project#6923 Signed-off-by: Halit Ozkaya <hozkaya@amazon.com>
1e06231 to
f33698e
Compare
|
Persistent review updated to latest commit f33698e |
Description
Role-based
AWS_MSK_IAMSASL (data-plane / broker) authentication assumed the customer role inside the vendoredaws-msk-iam-authlibrary, which has no hook forsts_header_overrides. As a result thex-amz-source-arn/x-amz-source-accountheaders were dropped on the data-planests:AssumeRole, leaving a confused-deputy gap on the one MSK code path (the bootstrap-broker lookup already carried the headers correctly).This PR routes the ROLE path through a dedicated callback handler that sources credentials exclusively from Data Prepper's own header-aware STS provider — the same
StsAssumeRoleCredentialsProvideralready built (with the header overrides applied) for the broker lookup — so both STS call sites on the MSK path now carry the headers.This supersedes #6925 and reworks it per review feedback.
Key changes
MskIamAuthCredentialsCallbackHandlernow implementsAuthenticateCallbackHandlerdirectly instead of extendingIAMClientCallbackHandler. Itsconfigure(...)only validates the mechanism — it never reads JAAS options and never constructs anMSKCredentialProvideror falls back to the SDK default chain. Credentials come only fromKafkaSecurityConfigurer.getMskCredentialsProvider().KafkaSecurityConfigurer.setAwsIamAuthProperties— the ROLE path is now unconditional (single auth path): it always installs our handler + the bareIAMLoginModule required;JAAS config, so the library never assembles its own header-less provider. No longer gated on whethersts_header_overridesare present.internalsSASL machinery is untouched; the SDK-v2 provider's credentials are translated to SDK-v1 for theAWSCredentialsCallbackbridge.DEFAULTMSK-IAM mode is unchanged (no role assumption, no headers to carry).Why implement the interface directly
Review feedback (@dlvenable) on #6925: avoid the split authentication approach and never let credentials load through the library's own
MSKCredentialProvideror the SDK default chain. ImplementingAuthenticateCallbackHandlerdirectly makes the header-less assembler unreachable and gives a single, always-on auth path.Testing
MskIamAuthCredentialsCallbackHandlerTest— 7/7 passing (credential resolution, v2→v1 translation,configureaccept/reject, unsupported-callback).KafkaSecurityConfigurerTest— 35/35 passing (including the always-on ROLE assertions).sts_header_overridesreach the data-planests:AssumeRoleon theaws_msk_iam: rolebranch: driving the realKafkaSecurityConfigurer.setAuthProperties(...)path from a ROLE config, the SDK's own request logger showed the outbound AssumeRole to STS carryingx-amz-source-account/x-amz-source-arnin its headers and returning HTTP 200.Resolves #6923