oauth2: generic client and server side auth implementation#11286
oauth2: generic client and server side auth implementation#11286
Conversation
📝 WalkthroughWalkthroughAdds 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
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 <eduardo@chronosphere.io>
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 <eduardo@chronosphere.io>
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 <eduardo@chronosphere.io>
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 <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/flb_crypto.c (1)
153-153: Prefer consistent version checking withOPENSSL_VERSION_NUMBER.Line 153 uses
OPENSSL_VERSION_MAJOR, which was introduced in OpenSSL 3.0. While the code works (undefined macros evaluate to 0 in preprocessor conditionals), this is less explicit than the approach used at line 32.For consistency and clarity, consider using the same format as line 32:
-#if OPENSSL_VERSION_MAJOR < 3 +#if OPENSSL_VERSION_NUMBER < 0x30000000Lsrc/flb_oauth2_jwt.c (1)
1221-1426: Consider adding concurrency protection for JWKS cache access.The validation function is comprehensive and well-structured, properly validating all JWT claims. However, the JWKS cache is accessed and modified without explicit locking:
- Lines 1312-1318: Fetches JWKS if needed
- Line 1321-1323: Looks up key in cache
- Lines 1326-1335: Re-fetches and re-looks up if not found
If multiple threads validate tokens concurrently, race conditions could occur during JWKS refresh. Consider whether the HTTP input plugin serializes requests or if cache access needs protection.
If concurrent access is possible, add a mutex around JWKS cache operations or document thread-safety assumptions.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/flb_crypto.c(3 hunks)src/flb_oauth2.c(5 hunks)src/flb_oauth2_jwt.c(1 hunks)tests/internal/input_chunk_routes.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:
src/flb_oauth2.csrc/flb_oauth2_jwt.c
🧬 Code graph analysis (1)
src/flb_oauth2.c (1)
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 (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 (-DFLB_JEMALLOC=On, 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_SANITIZE_THREAD=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 (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=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_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 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_SIMD=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 (-DSANITIZE_ADDRESS=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_SMALL=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_JEMALLOC=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=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=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, clang, clang++, 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, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-centos-7
- GitHub Check: pr-compile-without-cxx (3.31.6)
- GitHub Check: PR - fuzzing test
🔇 Additional comments (29)
src/flb_crypto.c (4)
20-35: LGTM! Good OpenSSL compatibility scaffolding.The added headers and version compatibility macros properly support OpenSSL 1.0.2 through 3.x. The EVP_MD_CTX aliasing is a standard pattern for backward compatibility.
529-576: LGTM! Clean RSA components-based initialization.The function properly validates inputs, propagates errors via
last_error, and cleans up on failures. The structure mirrorsflb_crypto_initand integrates well with the existing crypto context pattern.
578-641: LGTM! Proper RSA signature verification with OpenSSL version handling.The function correctly:
- Uses EVP_DigestVerify for OpenSSL 1.1.0+ (single-call convenience)
- Falls back to Update/Final pattern for OpenSSL 1.0.2
- Manages EVP_MD_CTX lifecycle properly (pkey_ctx is owned by md_ctx)
- Records OpenSSL errors via
last_erroron all failure paths
643-674: LGTM! Clean convenience wrapper for RSA verification.This follows the established pattern of
_simpleconvenience functions in the crypto module. Proper initialization, verification, and cleanup in all code paths.src/flb_oauth2_jwt.c (13)
66-104: LGTM!The status message function provides clear error descriptions for all JWT validation error codes.
225-335: LGTM!The base64URL decoding implementation is robust with proper:
- Whitespace handling and character validation
- Padding calculation and normalization
- Two-pass decoding to determine buffer size
- Error handling and cleanup on all failure paths
337-418: LGTM!The header parsing implementation correctly:
- Converts JSON to msgpack for reliable parsing
- Extracts required claims (kid, alg)
- Validates that the algorithm is RS256
- Provides proper error handling and cleanup
420-554: LGTM!The payload parsing correctly:
- Handles audience as both string and array (per JWT spec)
- Includes clear comments explaining multi-audience validation strategy (lines 503-507)
- Extracts all required claims with proper type checking
- Validates presence of required fields (exp, iss, aud)
The audience array handling properly addresses the past review concern.
556-660: LGTM!The JWT parsing implementation:
- Correctly splits the token into three segments
- Constructs signing_input efficiently using direct allocation and memcpy (addressing past review feedback)
- Decodes all three segments with proper error handling
- Cleans up on all error paths via
flb_oauth2_jwt_destroy
662-766: LGTM!The JWKS key parsing properly:
- Validates the key type is RSA
- Extracts required fields (kid, modulus, exponent)
- Handles nested structures like x5c arrays naturally via msgpack (addressing past JSMN token-counting issue)
- Provides thorough error handling with proper cleanup
The switch to msgpack-based parsing resolved the previous token boundary calculation concerns.
768-865: LGTM with cache clearing concern noted earlier.The JWKS parsing implementation:
- Uses msgpack for reliable JSON parsing (addressing past review feedback)
- Correctly iterates through the keys array
- Stores parsed keys in the hash table by kid
- Validates at least one key was found
The potential cache clearing issue when refreshing JWKS was flagged in an earlier review comment.
868-918: LGTM!The RSA signature verification:
- Uses the crypto abstraction layer for OpenSSL compatibility
- Properly decodes base64URL-encoded modulus and exponent
- Provides appropriate error handling and cleanup
- Logs debug information on verification failure
920-1075: LGTM!The JWKS fetching implementation properly:
- Handles URLs without explicit ports by using protocol defaults (443 for HTTPS, 80 for HTTP) — addressing past review feedback
- Uses a separate success flag (ret_code) that persists through cleanup — addressing past return value logic concern
- Sets up TLS correctly for HTTPS endpoints
- Performs HTTP request with proper error checking
- Cleans up all resources in the cleanup block
All previously identified issues have been addressed.
1077-1169: LGTM!The audience checking implementation correctly:
- Handles both string and array audience formats per JWT spec
- Implements "any match" validation for array audiences
- Uses proper string comparison with length checking
- Provides appropriate error handling and cleanup
1171-1219: LGTM!The context lifecycle functions are well-implemented:
- Clear documentation explaining memory ownership (lines 1173-1177)
- Proper initialization with lazy JWKS loading to avoid blocking (lines 1204-1205)
- Clean destruction of all resources
1428-1463: LGTM!The OAuth2 JWT config map properly defines all necessary configuration options with:
- Appropriate types (bool, string, int, slist)
- Sensible defaults (validate disabled by default, 5-minute refresh interval)
- Clear descriptions for each option
- Correct multiple-value handling for allowed_clients
1465-1473: LGTM!The config map accessor follows the standard pattern for creating and returning configuration maps in Fluent Bit.
src/flb_oauth2.c (12)
34-84: LGTM!The OAuth2 config map properly defines client credentials flow configuration with:
- Comprehensive options for token endpoint, credentials, and optional parameters
- Sensible defaults (disabled by default, 60-second refresh skew)
- Clear documentation for each option
- Appropriate timeout handling (0 means use parent timeout)
100-133: LGTM!The helper functions are correctly implemented:
key_cmpperforms proper length-aware case-insensitive comparisonoauth2_reset_statesafely destroys SDS strings before clearing stateoauth2_apply_defaultssets appropriate default values
134-199: LGTM!The config cloning function properly addresses the past review concern about memory leaks:
- Every string allocation failure now calls
flb_oauth2_config_destroy(dst)before returning (lines 157, 166, 175, 184, 193)- This ensures all partially allocated fields are cleaned up
- Scalar fields are copied correctly with validation
201-217: LGTM!The config destruction properly:
- Checks for NULL config
- Destroys all allocated SDS string fields
- Sets pointers to NULL after freeing to prevent use-after-free
219-309: LGTM!The upstream setup correctly:
- Validates URL components thoroughly
- Creates appropriate TLS or TCP connections based on protocol
- Sets connect timeout when configured (lines 297-299)
- Uses synchronous mode for token fetching (line 295)
- Provides proper error handling and cleanup
311-405: LGTM!The JSON response parsing correctly:
- Uses JSMN for reliable JSON parsing
- Extracts required token fields with proper type checking
- Provides sensible defaults for missing optional fields (Bearer token type, 3600s expiry)
- Updates token state atomically after resetting old state
- Includes helpful logging for debugging
407-497: LGTM!The body building functions properly:
- URL-encode parameter values before appending (line 417)
- Check all SDS operations for NULL returns (addressing past review feedback)
- Support both auto-generated and manual payload modes
- Handle optional parameters (scope, audience)
- Support different authentication methods (Basic vs POST body credentials)
499-573: LGTM!The HTTP request function properly:
- Handles IPv6 fallback for connection establishment
- Configures appropriate timeouts when specified
- Adds required headers and authentication
- Validates response status before parsing
- Cleans up resources in all code paths
575-615: LGTM!The token refresh logic correctly:
- Separates refresh decision from refresh execution
- Checks multiple conditions for refresh need (force, missing token, expired, near expiry)
- Implements proactive refresh using refresh_skew to avoid expiry races
- Provides clear logic flow
617-722: LGTM!The lifecycle functions properly:
- Create contexts with appropriate initialization and validation
- Use config cloning for safe parameter handling
- Initialize locking for thread-safe token access
- Provide comprehensive cleanup of all allocated resources
- Handle errors consistently by calling destroy on partial initialization
724-788: LGTM!The payload manipulation functions correctly:
- Clear and reset token state when payload is modified
- Use
flb_sds_cat_safewith proper error checking (addressing past review feedback on unchecked flb_sds_cat)- Handle buffer growth when needed
- Set payload_manual flag to distinguish custom from auto-generated payloads
790-903: LGTM!The token access functions properly:
- Implement thread-safe token retrieval with locking
- Return pointers to cached tokens (caller must not free, as documented in learnings)
- Provide invalidation mechanism for forcing refresh
- Check expiration status correctly
- Expose configuration map for plugin integration
Based on learnings: The returned token pointer is managed by the OAuth2 context lifecycle, so callers must not free it.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
src/flb_oauth2_jwt.c (1)
1356-1388: JWKS fetch failures return generic error code.When JWKS fetch fails (lines 1360-1364, 1374-1378), the function returns
FLB_OAUTH2_JWT_ERR_INVALID_ARGUMENT, which doesn't distinguish between "JWKS endpoint unreachable", "invalid JWT format", or "signature verification failed". This makes debugging harder for operators trying to understand why validation failed.Consider adding specific error codes like
FLB_OAUTH2_JWT_ERR_JWKS_FETCH_FAILEDorFLB_OAUTH2_JWT_ERR_KEY_NOT_FOUNDto improve error reporting.src/flb_oauth2.c (10)
275-283: Consider documenting the NULL TLS parameters.The call to
flb_tls_createpassesNULLfor many parameters (ca_path, crt_file, key_file, etc.). While this may be intentional for basic TLS without custom certificates, adding a comment explaining that default system CA verification is used would improve clarity.
380-382: Consider validating theexpires_inparsing.Line 381 uses
strtoullwithout checking if the conversion succeeded. If the JSON contains a malformed number,strtoullreturns 0, which will trigger the default at line 397. While this is safe, an explicit check or log message for malformed values would improve debuggability.
413-452: Consider encoding the key as well.The function URL-encodes the value but not the key. While OAuth2 spec typically uses well-known key names (grant_type, scope, etc.) that don't need encoding, encoding both key and value would be more robust if custom parameters are ever added.
564-573: Consider accepting other 2xx status codes.The code only accepts status 200. Some OAuth2 servers may return 201 or 204 for successful token requests. Consider accepting the entire 2xx range for better compatibility.
🔎 Suggested change:
- if (c->resp.payload_size > 0 && c->resp.status == 200) { + if (c->resp.payload_size > 0 && c->resp.status >= 200 && c->resp.status < 300) {
623-640: Document or remove the unusedexpire_secparameter.Line 629 explicitly ignores the
expire_secparameter. If this function is for backward compatibility, add a comment explaining why. If the parameter is truly unused, consider removing it or marking it deprecated.
742-794: Consider documenting URL encoding responsibility.The function appends key-value pairs without URL encoding. This differs from
oauth2_append_kv(line 413) which does encode. Add a comment clarifying that callers must provide pre-encoded values, or consider adding encoding here for consistency.
853-856: Consider documenting or removing the duplicate function.
flb_oauth2_token_get_ngis identical toflb_oauth2_token_get. If this is for API compatibility during migration, add a comment. Otherwise, consider marking one as deprecated or removing the duplicate.
858-874: Consider logging lock acquisition failures.If
flb_lock_acquirefails (line 862-866), the function silently returns without invalidating the token. While this may be acceptable, logging a warning would help diagnose locking issues.
876-883: Consider adding lock protection.This function accesses
ctx->access_tokenwithout holding the lock. While the race is not critical (worst case returns stale length), using the lock would ensure consistency with other token accessors.
885-899: Consider adding lock protection and documenting the difference fromoauth2_token_needs_refresh.Similar to
flb_oauth2_token_len, this function accesses shared state without locking. Additionally, note that this checks exact expiration (expires_at <= now) whileoauth2_token_needs_refresh(line 616) includes therefresh_skewbuffer. Consider documenting this difference or consolidating the logic.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/flb_oauth2.c(5 hunks)src/flb_oauth2_jwt.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:
src/flb_oauth2.csrc/flb_oauth2_jwt.c
🧬 Code graph analysis (1)
src/flb_oauth2_jwt.c (5)
src/flb_hash_table.c (3)
flb_hash_table_create(99-137)flb_hash_table_add(401-494)flb_hash_table_get_ptr(591-603)src/flb_pack.c (1)
flb_pack_json_yyjson(548-554)lib/simdutf-amalgamation-5.5.0/src/simdutf/simdutf.cpp (1)
first(972-972)src/flb_crypto.c (1)
flb_crypto_verify_simple(643-674)src/flb_utils.c (1)
flb_utils_url_split(1441-1534)
⏰ 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 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: run-ubuntu-unit-tests (-DFLB_SIMD=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_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 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_SIMD=On, 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_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 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 (-DSANITIZE_UNDEFINED=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=Off, 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_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 (-DFLB_JEMALLOC=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 (-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-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-without-cxx (3.31.6)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: PR - fuzzing test
- GitHub Check: pr-compile-centos-7
🔇 Additional comments (16)
src/flb_oauth2.c (16)
100-107: LGTM!The helper correctly combines length checking with case-insensitive comparison, which is appropriate for JSON key matching.
109-123: LGTM!Properly cleans up token state with NULL checks and pointer reset after destruction.
140-205: LGTM!The previous memory leak issue has been properly addressed. Each allocation failure now calls
flb_oauth2_config_destroy(dst)before returning, ensuring all partially allocated resources are cleaned up.
207-223: LGTM!Properly destroys all config string fields with NULL safety. The function is idempotent and safe to call multiple times.
303-305: LGTM!Proper handling of
connect_timeoutfrom the config, setting it on the upstream connection when specified.
392-401: LGTM!Good defensive programming: the code provides sensible defaults for missing
token_type(Bearer) andexpires_in(3600s), with appropriate log messages.
454-503: LGTM!The function properly builds the OAuth2 token request body with appropriate error handling. The separation between BASIC auth (credentials in header) and POST auth (credentials in body) is correctly implemented.
581-596: LGTM!Clean implementation of the refresh logic. The "_locked" suffix correctly indicates that the caller must hold the context lock.
598-621: LGTM!The refresh logic correctly implements proactive token refresh by comparing against
expires_at - refresh_skew, ensuring tokens are refreshed before they expire.
642-700: LGTM!Excellent resource management with proper cleanup on all error paths using
flb_oauth2_destroy. The function validates required configuration, initializes all resources, and handles errors gracefully.
702-728: LGTM!Comprehensive cleanup function that properly destroys all resources in the correct order. Matches all allocations in the create functions.
730-740: LGTM!Properly clears the manual payload and resets token state, which is correct since changing the payload invalidates the current token.
771-790: LGTM!The previous issue with unchecked
flb_sds_catcalls has been properly fixed usingflb_sds_cat_safe, which handles error checking and pointer management.
796-812: LGTM!Clean token retrieval logic. As noted in the learnings, this returns a pointer to the cached token in the context, not a new allocation, so callers must not free it.
Based on learnings, the returned token pointer should not be freed by the caller.
814-838: LGTM!Proper locking pattern with retry logic. The lock is correctly released on all paths, including after errors.
72-75: The timeout fallback behavior is correctly implemented. No changes required.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/flb_oauth2_jwt.c (3)
1267-1472: No synchronization for concurrent JWKS cache access.
flb_oauth2_jwt_validatecan be called concurrently by multiple request threads, but accesses and modifiesctx->jwks_cachewithout synchronization. Concurrent calls race during:
- Cache refresh check and JWKS fetch (lines 1356-1364, 1372-1373)
- Hash table lookup (lines 1367-1369, 1378-1380)
- Cache clearing in
oauth2_jwks_fetch_keys(line 1072)This can lead to:
- Multiple threads simultaneously fetching JWKS
- Use-after-free if one thread clears the cache while another reads it
- Corruption of the hash table's internal state
This was flagged as a MAJOR issue in a previous review but remains unresolved.
Add a mutex to
struct flb_oauth2_jwt_ctxto protect cache operations, or document that callers must serialize access to the validation context.🔎 Suggested approach:
Add a mutex to
struct flb_oauth2_jwt_ctx(around line 59):struct flb_oauth2_jwt_ctx { struct flb_config *config; struct flb_oauth2_jwt_cfg cfg; struct flb_oauth2_jwks_cache jwks_cache; pthread_mutex_t cache_lock; /* Protect cache access */ };Initialize in
flb_oauth2_jwt_context_create(after line 1248):pthread_mutex_init(&ctx->cache_lock, NULL);Destroy in
flb_oauth2_jwt_context_destroy(before line 1262):pthread_mutex_destroy(&ctx->cache_lock);Wrap cache operations in
flb_oauth2_jwt_validate(around lines 1351-1399):pthread_mutex_lock(&ctx->cache_lock); /* All cache access and refresh logic */ pthread_mutex_unlock(&ctx->cache_lock);
556-573:azpandclient_idparsing order still depends on map iteration.When both
azpandclient_idare present in the JWT payload, whichever appears later in the msgpack map iteration overwrites the other. Per OAuth2 conventions,azp(authorized party) should take precedence, but msgpack map iteration order is not deterministic.This was flagged in a previous review but remains unresolved.
🔎 Apply this approach to give `azp` explicit precedence:
Add an
int has_azp;field tostruct flb_oauth2_jwt_claims, initialize it toFLB_FALSEwhen the claims struct is zeroed (line 610), then modify the parsing logic:else if (key_len == 3 && strncmp(key_str, "azp", 3) == 0) { if (v->type == MSGPACK_OBJECT_STR) { if (claims->client_id) { flb_sds_destroy(claims->client_id); } claims->client_id = flb_sds_create_len((const char *)v->via.str.ptr, v->via.str.size); + claims->has_azp = FLB_TRUE; /* Track that azp was present */ } } else if (key_len == 9 && strncmp(key_str, "client_id", 9) == 0) { if (v->type == MSGPACK_OBJECT_STR) { + /* Only use client_id if azp was not present */ + if (!claims->has_azp) { if (claims->client_id) { flb_sds_destroy(claims->client_id); } claims->client_id = flb_sds_create_len((const char *)v->via.str.ptr, v->via.str.size); + } } }
126-163: Hash table recreation failure still not propagated.If
flb_hash_table_createfails at line 156,cache->entriesbecomes NULL but the function doesn't return an error code. All subsequent cache operations (flb_hash_table_add,flb_hash_table_get_ptr) will crash with a null-pointer dereference.This was flagged in a previous review but remains unresolved.
🔎 Apply this diff to return an error code:
-static void oauth2_jwks_cache_clear(struct flb_oauth2_jwks_cache *cache) +static int oauth2_jwks_cache_clear(struct flb_oauth2_jwks_cache *cache) { int i; struct mk_list *head; struct mk_list *tmp; struct flb_hash_table_entry *entry; struct flb_hash_table_chain *table; int refresh_interval; if (!cache || !cache->entries) { - return; + return -1; } /* Save refresh interval before destroying */ refresh_interval = cache->refresh_interval; /* Iterate through all hash table chains and destroy keys */ for (i = 0; i < cache->entries->size; i++) { table = &cache->entries->table[i]; mk_list_foreach_safe(head, tmp, &table->chains) { entry = mk_list_entry(head, struct flb_hash_table_entry, _head); if (entry->val) { oauth2_jwks_key_destroy((struct flb_oauth2_jwks_key *)entry->val); entry->val = NULL; /* Prevent double-free */ } } } /* Destroy and recreate the hash table to clear all entries */ flb_hash_table_destroy(cache->entries); cache->entries = flb_hash_table_create(FLB_HASH_TABLE_EVICT_NONE, 64, 0); if (!cache->entries) { flb_error("[oauth2_jwt] failed to recreate JWKS cache after clear"); + return -1; } /* Restore refresh interval */ cache->refresh_interval = refresh_interval; + return 0; }Then update the caller at line 1072:
/* Clear existing cache entries before refreshing to ensure revoked/rotated keys are removed */ - oauth2_jwks_cache_clear(&ctx->jwks_cache); + ret = oauth2_jwks_cache_clear(&ctx->jwks_cache); + if (ret != 0) { + flb_error("[oauth2_jwt] failed to clear JWKS cache"); + goto cleanup; + }
🧹 Nitpick comments (2)
include/fluent-bit/flb_compat.h (1)
153-180: Fix remaining typos in the comment block.While some documentation improvements appear to have been made, several spelling errors remain in this important comment block:
- Line 156: "deliverately" → "deliberately"
- Line 157: "singled DWORD read" → "single DWORD read"
- Line 168: "accross" → "across"
- Line 172: "mistakenly optimizing" may be redundant with earlier phrasing
- Line 177: "deliverately" → "deliberately"
🔎 Apply this diff to fix the typos:
-/* Please do not modify these functions without a very solid understanding of +/* Please do not modify these functions without a solid understanding of * the reasoning behind. * - * These functions deliverately abuse the volatile qualifier in order to prevent - * the compiler from mistakenly optimizing the memory accesses into a singled + * These functions deliberately abuse the volatile qualifier in order to prevent + * the compiler from optimizing the memory accesses into a single * DWORD read (which in some architecture and compiler combinations it does regardless * of the flags). * * The reason why we decided to include this is that according to PR 9096, * when the linux kernel is built and configured to pass through memory alignment * exceptions rather than remediate them fluent-bit generates one while accessing a * packed field in the msgpack wire format (which we cannot modify due to interoperability * reasons). * - * Because of this, a potential patch using memcpy was suggested, however, this patch did - * not yield consistent machine code accross architecture and compiler versions with most + * Because of this, a potential patch using memcpy was suggested; however, this patch did + * not yield consistent machine code across architecture and compiler versions, with most * of them still generating optimized misaligned memory access instructions. * * Keep in mind that these functions transform a single memory read into seven plus a few - * writes as this was the only way to prevent the compiler from mistakenly optimizing the + * writes, as this was the only way to prevent the compiler from optimizing the * operations. * * In most cases, FLB_ENFORCE_ALIGNMENT should not be enabled and the operating system * kernel should be left to handle these scenarios, however, this option is present for - * those users who deliverately and knowingly choose to set up their operating system in + * those users who deliberately and knowingly choose to set up their operating system in * a way that requires it. * */src/flb_oauth2_jwt.c (1)
181-181: Minor indentation inconsistency.Line 181 appears to have inconsistent indentation compared to the surrounding code (missing one level of indentation). This is a minor formatting issue.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
include/fluent-bit/flb_compat.h(3 hunks)src/flb_oauth2_jwt.c(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-08T11:21:33.975Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10851
File: include/fluent-bit/flb_simd.h:60-66
Timestamp: 2025-09-08T11:21:33.975Z
Learning: Fluent Bit currently only supports MSVC compiler on Windows, so additional compiler compatibility guards may be unnecessary for Windows-specific code paths.
Applied to files:
include/fluent-bit/flb_compat.h
📚 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_jwt.c
🧬 Code graph analysis (1)
src/flb_oauth2_jwt.c (5)
src/flb_pack.c (1)
flb_pack_json_yyjson(548-554)src/flb_crypto.c (1)
flb_crypto_verify_simple(643-674)src/flb_utils.c (1)
flb_utils_url_split(1441-1534)src/flb_upstream.c (3)
flb_upstream_create(290-363)flb_upstream_conn_get(711-844)flb_upstream_conn_release(862-947)src/flb_http_client.c (3)
flb_http_client(816-861)flb_http_do(1595-1655)flb_http_client_destroy(1771-1778)
⏰ 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 (-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, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COMPILER_STRICT_POINTER_TYPES=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_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: PR - fuzzing test
- 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_SIMD=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 (-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_JEMALLOC=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_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- 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, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- 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, clang, clang++, 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-centos-7
- 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)
🔇 Additional comments (2)
include/fluent-bit/flb_compat.h (2)
142-142: LGTM! Completes cross-platformstrcasecmpsupport.The inclusion of
<strings.h>provides the proper declaration forstrcasecmpon POSIX systems, complementing the Windows macro definition on line 65. This is correctly placed in the non-Windows platform section.
65-65: Correct and necessary cross-platform compatibility addition.The strcasecmp macro correctly maps the POSIX function to Windows' _stricmp for case-insensitive string comparison, following the established pattern for other POSIX compatibility macros in this file. The codebase extensively uses strcasecmp across core functionality (parsers, HTTP client, configuration handling, utilities), input/output plugins, and OAuth2 code, confirming this macro is essential infrastructure for Windows support.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/flb_oauth2_jwt.c (1)
59-63: Missing synchronization for concurrent JWKS cache access.The
flb_oauth2_jwt_ctxstructure lacks a mutex to protect concurrent access tojwks_cache. According to past review comments (lines 1277-1482 in previous diff), a mutex was supposed to be added to prevent race conditions when multiple threads callflb_oauth2_jwt_validateconcurrently. Without synchronization:
- Multiple threads could simultaneously fetch JWKS (lines 1368-1373)
- Cache clearing could race with reads (line 1078)
- Hash table operations could corrupt internal state
🔎 Add mutex to protect cache operations
+#include <fluent-bit/flb_pthread.h> + struct flb_oauth2_jwt_ctx { struct flb_config *config; struct flb_oauth2_jwt_cfg cfg; struct flb_oauth2_jwks_cache jwks_cache; + pthread_mutex_t cache_lock; };Initialize in
flb_oauth2_jwt_context_create:ctx->config = config; + pthread_mutex_init(&ctx->cache_lock, NULL);Destroy in
flb_oauth2_jwt_context_destroy:oauth2_jwks_cache_destroy(&ctx->jwks_cache); + pthread_mutex_destroy(&ctx->cache_lock);Protect cache operations in
flb_oauth2_jwt_validatearound lines 1362-1409:+ pthread_mutex_lock(&ctx->cache_lock); /* Check if cache needs refresh or is empty */ if (ctx->jwks_cache.last_refresh == 0 || ... /* Verify RSA signature */ verify_ret = oauth2_jwt_verify_signature_rsa(...); + pthread_mutex_unlock(&ctx->cache_lock);Based on past review comments, this issue was marked as fixed by edsiper but the mutex is not present in the current code.
🧹 Nitpick comments (6)
include/fluent-bit/flb_compat.h (1)
153-180: Optional: Fix typos in reflowed comments.The comment reflow contains a few minor typos that could be corrected:
- "deliverately" → "deliberately" (lines 156, 163, 177)
- "singled DWORD read" → "single DWORD read" (line 157)
include/fluent-bit/flb_oauth2_jwt.h (2)
77-89: Consider removing unused validation request/response structs.The
flb_oauth2_jwt_validation_requestandflb_oauth2_jwt_validation_responsestructures are defined but not used anywhere in the implementation. The actual validation functionflb_oauth2_jwt_validateat src/flb_oauth2_jwt.c:1277-1482 takes simpler parameters (context and authorization header). These unused structs add unnecessary API surface.🔎 Consider removing or documenting future use
If these structs are intended for future use, add a comment documenting the planned usage. Otherwise, remove them to keep the API surface minimal:
-struct flb_oauth2_jwt_validation_request { - const char *token; /* raw JWT token */ - size_t token_length; /* JWT length */ - flb_sds_t issuer; /* required issuer */ - flb_sds_t audience; /* required audience */ - flb_sds_t client_id; /* required client id/azp */ - int64_t current_time; /* optional unix time override */ - int64_t leeway; /* optional expiration leeway */ -}; - -struct flb_oauth2_jwt_validation_response { - int status; /* validation status */ -}; -
105-108: Consider makingflb_oauth2_jwt_parseinternal.The
flb_oauth2_jwt_parsefunction is declared in the public header but is only used internally byflb_oauth2_jwt_validate(see src/flb_oauth2_jwt.c:1354). Exposing it as a public API increases the surface area and may allow misuse. Unless there's a specific need for external callers to parse JWTs without validation, consider making it a static function in the implementation file.🔎 Apply this change to make the function internal
In
include/fluent-bit/flb_oauth2_jwt.h, remove the declaration:-/* Parse a JWT and populate the supplied structure. */ -int flb_oauth2_jwt_parse(const char *token, - size_t token_len, - struct flb_oauth2_jwt *jwt); -In
src/flb_oauth2_jwt.c, change the function to static:-int flb_oauth2_jwt_parse(const char *token, size_t token_len, +static int flb_oauth2_jwt_parse(const char *token, size_t token_len, struct flb_oauth2_jwt *jwt)tests/internal/oauth2.c (2)
52-58: Potential buffer overflow in mock server response builder.The
snprintfat line 52 writes the HTTP response (headers + body) into a fixed buffer of sizeMOCK_BODY_SIZE(1024 bytes). If the body length exceeds the available space,snprintfwill truncate the output. Whilesnprintfis safe from buffer overflows, the truncation could cause the test server to send incomplete responses, leading to test failures.🔎 Add buffer size validation
static void compose_http_response(flb_sockfd_t fd, int status, const char *body) { char buffer[MOCK_BODY_SIZE]; int body_len = 0; + int header_len; ssize_t sent = 0; ssize_t total = 0; ssize_t len; if (body != NULL) { body_len = strlen(body); } - snprintf(buffer, sizeof(buffer), + header_len = 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 : ""); + + if (header_len >= sizeof(buffer)) { + flb_error("[oauth2_mock_server] Response too large for buffer"); + return; + }
307-324: Test bypasses public API by calling internal parse function.The
test_parse_defaultsfunction directly callsflb_oauth2_parse_json_response(line 316), which is an internal function in src/flb_oauth2.c. This creates a dependency on internal implementation details and bypasses the public OAuth2 API. If the internal function signature changes, this test will break.Consider refactoring to test through the public API (
flb_oauth2_get_access_token) or documenting that this is an intentional internal test. If testing internal parsing is important, the test file should be placed closer to the implementation or the function should be properly exposed for testing.src/flb_oauth2_jwt.c (1)
1445-1474: Simplify nested client list iteration with helper function.The nested iteration over
allowed_clients(lines 1450-1465) involves three levels of indirection: outermk_list→flb_config_map_val→ innermk_list→flb_slist_entry. This logic is dense and hard to read. Extracting it into a helper function would improve clarity and make it easier to test independently.🔎 Proposed helper function
Add a helper function:
static int oauth2_jwt_client_authorized(struct mk_list *allowed_clients, const char *client_id) { struct mk_list *allowed_client_head; struct flb_config_map_val *map_val; struct mk_list *client_list_head; struct flb_slist_entry *client_entry; if (!client_id || !allowed_clients || mk_list_size(allowed_clients) == 0) { return FLB_FALSE; } mk_list_foreach(allowed_client_head, allowed_clients) { map_val = mk_list_entry(allowed_client_head, struct flb_config_map_val, _head); if (!map_val || !map_val->val.list) { continue; } mk_list_foreach(client_list_head, map_val->val.list) { client_entry = mk_list_entry(client_list_head, struct flb_slist_entry, _head); if (client_entry && client_entry->str && strcmp(client_entry->str, client_id) == 0) { return FLB_TRUE; } } } return FLB_FALSE; }Then simplify the validation code:
/* Check allowed clients */ if (ctx->cfg.allowed_clients && mk_list_size(ctx->cfg.allowed_clients) > 0) { - allowed_client_authorized = FLB_FALSE; - - /* Iterate over flb_config_map_val entries (each contains a list of flb_slist_entry) */ - mk_list_foreach(allowed_client_head, ctx->cfg.allowed_clients) { - map_val = mk_list_entry(allowed_client_head, struct flb_config_map_val, _head); - if (!map_val || !map_val->val.list) { - continue; - } - - /* Iterate over flb_slist_entry in this map_val's list */ - mk_list_foreach(client_list_head, map_val->val.list) { - client_entry = mk_list_entry(client_list_head, struct flb_slist_entry, _head); - if (jwt.claims.client_id && client_entry && client_entry->str && - strcmp(client_entry->str, jwt.claims.client_id) == 0) { - allowed_client_authorized = FLB_TRUE; - goto client_check_done; - } - } - } - - client_check_done: - if (allowed_client_authorized == FLB_FALSE) { + if (!oauth2_jwt_client_authorized(ctx->cfg.allowed_clients, jwt.claims.client_id)) { flb_error("[oauth2_jwt] Client ID '%s' not in allowed list (rejecting request)", jwt.claims.client_id ? jwt.claims.client_id : "(null)"); status = FLB_OAUTH2_JWT_ERR_INVALID_ARGUMENT; goto jwt_end; } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
include/fluent-bit/flb_compat.h(3 hunks)include/fluent-bit/flb_oauth2_jwt.h(1 hunks)src/flb_oauth2_jwt.c(1 hunks)tests/internal/oauth2.c(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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.cinclude/fluent-bit/flb_oauth2_jwt.hsrc/flb_oauth2_jwt.c
📚 Learning: 2025-09-08T11:21:33.975Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10851
File: include/fluent-bit/flb_simd.h:60-66
Timestamp: 2025-09-08T11:21:33.975Z
Learning: Fluent Bit currently only supports MSVC compiler on Windows, so additional compiler compatibility guards may be unnecessary for Windows-specific code paths.
Applied to files:
include/fluent-bit/flb_compat.h
🧬 Code graph analysis (3)
tests/internal/oauth2.c (2)
src/flb_sds.c (4)
flb_sds_create_size(92-95)flb_sds_create(78-90)flb_sds_printf(336-387)flb_sds_destroy(389-399)src/flb_oauth2.c (5)
flb_oauth2_create_from_config(642-700)flb_oauth2_config_destroy(207-223)flb_oauth2_parse_json_response(317-411)flb_oauth2_get_access_token(814-838)flb_oauth2_destroy(702-728)
include/fluent-bit/flb_oauth2_jwt.h (1)
src/flb_oauth2_jwt.c (7)
flb_oauth2_jwt_context_create(1232-1264)flb_oauth2_jwt_context_destroy(1266-1275)flb_oauth2_jwt_validate(1277-1482)flb_oauth2_jwt_parse(600-704)flb_oauth2_jwt_destroy(240-263)flb_oauth2_jwt_status_message(65-103)flb_oauth2_jwt_get_config_map(1521-1528)
src/flb_oauth2_jwt.c (6)
src/flb_sds.c (2)
flb_sds_destroy(389-399)flb_sds_create_len(58-76)src/flb_hash_table.c (3)
flb_hash_table_create(99-137)flb_hash_table_add(401-494)flb_hash_table_get_ptr(591-603)src/flb_pack.c (1)
flb_pack_json_yyjson(548-554)src/flb_crypto.c (1)
flb_crypto_verify_simple(643-674)src/flb_utils.c (1)
flb_utils_url_split(1441-1534)src/flb_http_client.c (2)
flb_http_client(816-861)flb_http_client_destroy(1771-1778)
⏰ 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_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 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_COMPILER_STRICT_POINTER_TYPES=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 (-DSANITIZE_UNDEFINED=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_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=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_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=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_JEMALLOC=Off, 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 (-DSANITIZE_ADDRESS=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: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-centos-7
- 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, 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-without-cxx (3.31.6)
- GitHub Check: PR - fuzzing test
🔇 Additional comments (2)
include/fluent-bit/flb_compat.h (1)
65-65: LGTM! Cross-platformstrcasecmpcompatibility added correctly.The Windows macro mapping
strcasecmpto_stricmp(line 65) and the POSIX<strings.h>include (line 142) properly enable cross-platform use ofstrcasecmp, consistent with the existingstrncasecmppattern. These additions support the OAuth2 implementation, wherestrcasecmpis used for protocol validation inflb_oauth2_jwt.c.src/flb_oauth2_jwt.c (1)
1485-1519: LGTM: Config map definitions are well-structured.The OAuth2 JWT configuration map provides appropriate options for validation, including:
- Boolean toggle for validation
- Issuer and audience constraints
- JWKS URL and refresh interval
- Multiple allowed clients support
The documentation strings are clear, and the offsetof usage correctly maps to the
flb_oauth2_jwt_cfgstructure fields.
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/flb_oauth2_jwt.c (1)
1363-1397: Add mutex protection for JWKS cache concurrent access.The
oauth2_ctxis created once per HTTP plugin instance and shared across all concurrent connections. Multiple request handlers can simultaneously access and modifyctx->jwks_cache(check/updatelast_refresh, fetch keys, lookup entries) without synchronization, creating race conditions during:
- Cache refresh check and JWKS fetch (lines 1363-1370)
- Hash table lookups and insertions (lines 1374-1376)
- Cache clearing operations (line 1077)
Add a
pthread_mutex_ttostruct flb_oauth2_jwt_ctxand protect all jwks_cache access with lock/unlock calls.
🧹 Nitpick comments (3)
src/flb_oauth2.c (2)
317-411: Consider handling insufficient token buffer in JSON parsing.The JSON parser uses a fixed token buffer size of 32 (line 324). If the OAuth2 response contains more than 32 JSON tokens (e.g., additional fields, nested objects),
jsmn_parsewill fail. While OAuth2 responses are typically simple, some providers may include extra metadata or error details.Consider checking if
jsmn_parsereturnsJSMN_ERROR_NOMEMand reallocating the token buffer, or at minimum, document this limitation and log a more specific error message when the buffer is insufficient.🔎 Example approach to handle token buffer overflow
- jsmn_init(&parser); - tokens = flb_calloc(1, sizeof(jsmntok_t) * tokens_size); - if (!tokens) { - flb_errno(); - return -1; - } - - ret = jsmn_parse(&parser, json_data, json_size, tokens, tokens_size); - if (ret <= 0) { - flb_error("[oauth2] cannot parse payload"); - flb_free(tokens); - return -1; - } + jsmn_init(&parser); + tokens = flb_calloc(1, sizeof(jsmntok_t) * tokens_size); + if (!tokens) { + flb_errno(); + return -1; + } + + ret = jsmn_parse(&parser, json_data, json_size, tokens, tokens_size); + if (ret == JSMN_ERROR_NOMEM) { + flb_error("[oauth2] JSON response too complex (exceeds %d tokens)", tokens_size); + flb_free(tokens); + return -1; + } + else if (ret <= 0) { + flb_error("[oauth2] cannot parse payload"); + flb_free(tokens); + return -1; + }
742-794: Payload append function doesn't URL-encode values.The
flb_oauth2_payload_appendfunction appends raw key-value pairs to the payload without URL-encoding, unlikeoauth2_append_kvwhich encodes values usingflb_uri_encode. This may be intentional if callers are expected to provide pre-encoded values, but it could lead to malformed request bodies if special characters (e.g.,&,=, spaces) are present in keys or values.Consider whether URL encoding should be applied here, or document that callers must provide URL-safe values.
src/flb_oauth2_jwt.c (1)
996-1001: Consider more robust port parsing.The current code uses
atoiwhich has limited error detection. While the<= 0check catches most invalid cases,strtolwith proper error checking would be more robust for detecting malformed port strings.🔎 Optional improvement using strtol
if (port_str && port_str[0] != '\0') { - port = atoi(port_str); - if (port <= 0) { + char *endptr; + long port_long = strtol(port_str, &endptr, 10); + if (*endptr != '\0' || port_long <= 0 || port_long > 65535) { flb_error("[oauth2_jwt] invalid port in JWKS URL"); goto cleanup; } + port = (int)port_long; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/flb_oauth2.c(5 hunks)src/flb_oauth2_jwt.c(1 hunks)tests/internal/input_chunk_routes.c(2 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_jwt.csrc/flb_oauth2.c
🧬 Code graph analysis (2)
tests/internal/input_chunk_routes.c (1)
src/flb_kv.c (1)
flb_kv_release(123-133)
src/flb_oauth2.c (1)
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 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: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=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_SANITIZE_THREAD=On, 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 (-DFLB_JEMALLOC=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_COMPILER_STRICT_POINTER_TYPES=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_ARROW=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 (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- 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, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 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 (-DSANITIZE_ADDRESS=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 (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- 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, gcc, g++, ubuntu-24.04, clang-14)
- 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-without-cxx (3.31.6)
- GitHub Check: pr-compile-centos-7
🔇 Additional comments (15)
tests/internal/input_chunk_routes.c (1)
118-118: LGTM! Previous cleanup concern resolved.The initialization of
oauth2_jwt_propertiesat line 118 is now properly paired with cleanup at line 244. This follows the same pattern aspropertiesandnet_properties, ensuring no resource leaks.Also applies to: 244-244
src/flb_oauth2.c (12)
35-84: Config map is well-structured.The OAuth2 configuration map properly defines all necessary fields with appropriate types, defaults, and documentation. The use of
offsetoffor direct struct mapping is correct.
100-138: Helper functions look correct.The helper functions
key_cmp,oauth2_reset_state, andoauth2_apply_defaultsare properly implemented. The default forenablednow correctly matches the config map default ofFLB_FALSE.
140-223: Config cloning and cleanup is properly implemented.The
oauth2_clone_configfunction now correctly handles allocation failures by callingflb_oauth2_config_destroyon all error paths, preventing the memory leak identified in previous reviews. The cleanup function properly destroys all SDS strings.
225-315: Upstream setup handles both HTTP and HTTPS correctly.The
oauth2_setup_upstreamfunction properly parses the authentication URL, validates the protocol, creates the appropriate upstream connection (TCP or TLS), and applies the connect timeout from configuration. Error handling correctly cleans up temporary buffers.
505-579: HTTP request handling is correct.The
oauth2_http_requestfunction properly manages the upstream connection, sets appropriate timeouts from configuration, handles both Basic and POST authentication methods, and ensures the connection is released on all code paths (success and error).
581-621: Token refresh logic is well-implemented.The token refresh logic correctly determines when a token needs refreshing (considering the configured skew), builds the request body, makes the HTTP request, and properly cleans up the body buffer after use.
623-640: Creation function properly delegates to config-based constructor.The
flb_oauth2_createfunction correctly creates a temporary config and delegates toflb_oauth2_create_from_config, then cleans up the temporary config. Theexpire_secparameter is marked as unused (line 629), which suggests it may be retained for API compatibility or future use.
642-700: Config-based creation has comprehensive error handling.The
flb_oauth2_create_from_configfunction properly validates inputs (especiallytoken_url), allocates all necessary resources, and ensures all error paths callflb_oauth2_destroyfor consistent cleanup. Lock initialization and upstream setup are correctly validated.
702-728: Destroy function comprehensively cleans up all resources.The
flb_oauth2_destroyfunction properly cleans up all allocated resources including state, SDS strings, TLS context, upstream connection, config, and lock. The NULL check at the beginning prevents issues with double-free or invalid pointers.
796-899: Token access functions correctly implement locking and lifecycle management.The token access functions properly use the context lock to ensure thread-safe access to the cached token. The
flb_oauth2_get_access_tokenfunction correctly implements the retry-on-refresh pattern, andflb_oauth2_invalidate_tokensafely marks tokens as expired. Based on learnings, callers receive a pointer to the cached token and must not free it.
901-912: Config map accessor now properly handles errors.The
flb_oauth2_get_config_mapfunction now checks the return value offlb_config_map_createand logs an error if NULL is returned, addressing the concern from previous reviews.
413-452: Memory leak:tmpis not freed on error paths.The function
oauth2_append_kvallocates an encoded stringtmpat line 422 but only destroys it on the success path (line 449). If any of theflb_sds_catcalls at lines 429, 436, or 442 fail, the function returns NULL without freeingtmp, causing a memory leak.🔎 Proposed fix to free tmp on all error paths
static flb_sds_t oauth2_append_kv(flb_sds_t buffer, const char *key, const char *value) { flb_sds_t tmp; if (!value) { return buffer; } tmp = flb_uri_encode(value, strlen(value)); if (!tmp) { flb_errno(); return NULL; } if (flb_sds_len(buffer) > 0) { buffer = flb_sds_cat(buffer, "&", 1); if (!buffer) { flb_sds_destroy(tmp); return NULL; } } buffer = flb_sds_cat(buffer, key, strlen(key)); if (!buffer) { flb_sds_destroy(tmp); return NULL; } buffer = flb_sds_cat(buffer, "=", 1); if (!buffer) { flb_sds_destroy(tmp); return NULL; } buffer = flb_sds_cat(buffer, tmp, flb_sds_len(tmp)); flb_sds_destroy(tmp); return buffer; }Likely an incorrect or invalid review comment.
src/flb_oauth2_jwt.c (2)
1433-1462: LGTM: Allowed clients validation is well-structured.The nested iteration through
allowed_clientscorrectly handles the multi-level list structure (flb_config_map_val → flb_slist_entry). Thegototo break out of nested loops is appropriate, and the logic properly rejects tokens without a matchingclient_id.
1-1516: Strong OAuth2 JWT implementation with thorough validation.This is a comprehensive and well-structured implementation that addresses all previously identified issues:
- ✅ JWT parsing with proper base64url decoding and validation
- ✅ JWKS fetching with TLS support and system certificate verification
- ✅ RSA signature verification using crypto abstraction
- ✅ Complete policy enforcement (expiration, issuer, audience, client_id)
- ✅ Security hardening (time-based refresh prevents DoS from malicious
kidvalues)- ✅ Proper resource cleanup in all error paths
- ✅ Array audience handling per JWT spec (RFC 7519)
The code quality is high, with clear structure, helpful comments, and defensive checks throughout.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/flb_oauth2.c (1)
413-452: Memory leak: original buffer lost whenflb_sds_catfails.When
flb_sds_catreturns NULL (due to allocation failure during resize), the original buffer is not freed byflb_sds_catitself - it remains allocated. However,oauth2_append_kvassigns NULL tobufferand returns NULL, causing the caller (oauth2_build_body) to lose the reference to the original buffer.For example at line 429: if
flb_sds_cat(buffer, "&", 1)fails,bufferbecomes NULL but the original SDS memory is leaked.🔎 Proposed fix to free buffer on error
static flb_sds_t oauth2_append_kv(flb_sds_t buffer, const char *key, const char *value) { flb_sds_t tmp; + flb_sds_t new_buf; if (!value) { return buffer; } tmp = flb_uri_encode(value, strlen(value)); if (!tmp) { flb_errno(); + flb_sds_destroy(buffer); return NULL; } if (flb_sds_len(buffer) > 0) { - buffer = flb_sds_cat(buffer, "&", 1); - if (!buffer) { + new_buf = flb_sds_cat(buffer, "&", 1); + if (!new_buf) { + flb_sds_destroy(buffer); flb_sds_destroy(tmp); return NULL; } + buffer = new_buf; } - buffer = flb_sds_cat(buffer, key, strlen(key)); - if (!buffer) { + new_buf = flb_sds_cat(buffer, key, strlen(key)); + if (!new_buf) { + flb_sds_destroy(buffer); flb_sds_destroy(tmp); return NULL; } + buffer = new_buf; - buffer = flb_sds_cat(buffer, "=", 1); - if (!buffer) { + new_buf = flb_sds_cat(buffer, "=", 1); + if (!new_buf) { + flb_sds_destroy(buffer); flb_sds_destroy(tmp); return NULL; } + buffer = new_buf; - buffer = flb_sds_cat(buffer, tmp, flb_sds_len(tmp)); + new_buf = flb_sds_cat(buffer, tmp, flb_sds_len(tmp)); flb_sds_destroy(tmp); + if (!new_buf) { + flb_sds_destroy(buffer); + return NULL; + } - return buffer; + return new_buf; }This issue was previously flagged in past reviews on lines 454-503 but the fix was not fully applied to
oauth2_append_kvitself.src/flb_oauth2_jwt.c (1)
59-63: Add mutex protection to JWKS cache for concurrent access safety.The context structure lacks a mutex for the
jwks_cache, exposing race conditions during concurrent JWT validation calls. Multiple threads may simultaneously check cache expiration, fetch and parse JWKS keys, and perform lookups without synchronization. Add a mutex field toflb_oauth2_jwks_cache, initialize it inoauth2_jwks_cache_init(), and protect all cache access inflb_oauth2_jwt_validate()and JWKS fetch/parse operations.
🧹 Nitpick comments (1)
src/flb_oauth2.c (1)
885-899: Consider adding lock protection forflb_oauth2_token_expired.This function reads
ctx->expires_atwithout acquiring the lock, whileflb_oauth2_invalidate_tokenand the refresh logic modify it under lock. On most platforms reading atime_tis atomic, but for strict thread-safety consistency with the rest of the API, consider protecting this read.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
include/fluent-bit/flb_compat.h(3 hunks)include/fluent-bit/flb_oauth2_jwt.h(1 hunks)src/flb_oauth2.c(5 hunks)src/flb_oauth2_jwt.c(1 hunks)tests/internal/input_chunk_routes.c(2 hunks)tests/internal/oauth2.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/internal/input_chunk_routes.c
- include/fluent-bit/flb_compat.h
- tests/internal/oauth2.c
- include/fluent-bit/flb_oauth2_jwt.h
🧰 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_jwt.csrc/flb_oauth2.c
🧬 Code graph analysis (2)
src/flb_oauth2_jwt.c (1)
src/flb_utils.c (1)
flb_utils_url_split(1441-1534)
src/flb_oauth2.c (3)
src/flb_sds.c (5)
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)include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)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). (25)
- 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 (-DFLB_SANITIZE_MEMORY=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 (-DSANITIZE_UNDEFINED=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_COMPILER_STRICT_POINTER_TYPES=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, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=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_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_ADDRESS=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: PR - fuzzing test
- 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, 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: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
🔇 Additional comments (15)
src/flb_oauth2_jwt.c (9)
1-64: LGTM on file structure and type definitions.The file is well-organized with clear structure definitions for JWKS keys, cache, and JWT context. The includes cover all necessary dependencies for JWT parsing, HTTP client operations, and cryptographic verification.
126-165: LGTM on cache clear implementation.The function properly preserves the refresh interval, destroys all cached keys to prevent memory leaks, and recreates the hash table. Error propagation was correctly addressed with the return value.
265-374: LGTM on base64url decode implementation.The two-pass approach (sizing pass then decode pass) is correctly implemented. The function properly handles base64url to base64 conversion, padding, whitespace skipping, and character validation with appropriate cleanup on all error paths.
459-597: LGTM on JWT payload parsing.The implementation correctly handles all required claims (exp, iss, aud) and optional claims (azp, client_id). The
has_azpflag ensures proper precedence when bothazpandclient_idare present, and negative expiration values are properly rejected.
599-703: LGTM on JWT parse function.The signing input construction correctly uses
flb_sds_create_sizewith proper length calculation. All error paths properly cleanup viaflb_oauth2_jwt_destroy, and segment validation is thorough.
705-809: LGTM on JWKS key parsing.The function correctly validates RSA key type, extracts required components (kid, n, e), and properly cleans up partial allocations on error paths.
963-1125: LGTM on JWKS fetch implementation.The function properly handles default ports for HTTP/HTTPS, sets up TLS with hostname verification, clears the cache before refreshing (preventing stale keys), and uses a separate success flag for correct return value handling. The cleanup section properly releases all resources.
1127-1219: LGTM on audience check implementation.The function correctly handles both string and array
audclaim formats per JWT spec (RFC 7519), returning true if any element matches the allowed audience.
1274-1470: LGTM on JWT validation function.The validation flow correctly implements:
- Time-based only JWKS refresh (preventing DoS via random kid values)
- Proper signature verification using JWKS
- Expiration, issuer, and audience validation
- Nested config map iteration for allowed clients
The function properly cleans up the JWT structure on all exit paths.
src/flb_oauth2.c (6)
34-84: LGTM on OAuth2 config map definition.The config map properly defines all OAuth2 configuration options with appropriate types, defaults, and descriptions. The structure aligns well with the
flb_oauth2_configstruct.
140-205: LGTM on config clone implementation.Each string allocation failure properly calls
flb_oauth2_config_destroy(dst)before returning, preventing the partial allocation leak that was flagged in past reviews.
505-579: LGTM on HTTP request implementation.The function properly handles upstream connection with IPv6 fallback, applies configured timeouts, sets up basic authentication when required, and correctly parses the token response. All error paths release resources appropriately.
642-700: LGTM on context creation from config.The function properly validates required configuration (token_url), initializes the lock for thread safety, and cleans up on all error paths via
flb_oauth2_destroy.
814-838: LGTM on get access token implementation.The function correctly uses locking for thread safety. Per the retrieved learnings,
token_outreceives a pointer to the cached token stored in the OAuth2 context (ctx->access_token), not a new allocation - callers must not free the returned pointer. This is consistent with the implementation at line 809 inoauth2_get_token_locked.
901-912: LGTM on config map accessor.The function now properly checks for NULL return from
flb_config_map_createand logs an error before returning, addressing the previous review concern.
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
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 <eduardo@chronosphere.io>
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 <eduardo@chronosphere.io>
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.
1 participant
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.