[Asterisk-code-review] res_stir_shaken: Implemented signing of JSON payload. (asterisk[master])

[Joshua Colp]

Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/14031 )

Change subject: res_stir_shaken: Implemented signing of JSON payload.
......................................................................


Patch Set 1: Code-Review-1

(8 comments)

https://gerrit.asterisk.org/c/asterisk/+/14031/1/res/res_stir_shaken.c 
File res/res_stir_shaken.c:

https://gerrit.asterisk.org/c/asterisk/+/14031/1/res/res_stir_shaken.c@102 
PS1, Line 102: 	if (!val || strlen(val) == 0) {
Would it be better to use ast_strlen_zero instead?


https://gerrit.asterisk.org/c/asterisk/+/14031/1/res/res_stir_shaken.c@108 
PS1, Line 108: 			"required value '%s'\n", STIR_SHAKEN_PPT);
with messages like this it is good to provide what was actually present as well


https://gerrit.asterisk.org/c/asterisk/+/14031/1/res/res_stir_shaken.c@236 
PS1, Line 236: 	encoded_signature = ast_calloc(1, sizeof(unsigned char) * (signature_length + 1));
Does this actually provide enough space?


https://gerrit.asterisk.org/c/asterisk/+/14031/1/res/res_stir_shaken.c@324 
PS1, Line 324: 	payload = stir_shaken_verify_json(json);
If you are signing some parts won't be present which is critical, because they will be added by us. For that the checking needs to be relaxed to not check for them.


https://gerrit.asterisk.org/c/asterisk/+/14031/1/res/res_stir_shaken.c@329 
PS1, Line 329: 	/* From the payload section of the HSON, get the orig section, and then get
JSON


https://gerrit.asterisk.org/c/asterisk/+/14031/1/res/res_stir_shaken.c@332 
PS1, Line 332: 			ast_json_object_get(json, "payload"), "orig"), "tn"));
Verify that caller_id_num is actually a value, just in case


https://gerrit.asterisk.org/c/asterisk/+/14031/1/res/res_stir_shaken.c@335 
PS1, Line 335: 		ast_log(LOG_ERROR, "Failed to get private key to sign payload\n");
For what callerid?


https://gerrit.asterisk.org/c/asterisk/+/14031/1/res/res_stir_shaken.c@370 
PS1, Line 370: 
The public key URL also needs to be added



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I74fa41c0640ab2a64a1a80110155bd7062f13393
Gerrit-Change-Number: 14031
Gerrit-PatchSet: 1
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Mon, 30 Mar 2020 10:38:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200330/4881d3ad/attachment-0001.html>