[Asterisk-code-review] STIR/SHAKEN: Option split and response codes. (asterisk[16])

[Kevin Harwell]

Attention is currently required from: Benjamin Keith Ford.
Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/16497 )

Change subject: STIR/SHAKEN: Option split and response codes.
......................................................................


Patch Set 3:

(3 comments)

File include/asterisk/res_stir_shaken.h:

https://gerrit.asterisk.org/c/asterisk/+/16497/comment/54f7a3e4_5d2cf42a 
PS3, Line 94: int ast_stir_shaken_verify(const char *header, const char *payload, const char *signature, const char *algorithm,
> Yeah, I'm not sure what to do about this honestly. […]
You have to leave the old function call, and create a whole new one. Then in your .c file you have one call the other, or both call shared code. The old function still returning what it always has.

If more parameters need to be added in the future then you either have to:

1) create other function call(s) for those later, or
2) if you know for sure they'll be added then add them now to this new one and they'll essentially be noop  (saved for future use) until the code changes later. Or,
3) create a params and/or results structure to pass in and/or return that contains the values you need. Future expansion can then be done to the end of that structure without changing the function definition.


File res/res_pjsip_stir_shaken.c:

https://gerrit.asterisk.org/c/asterisk/+/16497/comment/d53bb36b_4eefc410 
PS3, Line 129: 	date_hdr_val = ast_sip_rdata_get_header_value(rdata, date_hdr_str);
> At the end of that function, the string is duplicated with null termination via pj_strdup_with_null, […]
Oh ya I missed the dupe part in that function. That's an unusual way to do such, but I suppose it works as long as rdata's pool stays valid, which it appears to be the case in these instances.


https://gerrit.asterisk.org/c/asterisk/+/16497/comment/12e423f4_381d7a44 
PS3, Line 140: 	sscanf(remainder, "%79s", timezone);
> This was actually snatched from pbx.c. It's possible timezone could be an empty string. […]
Yeah I believe checking the result is sufficient, and something you'd want to do either way.

Alternatively, you could check if timezone is empty prior to sscanf, and skip if so.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/16497
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I4ac1ecf652cd0e336006b0ca638dc826b5b1ebf7
Gerrit-Change-Number: 16497
Gerrit-PatchSet: 3
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Attention: Benjamin Keith Ford <bford at digium.com>
Gerrit-Comment-Date: Mon, 27 Sep 2021 15:33:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kevin Harwell <kharwell at digium.com>
Comment-In-Reply-To: Benjamin Keith Ford <bford at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210927/44313664/attachment.html>