Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add smoke test for greater API coverage #361

Merged
merged 2 commits into from
Jun 27, 2024
Merged

Conversation

sporkmonger
Copy link
Contributor

@sporkmonger sporkmonger commented Jun 27, 2024

We didn't previously have tests for every single plugin API call, so fairly obvious bugs could easily sneak in. This test doesn't thoroughly specify API behavior, but it does try to hit almost everything except Redis all at once. Redis already has acceptable integration test coverage, so I opted not to essentially just repeat the Redis tests.

Summary by CodeRabbit

  • New Features

    • Introduced a new smoke test suite for plugin validation, including various HTTP checks and decision handlers.
    • Added configuration support for smoke test plugins with detailed settings for service ports, proxy configurations, Redis URI, thresholds, and resource management.
  • Bug Fixes

    • Improved test reliability by adding sleep calls before interacting with the envoy service.
  • Chores

    • Updated .gitignore rules for the smoke-test plugin to exclude dist/ and target/ directories.
    • Enhanced project configuration to exclude additional directories from builds.
  • Refactor

    • Updated error messaging in HTTP domain verification for better clarity.
  • Documentation

    • Adjusted package metadata and dependencies for the smoke-test plugin to ensure compatibility and optimized build configurations.

@sporkmonger sporkmonger added size/xx-large Denotes a PR that changes 1000+ lines, ignoring generated files. rust Pull requests that update Rust code kind/testing Categorizes issue or PR as related to testing. labels Jun 27, 2024
@sporkmonger sporkmonger added this to the Release 0.6.0 milestone Jun 27, 2024
@sporkmonger sporkmonger self-assigned this Jun 27, 2024
Copy link

dryrunsecurity bot commented Jun 27, 2024

Hi there 👋, @DryRunSecurity here, below is a summary of our analysis and findings.

DryRun Security Status Findings
Server-Side Request Forgery Analyzer 0 findings
Configured Codepaths Analyzer 0 findings
Secrets Analyzer 0 findings
SQL Injection Analyzer 0 findings
Sensitive Files Analyzer 0 findings
Authn/Authz Analyzer 0 findings
IDOR Analyzer 0 findings

Note

🟢 Risk threshold not exceeded.

Change Summary (click to expand)

The following is a summary of changes in this pull request made by me, your security buddy 🤖. Note that this summary is auto-generated and not meant to be a definitive list of security issues but rather a helpful summary from a security perspective.

Summary:

The provided code changes cover a variety of files and components within the Bulwark project, a security platform that utilizes plugins to perform various security-related tasks. The changes include updates to the Cargo configuration, Envoy-based testing, smoke testing plugins, and configuration files.

From an application security perspective, the key points to highlight are:

  1. Dependency Management: The changes to the Cargo.toml and Cargo.lock files should be reviewed to ensure that the project's dependencies, including the bulwark-sdk and serde crates, are up-to-date and do not contain any known security vulnerabilities.

  2. WebAssembly Security: The project's use of WebAssembly for the smoke test plugin introduces additional security considerations, such as potential sandbox escape vulnerabilities and resource exhaustion attacks. These aspects should be carefully evaluated.

  3. Secure Configuration: The changes to the smoke test configuration file (smoke_test.toml) demonstrate a security-conscious approach, such as disabling administrative mode, managing proxy hops, and configuring permissions for the smoke test plugin. These configurations should be thoroughly reviewed to ensure they are appropriate and do not introduce any security risks.

  4. Comprehensive Testing: The additions of Envoy-based tests and the smoke test plugin indicate a focus on comprehensive testing, which can contribute to the overall security posture of the application. The tests cover various aspects of the application's behavior, including handling of malicious requests and enforcement of access control policies.

Overall, the provided code changes do not appear to introduce any obvious security vulnerabilities. However, it is essential to continue reviewing the project's dependencies, WebAssembly-related security, configuration settings, and testing practices to ensure the ongoing security and reliability of the Bulwark application.

Files Changed:

  1. Cargo.toml: The changes update the workspace configuration, excluding the "tests/plugins" directory from the build process.
  2. crates/host/src/context.rs: The changes modify the error message returned when the requested domain is not found in the list of allowed HTTP domains.
  3. tests/plugins/smoke-test/.gitignore: The changes add two new lines to the .gitignore file, excluding the dist/ and target/ directories.
  4. tests/envoy.rs: The changes improve the reliability of the Envoy-based tests by adding a delay after server setup and include tests for handling "evil" requests and multi-phase execution.
  5. tests/plugins/smoke-test/Cargo.lock: The changes update the dependency versions for the "smoke-test" package.
  6. tests/plugins/smoke-test/Cargo.toml: The changes update the package metadata, dependencies, and build configuration for the "smoke-test" crate.
  7. tests/plugins/smoke-test/src/lib.rs: The changes introduce a new "SmokeTestPlugin" that implements various checks and validations for the application's request and response handling.
  8. tests/smoke_test.rs: The changes implement a smoke test for a Bulwark plugin, including initialization, request/response handling, and decision feedback.
  9. tests/smoke_test.toml: The changes update the configuration for the smoke test, including port, admin mode, proxy hops, Redis connection, thresholds, and plugin permissions.

Powered by DryRun Security

Copy link
Contributor

coderabbitai bot commented Jun 27, 2024

Walkthrough

The changes encompass multiple enhancements across the project, including adjusting the Cargo.toml configuration to exclude specific directories, updating error messages in HTTP domain verification, adding reliable sleep calls in tests, and introducing a comprehensive smoke-test plugin with new functionalities for HTTP service testing. The addition of new configuration files and test functions ensures robust plugin operations and integrations.

Changes

File Change Summary
Cargo.toml Updated the exclude field to include tests/plugins.
crates/host/src/context.rs Modified error message in verify_http_domains function.
tests/envoy.rs Added sleep calls for reliability in envoy service test functions.
tests/plugins/smoke-test/.gitignore New file to exclude dist/ and target/ directories.
tests/plugins/smoke-test/Cargo.toml Updated package metadata, dependencies, and build configurations.
tests/plugins/smoke-test/src/lib.rs Introduced SmokeTestPlugin struct with various HTTP handling methods.
tests/smoke_test.rs Introduced test_smoke() function for comprehensive plugin operation tests.
tests/smoke_test.toml New configurations for service settings, plugin details, and resource setups.

Poem

In code and test, we surely invest,
With plugins checked, our work is blessed.
From sleep to smoke, each change bespoke,
New paths defined, no system choked.
Excludes are set and errors refined,
Our software shines, well-aligned! 🚀


Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f9bed6f and 4bfa96e.

Files ignored due to path filters (1)
  • tests/plugins/smoke-test/Cargo.lock is excluded by !**/*.lock
Files selected for processing (8)
  • Cargo.toml (1 hunks)
  • crates/host/src/context.rs (1 hunks)
  • tests/envoy.rs (3 hunks)
  • tests/plugins/smoke-test/.gitignore (1 hunks)
  • tests/plugins/smoke-test/Cargo.toml (1 hunks)
  • tests/plugins/smoke-test/src/lib.rs (1 hunks)
  • tests/smoke_test.rs (1 hunks)
  • tests/smoke_test.toml (1 hunks)
Files skipped from review due to trivial changes (4)
  • Cargo.toml
  • crates/host/src/context.rs
  • tests/plugins/smoke-test/.gitignore
  • tests/plugins/smoke-test/Cargo.toml
Additional comments not posted (3)
tests/smoke_test.toml (1)

1-29: Configuration Review for smoke_test.toml

  1. Service Configuration: The port is set to a non-default value to avoid collisions, which is a good practice. However, consider explicitly stating why port 3089 is chosen if it has specific significance.
  2. State Configuration: The Redis URI points to localhost, which is typical for development but ensure it's configurable for production environments.
  3. Plugin Configuration: The plugin configuration includes a complex type with nested values, which is well-defined. Ensure that the smoke and complex configurations are adequately documented in the project's README or relevant documentation.
  4. Resource Configuration: The timeout for the routes is set to 100 (milliseconds, presumably). Verify if this timeout is sufficient for expected network conditions and workload.

Overall, the configurations are well-structured and seem to follow best practices.

tests/smoke_test.rs (1)

9-171: Comprehensive Review of smoke_test.rs

  1. Test Setup: The test setup dynamically builds the plugin and configures it. This is a robust approach but ensure the paths and configurations are validated to prevent runtime issues.
  2. Redis Configuration: The Redis URI fallback to a default value is a practical approach for testing. However, make sure that this does not bypass any necessary environment-specific configurations in production.
  3. Assertions: The assertions in the test check for specific outcomes and states. Ensure these assertions align with the expected behavior of the plugin and add more if necessary to cover all functionalities.
  4. Error Handling: The test handles potential errors at various stages. Review if there are any more error scenarios that need to be addressed.

This test script is well-structured and appears to cover the necessary scenarios for a smoke test.

tests/envoy.rs (1)

Line range hint 1-231: Detailed Review of Envoy Integration Tests in envoy.rs

  1. Delay Handling: The use of SERVER_LAUNCH_DELAY is consistent across tests to handle startup timing, which is good for reliability. However, consider parameterizing this delay to adjust it based on environmental factors or configurations.
  2. Assertions: The assertions check for specific HTTP responses and body contents. Ensure these are comprehensive enough to catch potential issues with the Envoy integration.
  3. Error Handling: The tests appear to handle errors by retrying requests. Review if there are any additional error scenarios, particularly network-related, that should be considered.

The Envoy integration tests are well-implemented, but fine-tuning based on operational experience could enhance reliability.

tests/plugins/smoke-test/src/lib.rs Show resolved Hide resolved
@sporkmonger sporkmonger merged commit 429ca57 into main Jun 27, 2024
16 checks passed
@sporkmonger sporkmonger deleted the smoke-test-entire-api branch June 27, 2024 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/testing Categorizes issue or PR as related to testing. rust Pull requests that update Rust code size/xx-large Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant