oauth2: generic client and server side auth implementation (WIP) #11286
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds OAuth2 client and OAuth2‑JWT validation (JWKS fetch/cache, JWT parsing, RSA verification), integrates OAuth2 into HTTP client with retry-on-401, extends crypto API for RSA-from-components verification, wires config-map and properties for input/output plugins, and adds tests and build entries. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant InHTTP as in_http Plugin
participant JWTctx as flb_oauth2_jwt_ctx
participant JWKS as JWKS Server
participant Crypto as flb_crypto
participant OutHTTP as flb_http_client
participant OAuth2 as flb_oauth2
Note over Client,InHTTP: Incoming request validation
Client->>InHTTP: HTTP request (Authorization: Bearer ...)
InHTTP->>JWTctx: flb_oauth2_jwt_validate(Authorization)
JWTctx->>JWTctx: parse token, extract kid/alg/claims
JWTctx->>JWTctx: check JWKS cache for kid
alt JWKS miss/stale
JWTctx->>JWKS: GET JWKS URL
JWKS-->>JWTctx: JWKS JSON
JWTctx->>JWTctx: parse & cache keys
end
JWTctx->>Crypto: build EVP_PKEY from modulus/exponent
Crypto-->>JWTctx: verification result
JWTctx-->>InHTTP: VALID / INVALID
alt VALID
InHTTP->>Client: 201 / process request
else INVALID
InHTTP->>Client: 401 Unauthorized
end
Note over OutHTTP,OAuth2: Outbound request with OAuth2
OutHTTP->>OAuth2: request token (if needed)
OAuth2-->>OutHTTP: access_token + expires_in
OutHTTP->>OutHTTP: cache token (expires_at)
OutHTTP->>OutHTTP: send request with Authorization: Bearer
alt Upstream 401
OutHTTP->>OAuth2: invalidate/refresh token
OAuth2-->>OutHTTP: new token
OutHTTP->>OutHTTP: retry request with refreshed token
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
…ation This commit introduces a new OAuth2 JWT validation interface that allows Fluent Bit to validate incoming JWT tokens on the server side. The interface supports: - JWT parsing and validation - JWKS (JSON Web Key Set) fetching and caching - RSA signature verification using flb_crypto abstraction - Configurable validation rules (issuer, audience, client ID) Signed-off-by: Eduardo Silva <[email protected]>
This commit extends the flb_crypto abstraction layer to support RSA signature verification operations. The new functions handle OpenSSL 1.1.1 and 3.x compatibility internally, providing a unified API for cryptographic operations. New functions: - flb_crypto_build_rsa_public_key_from_components() - flb_crypto_init_from_rsa_components() - flb_crypto_verify() - flb_crypto_verify_simple() The implementation uses a hybrid approach for OpenSSL 3.x compatibility while maintaining functionality with OpenSSL 1.1.1. Signed-off-by: Eduardo Silva <[email protected]>
This commit adds OAuth2 JWT validation to the HTTP input plugin, allowing Fluent Bit to validate incoming bearer tokens. The implementation uses an independent config map pattern similar to net.* properties. Configuration properties: - oauth2.validate: enable/disable validation - oauth2.issuer: expected issuer claim - oauth2.jwks_url: JWKS endpoint URL - oauth2.allowed_audience: required audience claim - oauth2.allowed_clients: list of authorized client IDs - oauth2.jwks_refresh_interval: JWKS cache refresh interval The validation is performed lazily (JWKS is fetched on first use) to avoid blocking plugin initialization. Signed-off-by: Eduardo Silva <[email protected]>
This commit adds OAuth2 client credentials grant flow to the HTTP output plugin, allowing Fluent Bit to obtain and use OAuth2 access tokens for outgoing requests. The implementation uses an independent config map pattern similar to net.* properties. Configuration properties: - oauth2.enable: enable/disable OAuth2 - oauth2.token_url: OAuth2 token endpoint - oauth2.client_id: OAuth2 client ID - oauth2.client_secret: OAuth2 client secret - oauth2.scope: optional OAuth2 scope - oauth2.auth_method: authentication method (basic or post) - oauth2.refresh_skew_seconds: token refresh skew The implementation includes automatic token refresh on 401 responses and proper connection handling for retries. Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/flb_oauth2.c (1)
909-932: Consider adding lock protection for token_len and token_expired.These query functions read shared state (
access_token,expires_at) without lock protection, which could lead to race conditions during concurrent token refresh operations. On 32-bit systems, readingexpires_at(likely 64-bittime_t) may tear. While the impact is minor (stale reads), adding lock protection would ensure consistency.🔎 Proposed fix to add lock protection
int flb_oauth2_token_len(struct flb_oauth2 *ctx) { + int ret; + int len; + + ret = flb_lock_acquire(&ctx->lock, + FLB_LOCK_DEFAULT_RETRY_LIMIT, + FLB_LOCK_DEFAULT_RETRY_DELAY); + if (ret != 0) { + return -1; + } + if (!ctx->access_token) { + flb_lock_release(&ctx->lock, + FLB_LOCK_DEFAULT_RETRY_LIMIT, + FLB_LOCK_DEFAULT_RETRY_DELAY); return -1; } - return flb_sds_len(ctx->access_token); + len = flb_sds_len(ctx->access_token); + + flb_lock_release(&ctx->lock, + FLB_LOCK_DEFAULT_RETRY_LIMIT, + FLB_LOCK_DEFAULT_RETRY_DELAY); + + return len; } int flb_oauth2_token_expired(struct flb_oauth2 *ctx) { + int ret; + int expired; time_t now; + ret = flb_lock_acquire(&ctx->lock, + FLB_LOCK_DEFAULT_RETRY_LIMIT, + FLB_LOCK_DEFAULT_RETRY_DELAY); + if (ret != 0) { + return FLB_TRUE; + } + if (!ctx->access_token) { + flb_lock_release(&ctx->lock, + FLB_LOCK_DEFAULT_RETRY_LIMIT, + FLB_LOCK_DEFAULT_RETRY_DELAY); return FLB_TRUE; } now = time(NULL); - if (ctx->expires_at <= now) { - return FLB_TRUE; + expired = (ctx->expires_at <= now) ? FLB_TRUE : FLB_FALSE; + + flb_lock_release(&ctx->lock, + FLB_LOCK_DEFAULT_RETRY_LIMIT, + FLB_LOCK_DEFAULT_RETRY_DELAY); + + return expired; - } - - return FLB_FALSE; }
🧹 Nitpick comments (2)
src/flb_crypto.c (1)
156-264: Clarify the comment about encoding.The comment at line 156 says "base64url encoded", but the function signature accepts raw bytes (
unsigned char *modulus_bytes,unsigned char *exponent_bytes). The function expects decoded bytes, not base64url strings.Update the comment to clarify that these are raw bytes (e.g., "Build RSA public key from modulus and exponent (raw bytes, typically decoded from base64url JWK)").
🔎 Proposed comment fix
-/* Build RSA public key from modulus and exponent (base64url encoded) */ +/* Build RSA public key from modulus and exponent (raw bytes) */ static int flb_crypto_build_rsa_public_key_from_components(unsigned char *modulus_bytes,
Memory management is correct.
The ownership transfers are properly handled in both OpenSSL 1.x and 3.x paths, and the fix for the OpenSSL 3.x memory leak (storing duplicates in temporaries and freeing on RSA_set0_key failure) is confirmed as correct.
src/flb_oauth2.c (1)
225-315: Consider deferring TLS context creation to HTTPS-only paths.The function creates a TLS context at line 275 regardless of protocol, but only uses it for HTTPS at line 290. For HTTP connections (line 293), the TLS context remains unused yet allocated. This is a minor resource inefficiency.
🔎 Proposed refactor to conditionally create TLS
- ctx->tls = flb_tls_create(FLB_TLS_CLIENT_MODE, - FLB_TRUE, - -1, - NULL, - NULL, - NULL, - NULL, - NULL, - NULL); - if (!ctx->tls) { - flb_error("[oauth2] error initializing TLS context"); - goto error; - } - if (strcmp(prot, "https") == 0) { + ctx->tls = flb_tls_create(FLB_TLS_CLIENT_MODE, + FLB_TRUE, + -1, + NULL, + NULL, + NULL, + NULL, + NULL, + NULL); + if (!ctx->tls) { + flb_error("[oauth2] error initializing TLS context"); + goto error; + } ctx->u = flb_upstream_create_url(config, auth_url, FLB_IO_TLS, ctx->tls); } else { ctx->u = flb_upstream_create_url(config, auth_url, FLB_IO_TCP, NULL); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/flb_crypto.c(3 hunks)src/flb_oauth2.c(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T18:45:02.283Z
Learnt from: edsiper
Repo: fluent/fluent-bit PR: 11286
File: src/flb_http_client.c:1657-1715
Timestamp: 2025-12-17T18:45:02.283Z
Learning: In the Fluent Bit OAuth2 implementation (src/flb_oauth2.c), the function flb_oauth2_get_access_token returns a pointer to the cached token stored in the OAuth2 context (ctx->access_token), not a new allocation. The OAuth2 context manages the token lifecycle, so callers must not free the returned token pointer.
Applied to files:
src/flb_oauth2.c
🧬 Code graph analysis (1)
src/flb_oauth2.c (3)
src/flb_sds.c (6)
flb_sds_destroy(389-399)flb_sds_create(78-90)flb_sds_create_len(58-76)flb_sds_cat(120-141)flb_sds_increase(98-118)flb_sds_cat_safe(204-214)src/flb_lock.c (4)
flb_lock_init(6-17)flb_lock_destroy(19-30)flb_lock_acquire(32-65)flb_lock_release(67-100)src/flb_config_map.c (1)
flb_config_map_create(252-361)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (31)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COMPILER_STRICT_POINTER_TYPES=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: PR - fuzzing test
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-without-cxx (3.31.6)
- GitHub Check: pr-compile-centos-7
🔇 Additional comments (26)
src/flb_crypto.c (5)
20-23: LGTM!The new OpenSSL headers are necessary for RSA key construction from modulus/exponent components and version detection.
26-62: LGTM!The OpenSSL 1.0.2 compatibility layer is well-implemented:
- EVP_MD_CTX macros correctly map to create/destroy equivalents
- RSA_set0_key shim properly handles ownership transfer semantics (frees old values, assigns new values, returns 1 on success)
- Direct RSA field access is valid for the opaque-structure-free OpenSSL 1.0.x
556-603: LGTM!The new
flb_crypto_init_from_rsa_componentsAPI is well-structured, mirrors the existingflb_crypto_initpattern, and properly handles all error paths with cleanup.
670-701: LGTM!The
flb_crypto_verify_simpleconvenience wrapper follows the established pattern of other_simplefunctions in this module and correctly handles initialization, verification, and cleanup.
643-656: Verify EVP_DigestVerify availability in OpenSSL 1.1.0.The version check at line 643 assumes
EVP_DigestVerifyis available in OpenSSL 1.1.0+ (>= 0x10100000L). Confirm whether this function was introduced in 1.1.0 or 1.1.1, and adjust the ifdef if needed (e.g.,>= 0x10101000Lfor 1.1.1).src/flb_oauth2.c (21)
34-84: LGTM! OAuth2 config map is well-structured.The configuration map properly defines all OAuth2 parameters with appropriate types, offsets, and descriptions. The timeout defaults correctly indicate fallback behavior.
100-107: LGTM! Clean key comparison helper.The inline function correctly validates length before comparing, preventing out-of-bounds access.
109-123: LGTM! State reset is properly implemented.The function safely destroys existing tokens and resets expiry fields, preventing use-after-free issues.
125-138: LGTM! Defaults are correctly initialized.The function properly initializes all fields, including explicit NULL initialization for pointers to prevent use of uninitialized memory.
140-205: LGTM! Config cloning with proper error cleanup.The function correctly handles allocation failures by calling
flb_oauth2_config_destroy(dst)on each error path, preventing the memory leak identified in previous reviews.
207-223: LGTM! Config cleanup is thorough.The function safely destroys all allocated strings and nullifies pointers, ensuring clean resource release.
413-474: LGTM! Key-value appending with robust error handling.The function correctly checks all
flb_sds_catreturn values and destroys the buffer on failure, addressing the memory leak concern from previous reviews. URL encoding and error paths are properly handled.
476-536: LGTM! Request body building with proper cleanup.The function correctly handles allocation failures by destroying the body buffer on each error path, resolving the memory leak identified in previous reviews. The manual payload fallback is also handled correctly.
538-612: LGTM! HTTP request handling is robust.The function correctly manages the upstream connection lifecycle, handles both TCP and TLS protocols, conditionally applies timeouts, and properly cleans up resources on all paths. Basic authentication is correctly applied only when configured.
614-629: LGTM! Token refresh wrapper is clean.The function correctly builds the request body, executes the HTTP request, and ensures cleanup regardless of outcome.
631-654: LGTM! Token refresh logic is correct.The function properly evaluates all conditions requiring token refresh, including forced refresh, missing token, zero expiry, and time-based expiry with skew.
656-673: LGTM! Convenience constructor is correctly implemented.The function properly creates a temporary config, delegates to the main constructor, and cleans up the temporary config. The unused
expire_secparameter is appropriately marked.
675-733: LGTM! Main constructor with comprehensive initialization.The function properly initializes the OAuth2 context, validates required configuration (token_url), initializes synchronization primitives, and uses consistent error handling by calling
flb_oauth2_destroyon all failure paths.
735-761: LGTM! Destructor thoroughly cleans up all resources.The function systematically destroys all allocated resources including tokens, strings, TLS context, upstream, config, and synchronization primitives.
763-773: LGTM! Payload clearing is correctly implemented.The function safely clears the payload buffer, sets the manual flag, and resets token state to ensure consistency.
775-827: LGTM! Payload appending uses safe string operations.The function correctly uses
flb_sds_cat_safefor all append operations, addressing the unchecked return value concern from previous reviews. Buffer growth and reallocation are properly handled.
829-845: LGTM! Token retrieval logic is sound.The function correctly evaluates refresh necessity, refreshes if needed, and returns the cached token pointer as documented in the learnings.
847-871: LGTM! Thread-safe token access is correctly implemented.The function properly guards token retrieval with mutex acquisition and release, ensuring thread-safe access to the shared token state.
873-889: LGTM! Token getter functions are straightforward.The functions provide a simple interface to retrieve the cached access token without forcing refresh, consistent with the documented token lifecycle management.
891-907: LGTM! Token invalidation with lock protection.The function safely invalidates the token by resetting its expiry timestamp under lock protection. The silent return on lock acquisition failure appears intentional for a best-effort invalidation operation.
934-945: LGTM! Config map creation with NULL check.The function correctly validates the return value from
flb_config_map_createand logs an appropriate error, addressing the concern from previous reviews.
| if (!token_type) { | ||
| token_type = flb_sds_create("Bearer"); | ||
| flb_debug("[oauth2] token_type missing; defaulting to Bearer"); | ||
| } |
There was a problem hiding this comment.
Check allocation failure when defaulting token_type to "Bearer".
If flb_sds_create("Bearer") fails at line 393, token_type remains NULL, but the code continues and sets ctx->token_type = NULL at line 406. While allocation failure for a small static string is rare, this could cause issues if downstream code assumes a valid token_type exists.
🔎 Proposed fix to handle allocation failure
if (!token_type) {
token_type = flb_sds_create("Bearer");
+ if (!token_type) {
+ flb_errno();
+ oauth2_reset_state(ctx);
+ return -1;
+ }
flb_debug("[oauth2] token_type missing; defaulting to Bearer");
}🤖 Prompt for AI Agents
In src/flb_oauth2.c around lines 392-395, the code defaults token_type to
flb_sds_create("Bearer") but does not check for allocation failure; if
flb_sds_create returns NULL the code later assigns a NULL token_type into ctx.
Fix by checking the return of flb_sds_create: if it returns NULL log an error
about allocation failure and handle it deterministically (either return an
error/cleanup from the current function or fall back to a safe non-allocating
option such as using a pointer to a static "Bearer" constant and a flag
indicating it is not an sds), ensuring downstream code never receives a NULL
token_type.
Signed-off-by: Eduardo Silva <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/internal/oauth2.c (2)
52-58: Consider documenting buffer size assumption.The
snprintfwill safely truncate if the formatted response exceedsMOCK_BODY_SIZE(1024 bytes). For test payloads, this is adequate, but consider adding a comment or assertion to make the assumption explicit.
367-367: Consider reducing test duration.The
sleep(2)makes this test take over 2 seconds to run. For faster test execution, consider using shorter expiry times (e.g.,expires_in=1, refresh_skew=0withsleep(1)) while still validating the refresh behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/internal/oauth2.c(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T18:45:02.283Z
Learnt from: edsiper
Repo: fluent/fluent-bit PR: 11286
File: src/flb_http_client.c:1657-1715
Timestamp: 2025-12-17T18:45:02.283Z
Learning: In the Fluent Bit OAuth2 implementation (src/flb_oauth2.c), the function flb_oauth2_get_access_token returns a pointer to the cached token stored in the OAuth2 context (ctx->access_token), not a new allocation. The OAuth2 context manages the token lifecycle, so callers must not free the returned token pointer.
Applied to files:
tests/internal/oauth2.c
🧬 Code graph analysis (1)
tests/internal/oauth2.c (3)
src/flb_sds.c (2)
flb_sds_create_size(92-95)flb_sds_create(78-90)src/flb_oauth2.c (4)
flb_oauth2_create_from_config(675-733)flb_oauth2_parse_json_response(317-411)flb_oauth2_get_access_token(847-871)flb_oauth2_destroy(735-761)src/flb_config.c (1)
flb_config_init(233-486)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (31)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COMPILER_STRICT_POINTER_TYPES=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: PR - fuzzing test
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-centos-7
- GitHub Check: pr-compile-without-cxx (3.31.6)
🔇 Additional comments (8)
tests/internal/oauth2.c (8)
101-104: Simple authorization check is adequate for test code.The
strstrapproach will matchlatest_tokenanywhere in the request after theAuthorization:header, which could theoretically produce false positives. For a mock test server, this is acceptable.
144-158: Blocking recv is acceptable for test server.The socket is set to blocking mode, and
recvis called in a bounded loop. If a client sends incomplete data,recvcould block, but the loop has proper bounds and break conditions. For a test server with controlled clients, this is acceptable.
173-232: Well-structured server startup with proper error handling.The server binds to loopback with dynamic port assignment, which is ideal for test isolation. Error paths properly clean up resources.
234-277: Excellent defensive programming for test reliability.The platform-specific readiness check prevents race conditions on macOS where the server thread may not have entered its
select()loop immediately afterpthread_createreturns. This is good practice for cross-platform test reliability.
279-287: Proper server shutdown sequence.The use of
shutdown()to wake blocking calls followed bypthread_joinensures clean thread termination. Since the server thread checks the stop flag every 200ms,pthread_joinshould complete quickly.
289-310: Helper function properly manages resources.The OAuth2 config is correctly initialized, used to create the context, and then destroyed. Since
flb_oauth2_create_from_configclones the config internally, this is safe.
312-329: Unit test correctly validates default value handling.The test verifies that
token_typedefaults to "Bearer" andexpires_indefaults to the constant when not present in the JSON response. Cleanup is properly handled.
331-377: Integration test thoroughly validates OAuth2 token lifecycle.The test correctly verifies:
- Initial token fetch from server
- Cached token reuse
- Token refresh after expiry
The cleanup sequence properly destroys resources in the correct order.
Signed-off-by: Eduardo Silva <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/internal/oauth2.c (2)
44-73: Consider bounds checking and error handling improvements.The function has two minor robustness concerns:
Buffer overflow risk: If
bodyis very large or malformed,snprintfmay truncate but the calculation at line 56-62 doesn't verify the full response fits inbuffer. Whilesnprintfis safe from overruns, the response might be incomplete.Silent send failures: When
send()fails at line 68, the function silently breaks without logging. For a test mock server this is acceptable, but consider logging errors to aid debugging if send failures occur.🔎 Suggested improvements
static void compose_http_response(flb_sockfd_t fd, int status, const char *body) { char buffer[MOCK_BODY_SIZE]; int body_len = 0; ssize_t sent = 0; ssize_t total = 0; ssize_t len; if (body != NULL) { body_len = strlen(body); + /* Ensure body + headers fit in buffer */ + if (body_len > MOCK_BODY_SIZE - 200) { + printf("Warning: response body too large, truncating\n"); + } } snprintf(buffer, sizeof(buffer), "HTTP/1.1 %d\r\n" "Content-Length: %d\r\n" "Content-Type: application/json\r\n" "Connection: close\r\n\r\n" "%s", status, body_len, body ? body : ""); len = strlen(buffer); /* Ensure we send all data - loop until complete */ while (total < len) { sent = send(fd, buffer + total, len - total, 0); if (sent <= 0) { + printf("Warning: send() failed: %d\n", errno); break; } total += sent; } }
177-287: Refactor duplicated Windows cleanup code.The Windows
WSACleanupcleanup block is duplicated six times across the error paths (lines 205-210, 224-229, 236-241, 250-255, 263-268, 277-282). This increases maintenance burden.🔎 Refactor using goto for cleanup
static int oauth2_mock_server_start(struct oauth2_mock_server *server, int expires_in, int resource_challenge) { int on = 1; struct sockaddr_in addr; socklen_t len; #ifdef _WIN32 WSADATA wsa_data; int wsa_result; #endif memset(server, 0, sizeof(struct oauth2_mock_server)); server->expires_in = expires_in; server->resource_challenge = resource_challenge; #ifdef _WIN32 /* Initialize Winsock on Windows */ wsa_result = WSAStartup(MAKEWORD(2, 2), &wsa_data); if (wsa_result != 0) { flb_errno(); return -1; } server->wsa_initialized = 1; #endif server->listen_fd = socket(AF_INET, SOCK_STREAM, 0); if (server->listen_fd == FLB_INVALID_SOCKET) { flb_errno(); -#ifdef _WIN32 - if (server->wsa_initialized) { - WSACleanup(); - server->wsa_initialized = 0; - } -#endif - return -1; + goto cleanup; } setsockopt(server->listen_fd, SOL_SOCKET, SO_REUSEADDR, (const char *)&on, sizeof(on)); memset(&addr, 0, sizeof(addr)); addr.sin_family = AF_INET; addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK); addr.sin_port = 0; if (bind(server->listen_fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) { flb_errno(); flb_socket_close(server->listen_fd); -#ifdef _WIN32 - if (server->wsa_initialized) { - WSACleanup(); - server->wsa_initialized = 0; - } -#endif - return -1; + goto cleanup; } if (listen(server->listen_fd, 4) < 0) { flb_errno(); flb_socket_close(server->listen_fd); -#ifdef _WIN32 - if (server->wsa_initialized) { - WSACleanup(); - server->wsa_initialized = 0; - } -#endif - return -1; + goto cleanup; } len = sizeof(addr); memset(&addr, 0, sizeof(addr)); if (getsockname(server->listen_fd, (struct sockaddr *) &addr, &len) < 0) { flb_errno(); flb_socket_close(server->listen_fd); -#ifdef _WIN32 - if (server->wsa_initialized) { - WSACleanup(); - server->wsa_initialized = 0; - } -#endif - return -1; + goto cleanup; } server->port = ntohs(addr.sin_port); if (server->port == 0) { flb_errno(); flb_socket_close(server->listen_fd); -#ifdef _WIN32 - if (server->wsa_initialized) { - WSACleanup(); - server->wsa_initialized = 0; - } -#endif - return -1; + goto cleanup; } flb_net_socket_nonblocking(server->listen_fd); if (pthread_create(&server->thread, NULL, oauth2_mock_server_thread, server) != 0) { printf("pthread_create failed: %s\n", strerror(errno)); flb_socket_close(server->listen_fd); -#ifdef _WIN32 - if (server->wsa_initialized) { - WSACleanup(); - server->wsa_initialized = 0; - } -#endif - return -1; + goto cleanup; } printf("server started on port %d\n", server->port); return 0; + +cleanup: +#ifdef _WIN32 + if (server->wsa_initialized) { + WSACleanup(); + server->wsa_initialized = 0; + } +#endif + return -1; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/internal/oauth2.c
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T18:45:02.283Z
Learnt from: edsiper
Repo: fluent/fluent-bit PR: 11286
File: src/flb_http_client.c:1657-1715
Timestamp: 2025-12-17T18:45:02.283Z
Learning: In the Fluent Bit OAuth2 implementation (src/flb_oauth2.c), the function flb_oauth2_get_access_token returns a pointer to the cached token stored in the OAuth2 context (ctx->access_token), not a new allocation. The OAuth2 context manages the token lifecycle, so callers must not free the returned token pointer.
Applied to files:
tests/internal/oauth2.c
🧬 Code graph analysis (1)
tests/internal/oauth2.c (1)
src/flb_oauth2.c (5)
flb_oauth2_create_from_config(675-733)flb_oauth2_config_destroy(207-223)flb_oauth2_parse_json_response(317-411)flb_oauth2_get_access_token(847-871)flb_oauth2_destroy(735-761)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (31)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COMPILER_STRICT_POINTER_TYPES=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-without-cxx (3.31.6)
- GitHub Check: pr-compile-centos-7
- GitHub Check: PR - fuzzing test
🔇 Additional comments (8)
tests/internal/oauth2.c (8)
75-116: LGTM!The mock handlers correctly implement token issuance and resource authorization logic for testing purposes. The authorization check is appropriately simple for a test mock server.
118-175: LGTM!The server thread correctly implements a non-blocking accept loop with select, reliable request reading by making the client socket blocking, and proper cleanup. The routing logic is appropriately simple for test purposes.
289-332: LGTM!The readiness check correctly handles the race condition where the server thread may not have started its accept loop yet. The platform-specific error handling for non-blocking connect is appropriate, and the retry logic with backoff is reasonable.
334-349: LGTM!The server stop sequence is correct: setting the stop flag, shutting down the socket to wake blocked operations, joining the thread, and cleaning up resources. Windows-specific WSA cleanup is properly handled.
351-372: LGTM!The OAuth2 context creation correctly initializes the config structure, passes it to the library, and cleans up the config afterward. The refresh_skew parameter is properly forwarded.
393-439: LGTM!The test correctly exercises token caching and refresh behavior. The test logic is sound:
- Initial token fetch and cache verification
- Waiting for token expiry (with
sleep(2))- Refresh verification
The macOS-specific readiness check at lines 410-417 properly handles the thread startup race condition. The
sleep(2)makes the test slightly slow but is acceptable for an integration test that needs to verify time-based token expiry.
441-445: LGTM!The test list correctly registers both test cases.
374-391: No issue with partial initialization.The stack-allocated context is safely initialized via
memset(&ctx, 0, sizeof(ctx)), which zeros all fields. Theflb_oauth2_parse_json_responsefunction andoauth2_reset_stateonly access fields that are either zero-initialized or protected by NULL checks. The cleanup withflb_sds_destroyis correct since the parse function creates new tokens and the context takes ownership of them.
WIP --> This branch is under heavy development
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
Bug Fixes / Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.