Conversation
Security Findings Fixed: - F01: X-Forwarded-For trusted proxy validation (HIGH) - F02: PBKDF2-SHA256 password hashing (100k iterations) - F03: Cryptographic random for session tokens - F04: RBAC default-deny unmapped endpoints - F05: Atomic timing token key generation - F06: client_body_timeout configuration - F07: JSON depth limit protection - F08: Optional HMAC audit log integrity - F09: Auto-detect HTTPS for Secure cookie flag - F10: Redis TLS environment variable support - F11: Required WAF_ADMIN_PASSWORD (no default) - F12: Per-field size limit for multipart (1MB) - F13: Full IPv6 support in IP utilities - F14: Consistent IP extraction across all modules (HIGH) - F15: SSRF protection for outbound HTTP requests (HIGH) - F16: Cryptographic random for SSO state parameters New Files: - openresty/lua/trusted_proxies.lua - Secure client IP extraction - openresty/lua/password_utils.lua - PBKDF2-SHA256 hashing - openresty/lua/safe_json.lua - JSON depth limiting - openresty/lua/ssrf_protection.lua - SSRF attack prevention - docs/SECURITY_AUDIT.md - Full audit documentation Breaking Changes: - WAF_ADMIN_PASSWORD environment variable is now required - Legacy SHA256 passwords auto-upgrade to PBKDF2 on login
Add support for connecting to external Redis instances with TLS enabled
(e.g., AWS ElastiCache with in-transit encryption):
- Add externalRedis configuration section in values.yaml with TLS options
- Add stunnel sidecar container to OpenResty deployment when TLS enabled
- Create stunnel ConfigMap for TLS proxy configuration
- Update _helpers.tpl to route Redis connection through stunnel
- Update redis-init-job to support external Redis with TLS (--tls flag)
- Support external Redis password via direct value or existing secret
Usage (values.yaml):
redis:
enabled: false # Disable bundled Redis
externalRedis:
host: "my-cluster.xxxxx.use1.cache.amazonaws.com"
port: 6379
tls:
enabled: true
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request addresses 16 security findings from a comprehensive security audit of the forms-waf project. The changes implement critical security improvements including trusted proxy validation, PBKDF2 password hashing, cryptographic random number generation, SSRF protection, and IPv6 support.
Changes:
- Added four new security modules for trusted proxy validation, password hashing, safe JSON parsing, and SSRF protection
- Updated IP extraction across all modules to use secure trusted proxy validation
- Replaced weak SHA256 password hashing with PBKDF2-SHA256 (100,000 iterations) with auto-upgrade migration
- Replaced
math.randomwith cryptographic random (resty.random) for all security-sensitive tokens - Implemented comprehensive SSRF protection for outbound HTTP requests
- Added full IPv6 support to IP utilities with CIDR matching
- Added Helm chart support for Redis TLS via stunnel sidecar proxy
- Configured nginx request timeouts and real_ip module for proxy validation
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| openresty/lua/trusted_proxies.lua | New module implementing secure client IP extraction with trusted proxy validation (F01) |
| openresty/lua/password_utils.lua | New module implementing PBKDF2-SHA256 password hashing with 100K iterations (F02) |
| openresty/lua/safe_json.lua | New module wrapping cjson with depth limits for DoS prevention (F07) |
| openresty/lua/ssrf_protection.lua | New module preventing SSRF attacks on outbound requests (F15) |
| openresty/lua/waf_handler.lua | Updated to use trusted_proxies for IP extraction; added optional HMAC log integrity (F01, F08) |
| openresty/lua/admin_auth.lua | Replaced math.random with crypto random; uses PBKDF2 with auto-upgrade; added Secure cookie flag detection (F02, F03, F09) |
| openresty/lua/rbac.lua | Uses PBKDF2 for admin password; requires WAF_ADMIN_PASSWORD env var; denies unmapped endpoints (F02, F04, F11) |
| openresty/lua/timing_token.lua | Fixed atomic key generation race condition using shared:add() (F05) |
| openresty/lua/form_parser.lua | Uses safe_json with depth limits; added per-field size limit for multipart (F07, F12) |
| openresty/lua/ip_utils.lua | Added full IPv6 support including parsing, CIDR matching, and normalization (F13) |
| openresty/lua/http_utils.lua | Integrated SSRF protection for all outbound HTTP requests (F15) |
| openresty/lua/captcha_handler.lua | Uses trusted_proxies and crypto random (F03, F14) |
| openresty/lua/webhooks.lua | Uses trusted_proxies for secure IP extraction (F14) |
| openresty/lua/sso_oidc.lua | Uses http_utils with SSRF protection; crypto random for state parameter (F15, F16) |
| openresty/lua/sso_ldap.lua | Uses crypto random for session tokens (F16) |
| openresty/lua/redis_sync.lua | Added REDIS_TLS environment variable support with documentation (F10) |
| openresty/lua/api_handlers/users.lua | Uses PBKDF2 for password hashing; crypto random for temp passwords (F02, F03) |
| openresty/lua/api_handlers/providers.lua | Auto-detects HTTPS for Secure cookie flag (F09) |
| openresty/conf/nginx.conf | Added set_real_ip_from directives; added request timeouts; exposed security env vars (F01, F06) |
| helm/forms-waf/values.yaml | Added externalRedis.tls configuration for managed Redis services (F10) |
| helm/forms-waf/templates/stunnel-configmap.yaml | New stunnel configuration for Redis TLS proxy (F10) |
| helm/forms-waf/templates/redis-init-job.yaml | Updated to support external Redis with TLS (F10) |
| helm/forms-waf/templates/openresty-deployment.yaml | Added stunnel sidecar container for Redis TLS (F10) |
| helm/forms-waf/templates/_helpers.tpl | Added helper functions for Redis TLS configuration (F10) |
| docs/SECURITY_AUDIT.md | Comprehensive security audit documentation with findings and fixes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
HMAC Implementation (F08): - Use proper RFC 2104 HMAC-SHA256 from password_utils instead of non-standard H(key || message || key) construction - Export hmac_sha256 function from password_utils for reuse Password Auto-Upgrade: - Add error handling for Redis SET operation during password upgrade - Log errors but don't fail login if upgrade fails IPv6 Parsing: - Add validation to reject addresses with multiple :: occurrences - Fix IPv4-mapped IPv6 pattern to require exact ::ffff: prefix format - Support ::ffff:0:a.b.c.d SIIT format SSRF Protection: - Fix URL parsing to remove userinfo before hostname extraction - Add runtime DNS resolution check to prevent DNS rebinding attacks - Use resty.dns.resolver to validate resolved IPs against private ranges Trusted Proxies: - Remove 127.0.0.0/8 and ::1/128 from default trusted CIDRs - Simplify get_client_ip() function removing redundant logic - Add documentation about loopback security implications Helm Chart: - Add writable /tmp volume for stunnel sidecar container - Use REDISCLI_AUTH env var instead of -a flag to prevent password exposure in process listings
mclueppers
force-pushed
the
fix/security-audit-findings
branch
from
8bf6c62 to
7ce5cd1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.