-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[KIP-1139] Add support for OAuth jwt-bearer grant type #4978
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
2d22a3f to
42432e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces new JWT configuration options for OAuth-based authentication by adding several JWT-specific properties to the configuration documentation.
- Added new JWT configuration properties including private key id, private key secret, token signing algorithm, token subject, token issuer, token audience, and token target audience.
Files not reviewed (6)
- src/rdkafka.c: Language not supported
- src/rdkafka_conf.c: Language not supported
- src/rdkafka_conf.h: Language not supported
- src/rdkafka_sasl_oauthbearer.c: Language not supported
- src/rdkafka_sasl_oauthbearer_oidc.h: Language not supported
- src/rdunittest.c: Language not supported
CONFIGURATION.md
Outdated
| sasl.oauthbearer.extensions | * | | | low | Allow additional information to be provided to the broker. Comma-separated list of key=value pairs. E.g., "supportFeatureX=true,organizationId=sales-emea".Only used when `sasl.oauthbearer.method` is set to "oidc". <br>*Type: string* | ||
| sasl.oauthbearer.token.endpoint.url | * | | | low | OAuth/OIDC issuer token endpoint HTTP(S) URI used to retrieve token. Only used when `sasl.oauthbearer.method` is set to "oidc". <br>*Type: string* | ||
| sasl.oauthbearer.private.key.id | * | | | low | Private key id. Only used when `sasl.oauthbearer.method` is set to "jwt". <br>*Type: string* | ||
| sasl.oauthbearer.private.key.secret | * | | | low | Private key id. Only used when `sasl.oauthbearer.method` is set to "jwt". <br>*Type: string* |
Copilot
AI
Mar 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description for 'sasl.oauthbearer.private.key.secret' incorrectly repeats 'Private key id'. It should be updated to 'Private key secret' to accurately describe the configuration option.
| sasl.oauthbearer.private.key.secret | * | | | low | Private key id. Only used when `sasl.oauthbearer.method` is set to "jwt". <br>*Type: string* | |
| sasl.oauthbearer.private.key.secret | * | | | low | Private key secret. Only used when `sasl.oauthbearer.method` is set to "jwt". <br>*Type: string* |
CONFIGURATION.md
Outdated
| sasl.oauthbearer.private.key.id | * | | | low | Private key id. Only used when `sasl.oauthbearer.method` is set to "jwt". <br>*Type: string* | ||
| sasl.oauthbearer.private.key.secret | * | | | low | Private key id. Only used when `sasl.oauthbearer.method` is set to "jwt". <br>*Type: string* | ||
| sasl.oauthbearer.token.signing.algorithm | * | | | low | token_signing_algorithm. Only used when `sasl.oauthbearer.method` is set to "jwt". <br>*Type: string* | ||
| sasl.oauthbearer.token.subject | * | | | low | token_signing_algorithm. Only used when `sasl.oauthbearer.method` is set to "jwt". <br>*Type: string* |
Copilot
AI
Mar 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description for 'sasl.oauthbearer.token.subject' mistakenly uses 'token_signing_algorithm'. It should correctly describe the token subject instead.
| sasl.oauthbearer.token.subject | * | | | low | token_signing_algorithm. Only used when `sasl.oauthbearer.method` is set to "jwt". <br>*Type: string* | |
| sasl.oauthbearer.token.subject | * | | | low | token_subject. Only used when `sasl.oauthbearer.method` is set to "jwt". <br>*Type: string* |
c9b8cae to
d81eb64
Compare
emasab
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Anchit! Asking some changes to follow more closely the KIP.
src/rdkafka_sasl_oauthbearer.c
Outdated
| rk->rk_conf.sasl.oauthbearer.token_refresh_cb == | ||
| rd_kafka_oidc_token_refresh_cb) { | ||
| (rk->rk_conf.sasl.oauthbearer.token_refresh_cb == | ||
| rd_kafka_jwt_refresh_cb || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it's still a OIDC callback so it can be and the existing one renamed by adding client_credentials.
| rd_kafka_jwt_refresh_cb || | |
| rd_kafka_oidc_token_jwt_bearer_refresh_cb || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? The callback methods are different so don't we need to check for both of them?
c070f49 to
2237412
Compare
…mplementation for directly reading `sasl.oauthbearer.assertion.file` without changes or otherwise reading `sasl.oauthbearer.assertion.jwt.template.file` and modifying it with other configuration properties.
2237412 to
581b10d
Compare
milindl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| goto done; | ||
| } | ||
|
|
||
| jwt_token = rd_kafka_oidc_token_try_validate(json, "access_token", &sub, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment here why we try validating with access_token first and then id_token afterwards if it fails.
milindl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update on the PR
…ent, it's later mandatory after parsing the sub in the response token
ffeb194 to
3b3e408
Compare
milindl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newest changes lgtm!
emasab
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving too as I reviewed initially
|
Hi @anchitj could you sign the new CLA? Thank you again. |
|
Done @emasab |
No description provided.