[asterisk-dev] [Code Review]: named_acls: Named ACLs - a system for creating and applying ACLs with named profiles which can be shared
jrose
reviewboard at asterisk.org
Fri Jun 8 11:16:10 CDT 2012
> On June 7, 2012, 1:34 p.m., Mark Michelson wrote:
> > /trunk/configs/acl.conf.sample, lines 37-44
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28608#file28608line37>
> >
> > It's easier to understand this example if you just combine the rules into a single named ACL.
> >
> > To be pedantic, the example you give would not result in a single usable address. It would only result in a single usable subnet (10.24.20.0 to be specific). This is because the subnet mask is a /24 and not a /32. To this end, both of the permit lines you provided result in the same subnet being allowed due to the subnet mask.
Combining the two ACLs into one would completely undermine the example since the whole point is that you can include multiple named ACLs and they'll simply be combined into a single ACL in the final result. I could go ahead and show what it becomes equivalent to though.
Thanks for the refresher on subnet masks. I've stripped that off of the examples where I'm just trying to show the address in its plain form... only reason I included them in the first place is because it causes some kind of VIM error highlighting. Not sure if I should be adding /255.255.255.255 to compensate instead.
> On June 7, 2012, 1:34 p.m., Mark Michelson wrote:
> > /trunk/configs/acl.conf.sample, line 35
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28608#file28608line35>
> >
> > Here's another case where you used "nacl" where you likely meant to either use "named ACL" or "ACL"
Check
> On June 7, 2012, 1:34 p.m., Mark Michelson wrote:
> > /trunk/configs/acl.conf.sample, line 16
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28608#file28608line16>
> >
> > I think "specify" would may more sense here than "append"
check
> On June 7, 2012, 1:34 p.m., Mark Michelson wrote:
> > /trunk/configs/acl.conf.sample, line 15
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28608#file28608line15>
> >
> > s/confuration/configuration/
check
> On June 7, 2012, 1:34 p.m., Mark Michelson wrote:
> > /trunk/configs/acl.conf.sample, lines 26-31
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28608#file28608line26>
> >
> > All of these are weird because they specify the final octet of the IP address but the subnet mask is a /24. So for instance, the two deny lines you provide in example_named_acl2 are redundant since they are blocking the same subnet.
check
> On June 7, 2012, 1:34 p.m., Mark Michelson wrote:
> > /trunk/channels/chan_h323.c, lines 1491-1493
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28602#file28602line1491>
> >
> > Given the naming convention for other options, this should probably be "acl" instead of "nacl"
check
> On June 7, 2012, 1:34 p.m., Mark Michelson wrote:
> > /trunk/include/asterisk/acl.h, line 271
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28613#file28613line271>
> >
> > s/an/a/
check from above review
> On June 7, 2012, 1:34 p.m., Mark Michelson wrote:
> > /trunk/configs/acl.conf.sample, lines 33-34
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28608#file28608line33>
> >
> > The example should use the guideline you specified of denying everything and then whitelisting specific addresses.
The idea here actually was that the configuration using the ACL would deny everything and then include the ACL rather than the ACL itself so that multiple whitelists could be easily combined... like so:
<acl.conf>
[acl_lab1]
; a bunch of permits in here listing all of the ip addresses of phones in lab 1
[acl_lab2]
; a bunch of permits in here listing all of the ip addresses of phones in lab 2
<sip.conf>
[some_peer_that_could_be_anything_from_lab1_or_lab2]
; We want to allow any ip address from lab1 or lab2 to use this profile
deny=0.0.0.0/0.0.0.0
acl=acl_lab1
acl=acl_lab2
I'm still working on figuring out the best way to represent this usage in the sample...
> On June 7, 2012, 1:34 p.m., Mark Michelson wrote:
> > /trunk/include/asterisk/acl.h, line 294
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28613#file28613line294>
> >
> > Give this an ast_ prefix.
check
> On June 7, 2012, 1:34 p.m., Mark Michelson wrote:
> > /trunk/main/manager.c, lines 7319-7321
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28617#file28617line7319>
> >
> > It seems a bit drastic to completely reload manager because named ACLs have changed. Perhaps just update the named ACLs.
That's actually very tricky to do at the moment because we aren't appending the named ACLs in a way that keeps them distinguishable from the parts that were added manually. I could make some changes to handle that though.
> On June 7, 2012, 1:34 p.m., Mark Michelson wrote:
> > /trunk/main/named_acl.c, line 4
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28618#file28618line4>
> >
> > Just 2012 for the copyright.
check
> On June 7, 2012, 1:34 p.m., Mark Michelson wrote:
> > /trunk/main/named_acl.c, lines 88-91
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28618#file28618line88>
> >
> > A possible suggestion here that you don't necessarily have to take:
> >
> > Since you only have one string on this structure, you could redefine the structure like:
> >
> > struct named_acl {
> > struct ast_ha *ha;
> > char name[1];
> > }
> >
> > Then just allocate enough space for the name instead of having it be 80 bytes every time.
I'm going to wait on this one since that's more of a minor optimization thing than functionality. I'll come back to it.
> On June 7, 2012, 1:34 p.m., Mark Michelson wrote:
> > /trunk/main/named_acl.c, line 330
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28618#file28618line330>
> >
> > Optional elements are typically enclosed in square brackets.
check
- jrose
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1978/#review6404
-----------------------------------------------------------
On June 7, 2012, 10:48 a.m., jrose wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1978/
> -----------------------------------------------------------
>
> (Updated June 7, 2012, 10:48 a.m.)
>
>
> Review request for Asterisk Developers, Mark Michelson, Terry Wilson, and Olle E Johansson.
>
>
> Summary
> -------
>
> This feature is based on oej's deluxepine (or something like that) branch with a similarly named feature. ACLs are defined in acl.conf and can be used by pretty much anything that has ACL options permit/deny (acl='aclname'). acl= works similarly to permit= and deny= in that it simply appends to the working ACL, so they can be combined with other uses of permit/deny/acl.
>
> Also in use in this patch are twilson's new config options.
>
> Since named acls are duplicated when used in another configuration, configurations that use named acls need to be updated if acl.conf is reloaded. This is accomplished with a new event type and the consumption of that event is demonstrated currently only in manager.conf
> If this seems like a proper approach to this problem, that will be replicated across other consumers of named acls.
>
>
> Diffs
> -----
>
> /trunk/CHANGES 368662
> /trunk/channels/chan_h323.c 368662
> /trunk/channels/chan_iax2.c 368662
> /trunk/channels/chan_mgcp.c 368662
> /trunk/channels/chan_sip.c 368662
> /trunk/channels/chan_skinny.c 368662
> /trunk/channels/chan_unistim.c 368662
> /trunk/configs/acl.conf.sample PRE-CREATION
> /trunk/configs/iax.conf.sample 368662
> /trunk/configs/manager.conf.sample 368662
> /trunk/configs/sip.conf.sample 368662
> /trunk/configs/skinny.conf.sample 368662
> /trunk/include/asterisk/acl.h 368662
> /trunk/include/asterisk/event_defs.h 368662
> /trunk/main/acl.c 368662
> /trunk/main/asterisk.c 368662
> /trunk/main/manager.c 368662
> /trunk/main/named_acl.c PRE-CREATION
>
> Diff: https://reviewboard.asterisk.org/r/1978/diff
>
>
> Testing
> -------
>
> Various tests for configuring and using named acls were performed, and a task for writing comprehensive testsuite tests is in the queue. Additionally, various means of reloading the configuration have been performed, and so far they pan out aside from a bug with an unchanged acl.conf which is a generic problem against config options accidentally introduced a little while back.
>
>
> Thanks,
>
> jrose
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120608/bf94390e/attachment-0001.htm>
More information about the asterisk-dev
mailing list