fix(tasks): force system OpenSSL in testssl to fix missing libproviders.so in Docker#1134
fix(tasks): force system OpenSSL in testssl to fix missing libproviders.so in Docker#1134ocervell wants to merge 1 commit into
Conversation
…rs.so in Docker testssl.sh bundles its own OpenSSL 3.x binary but the provider library (libproviders.so) is absent when installed via git clone, causing "Error configuring OpenSSL" before any scan can start. Setting the OPENSSL env var to the system openssl binary bypasses the bundled one. Also adds a generic `extra_env` dict to the Command base class so any task can inject subprocess environment variables cleanly. Fixes #1056 (Bug 3) Co-authored-by: Olivier Cervello <ocervell@users.noreply.github.com>
WalkthroughThe Command runner now supports injecting extra environment variables into subprocesses via a new ChangesEnvironment Variable Injection for Subprocesses
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
secator/tasks/testssl.py (1)
72-76: ⚡ Quick winConsider logging when system OpenSSL is not found.
The code silently falls back to bundled OpenSSL when
shutil.which('openssl')returnsNone. While this best-effort approach is reasonable, users may not realize the fix didn't apply when system OpenSSL is unavailable. A debug log or info message would improve observability.📝 Optional logging enhancement
`@staticmethod` def on_cmd(self): # Force system OpenSSL to avoid missing libproviders.so with bundled OpenSSL in Docker openssl_path = shutil.which('openssl') if openssl_path: self.extra_env = {'OPENSSL': openssl_path} + self.debug(f'Using system OpenSSL: {openssl_path}', sub='init') + else: + self.debug('System OpenSSL not found, using bundled OpenSSL', sub='init') output_path = self.get_opt_value(OUTPUT_PATH)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@secator/tasks/testssl.py` around lines 72 - 76, In on_cmd, detect when shutil.which('openssl') returns None and emit a log (debug or info) so users know the system OpenSSL wasn't found and the task will use the bundled OpenSSL; update the behavior around the openssl_path check (the openssl_path variable and assignment to self.extra_env) to log a clear message like "system openssl not found, falling back to bundled openssl" (use the task's logger or the module logger) so it's recorded when the fallback occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@secator/runners/command.py`:
- Around line 63-64: The class currently defines a class-level mutable default
extra_env = {} which can lead to shared-state bugs if instances mutate it;
change the declaration to extra_env = None and in the class __init__ (e.g.,
inside the constructor for the runner/command class) set self.extra_env = {}
when it's None, and update any code that expects to assign rather than mutate
this dict (references: extra_env, self.extra_env, __init__, and usages such as
in testssl.py) so each instance gets its own dict.
---
Nitpick comments:
In `@secator/tasks/testssl.py`:
- Around line 72-76: In on_cmd, detect when shutil.which('openssl') returns None
and emit a log (debug or info) so users know the system OpenSSL wasn't found and
the task will use the bundled OpenSSL; update the behavior around the
openssl_path check (the openssl_path variable and assignment to self.extra_env)
to log a clear message like "system openssl not found, falling back to bundled
openssl" (use the task's logger or the module logger) so it's recorded when the
fallback occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c93b459-1a86-41ba-b64e-58ba72739e31
📒 Files selected for processing (2)
secator/runners/command.pysecator/tasks/testssl.py
| # Extra environment variables for subprocess | ||
| extra_env = {} |
There was a problem hiding this comment.
Use immutable default or initialize in __init__ to avoid shared mutable state.
The class-level mutable default {} can cause subtle bugs if a task mutates self.extra_env (e.g., self.extra_env['KEY'] = 'value') instead of assigning a new dict. While the current usage in testssl.py uses assignment (safe), this pattern is a footgun for future task implementations.
🛡️ Recommended fix
Option 1: Initialize to None and set in __init__
- # Extra environment variables for subprocess
- extra_env = {}
+ # Extra environment variables for subprocess
+ extra_env = NoneThen in __init__ (after line 142):
# Initialize extra_env if not set
if self.extra_env is None:
self.extra_env = {}And update line 532:
- env = {**os.environ, **self.extra_env}
+ env = {**os.environ, **(self.extra_env or {})}Option 2: Document the assignment-only contract
Add a docstring comment:
# Extra environment variables for subprocess
+ # Note: Tasks must use assignment (self.extra_env = {...}), not mutation
extra_env = {}🧰 Tools
🪛 Ruff (0.15.15)
[warning] 64-64: Mutable default value for class attribute
(RUF012)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@secator/runners/command.py` around lines 63 - 64, The class currently defines
a class-level mutable default extra_env = {} which can lead to shared-state bugs
if instances mutate it; change the declaration to extra_env = None and in the
class __init__ (e.g., inside the constructor for the runner/command class) set
self.extra_env = {} when it's None, and update any code that expects to assign
rather than mutate this dict (references: extra_env, self.extra_env, __init__,
and usages such as in testssl.py) so each instance gets its own dict.
Source: Linters/SAST tools
1 participant
testssl.sh bundles its own OpenSSL 3.x binary, but when installed via git clone, the libproviders.so provider library is absent from the bundle. This causes "Error configuring OpenSSL" before any scan can start.
Fix: Set the OPENSSL environment variable to the system openssl binary so testssl.sh bypasses its broken bundled OpenSSL.
Also adds a generic
extra_envdict to the Command base class so any task can inject subprocess environment variables cleanly.Fixes #1056 (Bug 3)
Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes