SSRF-aware SSO domain provisioning (on top of #6521)#6522
Conversation
| allow_reserved_ips?: | ||
| config_env() in [:dev, :ce_dev] and | ||
| get_bool_from_path_or_env(config_dir, "ALLOW_RESERVED_IPS", false) |
There was a problem hiding this comment.
suggestion, non-blocking: Rather than an on-off switch, I suggest we configure a whitelist of reserved IPs. That way, we still exercise the feature in dev
There was a problem hiding this comment.
My network setup is different than yours, someone's docker is different than those, I don't want to deal with that
There was a problem hiding this comment.
In my opinion, needing to adjust the whitelist for my particular dev env is the lesser evil here, compared to disabling the logic completely in dev. To be able to disable it, we create code paths around the feature which may become exploitable in prod.
There was a problem hiding this comment.
How do you exploit that on prod?
There was a problem hiding this comment.
Hmm, that's a valid question, but more appropriate if this were already deployed to prod and we were discussing whether it needs to be patched, and how urgently.
It's not yet, so IMO the matter to resolve is whether a whitelist config is sufficiently better than an on-off config to justify refactoring this PR.
My arguments are that a whitelist keeps exercising the feature even in dev (good for parity) and that having a code path to toggle it off is riskier for us (we mustn't forget to use the toggled-on path when working on XYZ) than having it always on but respecting a whitelist. The counterargument is that a whitelist may cause dev frustration when setting up a non-standard dev env.
There was a problem hiding this comment.
Guarding that switch with config_env is strong enough measure to make it unexploitable in production. The same way it is for test routes in the router or other parts of logic guarded in a similar fashion.
There was a problem hiding this comment.
@zoldar But the true/false config value isn't the guard. skip_checks? propagating through the application code is.
By going the whitelist route, there's no need for skip_checks? code path: in every env - dev, test, prod - we will go through the all the motions to guard against SSRF, with one of the checks being the whitelist.
In some envs, the whitelist will have a few values. In prod, no values.
It's common practice in this repo to have these separate code paths, that's why I'm not requesting changes here, just trying to highlight how there's other ways as well.
There was a problem hiding this comment.
This still applies a safe default, like in many other similar cases.
The whitelist will IMHO be a PITA to manage, adding complexity for little benefit.
|
|
||
| def allowed?(ip) when is_tuple(ip) and tuple_size(ip) in [4, 8] do | ||
| not reserved?(ip) | ||
| case allow_reserved_ips?() do |
| allow_reserved_ips?: | ||
| config_env() in [:dev, :ce_dev] and | ||
| get_bool_from_path_or_env(config_dir, "ALLOW_RESERVED_IPS", false) |
There was a problem hiding this comment.
This still applies a safe default, like in many other similar cases.
The whitelist will IMHO be a PITA to manage, adding complexity for little benefit.
3 participants
Changes
Base branch: ip-tools-ssrf-client (#6521)
SSRF-aware SSO domain provsioning verification