[asterisk-dev] [Code Review] 2496: Stasis: Update security events to use stasis. Move ACL messages to the security topic.

[Matt Jordan]

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



/trunk/include/asterisk/acl.h
<https://reviewboard.asterisk.org/r/2496/#comment16315>

    Add a '\ref' for the reference to ast_security_topic.



/trunk/include/asterisk/security_events.h
<https://reviewboard.asterisk.org/r/2496/#comment16316>

    A few global comments on the doxygen:
    
    1) Capitalization is your friend. '\brief' is markup for doxygen to process, not the beginning of a sentence.
    
    2) Typically, accessors are denoted by 'get' or 'set' in their names. While this is technically an accessor, we haven't been documenting these functions as such. When providing new functions that belong to a family of functions, it is generally good to follow the convention already established.
    
    For topics:
    
    \brief A topic which [publishes|provides|etc.] messages for [item]
    
    \retval Topic for [item]
    
    For messages:
    
    \brief Message type for \ref [item]
    
    \retval Message type for \ref [item]
    
    3) Make use of \ref where appropriate.



/trunk/main/asterisk.c
<https://reviewboard.asterisk.org/r/2496/#comment16317>

    This should return an integer value. Allocation of topics and message types can fail.



/trunk/main/security_events.c
<https://reviewboard.asterisk.org/r/2496/#comment16319>

    1) Allocation of these can fail. Return -1 if they do so.
    
    2) Add a cleanup function to dispose of these, and register it at exit. These are ao2 objects and need to be cleaned up.
    
    Users of valgrind and ref count debugging tools will thank you.



/trunk/main/security_events.c
<https://reviewboard.asterisk.org/r/2496/#comment16320>

    I don't think having this as a ternary expression improves readability. Since this is already a RAII_VAR, check for success of string creation, returning -1 if it failed. Then check for success in setting the object, returning -1 if it failed. Return 0 otherwise.



/trunk/main/security_events.c
<https://reviewboard.asterisk.org/r/2496/#comment16321>

    Don't combine the ref bump of the JSON string with setting the JSON object. If setting the JSON object fails, you have a ref leak.
    
    You should only ref bump the string if everything succeeds.



/trunk/main/security_events.c
<https://reviewboard.asterisk.org/r/2496/#comment16322>

    Don't ref bump the string if setting the JSON object fails. Ref bump it just prior to returning success.



/trunk/main/security_events.c
<https://reviewboard.asterisk.org/r/2496/#comment16323>

    Same findings here



/trunk/main/security_events.c
<https://reviewboard.asterisk.org/r/2496/#comment16325>

    There's no point in making these RAII_VAR if you're not going to return at any point in time. Here, RAII_VAR feels like it just complicates the code, as you have to ref bump the string on success.



/trunk/main/security_events.c
<https://reviewboard.asterisk.org/r/2496/#comment16324>

    And here



/trunk/main/security_events.c
<https://reviewboard.asterisk.org/r/2496/#comment16326>

    I'm not sure about the error checking in this function. I don't think we should have silent failures if we fail to store one of the IEs.
    
    I would expect that a failure to add an IE should return NULL, indicating that we could not allocate a security event object.


- Matt Jordan


On May 3, 2013, 7:42 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2496/
> -----------------------------------------------------------
> 
> (Updated May 3, 2013, 7:42 p.m.)
> 
> 
> Review request for Asterisk Developers, David Lee, kmoore, and Matt Jordan.
> 
> 
> Bugs: ASTERISK-21103
>     https://issues.asterisk.org/jira/browse/ASTERISK-21103
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Stage 3/3 of ASTERISK-21103. In order to convert this set of messages I had to change all of the event blob stuff into JSON strings inside of a JSON blob loaded onto a JSON payload and sent out over stasis to the security topic where it is then consumed by stasis and read into a log message in the appropriate fashion with the fields in the right order to catch the bird to catch the spider to catch the fly.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_iax2.c 387594 
>   /trunk/channels/chan_sip.c 387594 
>   /trunk/include/asterisk/acl.h 387594 
>   /trunk/include/asterisk/security_events.h 387594 
>   /trunk/main/asterisk.c 387594 
>   /trunk/main/manager.c 387594 
>   /trunk/main/named_acl.c 387594 
>   /trunk/main/security_events.c 387594 
>   /trunk/res/res_security_log.c 387594 
> 
> Diff: https://reviewboard.asterisk.org/r/2496/diff/
> 
> 
> Testing
> -------
> 
> Made sure that the security messages were being generated in the same way as they were prior to the stasis change by using the following CLI command:
> 
> securityevents test generation
> 
> 
> Examples Before:
> [May  3 14:40:28] SECURITY[1305]: res_security_log.c:134 security_event_cb: SecurityEvent="FailedACL",EventTV="1367610028-869814",Severity="Error",Service="TEST",EventVersion="1",AccountID="Username",SessionID="Session123",LocalAddress="IPV4/UDP/192.168.1.1/12121",RemoteAddress="IPV4/UDP/192.168.1.2/12345",Module="test_security_events",ACLName="TEST_ACL",SessionTV="1367610028-869754"
> [May  3 14:40:28] SECURITY[1305]: res_security_log.c:134 security_event_cb: SecurityEvent="InvalidAccountID",EventTV="1367610028-869895",Severity="Error",Service="TEST",EventVersion="1",AccountID="FakeUser",SessionID="Session456",LocalAddress="IPV4/TCP/10.1.2.3/4321",RemoteAddress="IPV4/TCP/10.1.2.4/123",Module="test_security_events",SessionTV="1367610028-869854"
> [May  3 14:40:28] SECURITY[1305]: res_security_log.c:134 security_event_cb: SecurityEvent="SessionLimit",EventTV="1367610028-869960",Severity="Error",Service="TEST",EventVersion="1",AccountID="Jenny",SessionID="8675309",LocalAddress="IPV4/TLS/10.5.4.3/4444",RemoteAddress="IPV4/TLS/10.5.4.2/3333",Module="test_security_events",SessionTV="1367610028-869923"
> 
> 
> Examples After:
> [May  3 14:33:35] SECURITY[31607]: res_security_log.c:122 security_event_stasis_cb: SecurityEvent="FailedACL",EventTV="1367609615-957822",Severity="Error",Service="TEST",EventVersion="1",AccountID="Username",SessionID="Session123",LocalAddress="IPV4/UDP/192.168.1.1/12121",RemoteAddress="IPV4/UDP/192.168.1.2/12345",Module="test_security_events",ACLName="TEST_ACL",SessionTV="1367609615-957750"
> [May  3 14:33:35] SECURITY[31608]: res_security_log.c:122 security_event_stasis_cb: SecurityEvent="InvalidAccountID",EventTV="1367609615-958101",Severity="Error",Service="TEST",EventVersion="1",AccountID="FakeUser",SessionID="Session456",LocalAddress="IPV4/TCP/10.1.2.3/4321",RemoteAddress="IPV4/TCP/10.1.2.4/123",Module="test_security_events",SessionTV="1367609615-957969"
> [May  3 14:33:35] SECURITY[31608]: res_security_log.c:122 security_event_stasis_cb: SecurityEvent="SessionLimit",EventTV="1367609615-958404",Severity="Error",Service="TEST",EventVersion="1",AccountID="Jenny",SessionID="8675309",LocalAddress="IPV4/TLS/10.5.4.3/4444",RemoteAddress="IPV4/TLS/10.5.4.2/3333",Module="test_security_events",SessionTV="1367609615-958360"
> 
> 
> As you can see it's basically identical aside from things that are expected to change between runs and the name of the function generating the messages.
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130506/e3d83504/attachment-0001.htm>