-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Implementation of OAUTHBEARER/OIDC metadata based authentication #5155
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. |
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 implements OAUTHBEARER/OIDC metadata-based authentication, initially supporting the Azure UAMI (User Assigned Managed Identity) method for token retrieval.
- Adds Azure UAMI metadata authentication support for OAUTHBEARER/OIDC
- Refactors HTTP client to support custom headers and retry logic
- Introduces configuration parameter for metadata authentication type
Reviewed Changes
Copilot reviewed 13 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/rdkafka_sasl_oauthbearer_oidc.c | Implements Azure metadata token refresh callback with HTTP calls to Azure UAMI endpoint |
| src/rdkafka_sasl_oauthbearer_oidc.h | Declares the new Azure metadata token refresh callback function |
| src/rdkafka_conf.h | Adds metadata authentication type enum and configuration structure |
| src/rdkafka_conf.c | Adds configuration property and refactors OIDC validation logic into separate functions |
| src/rdkafka_sasl_oauthbearer.c | Simplifies builtin token refresh callback detection logic |
| src/rdkafka.c | Integrates Azure metadata authentication into the OIDC token refresh mechanism |
| src/rdhttp.h | Updates HTTP client interface to support headers, timeouts, and retries |
| src/rdhttp.c | Enhances HTTP client with parameter appending, custom headers, and retry functionality |
| CONFIGURATION.md | Documents the new metadata authentication type configuration parameter |
| tests/0126-oauthbearer_oidc.c | Removes duplicate configuration line in test |
e1c95a8 to
45f2a29
Compare
45f2a29 to
26d990a
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.
some minor comments, one not-so-minor comment, thanks for making the PR
src/rdkafka_sasl_oauthbearer_oidc.c
Outdated
| * @brief Implementation of Oauth/OIDC token refresh callback function | ||
| * for Azure IMDS, | ||
| * will receive the JSON response after HTTP(S) GET call to token | ||
| * provider, then extract the jwt from the JSON response, and forward it to the |
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.
nit: fix alignment
INTRODUCTION.md
Outdated
|
|
||
| ###### Azure IMDS | ||
|
|
||
| to use this method you set: |
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.
| to use this method you set: | |
| To use this method you set: |
INTRODUCTION.md
Outdated
| * `sasl.oauthbearer.metadata.authentication.type=azure_imds` this make that ` sasl.oauthbearer.client.id` | ||
| and `sasl.oauthbearer.client.secret` aren't required |
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.
| * `sasl.oauthbearer.metadata.authentication.type=azure_imds` this make that ` sasl.oauthbearer.client.id` | |
| and `sasl.oauthbearer.client.secret` aren't required | |
| * `sasl.oauthbearer.metadata.authentication.type=azure_imds` this makes it so that ` sasl.oauthbearer.client.id` | |
| and `sasl.oauthbearer.client.secret` aren't required. |
INTRODUCTION.md
Outdated
| * `sasl.oauthbearer.config` is a general purpose configuration property | ||
| In this case it's accepts comma-separated `key=value` pairs. |
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.
| * `sasl.oauthbearer.config` is a general purpose configuration property | |
| In this case it's accepts comma-separated `key=value` pairs. | |
| * `sasl.oauthbearer.config` is a general purpose configuration property. | |
| In this case its accepts comma-separated `key=value` pairs. |
INTRODUCTION.md
Outdated
| The `params` key is required and its value is the GET query string to append | ||
| to the token endpoint URL. Such query string contains params required by | ||
| Azure IMDS such as `client_id` (the UAMI), `resource` for determining the | ||
| target audience and `api-version` for the API version to be used by the endpoint | ||
| * `sasl.oauthbearer.token.endpoint.url` (optional) is set automatically | ||
| when chosing `sasl.oauthbearer.metadata.authentication.type=azure_imds` but can | ||
| be customized |
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 `params` key is required and its value is the GET query string to append | |
| to the token endpoint URL. Such query string contains params required by | |
| Azure IMDS such as `client_id` (the UAMI), `resource` for determining the | |
| target audience and `api-version` for the API version to be used by the endpoint | |
| * `sasl.oauthbearer.token.endpoint.url` (optional) is set automatically | |
| when chosing `sasl.oauthbearer.metadata.authentication.type=azure_imds` but can | |
| be customized | |
| The `params` key is required and its value is the GET query string to append | |
| to the token endpoint URL. Such a query string contains params required by | |
| Azure IMDS, such as `client_id` (the UAMI), `resource` for determining the | |
| target audience and `api-version` for the API version to be used by the endpoint. | |
| * `sasl.oauthbearer.token.endpoint.url` (optional) is set automatically | |
| when choosing `sasl.oauthbearer.metadata.authentication.type=azure_imds` but can | |
| be customized. |
| * | ||
| * @returns A newly allocated string with the appended parameters. | ||
| */ | ||
| char *rd_http_get_params_append(const char *url, const char *params) { |
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.
rather than rolling our url builder, is it okay to use curl instead? that way well tested code is used, for example:
CURLUcode rc = curl_url_set(h, CURLUPART_URL,
"https://example.com:449/foo/bar#abc", 0);
rc = curl_url_set(h, CURLUPART_QUERY, "param1=N",
CURLU_APPENDQUERY | CURLU_URLENCODE); // can choose to urlencode or not
rc = curl_url_set(h, CURLUPART_QUERY, "param2=3",
CURLU_APPENDQUERY | CURLU_URLENCODE);
rc = curl_url_set(h, CURLUPART_FRAGMENT, NULL, 0); // remove hash fragment
char *url;
rc = curl_url_get(h, CURLUPART_URL, &url, 0);
printf("%s\n", url);|
trviup PR is here |
…tially supporting the Azure UAMI method.
based authentications
…ntity with IMDS that is the authentication service
function and enums
…" as in other implementations and to make it optional if the default endpoint is overridden.
a7e64d8 to
7d356de
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.
Thanks for addressing the comments
* Implementation of OAUTHBEARER/OIDC metadata based authentication, initially supporting the Azure UAMI method. * Tests with trivup 0.14.0 supporting metadata based authentications * Add documentation and changelog entry * Rename `azure` value to `azure_imds` and replace UAMI that is the identity with IMDS that is the authentication service * Extract authentication URL and rename internal function and enums * Changes to name the configuration property "query" instead of "params" as in other implementations and to make it optional if the default endpoint is overridden.
* Implementation of OAUTHBEARER/OIDC metadata based authentication, initially supporting the Azure UAMI method. * Tests with trivup 0.14.0 supporting metadata based authentications * Add documentation and changelog entry * Rename `azure` value to `azure_imds` and replace UAMI that is the identity with IMDS that is the authentication service * Extract authentication URL and rename internal function and enums * Changes to name the configuration property "query" instead of "params" as in other implementations and to make it optional if the default endpoint is overridden.
initially supporting the Azure UAMI method.