Skip to content

Conversation

dguido
Copy link
Member

@dguido dguido commented Aug 3, 2025

Summary

This PR implements comprehensive privacy and security enhancements for Algo VPN, addressing sensitive data exposure in logs (#1617) while maintaining operational visibility for troubleshooting.

Key Improvements

1. Credential Protection with no_log

Added no_log: true to 50+ Ansible tasks across all cloud providers and VPN components to prevent sensitive data from being logged:

  • Cloud API credentials (AWS, Azure, GCP, DigitalOcean, etc.)
  • VPN private keys and certificates
  • User passwords and authentication tokens
  • SSH keys and configuration data

2. Privacy Enhancements Role

New roles/privacy/ implements configurable privacy features:

  • Log rotation: Automatic 7-day retention with compression
  • VPN log filtering: Hides connection patterns while preserving errors
  • History clearing: Removes deployment traces
  • Automatic cleanup: Scheduled temporary file removal

3. Simplified Configuration

Streamlined config.cfg from 308 to 198 lines (35% reduction):

### Privacy Settings ###
# StrongSwan connection logging (-1 = disabled, 2 = debug)
strongswan_log_level: -1

# Master switch for privacy enhancements
privacy_enhancements_enabled: true

# Hide sensitive data in Ansible output (passwords, keys, etc.)
algo_no_log: true

4. Documentation Improvements

  • Restructured FAQ to be more concise with link to detailed information
  • Added comprehensive "Privacy and Logging" section to README
  • Added privacy features to the main features list
  • Clear distinction between deployment privacy (algo_no_log) and server privacy

What Gets Protected

Hidden from logs:

  • VPN connection handshakes and user IP addresses
  • DNS queries (when privacy enhanced)
  • API credentials and authentication tokens
  • Private keys and certificates

Preserved for debugging:

  • Failed authentication attempts (security monitoring)
  • Service errors and warnings
  • System security events
  • Network interface issues

Configuration Options

Default (Balanced):

privacy_enhancements_enabled: true  # Privacy features on
strongswan_log_level: -1            # Connection logging off
algo_no_log: true                   # Credential protection on

Debug Mode:

privacy_enhancements_enabled: false # Full logging
strongswan_log_level: 2             # Debug verbosity
algo_no_log: false                  # Show all output

Testing

All tests pass:

  • ✅ 59 unit tests passing
  • ✅ Ansible-lint compliant
  • ✅ All linters passing (yamllint, ruff, shellcheck)
  • ✅ Successfully deployed on Ubuntu 22.04

Compatibility

  • No breaking changes for existing deployments
  • Old config.cfg files work unchanged
  • Can disable all privacy features if needed
  • Ubuntu 22.04 iptables compatibility fixed

This implementation balances privacy with operational needs, ensuring sensitive data is protected while maintaining the debugging capabilities essential for a reliable VPN service.

Fixes #1617

- Add no_log: true to OpenSSL commands that contain passwords/passphrases
- Add no_log: true to WireGuard key generation commands
- Add no_log: true to password/CA password generation tasks
- Add no_log: true to AWS credential handling tasks
- Add no_log: true to QR code generation that contains full configs

This prevents sensitive information like passwords, private keys, and
WireGuard configurations from being logged to syslog/journald.

Fixes #1617
@dguido dguido requested a review from jackivanov as a code owner August 3, 2025 07:11
dguido and others added 3 commits August 3, 2025 03:42
- Add no_log directives to all cloud provider credential handling
- Set privacy-focused defaults (StrongSwan logging disabled, DNSCrypt syslog off)
- Implement privacy role with log rotation, history clearing, and log filtering
- Add Privacy Considerations section to README
- Make all privacy features configurable and enabled by default

This update significantly reduces Algo's logging footprint to enhance user privacy
while maintaining the ability to enable logging for debugging when needed.
- Remove Privacy Considerations section from README
- Add expanded 'Does Algo support zero logging?' question to FAQ
- Better placement alongside existing logging/monitoring questions
- More detailed explanation of privacy features and limitations
The privacy-monitor.sh.j2 template was using '| bool' which is not a valid
Jinja2 filter. The 'bool' is a built-in Python function, not a Jinja2 filter.

Fixed by removing the '| bool' filter and directly outputting the boolean
variables as they will be rendered correctly by Jinja2.

This resolves the template syntax error that was causing CI tests to fail:
"No filter named 'bool'" error in privacy monitoring script template.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@dguido
Copy link
Member Author

dguido commented Aug 3, 2025

I'm not fully happy with the documentation for this PR yet. Will fix soon.

Copy link
Contributor

claude bot commented Aug 3, 2025

Claude finished @dguido's task —— View job


Code Review Analysis

I've completed a comprehensive review of this privacy implementation PR. Here's my detailed feedback:

🟢 Strengths & Positive Aspects

1. Comprehensive Credential Protection

  • Excellent coverage: 49 tasks now properly protected with no_log: true across all cloud providers
  • Consistent implementation: All sensitive operations (API keys, passwords, certificates) are protected
  • Follows Ansible best practices: Uses conditional algo_no_log | default(true) pattern

2. Well-Designed Privacy Architecture

  • Surgical approach: Filters user activity logs while preserving security/debugging capabilities
  • Modular design: Privacy role is cleanly separated with clear responsibilities
  • Granular control: Individual toggles for different privacy features

3. Security-Conscious Implementation

  • Preserves critical logs: Failed connections, errors, and security events still logged
  • Proper defaults: Conservative settings that balance privacy with operational needs
  • Certificate protection: VPN keys and credentials properly secured

4. Strong Configuration Management

  • Backward compatibility: Existing deployments continue to work unchanged
  • Clear documentation: Comprehensive README with usage examples and security considerations
  • Flexible options: Users can disable privacy features when debugging needed

⚠️ Areas of Concern

1. Log Filtering Regex Security

File: roles/privacy/templates/49-privacy-vpn-filter.conf.j2:23-24

:msg, regex, "IN=wg[0-9]+" stop
:msg, regex, "OUT=wg[0-9]+" stop

Issues:

  • Unanchored regex patterns are vulnerable to bypass attacks
  • Missing start/end anchors (^ and $)
  • Should use more specific patterns to prevent false matches

Recommendation:

:msg, regex, "^[^:]*: IN=wg[0-9]+" stop
:msg, regex, "^[^:]*: OUT=wg[0-9]+" stop

2. Overly Broad Filtering

File: roles/privacy/templates/49-privacy-vpn-filter.conf.j2:29

:msg, contains, "netfilter" stop

Issue: This could filter legitimate security events and firewall logs that contain "netfilter"

Recommendation: Use more specific patterns targeting only VPN-related netfilter messages

3. Missing IPv6 Support

The current regex patterns don't account for IPv6 addresses in log filtering, which could lead to privacy leaks for IPv6 VPN traffic.

Recommendation: Add IPv6-specific patterns:

:msg, regex, "^[^:]*: IN=wg[0-9]+ .* SRC=[0-9a-fA-F:]+::" stop

4. Test Coverage Gap

Missing: No specific tests found for the privacy role functionality
Risk: Changes could break privacy filtering without detection

Recommendation: Add unit tests for:

  • Log filtering regex patterns
  • Configuration validation
  • Template rendering with different privacy settings

📊 Configuration Assessment

Strong Points:

  • Sensible default: strongswan_log_level: -1 (disabled logging)
  • Conditional privacy role execution: when: privacy_enhancements_enabled | default(true)
  • Clear separation of privacy vs. debugging modes

Concerns:

File: config.cfg:49

strongswan_log_level: -1

Issue: The PR description mentions level 1 (alerts) as the implementation, but config shows -1 (disabled). This inconsistency could lead to either broken debugging or unintended logging.

🚀 Performance Considerations

Positive:

  • Log filtering at rsyslog level is efficient
  • Reduced disk I/O from smaller log files
  • Faster log searches due to reduced volume

Potential Concerns:

  • Regex processing overhead for every log message
  • Additional rsyslog configuration files to process

🔒 Security Analysis

Excellent Security Practices:

  1. Defense in Depth: Multiple layers of credential protection
  2. Principle of Least Privilege: Only filters user activity, not security events
  3. Secure Defaults: Privacy enabled by default but debuggable when needed
  4. Certificate Security: All VPN key operations properly protected

Security Recommendations:

  1. Regex Hardening: Anchor all regex patterns to prevent bypass
  2. Pattern Specificity: Use more targeted filtering to avoid blocking security logs
  3. IPv6 Coverage: Add IPv6-aware filtering patterns
  4. Test Validation: Add security tests to prevent regression

📝 Code Quality

Positive:

  • Clean Ansible role structure
  • Well-documented templates with clear comments
  • Consistent variable naming
  • Proper file permissions and ownership

Areas for Improvement:

  • Some complex Jinja2 expressions could be simplified
  • Consider extracting complex regex patterns to variables for maintainability

🏆 Overall Assessment

This is a solid, security-conscious implementation that successfully addresses the core privacy concerns while maintaining operational capabilities. The surgical approach to privacy filtering is well-designed and the comprehensive credential protection is excellent.

Key Strengths:

  • ✅ Comprehensive credential protection across all components
  • ✅ Thoughtful balance between privacy and debugging capabilities
  • ✅ Backward compatibility and clear configuration options
  • ✅ Security-first approach that preserves critical monitoring

Must-Fix Issues:

  • ❌ Regex security vulnerabilities (unanchored patterns)
  • ❌ Missing IPv6 support in log filtering
  • ❌ Configuration inconsistency (strongswan log level)

Recommended Improvements:

  • ⚠️ Add comprehensive test coverage for privacy functionality
  • ⚠️ More specific netfilter filtering patterns
  • ⚠️ Consider extracting complex regex to variables

Verdict: Approve with Required Changes - The implementation is fundamentally sound but needs the regex security issues fixed before merge.


@dguido dguido force-pushed the fix/ansible-no-log-sensitive branch 3 times, most recently from e4b6849 to fa77f7d Compare August 4, 2025 17:47
@dguido dguido marked this pull request as draft August 5, 2025 04:52
dguido added 2 commits August 7, 2025 11:20
- Resolved conflicts in cloud provider prompt files
- Updated to use new Ansible crypto modules from master
- Added no_log directives to sensitive Ansible crypto tasks
- Preserved privacy enhancements from the branch
@dguido dguido force-pushed the fix/ansible-no-log-sensitive branch from fa77f7d to 540b250 Compare August 7, 2025 18:22
dguido and others added 4 commits August 17, 2025 13:24
Resolved conflicts by keeping both:
- Added spaces after commas in lookup('env', 'VAR') calls (from master)
- Kept no_log: true directives for sensitive tasks (from our branch)

This ensures proper code formatting while maintaining security improvements.
- Fixed all shellcheck warnings in test scripts:
  - Quoted variables to prevent word splitting
  - Replaced A && B || C constructs with proper if-then-else
  - Changed unused loop variable to _
  - Added shellcheck directives for FreeBSD rc.d script

- Fixed ansible-lint risky-file-permissions warnings:
  - Added explicit file permissions for sensitive files (mode 0600)
  - Added permissions for config files and certificates (mode 0644)
  - Set proper permissions for directories (mode 0755)

- Fixed yamllint compatibility with ansible-lint:
  - Added required octal-values configuration
  - Quoted all octal mode values to prevent YAML misinterpretation
  - Added comments-indentation: false as required

All tests pass and functionality remains unchanged.
This directory is generated by Python package tools (pip/setuptools) and
should not be tracked in git. It's already listed in .gitignore but was
accidentally committed. The directory contains build metadata that is
regenerated when the package is installed.
- Simplified FAQ entry to be concise with link to README for details
- Added comprehensive Privacy and Logging section to README
- Clarified what IS logged by default vs what is not
- Explained two separate privacy settings (strongswan_log_level and privacy_enhancements_enabled)
- Added clear debugging instructions (need to change both settings)
- Removed confusing language about "enabling additional features"
- Made documentation more natural and less AI-generated sounding

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@dguido dguido marked this pull request as ready for review August 17, 2025 18:53
dguido and others added 5 commits August 17, 2025 15:03
Issues fixed:
1. Added base 'iptables' package to batch installation list (was missing, only iptables-persistent was included)
2. Fixed alternatives configuration for Ubuntu 22.04+ - only configure main iptables/ip6tables alternatives, not save/restore (they're handled as slaves)

Config.cfg improvements:
- Reduced from 308 to 198 lines (35% reduction)
- Moved privacy settings above "Advanced users only" line for better accessibility
- Clarified algo_no_log is for Ansible output, not server privacy
- Simplified verbose comments throughout
- Moved experimental performance options to commented section at end
- Better organized into logical sections

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Added privacy-focused feature bullet highlighting minimal logging and privacy enhancements
- Simplified IKEv2 bullet (removed redundant platform list)
- Updated helper scripts description to be more comprehensive
- Specified Ubuntu 22.04 LTS and automatic security updates
- Made feature list more concise and accurate

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
The privacy role was creating logrotate configs that duplicated the default
Ubuntu rsyslog logrotate rules, causing deployment failures with errors like
'duplicate log entry for /var/log/syslog'.

Changes:
- Disable default rsyslog logrotate config before applying privacy configs
- Consolidate system log rotation into single config file
- Add missingok flag to handle logs that may not exist on all systems
- Remove forced immediate rotation that was triggering the error

This ensures privacy-enhanced log rotation works without conflicts.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
The 'history -c' command was failing because history is a bash built-in
that doesn't exist in /bin/sh (Ubuntu's default shell for scripts).

Changes:
- Removed the 'Clear current session history' task since it's ineffective
  in Ansible context (each task runs in a new shell)
- History files are already cleared by the existing file removal tasks
- Added explanatory comment about why session history clearing is omitted

This fixes the deployment failure while maintaining all effective history
clearing functionality.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
The net.core.bpf_jit_enable sysctl parameter was failing on some systems
because BPF JIT support is not available in all kernel configurations.

Changes:
- Separated BPF JIT setting into its own task with ignore_errors
- Made BPF JIT disabling optional since it's not critical for privacy
- Added explanatory comments about kernel support variability
- Both runtime sysctl and persistent config now handle missing parameter

This allows deployments to succeed on systems without BPF JIT support
while still applying the setting where available.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@dguido dguido merged commit 454faa9 into master Aug 17, 2025
24 checks passed
@dguido dguido deleted the fix/ansible-no-log-sensitive branch August 17, 2025 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ansible logs sensitive information
1 participant