<div dir="ltr"><div dir="ltr">On Wed, Dec 4, 2019 at 5:05 AM Joshua C. Colp <<a href="mailto:jcolp@sangoma.com">jcolp@sangoma.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Wed, Dec 4, 2019 at 5:05 AM Jaco Kroon <<a href="mailto:jaco@uls.co.za" target="_blank">jaco@uls.co.za</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
  

    
  
  <div>
    <p>Hi All,</p>
    <p>In ast_apply_acl (main/acl.c) there is two lines that's issuing a
      LOG_WARNING when an ACL gets denied.</p>
    <p>The first happens if the ACL is invalid.  I'm not too worried
      about this specific one, it's probably a good thing if this gets
      logged always.</p>
    <p>The latter, in the case of AST_SENSE_DENY is a bit problematic
      for me.  I've submitted patches now to use ACLs in
      res_rtp_asterisk, and with large number of rejects this can
      quickly spam the logs, and frankly, confuse consumers.</p>
    <p>As I see it, there are two possible solutions:</p>
    <p>Solution 1:<br>
    </p>
    <p>1.  Add AST_SENSE_INVALID as a possible return.<br>
      2.  Rename the current function to ast_apply_acl_(silent|nolog),
      and remove the logging.<br>
      3.  Add a replacement ast_apply_acl function which will generate
      the log entries as per current.</p>
    <p>Solution 2:<br>
    </p>
    <p>Simply don't log at all if the purpose argument is NULL.</p>
    <p>Solution two is the simpler fix, but it's probably also the less
      ideal one.</p>
    <p>The adding of the AST_SENSE_INVALID will also mean that the
      replacement function will need to rewrite AST_SENSE_INVALID =>
      AST_SENSE_DENY, or we need to audit all consumers of the function
      (there fortunately isn't a great many of these) and wherever
      ast_apply_acl(...) == AST_SENSE_DENY is found, it should be
      rewritten as ast_apply_acl(...) != AST_SENSE_ALLOW.</p>
    <p>Would dearly like some opinions on the matter.<br>
    </p>
    <p>PS:  The advantage for me on using ACL over HA is simply the
      named ACL functionality, so in rtp.conf I can state ice_acl =
      named_acl instead of having to embed the ACL into rtp.conf<br>
    </p>
    </div></blockquote></div><div><br></div>I would prefer #1 however the existing function should behave as it does now. Each consumer should not need to be touched, unless they are to be switched to silent. We have an obligation to maintain ABI and behavior of API functions as best we can in case there are any outside consumers as well.</div></blockquote><div><br></div><div>Unfortunately due to how consumer possibly use of the current return value I think the only way to handle this is with #2. Or well by adding another function ast_apply_acl_silent, and removing the logging that is. Your new changes could call this function, and nothing else would be potentially affected.</div></div><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div dir="ltr">Kevin Harwell<div>Senior Software Developer</div><div>Sangoma Technologies<br><div>Check us out at: <a href="https://sangoma.com/" target="_blank">https://sangoma.com</a> & <a href="https://asterisk.org" target="_blank">https://asterisk.org</a></div></div></div></div></div></div></div></div>