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

Mon May 6 12:19:09 CDT 2013

> On May 6, 2013, 2:13 p.m., Matt Jordan wrote:
> > /trunk/main/security_events.c, lines 568-569
> > <https://reviewboard.asterisk.org/r/2496/diff/1/?file=37181#file37181line568>
> >
> >     Don't ref bump the string if setting the JSON object fails. Ref bump it just prior to returning success.

I actually asked David about this function (ast_json_object_set) as I was writing these. The documentation describes it with the following:

 * The object steals the \a value reference; use ast_json_ref() to safely keep a pointer
 * to it.

Specifically, what I asked was whether or not the reference would still be stolen if json_object_set fails. His response was that he believes this to be the case, and if not that we should adjust ast_json_object_set so that if json_object_set fails and it doesn't take the reference that we take the reference away anyway.

Anyway, what I'm getting is that for the json_string here, bumping the ref count after doing ast_json_object_set strikes me as technically incorrect. Yes, the reference should still be valid as long as ast_json_object_set succeeded, but it isn't valid through the json_string pointer.

In truth though, the way I'm using ast_json_object_set around here, it just isn't necessary to hold onto that reference anyway. My line of thinking here was that I should keep a reference for when the function exits because of RAII_VAR, but according to David json_decref is tolerant to NULL pointers anyway.

I'm doing away with RAII_VAR for the json_strings since their only purpose ever is to be created and then immediately shoved in the json object. There are only two sides to the operation. If it fails on the string creation side, we didn't get a reference to the string so there is no need to unref. If it fails on the object set side then the reference was already taken.

I'll add a short note about that in comments for the more elaborate function with multiple sets since it isn't explicitly clear.

> On May 6, 2013, 2:13 p.m., Matt Jordan wrote:
> > /trunk/main/security_events.c, line 535
> > <https://reviewboard.asterisk.org/r/2496/diff/1/?file=37181#file37181line535>
> >
> >     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.

See below regarding when ast_json_object_set fails.

- jrose

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

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/1bbbbe67/attachment.htm>