varnishcache-friends / libvmod-geoip2

Varnish VMOD to query MaxMind GeoIP2 DB files
BSD 2-Clause "Simplified" License
39 stars 17 forks source link

method to look up all addresses from a string / extended workspace api / misc #17

Closed nigoroll closed 7 years ago

nigoroll commented 7 years ago

The main use case here is to look up all ip addresses from X-Forwarded-For. I've based this on a suggestion for an extended workspace API which I'll propose for inclusion to varnish-cache.

fgsch commented 7 years ago

I'm not sure about this.

There are unrelated changes and it further complicates the interface. Besides, I'd prefer if we address the iteration over the input in Varnish rather than doing it in per-VMOD basis.

Also, I'm not sure is a good idea. xff is either under your control or could be spoofed. If it's the former looking up all the entries shouldn't be needed. OTOH, if it's the latter you cannot trust the results.

I will ponder a bit more.

nigoroll commented 7 years ago

There are unrelated changes

Yes. I did not see much sense in putting them in an extra PR

and it further complicates the interface.

It's always good to simplify. Ideas?

Besides, I'd prefer if we address the iteration over the input in Varnish rather than doing it in per-VMOD basis.

Ha, yeah, that would be ideal, but I do not see a good way to do this in VCL (inline C ok, but that doesn't count). We neither got arrays, nor a map/apply operator. Again, if you got a good idea, I'm all ears

Also, I'm not sure is a good idea. xff is either under your control or could be spoofed.

Absolutely. But for geo-restrictions, unfortunately, this is a compromise one needs to take if there are intermediaries.

If it's the former looking up all the entries shouldn't be needed. OTOH, if it's the latter you cannot trust the results.

There are, for instance, xffs with the client IP being private and the first or second proxy being the first address from the real originating country.

Thanks, Nils

fgsch commented 7 years ago

Yes. I did not see much sense in putting them in an extra PR

Are these changes really needed though? Also related to the following point.

It's always good to simplify. Ideas?

Not really but I don't know your usecase in detail. Do you call strstr or use regexp on the resulting string? How do you check it? If something is not in the GeoIP2 database, and thus you end up with , , what do you do? Is this an error? Do you fail because it does not pass the criteria or?

Ha, yeah, that would be ideal, but I do not see a good way to do this in VCL (inline C ok, but that doesn't count). We neither got arrays, nor a map/apply operator. Again, if you got a good idea, I'm all ears

I meant we should try to address this in Varnish itself adding some mechanism like apply/map. Doing it in the VMODs doesn't really help moving forward and reduces the chances to have a proper implementation.

Absolutely. But for geo-restrictions, unfortunately, this is a compromise one needs to take if there are intermediaries. There are, for instance, xffs with the client IP being private and the first or second proxy being the first address from the real originating country.

Do you control the intermediaries or just trust them? What's the maximum entries you expect in the xff header? Wouldn't be better to convert one at the time rather than possible looking up severals? Isn't this easily achievable using the re VMOD?

nigoroll commented 7 years ago

Are these changes really needed though?

could you please be specific? The ws_ext is not exactly needed, but these functions help code clarity and simplicity, IMHO. I've previously used similar functions in some vmods and went over them again with the goal of getting towards a suggestion for inclusion in core.

Not really but I don't know your usecase in detail.

I don't think there is much more detail then what I already mentioned:

The behavior is intended to be equivalent to a particular configuration option available on a CDN. We've already discussed the fact that allowing (parts of) XFF makes geo-restrictions even more useless, but I don't think we should discuss this here, this leads us nowhere. What it really boils down to is that geo-restrictions are useless in the first place because they can always be circumvented easily (vpn, tunnel, proxies ...).

Do you call strstr or use regexp on the resulting string? How do you check it?

The basic idea would be to use a regexp for meta-classification, probably something like `\b(?:DE|AT|CH)\b'. I have not yet made up I mind about whether to match against all of XFF or just parts of it, but the idea behind the method I proposed is that, by keeping the exact field formatting of the input, VCL / regexen could be used to correlate IPs with geo lookup results.

If something is not in the GeoIP2 database, and thus you end up with , , what do you do?

The proposed code should substitute the IP with ?? for the "not found" case.

Is this an error? Do you fail because it does not pass the criteria or?

Up to the VCL details. Again, I think it would be good to just have the lookup results available, without having to worry about the exact business logic in the vmod.

I meant we should try to address this in Varnish itself adding some mechanism like apply/map.

Sorry, we do not even have native variable support, I disagree that this is an option at this point.

Doing it in the VMODs doesn't really help moving forward and reduces the chances to have a proper implementation.

My experience is that creating dependencies on core features for which no PR exists which is at least likely to be OKed is a hopeless endeavor..

What's the maximum entries you expect in the xff header? Wouldn't be better to convert one at the time rather than possible looking up severals? Isn't this easily achievable using the re VMOD?

If you wanted to trust all entries, how would you loop?

Yeah sure, implementing an if-ladder to check a fixed number of entries is easy.

nigoroll commented 7 years ago

rebased

nigoroll commented 7 years ago

rebased

fgsch commented 7 years ago

After much pondering I've decided to not add this interface.

I will update devel later to sync it with recent changes in master.