[asterisk-dev] [Code Review] named_acls: Named ACLs - a system for creating and applying ACLs with named profiles which can be shared

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1978/#review6404
-----------------------------------------------------------


I started this review a while ago and Terry and Josh got their say in before I was able to publish, so sorry if there's overlap.

This is mostly looking good. One thing to look for globally is that "ACL" should be capitalized in documentation and other user-readable output. The exception, of course, is when talking about the name of the option "acl" and in CLI commands and such.


/trunk/channels/chan_h323.c
<https://reviewboard.asterisk.org/r/1978/#comment11994>

    Given the naming convention for other options, this should probably be "acl" instead of "nacl"



/trunk/configs/acl.conf.sample
<https://reviewboard.asterisk.org/r/1978/#comment11995>

    s/confuration/configuration/



/trunk/configs/acl.conf.sample
<https://reviewboard.asterisk.org/r/1978/#comment11996>

    I think "specify" would may more sense here than "append"



/trunk/configs/acl.conf.sample
<https://reviewboard.asterisk.org/r/1978/#comment12001>

    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.



/trunk/configs/acl.conf.sample
<https://reviewboard.asterisk.org/r/1978/#comment11998>

    The example should use the guideline you specified of denying everything and then whitelisting specific addresses.



/trunk/configs/acl.conf.sample
<https://reviewboard.asterisk.org/r/1978/#comment11999>

    Here's another case where you used "nacl" where you likely meant to either use "named ACL" or "ACL"



/trunk/configs/acl.conf.sample
<https://reviewboard.asterisk.org/r/1978/#comment12000>

    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.



/trunk/include/asterisk/acl.h
<https://reviewboard.asterisk.org/r/1978/#comment12002>

    s/an/a/



/trunk/include/asterisk/acl.h
<https://reviewboard.asterisk.org/r/1978/#comment12011>

    Give this an ast_ prefix.



/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/1978/#comment12005>

    It seems a bit drastic to completely reload manager because named ACLs have changed. Perhaps just update the named ACLs.



/trunk/main/named_acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12006>

    Just 2012 for the copyright.



/trunk/main/named_acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12025>

    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.



/trunk/main/named_acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12012>

    Optional elements are typically enclosed in square brackets.


- Mark


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/20120607/804c08dd/attachment-0001.htm>