• 🔍 Trigger a full review

Walkthrough

Adds support for reading sensitive configuration values from external secret files. Implementation includes a new file-source value type, secret file loading with size validation and retry logic, integration into dynamic configuration reload flow, catalog display updates, and comprehensive test coverage across multiple authentication paths.

Changes

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Suggested labels

Core

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 help to get the list of available commands and usage tips.}

@CodeRabbit please critically review this change

#!/bin/bash

# Get the PR diff to understand what changed
gh pr diff 6719 --repo questdb/questdb

echo "=== Listing all changed files ==="
gh pr view 6719 --repo questdb/questdb --json files --jq '.files[].path'

echo -e "\n=== Finding test files related to PropServerConfiguration ==="
fd -t f -e java -e kt | xargs rg -l "PropServerConfiguration" | grep -i test

echo -e "\n=== Finding test files that test file-based secrets ==="
rg -l "PASSWORD_FILE|password.*file|_FILE" --type java --type kotlin | grep -i test

@bluestreak01, I'll provide a critical review of this change. The feature implementation has several security vulnerabilities and insufficient test coverage for a security-sensitive feature.

🔴 Critical Security Issues

1. Path Traversal Vulnerability

The readSecretFromFile() method accepts file paths without validation. An attacker could potentially use path traversal:

// No validation here - allows ../../../etc/passwd
protected String readSecretFromFile(String filePath) {
    try (Path path = new Path()) {
        path.of(filePath);  // ← No sanitization
        fd = filesFacade.openRO(path.$());

Recommendation: Validate that the file path is within expected directories (e.g., /run/secrets/, /var/secrets/). Consider using a whitelist approach or canonicalize paths and check boundaries.

2. No File Type Validation

The code doesn't verify that the path is a regular file. It could potentially read:

• Directories (will fail but waste resources)
• Character/block devices (could hang or expose system info)
• FIFOs/pipes (could hang indefinitely)
• Symlinks to unauthorized locations

Recommendation: Add checks using filesFacade to verify it's a regular file before opening.

3. Symlink Following Without Validation

The implementation follows symbolic links without restriction, which could be exploited to read arbitrary files:

# Attacker creates symlink
ln -s /etc/shadow /run/secrets/pg_password

Recommendation: Either disable symlink following or validate the resolved path is within allowed directories.

4. Information Disclosure in Logs

File paths are logged at INFO level, potentially exposing deployment topology:

log.info().$("reading secret from file [key=").$(key.getPropertyPath()).$(", file=").$(secretFilePath).I$();

Recommendation: Consider logging at DEBUG level or omit file paths in production.

🟡 Error Handling Gaps

5. Silent Failure in checkSecretFileChanges()

When file reading fails during reload, the error is logged but reload continues:

} catch (CairoException e) {
    LOG.error().$("could not read secret file for reload check...").I$();
    continue;  // ← Silent failure
}

This means a deleted or corrupted secret file won't prevent startup with stale credentials.

Recommendation: Consider making this a hard failure or at least setting a flag that the configuration is degraded.

6. Missing UTF-8 Validation

The code uses Utf8s.stringFromUtf8Bytes() without catching potential exceptions from malformed UTF-8:

return Utf8s.stringFromUtf8Bytes(address, address + size).trim();

Recommendation: Wrap in try-catch and provide clear error message.

7. Empty File Behavior

Files containing only whitespace will result in empty password after trimming:

if (size == 0) {
    return "";  // ← Intentional empty password?
}
// ...
return Utf8s.stringFromUtf8Bytes(address, address + size).trim();  // ← Could also be empty

Recommendation: Decide if empty passwords should be rejected and validate accordingly.

🟠 Test Coverage Deficiencies

The 19.15% coverage for security-critical code is concerning. Missing test cases:

Error Scenarios (0% covered):

• ❌ File not found
• ❌ Permission denied (chmod 000)
• ❌ File exceeds 64KB limit
• ❌ File is a directory
• ❌ File is a device/pipe/socket
• ❌ File contains invalid UTF-8
• ❌ File is deleted during reload
• ❌ File permissions change to unreadable
• ❌ Disk I/O errors
• ❌ Out of memory during file read

Edge Cases (0% covered):

• ❌ Empty file
• ❌ File with only whitespace
• ❌ File with trailing newlines (common with Kubernetes secrets)
• ❌ Symbolic link to another secret file
• ❌ Symbolic link to non-existent file
• ❌ Relative vs absolute paths
• ❌ Concurrent file reads during reload
• ❌ File modified between size check and read

Integration Tests Missing:

• ❌ http.password from file
• ❌ pg.readonly.password from file
• ❌ Multiple passwords from files simultaneously
• ❌ Mixed file and direct password configs
• ❌ Reload when switching from file to direct password (reverse direction)

🔵 Design & Performance Concerns

8. Removed Optimization

-        if (newProperties.equals(oldProperties)) {
-            return false;
-        }

This early-return optimization was removed, meaning every reload now iterates through all properties even when nothing changed. This could impact systems with frequent reload calls.

Recommendation: Restore optimization but add separate check for secret file changes.

9. N+1 File Reads on Reload

checkSecretFileChanges() reads each secret file individually during reload. With multiple secrets, this creates multiple I/O operations.

Recommendation: Consider batching or at least document the I/O overhead.

10. Race Condition Window

Between checking file size and reading contents, the file could be modified:

size = filesFacade.length(fd);  // ← Time-of-check
// ...
long bytesRead = filesFacade.read(fd, address, size, 0);  // ← Time-of-use
if (bytesRead != size) {  // This catches it, but...

The check handles this, but malformed data could still be read if file shrinks.

📋 Recommendations Summary

High Priority:

1. Add path validation to prevent traversal attacks
2. Validate file is a regular file before reading
3. Add comprehensive error handling tests (target 90%+ coverage)
4. Document security considerations for file paths
5. Consider fail-fast on deleted secret files during reload

Medium Priority:
6. Add edge case tests (empty, whitespace, invalid UTF-8)
7. Restore the newProperties.equals() optimization
8. Add integration tests for all password types
9. Reduce logging verbosity for file paths
10. Add validation for empty passwords

Low Priority:
11. Document I/O overhead of multiple secret files
12. Consider restricting symlink following
13. Add metrics for secret reload failures

Would you like me to create a follow-up issue to track these security improvements, or would you prefer to address them in this PR?

@CodeRabbit - can you review again, think ive addressed some of these issues. Let me know what's left

#!/bin/bash

# Get the latest PR diff
echo "=== Latest PR Diff ==="
gh pr diff 6719 --repo questdb/questdb

echo -e "\n=== Changed files ==="
gh pr view 6719 --repo questdb/questdb --json files --jq '.files[].path'

echo -e "\n=== Checking PropServerConfiguration for security validations ==="
# Look for path validation, file type checks, etc.
rg -A 10 -B 2 "readSecretFromFile" --type java

echo -e "\n=== Checking for symlink handling ==="
rg "symlink|canonical|normalize|isRegularFile" --type java

echo -e "\n=== Checking error handling in checkSecretFileChanges ==="
rg -A 20 "checkSecretFileChanges" --type java

echo -e "\n=== Checking for empty password validation ==="
rg "trim\(\)|isEmpty|isBlank" --type java | grep -i secret -A 2 -B 2

echo -e "\n=== Looking for test files ==="
fd -e java -e kt | xargs rg -l "readSecretFromFile|PASSWORD_FILE" | grep -i test

echo -e "\n=== Checking test coverage scenarios ==="
rg -A 5 "testSecret|test.*[Ff]ile|test.*[Pp]assword" --type java | grep -i test

#!/bin/bash

# Check the latest test coverage more specifically
echo "=== Checking test methods related to secret files ==="
rg "public void test.*[Ss]ecret|public void test.*[Pp]assword.*[Ff]ile" --type java -A 1 core/src/test/java/io/questdb/test/DynamicPropServerConfigurationTest.java

echo -e "\n=== Checking error handling for deleted files ==="
rg "testSecretFileDeleted|testSecretFileNotFound" --type java -A 10 core/src/test/java/io/questdb/test/

echo -e "\n=== Checking for path validation in readSecretFromFile ==="
rg "canonicalize|realpath|normalize|resolve" --type java core/src/main/java/io/questdb/PropServerConfiguration.java

echo -e "\n=== Check if removed optimization was restored ==="
rg "if.*newProperties\.equals\(oldProperties\)" --type java core/src/main/java/io/questdb/DynamicPropServerConfiguration.java

echo -e "\n=== Check logging level for secret file paths ==="
rg "log\.(info|debug|advisory).*reading secret from file" --type java core/src/main/java/io/questdb/PropServerConfiguration.java

@bluestreak01 Great progress — most high-risk items are now covered. Here’s the status and what’s still worth tightening:

What’s addressed

• Fail-fast reload on secret file errors: reload aborts on CairoException from secret read; old values remain intact. Tests: deleted/missing file.
• Size and content handling: 64KB cap; trimming; empty/whitespace supported with advisory log; trailing newlines handled; invalid UTF‑8 fails startup.
• Directory rejection: directories and soft-link directories are rejected.
• SHOW PARAMETERS: value_source shows file for secrets; covered via env and property.
• Live updates: switching between direct and file-based secrets, and multi-secret reloads, are tested.
• Log minimization: file path no longer logged; only the key.

What still remains or could be improved

1. Path and symlink hardening

• Today any path (including symlink to an arbitrary file) is accepted if it’s not a directory. Consider:
  • Canonicalize path then enforce it’s under an allowed prefix (e.g., /run/secrets or a configurable list).
  • Optionally reject symlinks, or resolve and then enforce the allowlist.
  • If your FilesFacade supports O_NOFOLLOW or lstat-style checks, prefer those to avoid TOCTOU on links.

2. Regular file check to avoid FIFOs/devices

• There’s no explicit “regular file” check, so a FIFO/char device could block on open/read.
• If available, use a stat/type check (reject S_IFIFO, S_IFCHR, S_IFBLK, S_IFSOCK). Otherwise, add a best-effort guard (e.g., reject zero-length on first read with a clear error when open would block or when length returns an unsupported value).

3. Early-return optimization on property equality

• The original fast-path newProperties.equals(oldProperties) remains removed. You can combine both worlds:
  • If properties equal and no secretFilesChanged, skip reload work; otherwise proceed.

4. Log level nit

• “reading secret from file [key=…]” at INFO may be chatty in some environments. Optional: demote to DEBUG.

Concrete suggestions (illustrative)

• PropServerConfiguration.readSecretFromFile hardening

// Add near existing constants
private static final String[] DEFAULT_ALLOWED_SECRET_DIRS = {
    "/run/secrets", "/var/run/secrets", "/var/secrets"
};

// Utility: canonicalize and enforce allowlist (reuse your existing canonical path utilities if available)
private boolean isUnderAllowedDirs(CharSequence pathUtf8) {
    try (Path tmp = new Path()) {
        tmp.of(pathUtf8);
        // If Files has a canonicalize/realpath, prefer that. Otherwise, normalize via java.nio as a fallback.
        java.nio.file.Path nio = java.nio.file.Paths.get(tmp.toString());
        java.nio.file.Path real = nio.toRealPath(java.nio.file.LinkOption.NOFOLLOW_LINKS);
        String realStr = real.toString();
        for (String allowed : DEFAULT_ALLOWED_SECRET_DIRS) {
            if (realStr.startsWith(allowed + Files.SEPARATOR) || realStr.equals(allowed)) {
                return true;
            }
        }
        return false;
    } catch (Exception e) {
        return false;
    }
}

protected String readSecretFromFile(String filePath) {
    long fd = -1, address = 0, size = 0;
    try (Path path = new Path()) {
        path.of(filePath);

        // 1) Disallow symlinks or resolve and check allowlist
        // If FilesFacade exposes isSoftLink(), prefer checking that first.
        // Otherwise, use java.nio to NOFOLLOW for the allowlist check above.
        if (!isUnderAllowedDirs(filePath)) {
            throw CairoException.critical(0).put("secret file path not allowed [path=").put(filePath).put(']');
        }

        // 2) Reject directories (already present) and non-regular files if you can query type
        if (filesFacade.isDirOrSoftLinkDir(path.$())) {
            throw CairoException.critical(0).put("secret file path is a directory [path=").put(filePath).put(']');
        }
        // Optional: if FilesFacade exposes a type/lstat, enforce regular files here.

        fd = filesFacade.openRO(path.$()); // Prefer O_NOFOLLOW if available.
        if (fd < 0) {
            throw CairoException.critical(filesFacade.errno()).put("cannot open secret file [path=").put(filePath).put(']');
        }

        // ... keep your existing retry + size cap + read + trim logic ...
    } finally {
        if (address != 0) Unsafe.free(address, size, MemoryTag.NATIVE_DEFAULT);
        if (fd >= 0) filesFacade.close(fd);
    }
}

• DynamicPropServerConfiguration.reload fast-path sketch

public boolean reload() {
    try (Path confPath = new Path().of(this.confPath).$()) {
        final Properties newProperties = readProperties(confPath);
        // Fast-path equality check first
        boolean propsEqual = newProperties.equals(properties);
        boolean secretFilesChanged = false;
        try {
            secretFilesChanged = checkSecretFileChanges(newProperties, changedKeys);
        } catch (CairoException e) {
            LOG.error().$("could not read secret file, reload aborted [error=").$((Throwable) e).I$();
            return false;
        }
        if (propsEqual && !secretFilesChanged) {
            LOG.info().$("nothing to reload [file=").$(confPath).I$();
            return false;
        }
        boolean propsChanged = !propsEqual && updateSupportedProperties(properties, newProperties, dynamicProps, keyResolver, changedKeys, LOG);
        if (propsChanged || secretFilesChanged) {
            reload0();
            LOG.info().$("reloaded, [file=").$(confPath).I$();
            watchRegistry.notifyWatchers(changedKeys);
            LOG.info().$("notified config change listeners").$();
            return true;
        }
    }
    return false;
}

Nice-to-have tests to close remaining gaps

• Permission denied: simulate openRO returning -1 with errno EACCES using TestFilesFacadeImpl; assert startup fails and reload aborts with old value retained.
• Symlink policy: add tests for symlink to allowed file (if you choose to allow) and symlink to disallowed target (e.g., outside allowed roots) to ensure it’s rejected.

If you want, I can open a follow-up “security hardening” issue to track symlink/allowlist + regular-file checks and the fast-path optimization. Prefer to land those in a small subsequent PR to keep this one focused.

@CodeRabbit comments adjusted, review - path validation and symlinking should be allowed on trusted paths. I cannot see any attack vectors on the paths configuration is read from. Can you?

#!/bin/bash
# Get the latest implementation of readSecretFromFile
echo "=== PropServerConfiguration.readSecretFromFile implementation ==="
rg -A 50 "protected String readSecretFromFile" --type java core/src/main/java/io/questdb/PropServerConfiguration.java

echo -e "\n=== Check for path validation logic ==="
rg -B 5 -A 15 "isDirOrSoftLinkDir|canonical|realpath|normalize" --type java core/src/main/java/io/questdb/PropServerConfiguration.java

echo -e "\n=== Check DynamicPropServerConfiguration for secret file handling ==="
rg -A 30 "checkSecretFileChanges" --type java core/src/main/java/io/questdb/DynamicPropServerConfiguration.java

echo -e "\n=== Look for any symlink-related tests ==="
rg "symlink|Symlink" --type java core/src/test/

echo -e "\n=== Check what file operations FilesFacade supports ==="
rg "interface FilesFacade|class.*FilesFacade" --type java -A 20 | head -100

symlink attacks in Kubernetes containers CVE 2024 2025

@bluestreak01 You raise a fair point about trusted paths, but there are still attack vectors worth considering for defense-in-depth:

🔴 Remaining Attack Vectors

1. Configuration Injection (Primary Risk)

The biggest risk is who controls the configuration values at deployment time:

# Malicious K8s manifest
env:
  - name: QDB_PG_PASSWORD_FILE
    value: "/etc/shadow"  # ← Read arbitrary host file
  # or
  - name: QDB_PG_PASSWORD_FILE  
    value: "/proc/self/environ"  # ← Info disclosure
  # or
  - name: QDB_PG_PASSWORD_FILE
    value: "/dev/urandom"  # ← DoS via infinite read

This requires deploy-time access (ability to modify K8s manifests, ConfigMaps, or Helm values), but:

• In multi-tenant clusters, namespace admins might have this access without cluster-admin rights
• Supply-chain attacks could inject malicious manifests
• Insider threats with limited RBAC could exploit this

2. Special File Attacks

Currently no check prevents reading:

• FIFOs/pipes → can hang indefinitely waiting for writer
• Character devices (/dev/random, /dev/zero) → resource exhaustion
• Block devices → unintended disk access
• Sockets → unexpected behavior

3. Symlink to Arbitrary Location

While Kubernetes Secrets are typically safe, consider:

• hostPath mounts (sometimes used with external secret managers)
• Shared volumes across containers (CVE-2024-45310 exploited this)
• Volume subPath mounts (can preserve existing symlinks)
• External secret managers (Vault, AWS Secrets Manager) might use symlinks

Example: If /run/secrets/pg_password is a symlink → /host-root/etc/shadow, QuestDB will happily read it.

4. Active Symlink CVEs (2024-2025)

Recent runc/KubeVirt CVEs show symlink attacks in containers remain a live threat:

• CVE-2024-45310 (runc): shared volume symlink race
• CVE-2025-31133 (runc): maskedPaths bypass via symlinks
• CVE-2025-52565, CVE-2025-52881 (runc): mount/symlink races
• CVE-2025-64433 (KubeVirt): symlink to read arbitrary files from pod filesystem

These demonstrate that "trusted container environments" still face symlink-based attacks.

✅ Reasonable Compromise

I agree symlinks should be allowed for legitimate use cases (Kubernetes secret versioning, some secret managers). Here's a balanced approach:

Recommended Security Hardening

// Add after existing constants
private static final String[] ALLOWED_SECRET_DIRS = {
    "/run/secrets", "/var/run/secrets", "/secrets", 
    "/vault/secrets", "/var/secrets"
};

protected String readSecretFromFile(String filePath) {
    long fd = -1, address = 0, size = 0;
    try (Path path = new Path()) {
        path.of(filePath);
        
        // 1) Reject obvious path traversal
        if (filePath.contains("..")) {
            throw CairoException.critical(0)
                .put("secret file path contains path traversal [path=").put(filePath).put(']');
        }
        
        // 2) Reject special files (if FilesFacade supports it)
        // Check before opening to avoid hanging on FIFOs
        if (filesFacade.isDirOrSoftLinkDir(path.$())) {
            throw CairoException.critical(0)
                .put("secret file path is a directory [path=").put(filePath).put(']');
        }
        
        fd = filesFacade.openRO(path.$());
        if (fd < 0) {
            throw CairoException.critical(filesFacade.errno())
                .put("cannot open secret file [path=").put(filePath).put(']');
        }
        
        // 3) After opening, validate the resolved path is in allowed dirs
        // (This catches symlinks to arbitrary locations)
        // Note: Implement getRealPath() using your Path utilities if available
        String realPath = getRealPath(fd, path);  
        if (!isPathAllowed(realPath)) {
            throw CairoException.critical(0)
                .put("secret file resolved path not in allowed directories [path=")
                .put(filePath).put(", resolved=").put(realPath).put(']');
        }
        
        // ... rest of existing logic ...

Minimal Alternative (If Full Validation is Overkill)

If you believe the deployment environment is sufficiently locked down, at minimum add:

// Reject obvious path traversal and special files
if (filePath.contains("..") || filePath.startsWith("/dev/") || filePath.startsWith("/proc/")) {
    throw CairoException.critical(0)
        .put("secret file path not allowed [path=").put(filePath).put(']');
}

This provides basic protection against the most obvious attacks while allowing flexibility.

🤔 My Assessment

You're right that:

• Configuration paths are typically controlled by cluster admins with RBAC
• Kubernetes Secret mounts are generally safe and read-only
• Symlinks have legitimate uses

However:

• QuestDB is database infrastructure (high-value target)
• CVE history shows container symlink attacks are real and ongoing
• Defense-in-depth: even "trusted" config benefits from validation
• The validation cost is minimal (one-time at startup/reload)

Recommendation: Add at minimum the path traversal + special file checks. Ideally add the allowlist validation for defense-in-depth, but I acknowledge your point about trusted environments.

What's your take? Would a simple /dev/, /proc/, .. blocklist address your concerns while maintaining flexibility?

@CodeRabbit values that are read from files are never printed. Reading files like that would require privileged host access. Lets assume they injected config such as /etc/shadow, then what, how do they read it?

#!/bin/bash

# Check current readSecretFromFile implementation
echo "=== Current readSecretFromFile implementation ==="
rg -A 40 "protected String readSecretFromFile" --type java core/src/main/java/io/questdb/PropServerConfiguration.java

echo -e "\n=== Check if there are any other potential information leakage vectors ==="
# Check if secret values could leak through error messages
rg "password.*Exception|password.*error|password.*Error" --type java core/src/main/java/io/questdb/ -C 3

echo -e "\n=== Check SHOW PARAMETERS implementation for masking ==="
rg "getStrA.*sensitive|mask|\*\*\*\*" --type java core/src/main/java/io/questdb/griffin/engine/functions/catalogue/ShowParametersCursorFactory.java -C 5

echo -e "\n=== Check if passwords appear in any SQL result sets or logs ==="
rg "pg\.password|http\.password" --type java core/src/main/java/io/questdb/ | grep -v "test" | grep -v "\.conf" | head -20

@bluestreak01 Excellent point—you've identified the key limitation. Let me address it directly:

🎯 The Exfiltration Mechanism (Authentication-as-Oracle)

The only practical exfiltration vector is using authentication itself as a side channel:

1. Attacker injects QDB_PG_PASSWORD_FILE=/proc/sys/kernel/random/boot_id (deploy access required)
2. QuestDB reads the file → password becomes the 36-char UUID
3. Attacker connects via network and tries authentication attempts
4. Successful authentication = file contents disclosed

Your Counter-Arguments Are Valid

You're absolutely right that this attack requires:

• ✅ Deploy-time access (to inject malicious config)
• ✅ Network access to QuestDB (to attempt authentication)
• ✅ Target file must be single-line and trimmed-friendly (rules out /etc/shadow)

If an attacker has deploy access, they likely have easier attack vectors anyway (inject malicious containers, access other secrets, etc.).

Script Results Confirm Your Point

The code inspection shows:

• ✅ SHOW PARAMETERS masks sensitive values: if (entry.key.isSensitive()) { yield "****"; }
• ✅ No password leakage in error messages
• ✅ Secrets never logged or printed

So authentication is indeed the only exfiltration channel.

🤔 Minimal Hardening (Optional)

Given your threat model, here's a minimal compromise that doesn't require complex allowlists:

// In readSecretFromFile(), add before opening:
if (filePath.contains("..") || 
    filePath.startsWith("/dev/") || 
    filePath.startsWith("/proc/") || 
    filePath.startsWith("/sys/")) {
    throw CairoException.critical(0)
        .put("secret file path not allowed [path=").put(filePath).put(']');
}

Rationale:

• /dev/ → prevents reading devices, FIFOs (can cause hangs)
• /proc/, /sys/ → blocks common info-disclosure paths
• .. → rejects obvious path traversal
• No allowlist needed → maintains flexibility for legitimate secret managers

This adds defense-in-depth against accidental misconfigurations (e.g., someone typos a path) without assuming sophisticated attackers.

✅ My Assessment

You're right:

• The current implementation with directory checking + 64KB cap + trimming is reasonably secure
• The authentication-oracle attack requires multiple preconditions that indicate the attacker already has significant access
• Complex path validation may be overkill for QuestDB's deployment model

I'd be comfortable with:

• Shipping as-is (current protections are solid)
• OR adding the minimal 4-line check above (defense against misconfig, not sophisticated attacks)

Your call—both are defensible given the access requirements. What's your preference?

[PR Coverage check]

😍 pass : 119 / 137 (86.86%)

file detail