BED-5301/5298: PenTest Result: Internal IP Address Disclosure AND Rate limit Bypass #1163
Conversation
…ementing our current rate limiting middleware logic
… I need to fix (maybe even add)
ALCooper12
added
the
work in progress
ALCooper12
removed
the
work in progress
| ipGetter := stdlib.WithKeyGetter( | ||
| func(r *http.Request) string { | ||
| if xff := r.Header.Get("X-Forwarded-For"); xff == "" { | ||
| slog.DebugContext(r.Context(), "No data found in X-Forwarded-For header") | ||
| return r.RemoteAddr | ||
| } else { | ||
| ips := strings.Split(xff, ",") | ||
| rightMostIP := strings.TrimSpace(ips[len(ips)-1]) | ||
|
|
||
| return rightMostIP | ||
| } | ||
| }, | ||
| ) | ||
|
|
||
| middleware := stdlib.NewMiddleware(limiter, ipGetter) | ||
|
|
||
| return middleware.Handler | ||
|
|
There was a problem hiding this comment.
I was trying to figure out why we couldn't use the reference X Forwarded For implementation from limiter but stumbled on the fact that you'd expect the leftmost to be the IP, but because we don't have multiple reverse proxies and the loadbalancer doesn't strip incoming X Forwarded For (in our case, almost always spoofed IPs), we can't rely on it. However, this puts us in a weird position because if someone is self hosting BloodHound, this will make it so anyone hosting with multiple reverse proxies/load balancers in front of them will end up detecting internal IPs for rate limiting.
I don't know if there's a one size fits all solution here, but we should probably consider the consequences before moving forward.
Description
/api/v2/loginendpoint in order to address part of the requirements for BED-5300Motivation and Context
This PR addresses:
Why is this change required? What problem does it solve?
How Has This Been Tested?
Screenshots (optional):
UI
Terminal
Types of changes
Checklist: