Skip to content

Conversation

anchitj
Copy link
Contributor

@anchitj anchitj commented Feb 28, 2025

No description provided.

@confluent-cla-assistant
Copy link

confluent-cla-assistant bot commented Feb 28, 2025

🎉 All Contributor License Agreements have been signed. Ready to merge.
✅ anchitj
Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.

@anchitj anchitj marked this pull request as ready for review March 19, 2025 10:10
@Copilot Copilot AI review requested due to automatic review settings March 19, 2025 10:10
@anchitj anchitj requested a review from a team as a code owner March 19, 2025 10:10
@anchitj anchitj changed the title Dev oauth jwt Support for JWT Oauth Mar 19, 2025
Copy link

@Copilot Copilot AI left a 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*
Copy link
Preview

Copilot AI Mar 19, 2025

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.

Suggested change
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*
Copy link
Preview

Copilot AI Mar 19, 2025

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.

Suggested change
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.

@anchitj anchitj changed the title Support for JWT Oauth [WIP] Support for JWT Oauth Mar 19, 2025
@emasab emasab changed the title [WIP] Support for JWT Oauth [KIP-1139] Add support for OAuth jwt-bearer grant type May 28, 2025
Copy link
Contributor

@emasab emasab left a 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.

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 ||
Copy link
Contributor

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.

Suggested change
rd_kafka_jwt_refresh_cb ||
rd_kafka_oidc_token_jwt_bearer_refresh_cb ||

Copy link
Contributor Author

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?

@airlock-confluentinc airlock-confluentinc bot force-pushed the dev_oauth_jwt branch 3 times, most recently from c070f49 to 2237412 Compare June 3, 2025 05:35
…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.
Copy link
Contributor

@milindl milindl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with minor feedback, thanks @anchitj and @emasab

goto done;
}

jwt_token = rd_kafka_oidc_token_try_validate(json, "access_token", &sub,
Copy link
Contributor

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.

Copy link
Contributor

@milindl milindl left a 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

@airlock-confluentinc airlock-confluentinc bot force-pushed the dev_oauth_jwt branch 2 times, most recently from ffeb194 to 3b3e408 Compare June 30, 2025 13:43
Copy link
Contributor

@milindl milindl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newest changes lgtm!

Copy link
Contributor

@emasab emasab left a 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

@emasab emasab closed this Jun 30, 2025
@emasab emasab reopened this Jun 30, 2025
@emasab
Copy link
Contributor

emasab commented Jun 30, 2025

Hi @anchitj could you sign the new CLA? Thank you again.

@anchitj
Copy link
Contributor Author

anchitj commented Jun 30, 2025

Done @emasab

@emasab emasab merged commit 606d0e4 into master Jul 1, 2025
2 checks passed
@emasab emasab deleted the dev_oauth_jwt branch July 1, 2025 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants