output_azure: OAuth authentication support for Azure output plugins#11571
Open
zshuang0316 wants to merge 4 commits intofluent:masterfrom
Open
output_azure: OAuth authentication support for Azure output plugins#11571zshuang0316 wants to merge 4 commits intofluent:masterfrom
zshuang0316 wants to merge 4 commits intofluent:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Azure authentication core (MSI, service principal, workload identity) with public APIs and macros; integrates OAuth flows into Azure Blob and Kusto plugins, extends plugin configs and structs, updates CMake to include the new source under TLS, and adds runtime tests for Azure Blob auth methods. Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin as Azure Plugin
participant Core as flb_azure_auth
participant IMDS as IMDS (169.254.169.254)
participant MSAL as MSAL Token Endpoint
participant File as Token File
Plugin->>Core: Request token (auth_type, client_id, tenant_id, resource)
alt Managed Identity (system)
Core->>IMDS: HTTP GET /metadata/identity/oauth2/token?resource=...
IMDS-->>Core: JSON {access_token, expires_in}
Core-->>Plugin: "Bearer <access_token>"
else Managed Identity (user)
Core->>IMDS: HTTP GET /metadata/identity/oauth2/token?resource=...&client_id=...
IMDS-->>Core: JSON {access_token}
Core-->>Plugin: "Bearer <access_token>"
else Service Principal
Core->>MSAL: POST /{tenant}/oauth2/v2.0/token (client_id, client_secret, scope)
MSAL-->>Core: JSON {access_token}
Core-->>Plugin: "Bearer <access_token>"
else Workload Identity
Core->>File: read federated token (token_file)
File-->>Core: federated_assertion
Core->>MSAL: POST /{tenant}/oauth2/v2.0/token (assertion, grant_type, scope)
MSAL-->>Core: JSON {access_token}
Core-->>Plugin: "Bearer <access_token>"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 250b0c5cb4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+74
to
+75
| ret = flb_oauth2_payload_append(ctx->o, "scope", 5, | ||
| FLB_AZURE_BLOB_RESOURCE_SCOPE ".default", 29); |
There was a problem hiding this comment.
In get_azure_blob_token, the scope payload uses a hard-coded length of 29 for FLB_AZURE_BLOB_RESOURCE_SCOPE ".default", but that string is 34 bytes long. flb_oauth2_payload_append honors the provided length, so the scope is truncated (e.g. https://storage.azure.com/.de), which causes Azure AD token requests to fail with invalid_scope for auth_type service_principal.
Useful? React with 👍 / 👎.
src/flb_azure_auth.c
Outdated
Comment on lines
+52
to
+53
| flb_error("[azure msi auth] could not get an upstream connection to %s:%s", | ||
| ctx->u->tcp_host, ctx->u->tcp_port); |
There was a problem hiding this comment.
This error log formats ctx->u->tcp_port with %s, but tcp_port is an integer field. If MSI auth cannot acquire an upstream connection and this branch runs, the variadic logger will treat the integer as a pointer, which can lead to invalid memory access or a crash while handling the failure.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/out_azure_blob/azure_blob_conf.c (1)
667-673:⚠️ Potential issue | 🟠 MajorInconsistent auth type constant used in new OAuth code.
Lines 667 and 821 use the legacy constant
AZURE_BLOB_AUTH_KEYwhile the rest of the OAuth implementation usesFLB_AZURE_AUTH_KEY(defined inflb_azure_auth.has a standard enum). Although both constants have the same numeric value (0), using the legacy constant alongside the standardized one creates inconsistency in code that was just refactored to support multiple authentication types. Line 650 in the same file already usesFLB_AZURE_AUTH_KEY, reinforcing that these two lines should be updated for consistency.Proposed fix
/* If the shared key is set decode it */ - if (ctx->atype == AZURE_BLOB_AUTH_KEY && + if (ctx->atype == FLB_AZURE_AUTH_KEY && ctx->shared_key != NULL) { ret = set_shared_key(ctx);And at line 821:
/* Prepare shared key buffer */ - if (ctx->atype == AZURE_BLOB_AUTH_KEY) { + if (ctx->atype == FLB_AZURE_AUTH_KEY) { ctx->shared_key_prefix = flb_sds_create_size(256);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/out_azure_blob/azure_blob_conf.c` around lines 667 - 673, The auth-type check uses the legacy AZURE_BLOB_AUTH_KEY constant causing inconsistency with the rest of the OAuth refactor; update the condition that checks ctx->atype (and the other occurrence near line 821) to use the standardized enum constant FLB_AZURE_AUTH_KEY instead of AZURE_BLOB_AUTH_KEY, keeping the same logic that calls set_shared_key(ctx) and returns NULL on failure so behavior is unchanged.
🧹 Nitpick comments (4)
tests/runtime/out_azure_blob.c (1)
42-71: Consider documenting the "system" convention for system-assigned managed identity.The test uses
client_id = "system"to indicate system-assigned managed identity. This convention should be documented or verified against the configuration parsing logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/runtime/out_azure_blob.c` around lines 42 - 71, The test flb_test_azure_blob_managed_identity_system uses client_id = "system" to represent a system-assigned managed identity but this convention isn't documented or validated; update the test and/or project documentation and configuration parsing to clarify and accept this convention: add a brief comment in flb_test_azure_blob_managed_identity_system explaining that "client_id" == "system" denotes system-assigned managed identity and ensure the azure_blob auth_type handling (lookup for "auth_type" and "client_id" in the Azure Blob output plugin) explicitly recognizes and treats client_id == "system" as system-assigned identity (or add a validation assertion in the test that the plugin config parsing accepts "system").plugins/out_azure_blob/azure_blob_http.c (1)
55-61: Unusedtokenvariable in MSI case.The
tokenvariable is assigned fromflb_azure_msi_token_get()but never used beyond the NULL check. The function stores the token internally inctx->o, so the check is valid, but the variable assignment is misleading.♻️ Suggested clarification
case FLB_AZURE_AUTH_MANAGED_IDENTITY_SYSTEM: case FLB_AZURE_AUTH_MANAGED_IDENTITY_USER: - token = flb_azure_msi_token_get(ctx->o); - if (!token) { + /* MSI token is stored internally in ctx->o */ + if (!flb_azure_msi_token_get(ctx->o)) { ret = -1; } break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/out_azure_blob/azure_blob_http.c` around lines 55 - 61, The code assigns the result of flb_azure_msi_token_get(ctx->o) to a local variable token in the MSI auth case but never uses it, which is misleading; remove the unused variable and simply call flb_azure_msi_token_get(ctx->o) and check its return value (or assign to a temporary boolean) so the NULL-check remains but no unused token variable exists. Update the switch cases FLB_AZURE_AUTH_MANAGED_IDENTITY_SYSTEM and FLB_AZURE_AUTH_MANAGED_IDENTITY_USER to call flb_azure_msi_token_get(ctx->o) and set ret = -1 when it returns NULL, leaving token out entirely.src/flb_azure_auth.c (2)
183-191: Hardcoded string lengths are fragile.The
flb_sds_catcalls use hardcoded lengths (e.g.,10,30,77,18,7,8). If these strings are ever modified, the lengths must be updated manually, which is error-prone.♻️ Consider using strlen or sizeof-1 for literals
- body = flb_sds_cat(body, "client_id=", 10); + body = flb_sds_cat(body, "client_id=", sizeof("client_id=") - 1); body = flb_sds_cat(body, client_id, strlen(client_id)); - body = flb_sds_cat(body, "&grant_type=client_credentials", 30); + body = flb_sds_cat(body, "&grant_type=client_credentials", sizeof("&grant_type=client_credentials") - 1);Or define a helper macro like
#define SDS_CAT_LITERAL(sds, lit) flb_sds_cat(sds, lit, sizeof(lit) - 1).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flb_azure_auth.c` around lines 183 - 191, The flb_sds_cat calls building the token request body use fragile hardcoded lengths (e.g., in calls around body = flb_sds_cat(...)), so replace those numeric literals with computed lengths: use strlen() for variable strings (client_id, resource) and sizeof(literal)-1 (or strlen) for string literals, or introduce a helper macro like SDS_CAT_LITERAL(sds, lit) that expands to flb_sds_cat(sds, lit, sizeof(lit)-1); update every occurrence (the calls involving client_id, federated_token, resource and the literal pieces like "&grant_type=...", "&client_assertion_type=...", "&client_assertion=", "&scope=", ".default") to use the safer length approach.
141-274: Missing token cache check in workload identity flow.Unlike
flb_azure_msi_token_getwhich checks if a valid cached token exists before making a request, this function always reads the federated token file and makes an HTTP request. Consider adding a similar cache validity check at the start to avoid unnecessary token exchanges when the current token is still valid.♻️ Proposed refactor to add caching
int flb_azure_workload_identity_token_get(struct flb_oauth2 *ctx, const char *token_file, const char *client_id, const char *tenant_id, const char *resource) { int ret; size_t b_sent; struct flb_connection *u_conn; struct flb_http_client *c; flb_sds_t federated_token; flb_sds_t body = NULL; + time_t now; + + /* Check if we have a valid cached token */ + now = time(NULL); + if (ctx->access_token) { + if (ctx->expires_at > now && flb_sds_len(ctx->access_token) > 0) { + flb_debug("[azure workload identity] using cached token"); + return 0; + } + } flb_info("[azure workload identity] initiating token exchange");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flb_azure_auth.c` around lines 141 - 274, flb_azure_workload_identity_token_get currently always reads the federated token and performs an exchange; add the same cached-token validity check used in flb_azure_msi_token_get to short-circuit when the current access token is still valid. At the top of flb_azure_workload_identity_token_get, check ctx->access_token/ctx->expires_at vs time(NULL) (and ctx->expires_in if needed) and return 0 immediately when still valid; ensure you reference the same fields and semantics as flb_azure_msi_token_get (use ctx->expires_at and ctx->expires_in) so the function skips reading the federated token and making the HTTP request when a cached token is usable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/fluent-bit/flb_azure_auth.h`:
- Around line 52-53: The defined Azure Kusto OAuth2 resource scope macro
FLB_AZURE_KUSTO_RESOURCE_SCOPE in flb_azure_auth.h is incorrect; change its
value from the help cluster URL to the proper OAuth2 scope for Azure Data
Explorer by updating the macro definition to the correct scope string
("https://kusto.kusto.windows.net/.default") so token requests use the right
audience for authentication.
In `@plugins/out_azure_blob/azure_blob_conf.c`:
- Around line 776-803: The mutex ctx->token_mutex is initialized with
pthread_mutex_init before all failure points, but on later errors (e.g., failure
creating oauth_url or flb_oauth2_create) the function returns without destroying
it; move the pthread_mutex_init call to after all operations that can fail
(after flb_azure_auth_build_oauth_url and flb_oauth2_create succeed), or add a
centralized cleanup path (goto cleanup) that calls
pthread_mutex_destroy(&ctx->token_mutex) on all error returns; update the block
around flb_azure_auth_build_oauth_url, flb_oauth2_create and the oauth2 init log
to either defer initialization or ensure pthread_mutex_destroy is invoked on
every early return so the token_mutex is never leaked.
In `@plugins/out_azure_blob/azure_blob_http.c`:
- Around line 74-75: The call to flb_oauth2_payload_append in azure_blob_http.c
uses an incorrect hardcoded length (29) for the scope string
FLB_AZURE_BLOB_RESOURCE_SCOPE ".default"; update the flb_oauth2_payload_append
invocation (the line assigning ret) to let the function compute the length by
passing -1 instead of 29 (or otherwise compute the correct length, e.g., 35) so
the full scope string is sent correctly.
In `@plugins/out_azure_blob/azure_blob.h`:
- Around line 134-137: pthread_mutex_init() return value is not checked in
flb_azure_blob_conf_create(), so if mutex initialization for token_mutex fails
later pthread mutex operations will crash; update flb_azure_blob_conf_create()
to check the result of pthread_mutex_init(&conf->token_mutex, NULL), handle
non-zero return by logging an error (use existing logging facility), clean up
allocated resources and return NULL (or an error code) so caller knows creation
failed, and ensure flb_azure_blob_conf_destroy() still safely handles
partially-initialized conf structures by only calling
pthread_mutex_destroy(&conf->token_mutex) when initialization succeeded.
In `@src/CMakeLists.txt`:
- Line 167: The Azure auth symbols (flb_azure_auth_build_oauth_url,
flb_azure_msi_token_get, flb_azure_workload_identity_token_get) are only
compiled when FLB_TLS is enabled but out_azure_blob and out_azure_kusto call
them unconditionally, causing link errors with -DFLB_TLS=OFF; fix by either
adding `#ifdef` FLB_TLS / `#endif` guards around the OAuth/token call sites in
out_azure_blob and out_azure_kusto so they are skipped when FLB_TLS is not
defined, or alternatively update the build configuration to disable the Azure
plugins when FLB_TLS is off, or add stub implementations of the three functions
when FLB_TLS is not defined so the plugins link cleanly.
In `@src/flb_azure_auth.c`:
- Around line 127-133: The comparison uses an unsigned size_t (bytes_read <= 0)
and misses read errors; change the logic in the block that calls fread
(involving bytes_read, fp and token_file) to: check bytes_read == 0 (not <= 0)
and, before fclose(fp), call ferror(fp) to detect I/O errors; if ferror(fp) is
set, log a descriptive error via flb_error including the token_file and
errno/strerror details, otherwise treat bytes_read == 0 as an empty read and
handle accordingly, then fclose(fp).
- Around line 33-47: The function flb_azure_msi_token_get reads and updates
ctx->access_token and ctx->expires_at without synchronization; wrap the token
access and validation with the flb_oauth2 lock (ctx->lock) by acquiring the lock
before reading ctx->access_token and ctx->expires_at and releasing it after the
check, and likewise acquire the same lock before updating ctx->access_token and
ctx->expires_at (the token update block referenced around the later assignment)
and release it after the update to ensure thread-safe reads/writes.
---
Outside diff comments:
In `@plugins/out_azure_blob/azure_blob_conf.c`:
- Around line 667-673: The auth-type check uses the legacy AZURE_BLOB_AUTH_KEY
constant causing inconsistency with the rest of the OAuth refactor; update the
condition that checks ctx->atype (and the other occurrence near line 821) to use
the standardized enum constant FLB_AZURE_AUTH_KEY instead of
AZURE_BLOB_AUTH_KEY, keeping the same logic that calls set_shared_key(ctx) and
returns NULL on failure so behavior is unchanged.
---
Nitpick comments:
In `@plugins/out_azure_blob/azure_blob_http.c`:
- Around line 55-61: The code assigns the result of
flb_azure_msi_token_get(ctx->o) to a local variable token in the MSI auth case
but never uses it, which is misleading; remove the unused variable and simply
call flb_azure_msi_token_get(ctx->o) and check its return value (or assign to a
temporary boolean) so the NULL-check remains but no unused token variable
exists. Update the switch cases FLB_AZURE_AUTH_MANAGED_IDENTITY_SYSTEM and
FLB_AZURE_AUTH_MANAGED_IDENTITY_USER to call flb_azure_msi_token_get(ctx->o) and
set ret = -1 when it returns NULL, leaving token out entirely.
In `@src/flb_azure_auth.c`:
- Around line 183-191: The flb_sds_cat calls building the token request body use
fragile hardcoded lengths (e.g., in calls around body = flb_sds_cat(...)), so
replace those numeric literals with computed lengths: use strlen() for variable
strings (client_id, resource) and sizeof(literal)-1 (or strlen) for string
literals, or introduce a helper macro like SDS_CAT_LITERAL(sds, lit) that
expands to flb_sds_cat(sds, lit, sizeof(lit)-1); update every occurrence (the
calls involving client_id, federated_token, resource and the literal pieces like
"&grant_type=...", "&client_assertion_type=...", "&client_assertion=",
"&scope=", ".default") to use the safer length approach.
- Around line 141-274: flb_azure_workload_identity_token_get currently always
reads the federated token and performs an exchange; add the same cached-token
validity check used in flb_azure_msi_token_get to short-circuit when the current
access token is still valid. At the top of
flb_azure_workload_identity_token_get, check ctx->access_token/ctx->expires_at
vs time(NULL) (and ctx->expires_in if needed) and return 0 immediately when
still valid; ensure you reference the same fields and semantics as
flb_azure_msi_token_get (use ctx->expires_at and ctx->expires_in) so the
function skips reading the federated token and making the HTTP request when a
cached token is usable.
In `@tests/runtime/out_azure_blob.c`:
- Around line 42-71: The test flb_test_azure_blob_managed_identity_system uses
client_id = "system" to represent a system-assigned managed identity but this
convention isn't documented or validated; update the test and/or project
documentation and configuration parsing to clarify and accept this convention:
add a brief comment in flb_test_azure_blob_managed_identity_system explaining
that "client_id" == "system" denotes system-assigned managed identity and ensure
the azure_blob auth_type handling (lookup for "auth_type" and "client_id" in the
Azure Blob output plugin) explicitly recognizes and treats client_id == "system"
as system-assigned identity (or add a validation assertion in the test that the
plugin config parsing accepts "system").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04bbeafd-b96d-4f93-beb2-1877e5c1702c
📒 Files selected for processing (13)
include/fluent-bit/flb_azure_auth.hplugins/out_azure_blob/azure_blob.cplugins/out_azure_blob/azure_blob.hplugins/out_azure_blob/azure_blob_conf.cplugins/out_azure_blob/azure_blob_http.cplugins/out_azure_kusto/CMakeLists.txtplugins/out_azure_kusto/azure_kusto.cplugins/out_azure_kusto/azure_kusto.hplugins/out_azure_kusto/azure_kusto_conf.csrc/CMakeLists.txtsrc/flb_azure_auth.ctests/runtime/CMakeLists.txttests/runtime/out_azure_blob.c
💤 Files with no reviewable changes (1)
- plugins/out_azure_kusto/CMakeLists.txt
include/fluent-bit/flb_azure_auth.h
Outdated
Comment on lines
+52
to
+53
| /* Azure Kusto default resource scope */ | ||
| #define FLB_AZURE_KUSTO_RESOURCE_SCOPE "https://help.kusto.windows.net/" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the correct Azure Kusto OAuth2 resource scope for authentication?
💡 Result:
For Azure Data Explorer (Kusto) data-plane authentication using OAuth2 / Entra ID v2 tokens, the resource scope to request is:
https://kusto.kusto.windows.net/.default[1]
Note: Some Microsoft tooling defaults the managed-identity “audience/scope” to https://api.kusto.windows.net / https://api.kusto.windows.net/.default, with an option to override the audience to your specific cluster URL. [2]
Fix the Kusto resource scope to use the correct OAuth2 scope.
The FLB_AZURE_KUSTO_RESOURCE_SCOPE is currently set to "https://help.kusto.windows.net/", but the correct OAuth2 resource scope for Azure Data Explorer (Kusto) authentication should be "https://kusto.kusto.windows.net/.default". The current value refers to a specific help cluster and will cause authentication failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/fluent-bit/flb_azure_auth.h` around lines 52 - 53, The defined Azure
Kusto OAuth2 resource scope macro FLB_AZURE_KUSTO_RESOURCE_SCOPE in
flb_azure_auth.h is incorrect; change its value from the help cluster URL to the
proper OAuth2 scope for Azure Data Explorer by updating the macro definition to
the correct scope string ("https://kusto.kusto.windows.net/.default") so token
requests use the right audience for authentication.
Comment on lines
+134
to
+137
| /* OAuth2 authentication */ | ||
| flb_sds_t oauth_url; | ||
| struct flb_oauth2 *o; | ||
| pthread_mutex_t token_mutex; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check mutex initialization and destruction for azure_blob
echo "=== Checking token_mutex initialization ==="
rg -n "pthread_mutex_init.*token_mutex|token_mutex.*pthread_mutex_init" plugins/out_azure_blob/
echo ""
echo "=== Checking token_mutex destruction ==="
rg -n "pthread_mutex_destroy.*token_mutex|token_mutex.*pthread_mutex_destroy" plugins/out_azure_blob/
echo ""
echo "=== Check azure_blob_conf.c for mutex handling ==="
rg -n -B2 -A2 "token_mutex" plugins/out_azure_blob/azure_blob_conf.cRepository: fluent/fluent-bit
Length of output: 779
🏁 Script executed:
# Check the function context for mutex init and destroy operations
echo "=== Context around mutex initialization (line 800) ==="
sed -n '770,810p' plugins/out_azure_blob/azure_blob_conf.c
echo ""
echo "=== Context around mutex destruction (line 941) ==="
sed -n '920,960p' plugins/out_azure_blob/azure_blob_conf.c
echo ""
echo "=== Overall structure of azure_blob_conf.c to understand lifecycle ==="
rg -n "^[a-zA-Z_].*\(.*\).*\{" plugins/out_azure_blob/azure_blob_conf.c | head -20Repository: fluent/fluent-bit
Length of output: 2497
🏁 Script executed:
# Get function names and full context
echo "=== Function containing initialization (around line 800) ==="
sed -n '650,810p' plugins/out_azure_blob/azure_blob_conf.c | grep -B 150 "Initialize token mutex" | head -20
echo ""
echo "=== Function containing destruction (around line 941) ==="
sed -n '850,960p' plugins/out_azure_blob/azure_blob_conf.c | grep -B 100 "pthread_mutex_destroy" | head -20
echo ""
echo "=== Check mutex usage in the codebase ==="
rg -n "token_mutex" plugins/out_azure_blob/ -B1 -A1Repository: fluent/fluent-bit
Length of output: 3949
🏁 Script executed:
# Get the function signatures
echo "=== Function containing initialization ==="
awk '/^[a-zA-Z_].*\{/,/pthread_mutex_init.*token_mutex/ {print NR": "$0}' plugins/out_azure_blob/azure_blob_conf.c | tail -100 | head -50
echo ""
echo "=== Function containing destruction ==="
awk '/^[a-zA-Z_].*\{/,/pthread_mutex_destroy.*token_mutex/ {print NR": "$0}' plugins/out_azure_blob/azure_blob_conf.c | tail -100 | head -50
echo ""
echo "=== Check error handling for pthread_mutex_init ==="
rg -n -B5 -A5 "pthread_mutex_init.*token_mutex" plugins/out_azure_blob/
echo ""
echo "=== Verify these are plugin init and exit functions ==="
grep -n "^static.*flb_output_plugin\|^static.*plugin_exit\|^static.*plugin_init\|^static.*config_destroy\|^flb_output_plugin_" plugins/out_azure_blob/azure_blob_conf.c | head -20Repository: fluent/fluent-bit
Length of output: 1055
🏁 Script executed:
# Try to find the function definitions more directly
echo "=== Find function declarations in azure_blob_conf.c ==="
rg -n "^[a-zA-Z_][a-zA-Z0-9_*\s]+\(" plugins/out_azure_blob/azure_blob_conf.c | head -30
echo ""
echo "=== Search for specific function patterns ==="
grep -n "^static.*\*" plugins/out_azure_blob/azure_blob_conf.c | head -20
echo ""
echo "=== Look for function at line 800 context ==="
sed -n '700,820p' plugins/out_azure_blob/azure_blob_conf.c | head -50
echo ""
echo "=== Look for function at line 941 context ==="
sed -n '850,960p' plugins/out_azure_blob/azure_blob_conf.c | tail -100Repository: fluent/fluent-bit
Length of output: 5595
Add error handling for mutex initialization.
The token_mutex is properly initialized in flb_azure_blob_conf_create() and destroyed in flb_azure_blob_conf_destroy(), preventing resource leaks. However, pthread_mutex_init() at line 800 does not check the return value; if initialization fails, the error is silently ignored and subsequent mutex operations may fail unexpectedly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/out_azure_blob/azure_blob.h` around lines 134 - 137,
pthread_mutex_init() return value is not checked in
flb_azure_blob_conf_create(), so if mutex initialization for token_mutex fails
later pthread mutex operations will crash; update flb_azure_blob_conf_create()
to check the result of pthread_mutex_init(&conf->token_mutex, NULL), handle
non-zero return by logging an error (use existing logging facility), clean up
allocated resources and return NULL (or an error code) so caller knows creation
failed, and ensure flb_azure_blob_conf_destroy() still safely handles
partially-initialized conf structures by only calling
pthread_mutex_destroy(&conf->token_mutex) when initialization succeeded.
Comment on lines
+127
to
+133
| bytes_read = fread(buf, 1, sizeof(buf) - 1, fp); | ||
| fclose(fp); | ||
|
|
||
| if (bytes_read <= 0) { | ||
| flb_error("[azure workload identity] could not read token from file: %s", token_file); | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
Incorrect comparison for size_t type.
bytes_read is declared as size_t (unsigned), so bytes_read <= 0 will only be true when bytes_read == 0. The < 0 part can never be true. While functionally this works for detecting zero bytes read, it won't detect read errors where fread returns 0 and sets an error indicator.
🐛 Proposed fix
bytes_read = fread(buf, 1, sizeof(buf) - 1, fp);
+ if (ferror(fp)) {
+ fclose(fp);
+ flb_error("[azure workload identity] error reading token file: %s", token_file);
+ return NULL;
+ }
fclose(fp);
- if (bytes_read <= 0) {
+ if (bytes_read == 0) {
flb_error("[azure workload identity] could not read token from file: %s", token_file);
return NULL;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bytes_read = fread(buf, 1, sizeof(buf) - 1, fp); | |
| fclose(fp); | |
| if (bytes_read <= 0) { | |
| flb_error("[azure workload identity] could not read token from file: %s", token_file); | |
| return NULL; | |
| } | |
| bytes_read = fread(buf, 1, sizeof(buf) - 1, fp); | |
| if (ferror(fp)) { | |
| fclose(fp); | |
| flb_error("[azure workload identity] error reading token file: %s", token_file); | |
| return NULL; | |
| } | |
| fclose(fp); | |
| if (bytes_read == 0) { | |
| flb_error("[azure workload identity] could not read token from file: %s", token_file); | |
| return NULL; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/flb_azure_auth.c` around lines 127 - 133, The comparison uses an unsigned
size_t (bytes_read <= 0) and misses read errors; change the logic in the block
that calls fread (involving bytes_read, fp and token_file) to: check bytes_read
== 0 (not <= 0) and, before fclose(fp), call ferror(fp) to detect I/O errors; if
ferror(fp) is set, log a descriptive error via flb_error including the
token_file and errno/strerror details, otherwise treat bytes_read == 0 as an
empty read and handle accordingly, then fclose(fp).
zshuang0316
force-pushed
the
azure-msi-auth
branch
from
250b0c5 to
20c9ffe
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/out_azure_kusto/azure_kusto_conf.c (1)
759-767:⚠️ Potential issue | 🟡 MinorAvoid leaking the default workload-identity token path.
This branch duplicates the default path, but
flb_azure_kusto_conf_destroy()never releasesctx->workload_identity_token_file. Either track ownership for the default assignment or avoid allocating a second copy here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/out_azure_kusto/azure_kusto_conf.c` around lines 759 - 767, The code currently duplicates the default token path with flb_strdup into ctx->workload_identity_token_file which is never freed by flb_azure_kusto_conf_destroy(), causing a leak; fix by either (A) assigning the constant string directly (ctx->workload_identity_token_file = FLB_AZURE_WORKLOAD_IDENTITY_TOKEN_FILE) so no allocation is made and update destroy to not free it, or (B) keep flb_strdup but update flb_azure_kusto_conf_destroy() to detect and free ctx->workload_identity_token_file (ensuring it handles both allocated and constant cases), referencing ctx->workload_identity_token_file, flb_strdup, FLB_AZURE_WORKLOAD_IDENTITY_TOKEN_FILE and flb_azure_kusto_conf_destroy in your changes.
♻️ Duplicate comments (3)
include/fluent-bit/flb_azure_auth.h (1)
52-53:⚠️ Potential issue | 🔴 CriticalStop using the help-cluster audience for Kusto tokens.
FLB_AZURE_KUSTO_RESOURCE_SCOPEis pointing athttps://help.kusto.windows.net/, which is the public help cluster, not the generic Azure Data Explorer audience. Because this symbol is later reused as the base resource for Kusto token requests, it needs to be the real ADX audience instead (for example, the basekusto.kusto.windows.netresource, with.defaultonly derived in the v2 token flows).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/fluent-bit/flb_azure_auth.h` around lines 52 - 53, The macro FLB_AZURE_KUSTO_RESOURCE_SCOPE is pointing to the help-cluster audience and must be changed to the generic Azure Data Explorer (Kusto) audience; update the definition of FLB_AZURE_KUSTO_RESOURCE_SCOPE in flb_azure_auth.h to use the standard ADX resource (e.g., the kusto.kusto.windows.net base audience) so token requests use the proper ADX resource scope (leave any .default suffixing to the token flow code that constructs v2 scopes).plugins/out_azure_blob/azure_blob_conf.c (1)
776-803:⚠️ Potential issue | 🟠 MajorMake the new OAuth setup failure-safe.
This block now owns
ctx->oauth_url,ctx->o, andctx->token_mutex, but it still returns directly on failure and never checkspthread_mutex_init(). Any laterreturn NULLafter this point leaks the OAuth objects, and a failed mutex init leaves the auth path unusable. Please route post-allocation failures through one cleanup path and only destroy the mutex when init actually succeeded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/out_azure_blob/azure_blob_conf.c` around lines 776 - 803, The OAuth initialization currently returns early on failures and doesn't check pthread_mutex_init, risking leaks of ctx->oauth_url and ctx->o and improper mutex handling; modify the block that allocates ctx->oauth_url and ctx->o and initializes ctx->token_mutex so all post-allocation failures jump to a single cleanup path that frees ctx->oauth_url and destroys ctx->o (via the appropriate flb_oauth2_destroy or equivalent) and only calls pthread_mutex_destroy if pthread_mutex_init succeeded; add an init flag (e.g., oauth_mutex_inited) or similar local boolean to track successful pthread_mutex_init, check its return value and log via flb_plg_error on failure, and replace direct return NULL statements after those allocations with a goto cleanup to ensure no resources are leaked.plugins/out_azure_blob/azure_blob_http.c (1)
74-75:⚠️ Potential issue | 🔴 CriticalPass the full blob scope string to Azure AD.
29truncatesFLB_AZURE_BLOB_RESOURCE_SCOPE ".default", so the service-principal token request asks for the wrong scope. Letflb_oauth2_payload_append()compute the length instead.Suggested fix
- ret = flb_oauth2_payload_append(ctx->o, "scope", 5, - FLB_AZURE_BLOB_RESOURCE_SCOPE ".default", 29); + ret = flb_oauth2_payload_append(ctx->o, "scope", 5, + FLB_AZURE_BLOB_RESOURCE_SCOPE ".default", -1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/out_azure_blob/azure_blob_http.c` around lines 74 - 75, The scope string is being truncated by the hardcoded length 29 in the call to flb_oauth2_payload_append; update the call that uses FLB_AZURE_BLOB_RESOURCE_SCOPE ".default" (the line calling flb_oauth2_payload_append(ctx->o, "scope", 5, FLB_AZURE_BLOB_RESOURCE_SCOPE ".default", 29)) so that the full scope is passed — either remove the hardcoded length so flb_oauth2_payload_append computes it or replace 29 with strlen(FLB_AZURE_BLOB_RESOURCE_SCOPE ".default") to provide the correct length.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/out_azure_blob/azure_blob_conf.c`:
- Around line 633-640: The default workload identity token SDS assigned to
ctx->workload_identity_token_file in the workload-identity branch is never freed
on teardown; add an ownership flag (e.g.,
ctx->workload_identity_token_file_owned or a bit in ctx->flags) and set it to
true when you allocate FLB_AZURE_WORKLOAD_IDENTITY_TOKEN_FILE in the assignment
block that creates the SDS, then in the config teardown/destroy routine (the
azure blob config destroy function that releases ctx fields) check that flag and
flb_sds_destroy(ctx->workload_identity_token_file) before clearing the field
and/or freeing ctx; apply the same ownership handling for the other default
assignment mentioned around lines 928-942 so both defaults are released
correctly.
In `@src/flb_azure_auth.c`:
- Around line 183-191: The sequence of flb_sds_cat() calls can return NULL on
failure and subsequent calls will dereference body and crash; modify the block
that builds the token request body (the uses of flb_sds_cat with variables body,
client_id, federated_token, resource) to check the return value after each
flb_sds_cat call: assign the result to a temporary sds (e.g., tmp), verify tmp
is non-NULL, then set body = tmp; if any append fails, free any allocated body
and return/error out immediately. Ensure every flb_sds_cat invocation (including
the "&grant_type...", "&client_assertion_type...", "&client_assertion=",
"&scope=", and ".default" appends) follows this pattern to avoid NULL
dereference.
---
Outside diff comments:
In `@plugins/out_azure_kusto/azure_kusto_conf.c`:
- Around line 759-767: The code currently duplicates the default token path with
flb_strdup into ctx->workload_identity_token_file which is never freed by
flb_azure_kusto_conf_destroy(), causing a leak; fix by either (A) assigning the
constant string directly (ctx->workload_identity_token_file =
FLB_AZURE_WORKLOAD_IDENTITY_TOKEN_FILE) so no allocation is made and update
destroy to not free it, or (B) keep flb_strdup but update
flb_azure_kusto_conf_destroy() to detect and free
ctx->workload_identity_token_file (ensuring it handles both allocated and
constant cases), referencing ctx->workload_identity_token_file, flb_strdup,
FLB_AZURE_WORKLOAD_IDENTITY_TOKEN_FILE and flb_azure_kusto_conf_destroy in your
changes.
---
Duplicate comments:
In `@include/fluent-bit/flb_azure_auth.h`:
- Around line 52-53: The macro FLB_AZURE_KUSTO_RESOURCE_SCOPE is pointing to the
help-cluster audience and must be changed to the generic Azure Data Explorer
(Kusto) audience; update the definition of FLB_AZURE_KUSTO_RESOURCE_SCOPE in
flb_azure_auth.h to use the standard ADX resource (e.g., the
kusto.kusto.windows.net base audience) so token requests use the proper ADX
resource scope (leave any .default suffixing to the token flow code that
constructs v2 scopes).
In `@plugins/out_azure_blob/azure_blob_conf.c`:
- Around line 776-803: The OAuth initialization currently returns early on
failures and doesn't check pthread_mutex_init, risking leaks of ctx->oauth_url
and ctx->o and improper mutex handling; modify the block that allocates
ctx->oauth_url and ctx->o and initializes ctx->token_mutex so all
post-allocation failures jump to a single cleanup path that frees ctx->oauth_url
and destroys ctx->o (via the appropriate flb_oauth2_destroy or equivalent) and
only calls pthread_mutex_destroy if pthread_mutex_init succeeded; add an init
flag (e.g., oauth_mutex_inited) or similar local boolean to track successful
pthread_mutex_init, check its return value and log via flb_plg_error on failure,
and replace direct return NULL statements after those allocations with a goto
cleanup to ensure no resources are leaked.
In `@plugins/out_azure_blob/azure_blob_http.c`:
- Around line 74-75: The scope string is being truncated by the hardcoded length
29 in the call to flb_oauth2_payload_append; update the call that uses
FLB_AZURE_BLOB_RESOURCE_SCOPE ".default" (the line calling
flb_oauth2_payload_append(ctx->o, "scope", 5, FLB_AZURE_BLOB_RESOURCE_SCOPE
".default", 29)) so that the full scope is passed — either remove the hardcoded
length so flb_oauth2_payload_append computes it or replace 29 with
strlen(FLB_AZURE_BLOB_RESOURCE_SCOPE ".default") to provide the correct length.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9365147-7ed7-453c-98f3-da85cc32ee79
📒 Files selected for processing (13)
include/fluent-bit/flb_azure_auth.hplugins/out_azure_blob/azure_blob.cplugins/out_azure_blob/azure_blob.hplugins/out_azure_blob/azure_blob_conf.cplugins/out_azure_blob/azure_blob_http.cplugins/out_azure_kusto/CMakeLists.txtplugins/out_azure_kusto/azure_kusto.cplugins/out_azure_kusto/azure_kusto.hplugins/out_azure_kusto/azure_kusto_conf.csrc/CMakeLists.txtsrc/flb_azure_auth.ctests/runtime/CMakeLists.txttests/runtime/out_azure_blob.c
💤 Files with no reviewable changes (1)
- plugins/out_azure_kusto/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/runtime/out_azure_blob.c
- plugins/out_azure_kusto/azure_kusto.h
- src/CMakeLists.txt
- plugins/out_azure_blob/azure_blob.c
src/flb_azure_auth.c
Outdated
Comment on lines
+183
to
+191
| body = flb_sds_cat(body, "client_id=", 10); | ||
| body = flb_sds_cat(body, client_id, strlen(client_id)); | ||
| body = flb_sds_cat(body, "&grant_type=client_credentials", 30); | ||
| body = flb_sds_cat(body, "&client_assertion_type=urn:ietf:params:oauth:client-assertion-type:jwt-bearer", 77); | ||
| body = flb_sds_cat(body, "&client_assertion=", 18); | ||
| body = flb_sds_cat(body, federated_token, flb_sds_len(federated_token)); | ||
| body = flb_sds_cat(body, "&scope=", 7); | ||
| body = flb_sds_cat(body, resource, strlen(resource)); | ||
| body = flb_sds_cat(body, ".default", 8); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and the specific lines mentioned
head -n 200 src/flb_azure_auth.c | tail -n 30Repository: fluent/fluent-bit
Length of output: 1261
🏁 Script executed:
# Search for the flb_sds_cat function definition and understand its behavior
rg "flb_sds_cat" --context 3 -A 3Repository: fluent/fluent-bit
Length of output: 50374
🏁 Script executed:
# Check the entire function containing these lines to see if there's error handling
sed -n '150,220p' src/flb_azure_auth.cRepository: fluent/fluent-bit
Length of output: 2612
🏁 Script executed:
# Find the flb_sds_cat function definition
rg "^flb_sds_cat\s*\(" -A 10 src/ include/Repository: fluent/fluent-bit
Length of output: 43
🏁 Script executed:
# Also search for the definition in different way
rg "flb_sds_cat.*{" -A 5 | head -50Repository: fluent/fluent-bit
Length of output: 152
🏁 Script executed:
# Look for the sds header file and implementation
find . -name "*sds*" -type f | grep -E "\.(c|h)$"Repository: fluent/fluent-bit
Length of output: 293
🏁 Script executed:
# Check the flb_sds_cat function definition
grep -n "flb_sds_cat" include/fluent-bit/flb_sds.h | head -20Repository: fluent/fluent-bit
Length of output: 337
🏁 Script executed:
# Get the implementation in src/flb_sds.c
sed -n '1,50p' src/flb_sds.c && echo "..." && grep -n "flb_sds_cat" src/flb_sds.c | head -5Repository: fluent/fluent-bit
Length of output: 1822
🏁 Script executed:
# Look for flb_sds_cat implementation with context
rg -A 20 "flb_sds_t.*flb_sds_cat\(" src/flb_sds.cRepository: fluent/fluent-bit
Length of output: 511
🏁 Script executed:
# Check the flb_sds_avail macro definition
grep -n "flb_sds_avail" include/fluent-bit/flb_sds.hRepository: fluent/fluent-bit
Length of output: 111
🏁 Script executed:
# Get more context on what flb_sds_avail does
rg -B 2 -A 5 "define flb_sds_avail" include/Repository: fluent/fluent-bit
Length of output: 43
🏁 Script executed:
# Get the implementation of flb_sds_avail
sed -n '72,80p' include/fluent-bit/flb_sds.hRepository: fluent/fluent-bit
Length of output: 274
🏁 Script executed:
# Get more context on the flb_sds_cat behavior with NULL input
rg -B 5 -A 15 "static inline size_t flb_sds_avail" include/fluent-bit/flb_sds.hRepository: fluent/fluent-bit
Length of output: 469
🏁 Script executed:
# Let me verify the FLB_SDS_HEADER macro to confirm NULL dereference behavior
rg "define FLB_SDS_HEADER" include/fluent-bit/flb_sds.hRepository: fluent/fluent-bit
Length of output: 200
🏁 Script executed:
# Check if flb_sds_cat does any NULL checking on the input parameter
sed -n '120,145p' src/flb_sds.cRepository: fluent/fluent-bit
Length of output: 601
🏁 Script executed:
# Let me check what happens if NULL is passed to flb_sds_cat
# by looking at the macro expansion and understanding the flow
rg -B 5 -A 10 "flb_sds_len" include/fluent-bit/flb_sds.h | head -30Repository: fluent/fluent-bit
Length of output: 636
🏁 Script executed:
# Let's trace what happens if NULL is passed to flb_sds_cat
# by creating a simple test case
python3 << 'EOF'
# Simulating the macro expansion:
# FLB_SDS_HEADER(s) = (s - FLB_SDS_HEADER_SIZE)
# If s is NULL:
s = None
FLB_SDS_HEADER_SIZE = 16 # sizeof(uint64_t) + sizeof(uint64_t)
try:
if s is None:
# This is what happens: None - constant causes error in C
result = s - FLB_SDS_HEADER_SIZE
print(f"Result: {result}")
except TypeError as e:
print(f"Error in C would be: undefined behavior with NULL pointer arithmetic")
print(f"In the code: flb_sds_avail(NULL) -> FLB_SDS_HEADER(NULL) -> (NULL - 16)")
print(f"This causes a segfault or crash")
EOFRepository: fluent/fluent-bit
Length of output: 233
🏁 Script executed:
# Let's verify by checking if there's any NULL check in flb_sds_avail or flb_sds_cat
grep -n "if.*(!s)" src/flb_sds.c | head -10Repository: fluent/fluent-bit
Length of output: 113
🏁 Script executed:
# Let me also check if flb_sds_cat_safe is indeed safer
sed -n '204,220p' src/flb_sds.cRepository: fluent/fluent-bit
Length of output: 436
Check each flb_sds_cat() append before reusing body.
If any append fails, body becomes NULL and the next flb_sds_cat() call will crash. The flb_sds_cat() function dereferences the input pointer immediately via FLB_SDS_HEADER(s) without NULL checking. Use a temporary variable and bail out after each append instead of chaining all writes into the same variable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/flb_azure_auth.c` around lines 183 - 191, The sequence of flb_sds_cat()
calls can return NULL on failure and subsequent calls will dereference body and
crash; modify the block that builds the token request body (the uses of
flb_sds_cat with variables body, client_id, federated_token, resource) to check
the return value after each flb_sds_cat call: assign the result to a temporary
sds (e.g., tmp), verify tmp is non-NULL, then set body = tmp; if any append
fails, free any allocated body and return/error out immediately. Ensure every
flb_sds_cat invocation (including the "&grant_type...",
"&client_assertion_type...", "&client_assertion=", "&scope=", and ".default"
appends) follows this pattern to avoid NULL dereference.
20c9ffe to
fc1b293
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/flb_azure_auth.c (1)
178-186:⚠️ Potential issue | 🟠 MajorGuard every
flb_sds_cat()append.
flb_sds_cat()can returnNULL. Overwritingbodydirectly means one failed append loses the original buffer and the next append dereferencesNULL. Please stage each append through a temporary SDS and bail out immediately on failure.Suggested fix
+ flb_sds_t tmp; + - body = flb_sds_cat(body, "client_id=", 10); - body = flb_sds_cat(body, client_id, strlen(client_id)); - body = flb_sds_cat(body, "&grant_type=client_credentials", 30); - body = flb_sds_cat(body, "&client_assertion_type=urn:ietf:params:oauth:client-assertion-type:jwt-bearer", 77); - body = flb_sds_cat(body, "&client_assertion=", 18); - body = flb_sds_cat(body, federated_token, flb_sds_len(federated_token)); - body = flb_sds_cat(body, "&scope=", 7); - body = flb_sds_cat(body, resource, strlen(resource)); + tmp = flb_sds_cat(body, "client_id=", 10); + if (!tmp) { goto body_error; } + body = tmp; + + tmp = flb_sds_cat(body, client_id, strlen(client_id)); + if (!tmp) { goto body_error; } + body = tmp; + + tmp = flb_sds_cat(body, "&grant_type=client_credentials", 30); + if (!tmp) { goto body_error; } + body = tmp; + + tmp = flb_sds_cat(body, + "&client_assertion_type=urn:ietf:params:oauth:client-assertion-type:jwt-bearer", + 77); + if (!tmp) { goto body_error; } + body = tmp; + + tmp = flb_sds_cat(body, "&client_assertion=", 18); + if (!tmp) { goto body_error; } + body = tmp; + + tmp = flb_sds_cat(body, federated_token, flb_sds_len(federated_token)); + if (!tmp) { goto body_error; } + body = tmp; + + tmp = flb_sds_cat(body, "&scope=", 7); + if (!tmp) { goto body_error; } + body = tmp; + + tmp = flb_sds_cat(body, resource, strlen(resource)); + if (!tmp) { goto body_error; } + body = tmp; + + goto body_ready; + +body_error: + flb_error("[azure workload identity] failed to build request body"); + flb_sds_destroy(federated_token); + flb_sds_destroy(body); + return -1; + +body_ready:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flb_azure_auth.c` around lines 178 - 186, Guard every flb_sds_cat() call by assigning its return to a temporary SDS, checking for NULL, and bailing out immediately on failure instead of overwriting body; e.g., for each append to body (the series that uses client_id, federated_token, resource and the literal strings in src/flb_azure_auth.c) do tmp = flb_sds_cat(body, ...); if (!tmp) { flb_sds_destroy(body); return error/NULL; } else body = tmp; apply this pattern for all flb_sds_cat() calls so a failed append does not lose or dereference the original buffer.plugins/out_azure_blob/azure_blob_http.c (1)
75-77:⚠️ Potential issue | 🔴 CriticalBlob service-principal auth is sending the wrong OAuth scope.
This branch uses the v2 token endpoint but sends
FLB_AZURE_BLOB_RESOURCE_SCOPEas thescopevalue. Because the same constant is also used as the managed-identity IMDSresource, it has to be the bare resource URI, not.../.default; Microsoft’s v2 client-credentials flow requiresscope={resource}/.default. The blob service-principal token request will therefore target the wrong scope. (learn.microsoft.com)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/out_azure_blob/azure_blob_http.c` around lines 75 - 77, The OAuth scope for service-principal (v2) token requests is wrong: the code calls flb_oauth2_payload_append(ctx->o, "scope", 5, FLB_AZURE_BLOB_RESOURCE_SCOPE, sizeof(FLB_AZURE_BLOB_RESOURCE_SCOPE) - 1) but FLB_AZURE_BLOB_RESOURCE_SCOPE is a bare resource URI (used as IMDS resource) and v2 client-credentials requires "{resource}/.default". Change the payload value passed to flb_oauth2_payload_append (in the service-principal/v2 branch) to a string that appends "/.default" to FLB_AZURE_BLOB_RESOURCE_SCOPE and pass the correct length (e.g., strlen(resource) + sizeof("/.default") - 1), keeping the same ctx->o and flb_oauth2_payload_append call site.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/out_azure_blob/azure_blob_conf.c`:
- Around line 603-643: The branches that set ctx->atype to
FLB_AZURE_AUTH_SERVICE_PRINCIPAL, FLB_AZURE_AUTH_MANAGED_IDENTITY_* or
FLB_AZURE_AUTH_WORKLOAD_IDENTITY must reject non-HTTPS blob endpoints; before
accepting those auth types (inside the service_principal, managed_identity and
workload_identity blocks) check that TLS is actually used (e.g. ctx->use_tls is
true or ctx->endpoint starts with "https://") and if not call flb_plg_error and
return NULL. Update the checks in the service_principal, managed_identity and
workload_identity branches (the code that sets ctx->atype and validates
tenant_id/client_id/client_secret/workload_identity_token_file) to perform this
TLS/HTTPS validation early and fail-fast using existing logging (flb_plg_error)
so bearer tokens are never attached to plain HTTP endpoints.
- Around line 593-600: The code mixes two enum families: comparisons and
switches use AZURE_BLOB_AUTH_* while ctx->atype is set to FLB_AZURE_AUTH_*,
which will break when auth types change; replace every use of
AZURE_BLOB_AUTH_KEY and AZURE_BLOB_AUTH_SAS (and any other AZURE_BLOB_AUTH_*
constants) with the corresponding FLB_AZURE_AUTH_* symbols so all checks against
ctx->atype use the same enum family (update checks in functions/units that
reference these symbols — e.g., the parsing logic where ctx->atype is set, the
auth checks in azure_blob_uri, and usage sites in azure_blob_appendblob and
azure_blob_blockblob), then rebuild to ensure no remaining AZURE_BLOB_AUTH_*
references remain.
In `@plugins/out_azure_blob/azure_blob_http.c`:
- Around line 464-467: azb_http_client_setup() can fail (returns -1) and callers
must abort instead of proceeding to flb_http_do(); update each call site that
currently ignores the return (the three places highlighted in azure_blob.c) to
check the result and return/propagate an error immediately when
azb_http_client_setup() returns -1 so the request is not sent unauthenticated.
Ensure you reference the azb_http_client_setup return value before calling
flb_http_do and return a non-zero/failure code from the enclosing function on
failure.
In `@src/flb_azure_auth.c`:
- Around line 77-84: The code currently logs the token endpoint response body
(c->resp.payload) via flb_debug/flb_info which leaks bearer tokens; change the
handling in src/flb_azure_auth.c so you never write the raw payload to logs. For
successful (c->resp.status == 200) and error branches, replace the payload dump
with a redacted message (e.g. "[redacted payload]") and instead parse the JSON
payload to extract and log only non-sensitive metadata such as expires_in /
expiry timestamp and token_type (use c->resp.payload and c->resp.payload_size
for parsing but do not log the raw value). Ensure flb_debug/flb_info calls
reference c->resp.status and the derived expiry/token_type fields, and remove
any direct printing of c->resp.payload.
---
Duplicate comments:
In `@plugins/out_azure_blob/azure_blob_http.c`:
- Around line 75-77: The OAuth scope for service-principal (v2) token requests
is wrong: the code calls flb_oauth2_payload_append(ctx->o, "scope", 5,
FLB_AZURE_BLOB_RESOURCE_SCOPE, sizeof(FLB_AZURE_BLOB_RESOURCE_SCOPE) - 1) but
FLB_AZURE_BLOB_RESOURCE_SCOPE is a bare resource URI (used as IMDS resource) and
v2 client-credentials requires "{resource}/.default". Change the payload value
passed to flb_oauth2_payload_append (in the service-principal/v2 branch) to a
string that appends "/.default" to FLB_AZURE_BLOB_RESOURCE_SCOPE and pass the
correct length (e.g., strlen(resource) + sizeof("/.default") - 1), keeping the
same ctx->o and flb_oauth2_payload_append call site.
In `@src/flb_azure_auth.c`:
- Around line 178-186: Guard every flb_sds_cat() call by assigning its return to
a temporary SDS, checking for NULL, and bailing out immediately on failure
instead of overwriting body; e.g., for each append to body (the series that uses
client_id, federated_token, resource and the literal strings in
src/flb_azure_auth.c) do tmp = flb_sds_cat(body, ...); if (!tmp) {
flb_sds_destroy(body); return error/NULL; } else body = tmp; apply this pattern
for all flb_sds_cat() calls so a failed append does not lose or dereference the
original buffer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1f22285-76a9-4fd7-b004-0c050026876c
📒 Files selected for processing (13)
include/fluent-bit/flb_azure_auth.hplugins/out_azure_blob/azure_blob.cplugins/out_azure_blob/azure_blob.hplugins/out_azure_blob/azure_blob_conf.cplugins/out_azure_blob/azure_blob_http.cplugins/out_azure_kusto/CMakeLists.txtplugins/out_azure_kusto/azure_kusto.cplugins/out_azure_kusto/azure_kusto.hplugins/out_azure_kusto/azure_kusto_conf.csrc/CMakeLists.txtsrc/flb_azure_auth.ctests/runtime/CMakeLists.txttests/runtime/out_azure_blob.c
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/runtime/CMakeLists.txt
- include/fluent-bit/flb_azure_auth.h
- tests/runtime/out_azure_blob.c
- plugins/out_azure_kusto/CMakeLists.txt
- plugins/out_azure_blob/azure_blob.h
zshuang0316
force-pushed
the
azure-msi-auth
branch
from
fc1b293 to
fa66b22
Compare
zshuang0316
force-pushed
the
azure-msi-auth
branch
from
b320747 to
b0686bb
Compare
Contributor
Author
|
Hello @cosmo0920 , we’d like to leverage AAD authentication to access Azure Blob Storage. Since there’s already similar auth logic in the Azure Kusto code, it might make sense to extract the authentication into a common component that both can reuse. What do you think? If that’s not preferred, I can limit the change to the Azure Blob plugin only. Please let me know—thanks! |
zshuang0316
force-pushed
the
azure-msi-auth
branch
from
b0686bb to
95895e3
Compare
zshuang0316
force-pushed
the
azure-msi-auth
branch
from
95895e3 to
9be14fa
Compare
cosmo0920
requested changes
Mar 18, 2026
Add centralized Azure authentication code for Managed Identity (MSI), Workload Identity, and Service Principal authentication. This provides shared authentication functionality for Azure-related output plugins. Features: - Support for system-assigned and user-assigned managed identities - Workload identity authentication with federated tokens - Service principal authentication with client credentials - Dynamic OAuth URL builder for different auth types and resources - Configurable resource scopes for different Azure services Signed-off-by: zshuang0316 <zshuang0316@163.com>
Integrate common Azure authentication module to support: - Managed Identity (system-assigned and user-assigned) - Workload Identity with federated tokens - Service Principal with client credentials This extends the existing shared key and SAS token authentication methods and maintains full backward compatibility. New configuration parameters: - tenant_id: Azure AD tenant ID - client_id: Azure AD client ID or 'system' for system-assigned MI - client_secret: Client secret for service principal auth - workload_identity_token_file: Token file path for workload identity Signed-off-by: zshuang0316 <zshuang0316@163.com>
Refactor Azure Kusto plugin to use the centralized Azure authentication module instead of local azure_msiauth implementation. Changes: - Remove azure_msiauth.c from build (deprecated) - Use common flb_azure_auth functions for MSI and workload identity - Replace local auth enum with common flb_azure_auth_type - Use flb_azure_auth_build_oauth_url for OAuth URL construction - Pass Kusto-specific resource scope to auth functions This unifies authentication behavior across all Azure output plugins while maintaining full backward compatibility. Signed-off-by: zshuang0316 <zshuang0316@163.com>
Add configuration validation tests for all Azure Blob authentication methods: - System-assigned managed identity - User-assigned managed identity - Service principal - Workload identity - Shared key (existing) - SAS token (existing) Tests verify configuration parsing and plugin initialization without requiring actual Azure connectivity or credentials. Signed-off-by: zshuang0316 <zshuang0316@163.com>
zshuang0316
force-pushed
the
azure-msi-auth
branch
from
9be14fa to
710e330
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds centralized Azure authentication support for Fluent Bit's Azure output plugins, enabling Managed Identity and Workload Identity authentication methods.
Changes
Support for system-assigned and user-assigned Managed Identities
Workload Identity authentication with federated tokens
Service Principal authentication with client credentials
Flexible resource scope configuration for different Azure services
Integrated OAuth authentication alongside existing shared key and SAS token methods
Full backward compatibility maintained
Migrated from local azure_msiauth to common authentication module
Unified authentication behavior across Azure plugins
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Chores
Tests