Skipper: "Secure" Source predicate which can't be manipulated by the client

[mikkeloscar]

Is your feature request related to a problem? Please describe.
Currently skipper has two predicates for allowing clients access based on IP address/range: Source and SourceFromLast

Both of them rely on first checking the X-Forwarded-For (XFF) header and falling back to the source IP of the request if XFF is not specified. This means that in some setups i.e. if skipper is getting traffic directly from the client (not through a proxy) then it's not possible to protect a route with Source or SourceFromLast because the client can modify the XFF header and work around any IP address limitation specified.

Describe the solution you would like
Skipper should support another predicate which only checks for the source IP such that it can't be faked by the client. I.e. ClientIP or SourceIP. (Alternative could also be a flag to the existing predicates such that it doesn't check XFF: Source("10.2.0.0/16", "xff-false") - something along those lines?)

Would you like to work on it?
Sure

[szuecs]

I would rather create a new predicate called RemoteIP(CIDRs) (or as suggested ClientIP) that only considers http.Request.RemoteIP, because Source() is already var args and the change would break all predicates compatability.

[aermakov-zalando]

I think we've discussed this before when talking about migrating to NLBs, but it might be a good idea to have a command-line switch for the behaviour of the Source predicate as well (or instead). For example, consider our current setup: we probably have more than one ingresses/routegroups relying on the Source filter to do IP-based filtering, which works because we use ELBs in front of Skipper. If we decide to switch to NLBs, however, ideally we'd want the predicates to keep working and doing the right thing in the new setup, because the alternative is a nightmarish migration for the users (which would also prevent us from switching back and forth instead of issues).

[szuecs]

@aermakov-zalando it's a different problem how to migrate an NLB and not really scope of this issue nor this project.
Maybe we can add some flag to be able to add replace P1 with P2 for example to have a generic possibility to rewrite predicates at our scale.
Let's create a new one to cover your idea!

[aermakov-zalando]

I don't see why this is out of scope. My point was that it might make more sense to give the users a higher-level predicate ("filter by source IP, I don't care how exactly it's determined") instead of very low-level ones that need to be changed if something is changed in the setup.

[AlexanderYastrebov]

Might be related: there is -reverse-source-predicate flag
https://github.com/zalando/skipper/blob/6008afe1eabdb5daac926a27c0278b78dee5a373/config/config.go#L272
https://github.com/zalando/skipper/blob/6008afe1eabdb5daac926a27c0278b78dee5a373/skipper.go#L595-L601
but apparently it only affects kubernetes healthcheck endpoint
https://github.com/zalando/skipper/blob/6008afe1eabdb5daac926a27c0278b78dee5a373/skipper.go#L729-L747

[szuecs]

If you check details, the cli switch will not help, because you want to have both at the time of LB switch. It does not matter what we do here. We can easily duplicate routes on PreProcessor or PostProcessor for enabling migration purposes from alb to nlb for example

[aermakov-zalando]

@szuecs The idea would be to run two separate deployments and switch between them, but my main point is that the predicates should separate the intent from the implementation instead of being extremely low level. It doesn't look like this is what you would like, though, so I'll leave it up to you.

[scherniavsky]

Thanks everyone for the quick fix!

[AlexanderYastrebov]

I get the point @aermakov-zalando makes, also it seems here the request is to have RemoteAddr filter right now.

it might be a good idea to have a command-line switch for the behaviour of the Source predicate as well

I think it is still possible to have such a switch that would define what IP check logic is used effectively making Source, SourceFromLast and ClientIP aliases of the same thing.

[szuecs]

@AlexanderYastrebov it doesn’t solve the problem, that you have to support both at the same time for a period of time and then I prefer a generic solution via PreProcessor, that can rewrite other predicates, too.

[AlexanderYastrebov]

@szuecs @aermakov-zalando

that you have to support both at the same time

I don't get it, as...

The idea would be to run two separate deployments and switch between them

I imagine all three of predicates are aliases to the same thing that is controlled by command switch when present. Say first deployment behind ALB uses SourceFromLast as an implementation for all three predicates and second deployment behind NLB uses ClientIP implementation for all three.

Predicate rewrite could be a useful thing if made generic enough though I personally do not see any other usecase so far.

[AlexanderYastrebov]

but my main point is that the predicates should separate the intent from the implementation instead of being extremely low level

Beside ALB->NLB migration there is a usecase when skipper is deployed as a managed infrastructure (say ingress) in known configuration and users can define their own routes.

In such a case users may not be fully aware of the implementation details difference between Source, SourceFromLast and ClientIP in relation to the infrastructure configuration which may lead to insecure routes if wrong predicate is used. Having switch that makes all of three to be aliases to the defined implementation allows infrastructure to ensure routes are safe regardless of predicate used by users.

[aermakov-zalando]

@AlexanderYastrebov I've meant two skipper deployments for the duration of the migration, one running behind the NLB and using IPs for filtering, the other running behind the ELB and using XFF.