Feat: Firejail sandbox for source-declarative-manifest (do not merge)#403
Feat: Firejail sandbox for source-declarative-manifest (do not merge)#403aaronsteers wants to merge 8 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughThis pull request enhances the container's security and updates the command-line interface for the source-declarative-manifest connector. The Dockerfile now installs Firejail to sandbox the environment, removes a fallback during dependency installation to ensure build strictness, and updates the entrypoint and environment variable to invoke the new sandboxed command. Corresponding changes are made in the connector’s Python modules by adding and exporting a new Does this accurately capture the overall changes, wdyt? Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as CLI Command
participant SR as sandboxed_run
participant FJ as Firejail Check
User->>CLI: Run "source-declarative-manifest-sandboxed"
CLI->>SR: Invoke sandboxed_run()
SR->>FJ: Check for Firejail availability and options
FJ-->>SR: Return result (available or error)
SR->>CLI: Execute command wrapped with Firejail if enabled
Does this flow capture the new control flow clearly, wdyt? Possibly related PRs
Suggested labels
Suggested reviewers
Would you like to add or adjust any of these details, wdyt? ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
airbyte_cdk/cli/source_declarative_manifest/_sandboxed_run.py (4)
1-95: LGTM! Well-structured implementation of sandboxed executionThe implementation properly handles different sandbox modes based on the environment variable and gracefully handles cases where Firejail is not available. The code structure is clean, with clear separation of concerns.
There's a formatting issue reported by the pipeline. Would you like to fix it using Ruff format?
# Make sure to run: # ruff format airbyte_cdk/cli/source_declarative_manifest/_sandboxed_run.py🧰 Tools
🪛 GitHub Actions: Linters
[error] 1-1: Ruff formatting check failed. 1 file would be reformatted. Run 'ruff format' to fix code style issues in this file.
28-57: Consider enhancing the sandbox security configurationThe current Firejail configuration uses basic isolation with
--privateand--net=none, which is a good start for sandboxing. However, you might want to consider additional security flags to further restrict the sandbox environment. Would something like this be useful? wdyt?- return ["firejail", "--private", "--net=none"] + cmd + return ["firejail", "--private", "--net=none", "--seccomp", "--no-u2f", "--x11=none"] + cmd
41-53: Consider adding installation instructions for FirejailWhen Firejail is not found but required, it might be helpful to provide installation instructions in the error message, especially for users who are unfamiliar with Firejail. This would improve the user experience. wdyt?
- print("Firejail not found. Running without sandboxing.") + print("Firejail not found. Running without sandboxing. To enable sandboxing, install Firejail (e.g., 'apt-get install firejail' on Debian/Ubuntu).")
72-72: Consider handling the case when source-declarative-manifest is not foundThe code assumes that the base
source-declarative-manifestentrypoint is installed and on the PATH. Consider adding a check for this and providing a helpful error message if it's not found. wdyt?- executable = "source-declarative-manifest" # Assume base SDM entrypoint is installed and on PATH + executable = "source-declarative-manifest" # Assume base SDM entrypoint is installed and on PATH + try: + subprocess.run([executable, "--help"], check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + except FileNotFoundError: + print(f"Error: {executable} not found. Please ensure it is installed and on your PATH.") + sys.exit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Dockerfile(2 hunks)airbyte_cdk/cli/source_declarative_manifest/__init__.py(1 hunks)airbyte_cdk/cli/source_declarative_manifest/_sandboxed_run.py(1 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/cli/source_declarative_manifest/_sandboxed_run.py
[error] 1-1: Ruff formatting check failed. 1 file would be reformatted. Run 'ruff format' to fix code style issues in this file.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
airbyte_cdk/cli/source_declarative_manifest/__init__.py (1)
2-2: LGTM! Properly exports the new sandboxed_run functionThe changes correctly import and expose the new sandboxed_run function as part of the public API of this module, which aligns well with the PR objectives to implement sandboxed execution for the source-declarative-manifest connector.
Also applies to: 6-6
pyproject.toml (1)
114-114: LGTM! Script entry properly definedThe new script entry correctly maps the
source-declarative-manifest-sandboxedcommand to thesandboxed_runfunction, making it available as a CLI command. This matches the PR objectives for providing a sandboxed execution option.Dockerfile (3)
12-16: LGTM! Firejail installation looks goodThe installation of Firejail is properly implemented with cleanup steps to minimize the image size. This is essential for the sandboxed execution feature.
24-24: Removed fallback for poetry install - intentional change?I noticed that the fallback mechanism (
|| true) was removed from the poetry install command. This makes the build more strict, which is generally good for reliability, but could potentially break builds that previously succeeded with warnings. Was this change intentional? wdyt?
43-44: LGTM! Entrypoint updated to use sandboxed executionThe entrypoint has been correctly updated to use the new sandboxed command, which aligns with the PR objectives.
aaronsteers
changed the title
2 participants
Adds a sandbox wrapper and integrates the Sandboxed runner as the Docker image entrypoint for
source-declarative-manifestdocker image. Optionally, we can consider making this it's own image.Usage info is available via
source-declarative-manifest-sandboxed --help.Notes:
AIRBYTE_CONNECTOR_SANDBOX_MODE=FIREJAIL.connector-builder-server, we can invoke via the CLI instead of the Docker image, and by using thesource-declarative-manifest-sandboxedCLI, the invocation should (in theory) still be properly sandboxed.