-
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
@@ -102,6 +102,13 @@ sasl.oauthbearer.client.secret | * | | | |||
sasl.oauthbearer.scope | * | | | low | Client use this to specify the scope of the access request to the broker. Only used when `sasl.oauthbearer.method` is set to "oidc". <br>*Type: string* | |||
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* |
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* |
Copilot uses AI. Check for mistakes.
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* |
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* |
Copilot uses AI. Check for mistakes.
c9b8cae
to
d81eb64
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.
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
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.
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
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!
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.