Comp 1145 Added Server-Side Request Forgery (SSRF) protection to the validate-creds API#962
Comp 1145 Added Server-Side Request Forgery (SSRF) protection to the validate-creds API#962munishchouhan merged 22 commits intomasterfrom
Conversation
Signed-off-by: munishchouhan <hrma017@gmail.com>
Signed-off-by: munishchouhan <hrma017@gmail.com>
Signed-off-by: munishchouhan <hrma017@gmail.com>
Signed-off-by: munishchouhan <hrma017@gmail.com>
Signed-off-by: munishchouhan <hrma017@gmail.com>
Signed-off-by: munishchouhan <hrma017@gmail.com>
Signed-off-by: munishchouhan <hrma017@gmail.com>
Signed-off-by: munishchouhan <hrma017@gmail.com>
Signed-off-by: munishchouhan <hrma017@gmail.com>
Signed-off-by: munishchouhan <hrma017@gmail.com>
Signed-off-by: munishchouhan <hrma017@gmail.com>
|
Tested locally:
|
Signed-off-by: munishchouhan <hrma017@gmail.com>
|
@claude review |
|
Claude finished @munishchouhan's task —— View job PR Review: SSRF Protection ImplementationReviewing the SSRF protection implementation for the validate-creds API endpoints. Todo List:
Review SummaryThis is a solid SSRF protection implementation with good security coverage. The code follows defensive security principles and includes comprehensive test coverage. Here's my detailed analysis: ✅ StrengthsSecurity Coverage
Implementation Quality
Test Coverage
🔍 Areas for ConsiderationDNS Rebinding Edge Cases
Error Information Disclosure
IPv6 Completeness
Configuration Validation
🏗️ Code QualityController Integration (
Test Quality (
🔒 Security AssessmentThis implementation effectively mitigates:
The fail-closed approach on DNS errors is particularly good - better to reject legitimate but unresolvable hostnames than allow potential attacks. 📋 Recommendations
Verdict: APPROVED ✅This is a well-implemented SSRF protection mechanism that significantly improves the security posture of the validate-creds endpoints. The code is production-ready and follows security best practices. |
|
tested locally: |
|
@pditommaso I have simplified and tested this PR |
Signed-off-by: munishchouhan <hrma017@gmail.com>
Signed-off-by: munishchouhan <hrma017@gmail.com>
Signed-off-by: munishchouhan <hrma017@gmail.com>
Signed-off-by: munishchouhan <hrma017@gmail.com>
pditommaso
left a comment
There was a problem hiding this comment.
Good security fix overall. The core SSRF protection logic is solid — DNS resolution + IP validation before the auth call is the right approach. A few things to address:
extractHostname — missing port stripping
The registry field can contain a port (e.g. 192.168.1.1:5000). When no scheme is present, extractHostname returns the string as-is, so InetAddress.getAllByName("192.168.1.1:5000") fails with UnknownHostException — giving a confusing "Unable to resolve host" error instead of a proper SSRF rejection.
Should strip the port for bare host:port inputs:
// Strip port from host:port
int colonIdx = host.lastIndexOf(':')
if (colonIdx > 0) {
return host.substring(0, colonIdx)
}validateIpAddress — error messages expose internal info
The error messages expose resolved IPs to the caller (e.g. "Access to private IP address is not allowed: 10.0.0.1"). This confirms to an attacker that DNS resolved to a specific internal address. Consider using a generic message like "Invalid registry hostname" in the exception, and logging the detailed reason server-side.
METADATA_IPS — other cloud providers
This only covers AWS metadata endpoints. For completeness you may want to add other cloud providers, e.g. 100.100.100.200 (Alibaba Cloud IMDS). GCP and Azure use 169.254.169.254 which is already covered by the link-local check, so those are fine.
Signed-off-by: munishchouhan <hrma017@gmail.com>
Signed-off-by: munishchouhan <hrma017@gmail.com>
Fixed |
src/test/groovy/io/seqera/wave/controller/ValidateCredsSsrfControllerTest.groovy
Outdated
Show resolved
Hide resolved
…trollerTest.groovy Co-authored-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Summary
metadata services
Test plan