Mailing List Archive: Varnish: Dev

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

Index | Next | Previous | View Threaded

lkarsten at varnish-software

Jul 31, 2013, 6:46 AM

Post #1 of 8 (56 views)
Permalink

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

Hello all.

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.

Attached is a patch against current master. Documentation and
test case included.

Please consider this for merging into the 4.0 release.

--
With regards,
Lasse Karstensen
Varnish Software AS

Attachments:

0001-Implement-std.ip-to-simplify-ACL-checking-in-VCL.patch (3.97 KB)

fgsch at lodoss

Jul 31, 2013, 6:03 PM

Post #2 of 8 (52 views)
Permalink

Re: [PATCH] Implement std.ip() to simplify ACL checking in VCL [In reply to]

On Wed, Jul 31, 2013 at 2:46 PM, Lasse Karstensen <
lkarsten [at] varnish-software> wrote:

> Hello all.
>
> 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.
>
> Attached is a patch against current master. Documentation and
> test case included.
>
> Please consider this for merging into the 4.0 release.
>

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.

- 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.

Cheers,

f.-

lkarsten at varnish-software

Aug 1, 2013, 3:41 AM

Post #3 of 8 (49 views)
Permalink

Re: [PATCH] Implement std.ip() to simplify ACL checking in VCL [In reply to]

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> 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.

> - 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"));

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.

--
With regards,
Lasse Karstensen
Varnish Software AS

Attachments:

add-ip-std.2013-08-01.patch (3.46 KB)

bilbo at hobbiton

Aug 1, 2013, 8:21 AM

Post #4 of 8 (48 views)
Permalink

Re: [PATCH] Implement std.ip() to simplify ACL checking in VCL [In reply to]

On Thu, Aug 1, 2013 at 5:41 AM, Lasse Karstensen <
lkarsten [at] varnish-software> 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> wrote:
>
> > - 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.
>
> 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've noticed that client.ip and server.ip can be used implicitly as
strings. (Is this without caveats?) So if std.ip accepts a string as the
fall-back, then the VCL never need specify the conversion explicitly, which
would make VCL code more succinct. That is, by accepting a string, both of
these would work, which I think would be helpful:

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

Of course, your point about a fallback fallback still stands if the string
is formatted badly. I suppose I'd just fall back to 0.0.0.0 or something in
that case.

PS. Useful feature, thanks. I wrote a couple of pieces of VCL code where I
had to resort to matching an IP received from a header against a range with
a regex instead of an ACL. This oughtta clean that up for me.

--

As implied by email protocols, the information in this message is
not confidential. Any middle-man or recipient may inspect, modify,
copy, forward, reply to, delete, or filter email for any purpose unless
said parties are otherwise obligated. As the sender, I acknowledge that
I have a lower expectation of the control and privacy of this message
than I would a post-card. Further, nothing in this message is
legally binding without cryptographic evidence of its integrity.

http://bilbo.hobbiton.org/wiki/Eat_My_Sig

fgsch at lodoss

Aug 1, 2013, 6:43 PM

Post #5 of 8 (42 views)
Permalink

Re: [PATCH] Implement std.ip() to simplify ACL checking in VCL [In reply to]

On Thu, Aug 1, 2013 at 11:41 AM, Lasse Karstensen <
lkarsten [at] varnish-software> 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> 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.-

fgsch at lodoss

Aug 1, 2013, 7:02 PM

Post #6 of 8 (42 views)
Permalink

Re: [PATCH] Implement std.ip() to simplify ACL checking in VCL [In reply to]

On Thu, Aug 1, 2013 at 4:21 PM, Leif Pedersen <bilbo [at] hobbiton> wrote:

> On Thu, Aug 1, 2013 at 5:41 AM, Lasse Karstensen <
> lkarsten [at] varnish-software> 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> wrote:
>>
>> > - 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.
>>
>> 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've noticed that client.ip and server.ip can be used implicitly as
> strings. (Is this without caveats?) So if std.ip accepts a string as the
> fall-back, then the VCL never need specify the conversion explicitly, which
> would make VCL code more succinct. That is, by accepting a string, both of
> these would work, which I think would be helpful:
>
> std.ip(req.http.X-Forwarded-For, "127.255.255.255");
> std.ip(req.http.X-Forwarded-For, client.ip);
>
[..]
>

Explicit conversion won't happen in the case above. That'd need to be
implemented first if fallback would change to STRING.

f.-

phk at phk

Aug 4, 2013, 10:12 PM

Post #7 of 8 (25 views)
Permalink

Re: [PATCH] Implement std.ip() to simplify ACL checking in VCL [In reply to]

In message <CAK-wPOj1RtSKwDzvO5pvZ2NMXEXHCiB2N+MDokppM5crHKqyaA [at] mail>
, Leif Pedersen writes:

>> > - 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.

VCL_IP is the right type.

I can't remember if VCC has code to type-convert a STRING to IP, but
if not, that should be added.

--
Poul-Henning Kamp | UNIX since Zilog Zeus 3.20
phk [at] FreeBSD | TCP/IP since RFC 956
FreeBSD committer | BSD since 4.3-tahoe
Never attribute to malice what can adequately be explained by incompetence.

_______________________________________________
varnish-dev mailing list
varnish-dev [at] varnish-cache
https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev

phk at phk

Aug 4, 2013, 11:06 PM

Post #8 of 8 (24 views)
Permalink

Re: [PATCH] Implement std.ip() to simplify ACL checking in VCL [In reply to]

In message <11698.1375679531 [at] critter>, "Poul-Henning Kamp" writes:
>In message <CAK-wPOj1RtSKwDzvO5pvZ2NMXEXHCiB2N+MDokppM5crHKqyaA [at] mail>
>, Leif Pedersen writes:
>
>>> > - 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.
>
>VCL_IP is the right type.
>
>I can't remember if VCC has code to type-convert a STRING to IP, but
>if not, that should be added.

I looked, it doesn't, I'm adding it now.

--
Poul-Henning Kamp | UNIX since Zilog Zeus 3.20
phk [at] FreeBSD | TCP/IP since RFC 956
FreeBSD committer | BSD since 4.3-tahoe
Never attribute to malice what can adequately be explained by incompetence.

_______________________________________________
varnish-dev mailing list
varnish-dev [at] varnish-cache
https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads