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

[Benjamin Keith Ford]

Attention is currently required from: Kevin Harwell.
Benjamin Keith Ford 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:

(6 comments)

File include/asterisk/res_stir_shaken.h:

https://gerrit.asterisk.org/c/asterisk/+/16497/comment/4bc4e0a4_d76f6ff4 
PS3, Line 94: int ast_stir_shaken_verify(const char *header, const char *payload, const char *signature, const char *algorithm,
> The altered function definition breaks ABI
Yeah, I'm not sure what to do about this honestly. It needs to be changed, and could even change again to have a response "reason" populated as well. Or return the payload and if payload is null on return, have a code / reason combo or struct that gets populated that you can check. Basically, we need a way to distinguish between "did this fail for valid reasons" or "did this fail because something was malformed". The way it is now, it's impossible to distinguish between the two because that logic that needs to be tested for response codes is nested inside of this function.


File res/res_pjsip_session.c:

https://gerrit.asterisk.org/c/asterisk/+/16497/comment/e8256412_36bccdcc 
PS3, Line 4070: ast_sip_rdata_get_header_value
> I was curious what this function returned. […]
Commented on this in other thread. Feel free to correct my thinking if it's wrong.


File res/res_pjsip_stir_shaken.c:

https://gerrit.asterisk.org/c/asterisk/+/16497/comment/a531733f_d116cae9 
PS3, Line 126: 	char timezone[80];
> Initialize this to NULL just in case, i.e. timezone[80] = {0}. […]
Ack


https://gerrit.asterisk.org/c/asterisk/+/16497/comment/b9141aed_ed4bea39 
PS3, Line 129: 	date_hdr_val = ast_sip_rdata_get_header_value(rdata, date_hdr_str);
> The return value of 'ast_sip_rdata_get_header_value' needs to be fixed somehow. […]
At the end of that function, the string is duplicated with null termination via pj_strdup_with_null, so it should be null-terminated. That function does an alloc and memcpy, so I think this should be ok unless I'm missing something.


https://gerrit.asterisk.org/c/asterisk/+/16497/comment/e5946700_9683b83d 
PS3, Line 140: 	sscanf(remainder, "%79s", timezone);
> Is remainder always guaranteed to be the expected timezone? […]
This was actually snatched from pbx.c. It's possible timezone could be an empty string. In that case, I think checking to see if sscanf populated timezone is all you can really do. Think that's the best approach?


https://gerrit.asterisk.org/c/asterisk/+/16497/comment/c5da989e_cbb0ec09 
PS3, Line 199: 	RAII_VAR(struct ast_json *, json, NULL, ast_json_unref);
             : 	RAII_VAR(char *, combined_str, NULL, ast_free);
> Maybe I am missing something, but these to variables appear to be unused. […]
Leftovers from another implementation that didn't get cleaned up!



-- 
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: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Mon, 27 Sep 2021 13:43:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210927/971aa1a8/attachment.html>