[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
Mon Jun 11 10:17:45 CDT 2012 


> On June 9, 2012, 8:07 a.m., Kevin Fleming wrote:
> > /trunk/CHANGES, line 47
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28601#file28601line47>
> >
> >     This is 'reload acl' now, right?

Yes, though that change isn't in this diff.  It will be in the next one posted.  Thanks for pointing out another place where that change needs to be represented though.


> On June 9, 2012, 8:07 a.m., Kevin Fleming wrote:
> > /trunk/include/asterisk/acl.h, lines 211-217
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28613#file28613line211>
> >
> >     The function as written does not actually require the destination list to exist. Either change the function to work the way the documentation indicates, or change the documentation so that users won't unnecessarily create an empty list just to pass into the function.
> >     
> >     Also, you did not document the function's return value.

New docs:

/*!
 * \brief append a copy of the contents of one list of host access rules
 *        to the end of another.
 *
 * \param destination May be NULL. If provided, the specified ast_ha will be appended to this.
 * \param source The host access rule list being copied from
 * \retval If destination is provided, destination will be returned with a duplicate of source attached.
 * \retval If destination is NULL, a duplicate of the source will be returned.
 */
struct ast_ha *ast_duplicate_and_append_ha(struct ast_ha *destination, struct ast_ha *source);


> On June 9, 2012, 8:07 a.m., Kevin Fleming wrote:
> > /trunk/include/asterisk/acl.h, line 276
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28613#file28613line276>
> >
> >     intact is one word, not two.

check


> On June 9, 2012, 8:07 a.m., Kevin Fleming wrote:
> > /trunk/main/acl.c, line 278
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28615#file28615line278>
> >
> >     'into this list'? This could be more easily described as 'If there is no destination structure to populate, just return a copy of the source'.

check


> On June 9, 2012, 8:07 a.m., Kevin Fleming wrote:
> > /trunk/include/asterisk/event_defs.h, line 60
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28614#file28614line60>
> >
> >     This is much too generic. If you are going to use the event subsystem to notify consumers of named ACL changes, then the event should be specific to that change.
> >     
> >     I think it would be reasonable for a subscriber to also be able to subscribe to this event for a specific named ACL (by name), so that it can optimize its processing of updates to only the entries that use that named ACL.

Oh, the reason I did this was because I thought most of the events were supposed to be fairly generic.  Changing that isn't a big deal.  The name for this event category is tentatively AST_EVENT_NAMED_ACL_CHANGE.

I'm not sure how easy it will be though to make consumers only update entries that use a particular named ACL since at present, named ACLs are just appended on to whatever ACL they were applied to without any additional information added to split them. Under the new configuration system, I think configs stay in tact. That might be one way to approach it, but since the configuration changes aren't being applied to everything right now, that's probably out.

I'm a little worried that the implementation of this would have to be consumer-specific and probably involve lists of peers/profiles/etc involved with specific named ACLs.  I've got an idea of how to replace the named ACLs within individual ast_ha linked lists that goes something like the following...

1. ast_ha gets a new field, that being a character pointer which may be either NULL or if it's part of a named ACL, point to a string with the same value as the name of the named ACL it is attached to.
2. A new function ast_ha_named_acl_update(struct ast_ha *updating, char *named_acl) could be added which receives an ast_ha and the name of an ACL which is being swapped.
The process for doing this is fairly simple linked list stuff.
  a. next with one trailing node pointer until a node with the name field is string equivalent to named_acl. The named_acl's beginning is stored in another ast_ha pointer.
  b. sever the next pointer link on the trailing node. Save this node in another pointer as well so that it can be reattached later.
  c. continue to iterate with a trailing node pointer until we find a node that doesn't match named_acl.
  d. sever the next pointer link on the trailing node.
  e. ast_free_ha on the beginning node for the separated chunk of the list that was made for the named_acl being refreshed.
  f. now we have two nodes to work with, the first one being the end of the list containing everything before the stripped named ACL (end_of_the_beginning) and the second being the beginning of everything after the named ACL (beginning_of_the_end).
  g. ast_append_named_acl(end_of_the_beginning, named_acl)
  h. iterate through end_of_the_beginning until next = NULL
  i. set that next to beginning_of_the_end.

Actually figuring out how to run that on all of the ast_ha's that use them is probably the tricky part though.  It'd definitely be a step in the right direction for getting oej's desired more dynamic use of named ACLs working though.


> On June 9, 2012, 8:07 a.m., Kevin Fleming wrote:
> > /trunk/main/acl.c, line 283
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28615#file28615line283>
> >
> >     'Find the last entry in the destination list'

check


> On June 9, 2012, 8:07 a.m., Kevin Fleming wrote:
> > /trunk/main/acl.c, line 288
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28615#file28615line288>
> >
> >     Since the API call uses the verb 'duplicate', your comment should as well, although that is repetitive. A better comment would be 'Attach copies of the entries in the source list to the destination list'.

check


> On June 9, 2012, 8:07 a.m., Kevin Fleming wrote:
> > /trunk/main/asterisk.c, lines 4013-4016
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28616#file28616line4013>
> >
> >     This should be occur before *anything* that accepts network connections is initialized (at least, before ast_http_init()). While the HTTP subsystem does not currently support ACLs, if someone was to add support for them in the future, the resulting behavior would be non-intuitive since normal ACLs would function as expected but named ACLs would not.

check

Well, I put it before ast_http_init() anyway.


> On June 9, 2012, 8:07 a.m., Kevin Fleming wrote:
> > /trunk/include/asterisk/acl.h, line 291
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28613#file28613line291>
> >
> >     s/with anything that needs it//
> >     
> >     There is no reason to initialize it other than for things that need it :-)

check


> On June 9, 2012, 8:07 a.m., Kevin Fleming wrote:
> > /trunk/include/asterisk/acl.h, lines 281-283
> > <https://reviewboard.asterisk.org/r/1978/diff/1/?file=28613#file28613line281>
> >
> >     The requested ACL can't be 'invalid'; it could be 'not found', though, as the last sentence in this comment indicates.
> >     
> >     Also, the usage of 'it' in the first sentence is unclear, as there are two nouns in the previous part of the sentence (and 'it' doesn't actually refer to either of them in this context).
> >     
> >     Please consistently capitalize NULL (and acl, as others have pointed out already).

Changed to: 
 *         If the original ast_ha was NULL and the requested ACL is found, then
 *         this function will return a copy of the requested ACL. The returned
 *         ast_ha will be NULL if both the given ast_ha is NULL and the
 *         requested ACL can't be found.


- jrose


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


On June 8, 2012, 2:04 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1978/
> -----------------------------------------------------------
> 
> (Updated June 8, 2012, 2:04 p.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.
> 
> NOTE: This code is very much WIP and not intended for merging.
> 
> 
> 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/20120611/94589765/attachment-0001.htm>

More information about the asterisk-dev mailing list