aws: Add AWS Greengrass schema to approved credential URIs #11369
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds Greengrass credential provider support by extending HTTP credential URI validation to accept a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39ef105875
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![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
🤖 Fix all issues with AI agents
In `@src/aws/flb_aws_credentials_http.c`:
- Around line 43-46: GREENGRASS_CREDENTIALS_PATH_LEN is off by one (defined 32
for the 31-char string GREENGRASS_CREDENTIALS_PATH), causing strncmp to
over-compare; fix by changing GREENGRASS_CREDENTIALS_PATH_LEN to the correct
value 31 or (better) replace the literal with
sizeof(GREENGRASS_CREDENTIALS_PATH) - 1 and update any callers using
GREENGRASS_CREDENTIALS_PATH_LEN (e.g., the strncmp comparisons) to use the
corrected constant.
🧹 Nitpick comments (1)
tests/internal/aws_credentials_http.c (1)
799-820: Consider adding a test for hostname prefix attacks.This test correctly validates that non-localhost hosts are rejected. However, given the security concern with
strncmpallowing prefix matches, consider adding a test case for hosts likelocalhost.evil.comto ensure such attempts are blocked.Suggested additional test case
static void test_http_validator_greengrass_invalid_host_prefix() { struct flb_aws_provider *provider; struct flb_config *config; /* Test that hosts starting with "localhost" but not exactly "localhost" are rejected */ setenv("AWS_CONTAINER_CREDENTIALS_FULL_URI", "http://localhost.evil.com:8080/2016-11-01/credentialprovider/", 1); setenv("AWS_CONTAINER_AUTHORIZATION_TOKEN", "greengrass-token", 1); flb_aws_client_mock_configure_generator(NULL); config = flb_calloc(1, sizeof(struct flb_config)); TEST_ASSERT(config != NULL); mk_list_init(&config->upstreams); /* provider creation should fail because host is not exactly localhost */ provider = flb_http_provider_create(config, flb_aws_client_get_mock_generator()); TEST_ASSERT(provider == NULL); flb_aws_client_mock_destroy_generator(); flb_free(config); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/aws/flb_aws_credentials_http.ctests/internal/aws_credentials_http.c
🧰 Additional context used
🧬 Code graph analysis (1)
tests/internal/aws_credentials_http.c (1)
src/aws/flb_aws_credentials_http.c (1)
flb_http_provider_create(330-404)
⏰ 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_SANITIZE_MEMORY=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_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_THREAD=On, 3.31.6, clang, clang++)
- 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_JEMALLOC=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_JEMALLOC=Off, 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 (-DFLB_SMALL=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, 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_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- 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, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-without-cxx (3.31.6)
- 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 - fuzzing test
🔇 Additional comments (5)
src/aws/flb_aws_credentials_http.c (2)
366-371: LGTM!The call site correctly passes the
pathparameter, and the error message is appropriately updated to mention the Greengrass credential provider path option.
67-68: LGTM!The comment accurately documents the new validation rule for Greengrass localhost endpoints.
tests/internal/aws_credentials_http.c (3)
758-797: LGTM!The test correctly validates the happy path for Greengrass credentials, following the established test patterns. It properly sets up the environment variables, mocks the HTTP response, and verifies that credentials are retrieved successfully.
822-866: LGTM!These tests provide good coverage for path validation:
test_http_validator_greengrass_invalid_pathensures completely wrong paths are rejectedtest_http_validator_greengrass_invalid_path_prefixensures the Greengrass path pattern must appear at the start, not embedded in the URLBoth tests follow the established patterns and properly clean up resources.
879-882: LGTM!All four new Greengrass tests are correctly registered in the test list.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Signed-off-by: Shelby Hagman <shelbyzh@amazon.com>
Signed-off-by: Shelby Hagman <shelbyzh@amazon.com>
ShelbyZ
force-pushed
the
aws-greengrass
branch
from
39ef105 to
adc6f65
Compare
2 participants
Add support for AWS Greengrass localhost endpoints that follow a schema like (only port is configurable):
This follows the same credential process as ECS/EKS using a URL and token to provide authorization to the credentials request. In the case of AWS Greengrass the following are provided:
Fixes: aws/aws-for-fluent-bit#948
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
fluent/fluent-bit-docs#2323
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.