Skip to content

Commit 14afbfb

Browse files
emasabemasab
committed
Address comments
1 parent a0879b4 commit 14afbfb

File tree

6 files changed

+93
-103
lines changed

6 files changed

+93
-103
lines changed

‎INTRODUCTION.md‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1249,23 +1249,25 @@ Currently these authentication types are supported:
12491249

12501250
###### Azure IMDS
12511251

1252-
to use this method you set:
1252+
To use this method you set:
12531253

1254-
* `sasl.oauthbearer.metadata.authentication.type=azure_imds` this make that ` sasl.oauthbearer.client.id`
1255-
and `sasl.oauthbearer.client.secret` aren't required
1254+
* `sasl.oauthbearer.metadata.authentication.type=azure_imds` this makes it so
1255+
that ` sasl.oauthbearer.client.id` and `sasl.oauthbearer.client.secret`
1256+
aren't required.
12561257
* `sasl.oauthbearer.config` is a general purpose configuration property
1257-
In this case it's accepts comma-separated `key=value` pairs.
1258-
The `params` key is required and its value is the GET query string to append
1258+
In this case it accepts comma-separated `key=value` pairs.
1259+
The `query` key is required in case `sasl.oauthbearer.token.endpoint.url` isn't
1260+
specified and its value is the GET query string to append
12591261
to the token endpoint URL. Such query string contains params required by
12601262
Azure IMDS such as `client_id` (the UAMI), `resource` for determining the
12611263
target audience and `api-version` for the API version to be used by the endpoint
1262-
* `sasl.oauthbearer.token.endpoint.url` (optional) is set automatically
1263-
when chosing `sasl.oauthbearer.metadata.authentication.type=azure_imds` but can
1264-
be customized
1264+
* `sasl.oauthbearer.token.endpoint.url` (optional) is set automatically.
1265+
when choosing `sasl.oauthbearer.metadata.authentication.type=azure_imds` but can
1266+
be customized.
12651267

12661268

12671269
_Example:_ `sasl.oauthbearer.metadata.authentication.type=azure_imds` and
1268-
`sasl.oauthbearer.config=params=api-version=2018-02-01&resource=api://<App registration client id>&client_id=<UAMI client id>`
1270+
`sasl.oauthbearer.config=params=api-version=2025-04-07&resource=api://<App registration client id>&client_id=<UAMI client id>`
12691271

12701272

12711273
<a name="sparse-connections"></a>

‎src/rdhttp.c‎

Lines changed: 44 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -603,49 +603,34 @@ rd_http_error_t *rd_http_post_expect_json(rd_kafka_t *rk,
603603
* @brief Append \p params to \p url, taking care of existing query parameters
604604
* and hash fragments. \p params must be already URL encoded.
605605
*
606-
* @returns A newly allocated string with the appended parameters.
606+
* @returns A newly allocated string with the appended parameters or NULL
607+
* on error.
607608
*/
608609
char *rd_http_get_params_append(const char *url, const char *params) {
609-
size_t params_len, of = 0;
610-
if (!params || !*params)
611-
return rd_strdup(url);
612-
613-
params_len = strlen(params);
614-
615-
char *hash_pos = strstr(url, "#"),
616-
*has_question_mark = strchr(url, '?');
617-
char *separator = "";
618-
char *ret;
619-
size_t insert_pos = strlen(url);
620-
if (hash_pos)
621-
insert_pos = hash_pos - url;
622-
623-
if (has_question_mark && (url + insert_pos) < has_question_mark)
624-
/* Skip question marks after URL end */
625-
has_question_mark = NULL;
626-
627-
if (has_question_mark &&
628-
insert_pos > (size_t)(has_question_mark - url) + 1)
629-
separator = "&";
630-
else if (!has_question_mark)
631-
separator = "?";
632-
633-
if (separator[0] == '&' && url[0] && url[insert_pos - 1] == '&')
634-
/* Don't duplicate last & */
635-
separator = "";
636-
637-
ret = rd_malloc(insert_pos + strlen(separator) + params_len + 1);
638-
memcpy(ret, url, insert_pos);
639-
of += insert_pos;
640-
if (separator[0]) {
641-
ret[insert_pos] = separator[0];
642-
of++;
643-
}
644-
memcpy(ret + of, params, params_len);
645-
of += params_len;
646-
ret[of] = '\0';
647-
648-
return ret;
610+
char *new_url;
611+
CURLU *u = curl_url();
612+
CURLUcode rc = curl_url_set(u, CURLUPART_URL, url, 0);
613+
if (rc != CURLUE_OK)
614+
goto err;
615+
616+
rc = curl_url_set(u, CURLUPART_QUERY, params, CURLU_APPENDQUERY);
617+
if (rc != CURLUE_OK)
618+
goto err;
619+
620+
rc = curl_url_set(u, CURLUPART_FRAGMENT, NULL,
621+
0); // remove hash fragment
622+
if (rc != CURLUE_OK)
623+
goto err;
624+
625+
rc = curl_url_get(u, CURLUPART_URL, &new_url, 0);
626+
if (rc != CURLUE_OK)
627+
goto err;
628+
629+
curl_url_cleanup(u);
630+
return new_url;
631+
err:
632+
curl_url_cleanup(u);
633+
return NULL;
649634
}
650635

651636
/**
@@ -773,17 +758,13 @@ int unittest_http_get_params_append(void) {
773758
rd_kafka_t *rk;
774759
char *res;
775760
RD_UT_BEGIN();
776-
char *tests[] = {"",
777-
"",
761+
char *tests[] = {"http://localhost:1234",
778762
"",
763+
"http://localhost:1234/",
779764

780-
"http://localhost:1234",
781-
"",
782-
"http://localhost:1234",
783-
784-
"http://localhost:1234",
765+
"http://localhost:1234/",
785766
"a=1",
786-
"http://localhost:1234?a=1",
767+
"http://localhost:1234/?a=1",
787768

788769
"https://localhost:1234/",
789770
"a=1&b=2",
@@ -793,30 +774,31 @@ int unittest_http_get_params_append(void) {
793774
"c=hi",
794775
"http://mydomain.com/?a=1&c=hi",
795776

796-
"https://mydomain.com?",
777+
"https://mydomain.com/?",
797778
"c=hi",
798-
"https://mydomain.com?c=hi",
779+
"https://mydomain.com/?c=hi",
799780

800-
"http://localhost:1234?a=1&b=2#&c=3",
781+
"http://localhost:1234/path?a=1&b=2#&c=3",
801782
"c=hi",
802-
"http://localhost:1234?a=1&b=2&c=hi",
783+
"http://localhost:1234/path?a=1&b=2&c=hi",
803784

804785
"http://localhost:1234#?c=3",
805786
"a=1",
806-
"http://localhost:1234?a=1",
787+
"http://localhost:1234/?a=1",
807788

808-
"https://otherdomain.io?a=1&#c=3",
789+
"https://otherdomain.io/path?a=1&#c=3",
809790
"b=2",
810-
"https://otherdomain.io?a=1&b=2",
791+
"https://otherdomain.io/path?a=1&b=2",
792+
NULL};
811793

812-
"",
813-
"a=2&b=3",
814-
"?a=2&b=3",
815794

816-
NULL};
817-
char **test = tests;
795+
res = rd_http_get_params_append("", "");
796+
RD_UT_ASSERT(!res, "Expected NULL result, got: \"%s\"", res);
797+
res = rd_http_get_params_append("", "a=2&b=3");
798+
RD_UT_ASSERT(!res, "Expected NULL result, got: \"%s\"", res);
818799

819-
rk = rd_calloc(1, sizeof(*rk));
800+
char **test = tests;
801+
rk = rd_calloc(1, sizeof(*rk));
820802
while (test[0]) {
821803
res = rd_http_get_params_append(test[0], test[1]);
822804
RD_UT_ASSERT(!strcmp(res, test[2]),

‎src/rdkafka_conf.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -372,7 +372,7 @@ struct rd_kafka_conf_s {
372372
struct {
373373
rd_kafka_oauthbearer_metadata_authentication_type_t
374374
type;
375-
const char *params;
375+
const char *query;
376376
} metadata_authentication;
377377

378378

‎src/rdkafka_sasl_oauthbearer_oidc.c‎

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1003,8 +1003,8 @@ void rd_kafka_oidc_token_client_credentials_refresh_cb(
10031003
* @brief Implementation of Oauth/OIDC token refresh callback function
10041004
* for Azure IMDS,
10051005
* will receive the JSON response after HTTP(S) GET call to token
1006-
* provider, then extract the jwt from the JSON response, and forward it to the
1007-
* broker.
1006+
* provider, then extract the jwt from the JSON response, and
1007+
* forward it to the broker.
10081008
*/
10091009
void rd_kafka_oidc_token_metadata_azure_imds_refresh_cb(
10101010
rd_kafka_t *rk,
@@ -1040,30 +1040,40 @@ void rd_kafka_oidc_token_metadata_azure_imds_refresh_cb(
10401040
return;
10411041

10421042
if (rk->rk_conf.sasl.oauthbearer_config &&
1043-
!rk->rk_conf.sasl.oauthbearer.metadata_authentication.params) {
1043+
!rk->rk_conf.sasl.oauthbearer.metadata_authentication.query) {
10441044
size_t i, oauthbearer_config_cnt;
10451045
char **config_pairs =
10461046
rd_string_split(rk->rk_conf.sasl.oauthbearer_config, ',',
10471047
rd_true, &oauthbearer_config_cnt);
10481048
for (i = 0; i < oauthbearer_config_cnt; i++) {
10491049
char *config_pair = config_pairs[i];
1050-
char *params_pos = strstr(config_pair, "params=");
1051-
if (params_pos == config_pair) {
1050+
char *query_pos = strstr(config_pair, "query=");
1051+
if (query_pos == config_pair) {
10521052
rk->rk_conf.sasl.oauthbearer
1053-
.metadata_authentication.params =
1054-
params_pos + strlen("params=");
1053+
.metadata_authentication.query =
1054+
query_pos + strlen("query=");
10551055
break;
10561056
}
10571057
}
1058-
if (!rk->rk_conf.sasl.oauthbearer.metadata_authentication
1059-
.params)
1058+
if (!rk->rk_conf.sasl.oauthbearer.metadata_authentication.query)
10601059
rk->rk_conf.sasl.oauthbearer.metadata_authentication
1061-
.params = "";
1060+
.query = "";
10621061
}
10631062

10641063
token_endpoint_url = rd_http_get_params_append(
10651064
rk->rk_conf.sasl.oauthbearer.token_endpoint_url,
1066-
rk->rk_conf.sasl.oauthbearer.metadata_authentication.params);
1065+
rk->rk_conf.sasl.oauthbearer.metadata_authentication.query);
1066+
if (token_endpoint_url == NULL) {
1067+
rd_snprintf(
1068+
set_token_errstr, sizeof(set_token_errstr),
1069+
"Failed to append params \"%s\" to token endpoint "
1070+
"URL \"%s\"",
1071+
rk->rk_conf.sasl.oauthbearer.metadata_authentication.query,
1072+
rk->rk_conf.sasl.oauthbearer.token_endpoint_url);
1073+
rd_kafka_log(rk, LOG_ERR, "OIDC", "%s", set_token_errstr);
1074+
rd_kafka_oauthbearer_set_token_failure(rk, set_token_errstr);
1075+
goto done;
1076+
}
10671077

10681078
herr = rd_http_get_json(rk, token_endpoint_url, headers_array, 1,
10691079
timeout_s, retries, retry_ms, &json);

‎tests/0126-oauthbearer_oidc.c‎

Lines changed: 16 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -424,41 +424,37 @@ static rd_kafka_conf_t *oidc_configuration_metadata_authentication(
424424
test_conf_set(conf,
425425
"sasl.oauthbearer.metadata.authentication.type",
426426
"azure_imds");
427-
test_conf_set(
428-
conf, "sasl.oauthbearer.config",
429-
"params=__metadata_authentication_type=azure_imds&"
430-
"api-version=2018-02-01&resource="
431-
"api://external_resource_id&client_id=client_id");
427+
test_conf_set(conf, "sasl.oauthbearer.config",
428+
"query=__metadata_authentication_type=azure_imds&"
429+
"api-version=2025-04-07&resource="
430+
"api://external_resource_id&client_id=client_id");
432431
break;
433432
case OIDC_CONFIGURATION_METADATA_AUTHENTICATION_VARIATION_AZURE_IMDS_MISSING_CLIENT_ID:
434433
test_conf_set(conf,
435434
"sasl.oauthbearer.metadata.authentication.type",
436435
"azure_imds");
437-
test_conf_set(
438-
conf, "sasl.oauthbearer.config",
439-
"params=__metadata_authentication_type=azure_imds&"
440-
"api-version=2018-02-01&resource="
441-
"api://external_resource_id");
436+
test_conf_set(conf, "sasl.oauthbearer.config",
437+
"query=__metadata_authentication_type=azure_imds&"
438+
"api-version=2025-04-07&resource="
439+
"api://external_resource_id");
442440
break;
443441
case OIDC_CONFIGURATION_METADATA_AUTHENTICATION_VARIATION_AZURE_IMDS_MISSING_RESOURCE:
444442
test_conf_set(conf,
445443
"sasl.oauthbearer.metadata.authentication.type",
446444
"azure_imds");
447-
test_conf_set(
448-
conf, "sasl.oauthbearer.config",
449-
"params=__metadata_authentication_type=azure_imds&"
450-
"api-version=2018-02-01&"
451-
"client_id=client_id");
445+
test_conf_set(conf, "sasl.oauthbearer.config",
446+
"query=__metadata_authentication_type=azure_imds&"
447+
"api-version=2025-04-07&"
448+
"client_id=client_id");
452449
break;
453450
case OIDC_CONFIGURATION_METADATA_AUTHENTICATION_VARIATION_AZURE_IMDS_MISSING_API_VERSION:
454451
test_conf_set(conf,
455452
"sasl.oauthbearer.metadata.authentication.type",
456453
"azure_imds");
457-
test_conf_set(
458-
conf, "sasl.oauthbearer.config",
459-
"params=__metadata_authentication_type=azure_imds&"
460-
"resource="
461-
"api://external_resource_id&client_id=client_id");
454+
test_conf_set(conf, "sasl.oauthbearer.config",
455+
"query=__metadata_authentication_type=azure_imds&"
456+
"resource="
457+
"api://external_resource_id&client_id=client_id");
462458
break;
463459
default:
464460
TEST_ASSERT(rd_false,

‎tests/trivup/trivup-0.14.0.tar.gz‎

74 Bytes
Binary file not shown.

0 commit comments

Comments
 (0)