[PATCH] Implement std.ip() to simplify ACL checking in VCL

Federico Schwindt fgsch at lodoss.net
Fri Aug 2 03:43:29 CEST 2013


On Thu, Aug 1, 2013 at 11:41 AM, Lasse Karstensen <
lkarsten at varnish-software.com> wrote:

> On Thu, Aug 01, 2013 at 02:03:14AM +0100, Federico Schwindt wrote:
> > On Wed, Jul 31, 2013 at 2:46 PM, Lasse Karstensen <
> > lkarsten at varnish-software.com> wrote:
> > > I've extended the std vmod to include an ip() method, which
> > > converts a string into VCL IP. The result can be used for
> > > matching against a VCL ACL.
> > A few comments:
> > - rp leaks if WS_Reserve() fails.
> > - WS_Reserve() is cheaper that getaddrinfo(), so I'd check first if there
> > is space and then do the getaddrinfo() call. That'd simplify the error
> path
> > too.
> > - Missing CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC).
> > - You could just check for getaddrinfo() != 0 instead of using s = ..
> since
> > it's not used anywhere else.
>
> Hi Federico.
> Thank you for taking the time to review this.
>
> I've implemented the changes you proposed. Updated (full) patch is
> attached.


Style aside looks good to me.


> > - Using VCL_IP for the fallback parameter restricts what you can use to
> > client.ip or server.ip. This might or might not be a problem.
> > I wrote a similar function a while ago that was using a STRING parameter
> as
> > suggested by Tollef. Not sure if this is still required.
>
> Yes, we discussed this for a bit in the office.
> I don't have strong opinions on either side.
>
> You can of course nest them to get an arbitrary fallback:
>         std.ip(req.http.X-Forwarded-For, std.ip("127.255.255.255"));


I take you meant:

std.ip(req.http.X-Forwarded-For, std.ip("127.255.255.255", server.ip));

or similar above.


> The error handling when the fallback conversion fails doesn't seem to have
> any obvious solution. If the user gets to pick a fallback by him/herself,
> then at least they know clearly what to check for.


True. Perhaps this is a good candidate for the VCL Examples wiki page.

f.-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://www.varnish-cache.org/lists/pipermail/varnish-dev/attachments/20130802/a2ab4c9a/attachment.html>


More information about the varnish-dev mailing list