Add Database Target and SSL Debug Logs for Scheduler Startup #3361#3362
Conversation
…y#3361 Add Database Target and SSL Debug Logs for Scheduler Startup bunkerity#3361
|
Single db-tls-validate.sh helper script so we can call it from linux scheduler and docker |
|
Hello @TheophileDiot the PR is ready for code audit and public preview |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Summary by Componentsrc/commonDatabase.py: Enhanced connection failure logging with URI masking and SSL/TLS diagnostics. Added db-tls-validate.sh: New shared shell helper script providing database URI parsing and TLS certificate validation utilities for both Linux scheduler and Docker environments. Exports three functions:
src/linuxbunkerweb-scheduler.sh: Added early src/schedulerentrypoint.sh: Mirrors Linux scheduler changes—sources db-tls-validate.sh, parses main.py: Added Impact AssessmentSecurity: Improves SSL/TLS diagnostics at scheduler startup; URI masking prevents credential exposure in logs. User-visible Changes: Enhanced startup logging now reports parsed database target details and TLS validation outcomes; no functional behaviour changes. Deployment: Adds shell helper script; relies on standard OpenSSL, Python URL parsing, and system CA bundle paths. Configuration/Schema: None. Documentation/Tests: Not addressed in this PR. WalkthroughDatabase URI masking and SSL/TLS diagnostics are now integrated across database initialisation pathways. A new shell utility provides URI parsing and certificate validation, called during scheduler startup for pre-connection verification of database TLS configurations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/common/db/Database.py`:
- Around line 352-357: The error logs in Database (self.logger.error calls that
currently include DATABASE_URI) must stop emitting the full connection URI and
must not prefer self.database_uri over the readonly URI when reporting which
target failed; update each logger.error (the ones after retry failures using
DATABASE_RETRY_TIMEOUT and in the dual-endpoint failure paths) to omit the URI
and instead log a non-secret target label plus the backend name (use a clear
label like "primary" or "readonly" derived from whether the attempt was against
self.database_uri or self.database_uri_readonly, and include
self.database_backend or similar backend identifier). Ensure the message uses
DATABASE_RETRY_TIMEOUT and the caught exception e, but replace any
_mask_db_uri(...) or direct URI interpolation with the non-secret target label
and backend name.
- Around line 333-341: The TLS hint logs inside the retry loop currently use
self.logger.error and should be lowered to avoid false-positive alerts; change
the two self.logger.error calls in the retrying connection path (the block that
logs "Database connection failed with SSL/TLS enabled..." and the subsequent
instruction about setting ssl_ca/sslrootcert) to self.logger.warning or
self.logger.debug (prefer warning for actionable hints, debug if very verbose)
and ensure only the final failure after exhausting retries still logs as error;
locate these calls in the Database class connection/retry method (the method
that performs the retry loop and currently calls self.logger.error with the
exception variable e) and update them accordingly.
- Around line 214-223: The helper functions _mask_db_uri and _ssl_expectations
currently catch broad Exception around sqlalchemy.engine.url.make_url(uri);
change those handlers to catch SQLAlchemy's specific ArgumentError instead
(import ArgumentError from sqlalchemy.exc) so only invalid-URI parsing falls
back to the existing masking logic and other unexpected errors propagate; keep
the existing fallback masking behavior unchanged.
In `@src/common/helpers/db-tls-validate.sh`:
- Around line 76-82: The printed assignments for variables (db_type, db_name,
host, port, user, ssl_enabled, pem_path) are emitted raw but are later consumed
via eval; update the print statements to emit shell-escaped values so untrusted
content cannot inject commands—use a shell-quoting helper such as Python's
shlex.quote (or an equivalent escape routine) to wrap each value before printing
(e.g., replace the current print(...) calls with prints that output
safely-quoted assignments using shlex.quote on db_type, db_name, host, port,
user, ssl_enabled, and pem_path).
- Around line 203-224: The Python expiry-check block (started in
cert_check=$(... python3 - << 'PY') only calls
x509.load_pem_x509_certificates(data) which fails for DER input; update the code
inside that heredoc to first try loading PEM via
x509.load_pem_x509_certificates(Path(pem_file).read_bytes()) and on ValueError
fall back to loading DER (use x509.load_der_x509_certificate for a single cert
or decode multiple DER certs if needed), then proceed to compute expiries as
before; ensure exceptions are still caught and that the PEM_FILE/ pem_path
variable names (PEM_FILE, pem_path) and the cert_check variable remain unchanged
so the surrounding shell logic continues to work.
- Around line 273-281: The openssl invocations build unsafe unquoted command
strings and do not handle IPv6 literals; update the code that constructs
direct_cmd and the later ip-based command to (1) add a helper (e.g.,
format_connect_target) that accepts host/ip and port and returns a properly
bracketed target for IPv6 like "[2001:db8::1]:5432" and plain "host:port" for
IPv4/hostnames, (2) stop concatenating into a single command string and instead
build an array for the command (e.g., cmd=(openssl s_client -connect "$target")
and append -CAfile "$pem_path" and -starttls options as separate array
elements), and (3) use the array form with timeout (or exec) so variables are
quoted and paths with spaces are safe; apply this change for both the direct_cmd
construction around direct_cmd/direc_cmd usage and the ip-based openssl
invocation in the later block. Ensure references: direct_cmd, $host/$ip/$port,
and the -starttls branches (postgres, mysql|mariadb) are updated to use the
helper and array-based command execution.
In `@src/linux/scripts/bunkerweb-scheduler.sh`:
- Around line 141-154: The parse_database_uri fallback branch currently leaves
port unset and does not re-run parsing after DATABASE_URI is replaced later;
update the error path in parse_database_uri handling so it initializes port
(e.g., set port="(unknown)" or a default) and ensure that after any code that
replaces DATABASE_URI (the block that updates DATABASE_URI around the later
lines) you call parse_database_uri again (or re-eval parse_output) so variables
produced by parse_database_uri—type, database, host, user, ssl_enabled,
pem_path, port—are refreshed before assigning db_ssl_enabled/db_pem_path/db_port
and before logging; locate the parse_database_uri invocation and the db_*
assignments (db_ssl_enabled/db_pem_path/db_port) to apply these fixes.
In `@src/scheduler/entrypoint.sh`:
- Around line 57-70: The fallback branch for parse_database_uri doesn't
initialise port (so db_port can be empty) and the script never re-parses after
DATABASE_URI is replaced; update the error path in parse_database_uri handling
to set port alongside type/database/host/user/ssl_enabled/pem_path (e.g.
port="(unknown)" or default port), and after you modify/replace DATABASE_URI
re-run parse_database_uri (call parse_database_uri and eval its output) before
assigning db_ssl_enabled/db_pem_path/db_port so that db_* variables reflect the
latest parse results; reference parse_database_uri, port, ssl_enabled, pem_path,
type, database, host, user, db_port, db_ssl_enabled and db_pem_path when making
the changes.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e05637c7-68c1-43c3-928f-6b2995f9dd92
📒 Files selected for processing (5)
src/common/db/Database.pysrc/common/helpers/db-tls-validate.shsrc/linux/scripts/bunkerweb-scheduler.shsrc/scheduler/entrypoint.shsrc/scheduler/main.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Follow BunkerWeb's Python standards and security posture:
- Use snake_case for functions and variables, PascalCase for classes, and provide concise, accurate docstrings for public classes, functions, and methods.
- Respect Black formatting with a 160-character line limit and the existing pre-commit conventions. Do not insist on adding type annotations to previously untyped code, but accept them when added consistently.
- Catch specific exceptions; never use bare
except:. CatchingExceptionis acceptable only at explicit process boundaries (for example scheduler loops, outer job runners, worker entrypoints, or graceful-shutdown boundaries) when the code logs enough context and either re-raises, returns an error status, or terminates safely.- Never use
os.system,subprocess.*(..., shell=True),eval, orexec. Pass subprocess arguments as a list and prefer explicit binary paths for privileged operations.- Do not use unsafe deserialisers (
pickle,marshal,shelve,jsonpickle,dill) for untrusted data. Useyaml.safe_load()rather than unsafe YAML loading.- Open files with an explicit encoding (normally
utf-8) and usewithstatements for files, sockets, database sessions, and temporary resources.- Use
secretsfor token generation andhmac.compare_digestfor token, HMAC, or signature comparisons.- For HTTP clients (
requests,httpx): always set an explicit timeout, validate destination URLs to block RFC1918/loopback/link-local ranges (SSRF), disable automatic redirects to internal hosts, and be careful with proxy settings.- For filesystem operations: resolve paths with
Path.resolve()and verify they remain under the intended base directory before reading or writing (path traversal).- Use
defusedxmlrather than stdlib XML parsers for untrusted XML.- For SQLAlchemy, use bound parameters and safe query construction. Never call
text()with f-strings or.format(), and flag.execute()calls built...
Files:
src/scheduler/main.pysrc/common/db/Database.py
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: Shell scripts must match BunkerWeb's portability expectations:
- If the script is POSIX shell, prefer
set -eu; if it explicitly requires Bash, useset -euo pipefail.- Quote variables and command substitutions consistently and prefer
${var}when concatenating.- Do not use Bash-only features in
/bin/shscripts.- Handle failures explicitly, use
trapfor cleanup where temporary files are created, and usemktempsafely.- Never use
curl | shorwget | sh; verify downloads by checksum or signature and avoid-k/--insecure.- Do not rely on inherited
PATHin privileged contexts; set it explicitly where needed.- Avoid
evaland unsafe command construction from untrusted data.
Files:
src/linux/scripts/bunkerweb-scheduler.shsrc/scheduler/entrypoint.shsrc/common/helpers/db-tls-validate.sh
🪛 Shellcheck (0.11.0)
src/linux/scripts/bunkerweb-scheduler.sh
[warning] 154-154: port is referenced but not assigned.
(SC2154)
src/scheduler/entrypoint.sh
[warning] 70-70: port is referenced but not assigned.
(SC2154)
src/common/helpers/db-tls-validate.sh
[style] 119-119: Use { ..; } instead of (..) to avoid subshell overhead.
(SC2235)
[style] 126-126: Use { ..; } instead of (..) to avoid subshell overhead.
(SC2235)
[info] 281-281: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 318-318: Double quote to prevent globbing and word splitting.
(SC2086)
🔇 Additional comments (1)
src/scheduler/main.py (1)
688-709: Nice simplification of scheduler DB initialisation.Reusing
_db_urifor both the target log andDatabase(...)removes the duplicated fallback expression and keeps the startup path consistent.
| try: | ||
| from sqlalchemy.engine.url import make_url | ||
|
|
||
| u = make_url(uri) | ||
| if u.password: | ||
| u = u.set(password="***") | ||
| return str(u) | ||
| except Exception: | ||
| # Fallback: mask :password@ in authority section | ||
| return re_sub(r"(//[^:/@]+:)[^@]*@", r"\1***@", uri) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/common/db/Database.py | head -230 | tail -30Repository: bunkerity/bunkerweb
Length of output: 1582
🏁 Script executed:
cat -n src/common/db/Database.py | sed -n '210,300p'Repository: bunkerity/bunkerweb
Length of output: 4407
🏁 Script executed:
# Check what exceptions make_url can raise
rg "def make_url" --type py -A 5Repository: bunkerity/bunkerweb
Length of output: 45
🏁 Script executed:
# Check the full function context for _mask_db_uri
ast-grep --pattern 'def _mask_db_uri($$$) {
$$$
}'Repository: bunkerity/bunkerweb
Length of output: 45
Catch specific exceptions from SQLAlchemy in helper functions, not Exception.
Lines 221 and 295 catch broad Exception in nested helper functions (_mask_db_uri() and _ssl_expectations()), which hides unexpected parser faults and masks programming errors. These are not process boundaries; they should catch only the exceptions make_url() actually raises.
SQLAlchemy's make_url() raises ArgumentError for invalid URIs. Catch that explicitly, and let other exceptions surface:
except ArgumentError:
# Fallback: mask :password@ in authority section
return re_sub(r"(//[^:/@]+:)[^@]*@", r"\1***@", uri)This aligns with the coding guideline: "Catch specific exceptions; never use bare except:. Catching Exception is acceptable only at explicit process boundaries (scheduler loops, job runners, worker entrypoints, graceful-shutdown boundaries)."
Applies to:
- Line 221 in
_mask_db_uri() - Line 295 in
_ssl_expectations()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/common/db/Database.py` around lines 214 - 223, The helper functions
_mask_db_uri and _ssl_expectations currently catch broad Exception around
sqlalchemy.engine.url.make_url(uri); change those handlers to catch SQLAlchemy's
specific ArgumentError instead (import ArgumentError from sqlalchemy.exc) so
only invalid-URI parsing falls back to the existing masking logic and other
unexpected errors propagate; keep the existing fallback masking behavior
unchanged.
| self.logger.error( | ||
| "Database connection failed with SSL/TLS enabled but no root CA configured. " | ||
| "Reason: %s", | ||
| e, | ||
| ) | ||
| self.logger.error( | ||
| "Set a CA file in DATABASE_URI (ssl_ca=... for MySQL/MariaDB, sslrootcert=... for PostgreSQL), " | ||
| "or disable SSL for testing only." | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use warning/debug for pre-timeout TLS hints.
These lines run inside the retry loop, so a transient startup failure can still emit error-level events even when the next retry succeeds. That is likely to create false-positive startup alerts.
Suggested change
- self.logger.error(
+ self.logger.warning(
"Database connection failed with SSL/TLS enabled but no root CA configured. "
"Reason: %s",
e,
)
- self.logger.error(
+ self.logger.warning(
"Set a CA file in DATABASE_URI (ssl_ca=... for MySQL/MariaDB, sslrootcert=... for PostgreSQL), "
"or disable SSL for testing only."
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.logger.error( | |
| "Database connection failed with SSL/TLS enabled but no root CA configured. " | |
| "Reason: %s", | |
| e, | |
| ) | |
| self.logger.error( | |
| "Set a CA file in DATABASE_URI (ssl_ca=... for MySQL/MariaDB, sslrootcert=... for PostgreSQL), " | |
| "or disable SSL for testing only." | |
| ) | |
| self.logger.warning( | |
| "Database connection failed with SSL/TLS enabled but no root CA configured. " | |
| "Reason: %s", | |
| e, | |
| ) | |
| self.logger.warning( | |
| "Set a CA file in DATABASE_URI (ssl_ca=... for MySQL/MariaDB, sslrootcert=... for PostgreSQL), " | |
| "or disable SSL for testing only." | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/common/db/Database.py` around lines 333 - 341, The TLS hint logs inside
the retry loop currently use self.logger.error and should be lowered to avoid
false-positive alerts; change the two self.logger.error calls in the retrying
connection path (the block that logs "Database connection failed with SSL/TLS
enabled..." and the subsequent instruction about setting ssl_ca/sslrootcert) to
self.logger.warning or self.logger.debug (prefer warning for actionable hints,
debug if very verbose) and ensure only the final failure after exhausting
retries still logs as error; locate these calls in the Database class
connection/retry method (the method that performs the retry loop and currently
calls self.logger.error with the exception variable e) and update them
accordingly.
| self.logger.error( | ||
| "Can't connect to database after %s seconds: %s (DATABASE_URI=%s)", | ||
| DATABASE_RETRY_TIMEOUT, | ||
| e, | ||
| _mask_db_uri(self.database_uri or self.database_uri_readonly), | ||
| ) |
There was a problem hiding this comment.
Stop logging DATABASE_URI in these paths.
Line 356, Line 373 and Line 380 still emit the connection URI, which the repo rules require you to scrub from logs. They also pick self.database_uri after the read-only fallback because self.database_uri or self.database_uri_readonly always prefers the primary target, so the message can identify the wrong backend during a dual-endpoint failure. Log a non-secret target label plus the backend name instead.
Suggested change
+ current_uri = self.database_uri_readonly if fallback or self.readonly else (self.database_uri or self.database_uri_readonly)
+ current_target = "readonly" if fallback or self.readonly else "primary"
+ current_backend = current_uri.split(":", 1)[0] if current_uri else "unknown"
self.logger.error(
- "Can't connect to database after %s seconds: %s (DATABASE_URI=%s)",
+ "Can't connect to %s database target (%s) after %s seconds: %s",
+ current_target,
+ current_backend,
DATABASE_RETRY_TIMEOUT,
e,
- _mask_db_uri(self.database_uri or self.database_uri_readonly),
)
@@
self.logger.warning(
- "Can't connect to database (%s). Retrying in 5 seconds ... (DATABASE_URI=%s)",
+ "Can't connect to %s database target (%s): %s. Retrying in 5 seconds ...",
+ current_target,
+ current_backend,
e,
- _mask_db_uri(self.database_uri or self.database_uri_readonly),
)
@@
self.logger.error(
- "Error when trying to connect to the database: %s (DATABASE_URI=%s)",
+ "Error when trying to connect to the %s database target (%s): %s",
+ current_target,
+ current_backend,
e,
- _mask_db_uri(self.database_uri or self.database_uri_readonly),
)As per coding guidelines, "Scrub secrets, tokens, cookies, database URIs, and Authorization headers from logs. Use the logging framework rather than print()."
Also applies to: 370-381
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/common/db/Database.py` around lines 352 - 357, The error logs in Database
(self.logger.error calls that currently include DATABASE_URI) must stop emitting
the full connection URI and must not prefer self.database_uri over the readonly
URI when reporting which target failed; update each logger.error (the ones after
retry failures using DATABASE_RETRY_TIMEOUT and in the dual-endpoint failure
paths) to omit the URI and instead log a non-secret target label plus the
backend name (use a clear label like "primary" or "readonly" derived from
whether the attempt was against self.database_uri or self.database_uri_readonly,
and include self.database_backend or similar backend identifier). Ensure the
message uses DATABASE_RETRY_TIMEOUT and the caught exception e, but replace any
_mask_db_uri(...) or direct URI interpolation with the non-secret target label
and backend name.
| print(f'type="{db_type or "(unknown)"}"') | ||
| print(f'database="{db_name or "(none)"}"') | ||
| print(f'host="{host or "(local)"}"') | ||
| print(f'port="{port or "(default)"}"') | ||
| print(f'user="{user or "(none)"}"') | ||
| print(f'ssl_enabled="{ssl_enabled}"') | ||
| print(f'pem_path="{pem_path or "(none)"}"') |
There was a problem hiding this comment.
Shell-escape these assignments before callers eval them.
These values come from DATABASE_URI, but the helper prints them inside double quotes and both callers immediately eval the result. A crafted username, database name, or ssl_ca value containing $(...) will execute during startup.
Suggested fix
-from urllib.parse import urlparse, parse_qs, unquote
+from urllib.parse import urlparse, parse_qs, unquote
+from shlex import quote
@@
-print(f'type="{db_type or "(unknown)"}"')
-print(f'database="{db_name or "(none)"}"')
-print(f'host="{host or "(local)"}"')
-print(f'port="{port or "(default)"}"')
-print(f'user="{user or "(none)"}"')
-print(f'ssl_enabled="{ssl_enabled}"')
-print(f'pem_path="{pem_path or "(none)"}"')
+def _emit(name: str, value: str) -> None:
+ print(f"{name}={quote(value)}")
+
+_emit("type", db_type or "(unknown)")
+_emit("database", db_name or "(none)")
+_emit("host", host or "(local)")
+_emit("port", port or "(default)")
+_emit("user", user or "(none)")
+_emit("ssl_enabled", ssl_enabled)
+_emit("pem_path", pem_path or "(none)")As per coding guidelines, "Avoid eval and unsafe command construction from untrusted data."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/common/helpers/db-tls-validate.sh` around lines 76 - 82, The printed
assignments for variables (db_type, db_name, host, port, user, ssl_enabled,
pem_path) are emitted raw but are later consumed via eval; update the print
statements to emit shell-escaped values so untrusted content cannot inject
commands—use a shell-quoting helper such as Python's shlex.quote (or an
equivalent escape routine) to wrap each value before printing (e.g., replace the
current print(...) calls with prints that output safely-quoted assignments using
shlex.quote on db_type, db_name, host, port, user, ssl_enabled, and pem_path).
| # PEM/DER format validation | ||
| if ! timeout 2s openssl x509 -in "$pem_path" -noout >/dev/null 2>&1; then | ||
| # Try DER format | ||
| if ! timeout 2s openssl x509 -inform DER -in "$pem_path" -noout >/dev/null 2>&1; then | ||
| log "$LOG_CONTEXT" "❌" "PEM certificate is not valid X.509 format: $pem_path" | ||
| return 1 | ||
| fi | ||
| fi | ||
|
|
||
| # Certificate expiry validation - skip for system CA bundles (they may contain expired certs) | ||
| if [ "$is_system_ca" != "yes" ]; then | ||
| local cert_check | ||
| cert_check=$(timeout 2s env PEM_FILE="$pem_path" python3 - << 'PY' | ||
| from os import getenv | ||
| from pathlib import Path | ||
| from datetime import datetime, timezone | ||
| from cryptography import x509 | ||
|
|
||
| try: | ||
| pem_file = getenv("PEM_FILE", "") | ||
| data = Path(pem_file).read_bytes() | ||
| certs = x509.load_pem_x509_certificates(data) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -n 240 src/common/helpers/db-tls-validate.sh | tail -n +195Repository: bunkerity/bunkerweb
Length of output: 1501
🏁 Script executed:
sed -n '213,245p' src/common/helpers/db-tls-validate.shRepository: bunkerity/bunkerweb
Length of output: 936
🌐 Web query:
cryptography x509 load_der_x509_certificate load_pem_x509_certificates Python
💡 Result:
In Python’s cryptography library, these X.509 loaders differ by encoding (PEM vs DER) and how many certs they can parse:
-
DER (binary ASN.1) → single cert
from cryptography import x509 der_bytes = open("cert.der", "rb").read() cert = x509.load_der_x509_certificate(der_bytes) # -> x509.Certificate
-
PEM (base64 text with BEGIN/END markers) → single cert
from cryptography import x509 pem_bytes = open("cert.pem", "rb").read() cert = x509.load_pem_x509_certificate(pem_bytes) # -> x509.Certificate
-
PEM chain/bundle (multiple adjacent PEM blocks) → multiple certs
from cryptography import x509 pem_chain_bytes = open("chain.pem", "rb").read() certs = x509.load_pem_x509_certificates(pem_chain_bytes) # -> list[x509.Certificate]
This is specifically intended for “multiple certificates (as adjacent PEMs) at once.” (cryptography.io)
Citations:
- 1: https://cryptography.io/en/3.4.8/hazmat/backends/interfaces.html?utm_source=openai
- 2: https://cryptography.io/en/latest/x509/reference/?utm_source=openai
- 3: https://cryptography.io/en/latest/x509/reference/?utm_source=openai
DER certificates pass shell validation but fail Python expiry check unconditionally.
Whilst lines 204–206 accept DER format via openssl x509 -inform DER, line 224 calls only x509.load_pem_x509_certificates(data), which raises ValueError when given DER input. The exception is caught at line 238 and prints ERROR, causing the validation to fail.
Fix required
Attempt PEM first, then fall back to DER on failure:
- certs = x509.load_pem_x509_certificates(data)
+ try:
+ certs = list(x509.load_pem_x509_certificates(data))
+ except ValueError:
+ certs = [x509.load_der_x509_certificate(data)]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # PEM/DER format validation | |
| if ! timeout 2s openssl x509 -in "$pem_path" -noout >/dev/null 2>&1; then | |
| # Try DER format | |
| if ! timeout 2s openssl x509 -inform DER -in "$pem_path" -noout >/dev/null 2>&1; then | |
| log "$LOG_CONTEXT" "❌" "PEM certificate is not valid X.509 format: $pem_path" | |
| return 1 | |
| fi | |
| fi | |
| # Certificate expiry validation - skip for system CA bundles (they may contain expired certs) | |
| if [ "$is_system_ca" != "yes" ]; then | |
| local cert_check | |
| cert_check=$(timeout 2s env PEM_FILE="$pem_path" python3 - << 'PY' | |
| from os import getenv | |
| from pathlib import Path | |
| from datetime import datetime, timezone | |
| from cryptography import x509 | |
| try: | |
| pem_file = getenv("PEM_FILE", "") | |
| data = Path(pem_file).read_bytes() | |
| certs = x509.load_pem_x509_certificates(data) | |
| # PEM/DER format validation | |
| if ! timeout 2s openssl x509 -in "$pem_path" -noout >/dev/null 2>&1; then | |
| # Try DER format | |
| if ! timeout 2s openssl x509 -inform DER -in "$pem_path" -noout >/dev/null 2>&1; then | |
| log "$LOG_CONTEXT" "❌" "PEM certificate is not valid X.509 format: $pem_path" | |
| return 1 | |
| fi | |
| fi | |
| # Certificate expiry validation - skip for system CA bundles (they may contain expired certs) | |
| if [ "$is_system_ca" != "yes" ]; then | |
| local cert_check | |
| cert_check=$(timeout 2s env PEM_FILE="$pem_path" python3 - << 'PY' | |
| from os import getenv | |
| from pathlib import Path | |
| from datetime import datetime, timezone | |
| from cryptography import x509 | |
| try: | |
| pem_file = getenv("PEM_FILE", "") | |
| data = Path(pem_file).read_bytes() | |
| try: | |
| certs = list(x509.load_pem_x509_certificates(data)) | |
| except ValueError: | |
| certs = [x509.load_der_x509_certificate(data)] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/common/helpers/db-tls-validate.sh` around lines 203 - 224, The Python
expiry-check block (started in cert_check=$(... python3 - << 'PY') only calls
x509.load_pem_x509_certificates(data) which fails for DER input; update the code
inside that heredoc to first try loading PEM via
x509.load_pem_x509_certificates(Path(pem_file).read_bytes()) and on ValueError
fall back to loading DER (use x509.load_der_x509_certificate for a single cert
or decode multiple DER certs if needed), then proceed to compute expiries as
before; ensure exceptions are still caught and that the PEM_FILE/ pem_path
variable names (PEM_FILE, pem_path) and the cert_check variable remain unchanged
so the surrounding shell logic continues to work.
| local direct_cmd="openssl s_client -connect $host:$port" | ||
| [ -f "$pem_path" ] && direct_cmd="$direct_cmd -CAfile $pem_path" | ||
| case "$db_type" in | ||
| postgres) direct_cmd="$direct_cmd -starttls postgres" ;; | ||
| mysql|mariadb) direct_cmd="$direct_cmd -starttls mysql" ;; | ||
| esac | ||
|
|
||
| log "$LOG_CONTEXT" "ℹ️" "No DNS resolution for ${host}, testing direct connection" | ||
| if echo "Q" | timeout 2s $direct_cmd >/dev/null 2>&1; then |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/common/helpers/db-tls-validate.sh | sed -n '265,320p'Repository: bunkerity/bunkerweb
Length of output: 2473
🏁 Script executed:
# Check openssl s_client documentation for IPv6 handling
man openssl-s_client 2>/dev/null | grep -A 5 "\-connect" | head -20Repository: bunkerity/bunkerweb
Length of output: 45
🏁 Script executed:
# Search for calls to this validation function to see if IPv6 might be passed
rg "db_tls_validate|db-tls-validate" --type sh -B 2 -A 2Repository: bunkerity/bunkerweb
Length of output: 857
🏁 Script executed:
# Check for IPv6 address patterns or usage in the codebase
rg "[0-9a-fA-F:]{2,}" --type sh | head -10Repository: bunkerity/bunkerweb
Length of output: 824
🏁 Script executed:
# Find the function definition of db_tls_validate
rg "db_tls_validate()" src/common/helpers/db-tls-validate.sh -A 10Repository: bunkerity/bunkerweb
Length of output: 45
🏁 Script executed:
# Check openssl s_client help for IPv6 syntax requirements
openssl s_client -help 2>&1 | grep -i connect || echo "Help not available, checking online docs"Repository: bunkerity/bunkerweb
Length of output: 103
🏁 Script executed:
# Find the function definition
grep -n "^db_tls_validate" src/common/helpers/db-tls-validate.shRepository: bunkerity/bunkerweb
Length of output: 45
🏁 Script executed:
# Check how the host parameter is derived
sed -n '1,272p' src/common/helpers/db-tls-validate.sh | tail -50Repository: bunkerity/bunkerweb
Length of output: 1584
🌐 Web query:
openssl s_client -connect IPv6 address syntax documentation
💡 Result:
For openssl s_client, when the host is an IPv6 literal, you must wrap the address in square brackets so OpenSSL can still parse the host:port separator:
openssl s_client -connect "[2001:db8::1]:443"
# or (positional target form)
openssl s_client "[2001:db8::1]:443"This same [...] rule also applies to options like -bind and -proxy when their host is an IPv6 address. (docs.openssl.org)
Citations:
🏁 Script executed:
# Find where the function is defined and its parameters
head -270 src/common/helpers/db-tls-validate.sh | grep -n "^db_tls_validate\|^function db_tls_validate" -A 5Repository: bunkerity/bunkerweb
Length of output: 45
🏁 Script executed:
# Search for the function definition more broadly
grep -n "db_tls_validate" src/common/helpers/db-tls-validate.sh | head -5Repository: bunkerity/bunkerweb
Length of output: 45
🏁 Script executed:
# Check the complete beginning of the file for function signature
head -50 src/common/helpers/db-tls-validate.shRepository: bunkerity/bunkerweb
Length of output: 1715
🏁 Script executed:
# Find validate_db_tls_certificate function definition
grep -n "validate_db_tls_certificate" src/common/helpers/db-tls-validate.sh | head -3Repository: bunkerity/bunkerweb
Length of output: 296
🏁 Script executed:
# Get the function signature
sed -n '180,270p' src/common/helpers/db-tls-validate.sh | head -100Repository: bunkerity/bunkerweb
Length of output: 3022
🏁 Script executed:
# Get the full function definition line
sed -n '149,160p' src/common/helpers/db-tls-validate.shRepository: bunkerity/bunkerweb
Length of output: 751
🏁 Script executed:
# Check how the host parameter is obtained and if IPv6 is possible
sed -n '40,150p' src/common/helpers/db-tls-validate.shRepository: bunkerity/bunkerweb
Length of output: 4354
Fix IPv6 literal handling and unsafe command construction in openssl invocations.
Lines 273–281 and 300–318 construct -connect targets as unquoted ${host}:${port} and ${ip}:${port} strings. This breaks IPv6 literals (Python's urlparse().hostname returns bare addresses like 2001:db8::1 without brackets; openssl requires [2001:db8::1]:5432), and unquoted command strings split paths containing spaces. Refactor to use arrays with a helper function to format targets correctly:
Suggested fix
+format_connect_target() {
+ case "$1" in
+ *:*) printf '[%s]:%s' "$1" "$2" ;;
+ *) printf '%s:%s' "$1" "$2" ;;
+ esac
+}
@@
- local direct_cmd="openssl s_client -connect $host:$port"
- [ -f "$pem_path" ] && direct_cmd="$direct_cmd -CAfile $pem_path"
+ local connect_target
+ connect_target=$(format_connect_target "$host" "$port")
+ local -a direct_cmd=(openssl s_client -connect "$connect_target")
+ [ -f "$pem_path" ] && direct_cmd+=(-CAfile "$pem_path")
@@
- if echo "Q" | timeout 2s $direct_cmd >/dev/null 2>&1; then
+ if echo "Q" | timeout 2s "${direct_cmd[@]}" >/dev/null 2>&1; then
@@
- local ip_cmd="openssl s_client -connect $ip:$port"
+ local connect_target
+ connect_target=$(format_connect_target "$ip" "$port")
+ local -a ip_cmd=(openssl s_client -connect "$connect_target")
@@
- ip_cmd="$ip_cmd -CAfile $pem_path"
+ ip_cmd+=(-CAfile "$pem_path")
@@
- ip_cmd="$ip_cmd -starttls postgres"
+ ip_cmd+=(-starttls postgres)
@@
- ip_cmd="$ip_cmd -starttls mysql"
+ ip_cmd+=(-starttls mysql)
@@
- if echo "Q" | timeout 2s $ip_cmd >/dev/null 2>&1; then
+ if echo "Q" | timeout 2s "${ip_cmd[@]}" >/dev/null 2>&1; thenThis complies with the coding guidelines: proper quoting of variables, avoiding unsafe command construction, and defending against word-splitting from spaces in paths or IPv6 colons.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 281-281: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/common/helpers/db-tls-validate.sh` around lines 273 - 281, The openssl
invocations build unsafe unquoted command strings and do not handle IPv6
literals; update the code that constructs direct_cmd and the later ip-based
command to (1) add a helper (e.g., format_connect_target) that accepts host/ip
and port and returns a properly bracketed target for IPv6 like
"[2001:db8::1]:5432" and plain "host:port" for IPv4/hostnames, (2) stop
concatenating into a single command string and instead build an array for the
command (e.g., cmd=(openssl s_client -connect "$target") and append -CAfile
"$pem_path" and -starttls options as separate array elements), and (3) use the
array form with timeout (or exec) so variables are quoted and paths with spaces
are safe; apply this change for both the direct_cmd construction around
direct_cmd/direc_cmd usage and the ip-based openssl invocation in the later
block. Ensure references: direct_cmd, $host/$ip/$port, and the -starttls
branches (postgres, mysql|mariadb) are updated to use the helper and array-based
command execution.
| if parse_output=$(parse_database_uri); then | ||
| eval "$parse_output" | ||
| else | ||
| log "SYSTEMCTL" "⚠️" "Failed to parse DATABASE_URI (error), using defaults" | ||
| type="(unknown)" | ||
| database="(none)" | ||
| host="(local)" | ||
| user="(none)" | ||
| ssl_enabled="no" | ||
| pem_path="(none)" | ||
| fi | ||
| db_ssl_enabled="$ssl_enabled" | ||
| db_pem_path="$pem_path" | ||
| db_port="$port" |
There was a problem hiding this comment.
Re-parse after Line 222 and initialise port in the fallback.
Line 222 replaces DATABASE_URI, but Line 227 still logs type/database/host from the first parse. Also, the error path on Lines 145-150 never sets port, so db_port and the later log can end up blank.
Suggested fix
else
log "SYSTEMCTL" "⚠️" "Failed to parse DATABASE_URI (error), using defaults"
type="(unknown)"
database="(none)"
host="(local)"
+ port="(default)"
user="(none)"
ssl_enabled="no"
pem_path="(none)"
fi
@@
DATABASE_URI=$(cat /var/tmp/bunkerweb/database_uri)
export DATABASE_URI
rm -f /var/tmp/bunkerweb/database_uri
+
+ if parse_output=$(parse_database_uri); then
+ eval "$parse_output"
+ else
+ type="(unknown)"
+ database="(none)"
+ host="(local)"
+ port="(default)"
+ user="(none)"
+ ssl_enabled="no"
+ pem_path="(none)"
+ fi
# Log parsed database connection info
log "SYSTEMCTL" "ℹ️" "Scheduler DB target: type=${type} database=${database} host=${host} port=${port} user=${user} ssl=${ssl_enabled} pem=${pem_path}"Also applies to: 222-227
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 154-154: port is referenced but not assigned.
(SC2154)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/linux/scripts/bunkerweb-scheduler.sh` around lines 141 - 154, The
parse_database_uri fallback branch currently leaves port unset and does not
re-run parsing after DATABASE_URI is replaced later; update the error path in
parse_database_uri handling so it initializes port (e.g., set port="(unknown)"
or a default) and ensure that after any code that replaces DATABASE_URI (the
block that updates DATABASE_URI around the later lines) you call
parse_database_uri again (or re-eval parse_output) so variables produced by
parse_database_uri—type, database, host, user, ssl_enabled, pem_path, port—are
refreshed before assigning db_ssl_enabled/db_pem_path/db_port and before
logging; locate the parse_database_uri invocation and the db_* assignments
(db_ssl_enabled/db_pem_path/db_port) to apply these fixes.
| if parse_output=$(parse_database_uri); then | ||
| eval "$parse_output" | ||
| else | ||
| log "ENTRYPOINT" "⚠️" "Failed to parse DATABASE_URI (error), using defaults" | ||
| type="(unknown)" | ||
| database="(none)" | ||
| host="(local)" | ||
| user="(none)" | ||
| ssl_enabled="no" | ||
| pem_path="(none)" | ||
| fi | ||
| db_ssl_enabled="$ssl_enabled" | ||
| db_pem_path="$pem_path" | ||
| db_port="$port" |
There was a problem hiding this comment.
Re-parse after Line 124 and initialise port in the fallback.
Line 124 replaces DATABASE_URI, but Line 129 still logs type/database/host from the first parse. Also, the error path on Lines 61-66 never sets port, so db_port and the later log can end up blank.
Suggested fix
else
log "ENTRYPOINT" "⚠️" "Failed to parse DATABASE_URI (error), using defaults"
type="(unknown)"
database="(none)"
host="(local)"
+ port="(default)"
user="(none)"
ssl_enabled="no"
pem_path="(none)"
fi
@@
DATABASE_URI=$(cat /var/tmp/bunkerweb/database_uri)
export DATABASE_URI
rm -f /var/tmp/bunkerweb/database_uri
+
+if parse_output=$(parse_database_uri); then
+ eval "$parse_output"
+else
+ type="(unknown)"
+ database="(none)"
+ host="(local)"
+ port="(default)"
+ user="(none)"
+ ssl_enabled="no"
+ pem_path="(none)"
+fi
# Log parsed database connection info
log "ENTRYPOINT" "ℹ️" "Scheduler DB target: type=${type} database=${database} host=${host} port=${port} user=${user} ssl=${ssl_enabled} pem=${pem_path}"Also applies to: 124-129
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 70-70: port is referenced but not assigned.
(SC2154)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/scheduler/entrypoint.sh` around lines 57 - 70, The fallback branch for
parse_database_uri doesn't initialise port (so db_port can be empty) and the
script never re-parses after DATABASE_URI is replaced; update the error path in
parse_database_uri handling to set port alongside
type/database/host/user/ssl_enabled/pem_path (e.g. port="(unknown)" or default
port), and after you modify/replace DATABASE_URI re-run parse_database_uri (call
parse_database_uri and eval its output) before assigning
db_ssl_enabled/db_pem_path/db_port so that db_* variables reflect the latest
parse results; reference parse_database_uri, port, ssl_enabled, pem_path, type,
database, host, user, db_port, db_ssl_enabled and db_pem_path when making the
changes.
2 participants
Add Database Target and SSL Debug Logs for Scheduler Startup #3361