<p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/14031">View Change</a></p><p>8 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/14031/1/res/res_stir_shaken.c">File res/res_stir_shaken.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14031/1/res/res_stir_shaken.c@102">Patch Set #1, Line 102:</a> <code style="font-family:monospace,monospace">       if (!val || strlen(val) == 0) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Would it be better to use ast_strlen_zero instead?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14031/1/res/res_stir_shaken.c@108">Patch Set #1, Line 108:</a> <code style="font-family:monospace,monospace">                  "required value '%s'\n", STIR_SHAKEN_PPT);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">with messages like this it is good to provide what was actually present as well</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14031/1/res/res_stir_shaken.c@236">Patch Set #1, Line 236:</a> <code style="font-family:monospace,monospace">        encoded_signature = ast_calloc(1, sizeof(unsigned char) * (signature_length + 1));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Does this actually provide enough space?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14031/1/res/res_stir_shaken.c@324">Patch Set #1, Line 324:</a> <code style="font-family:monospace,monospace"> payload = stir_shaken_verify_json(json);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14031/1/res/res_stir_shaken.c@329">Patch Set #1, Line 329:</a> <code style="font-family:monospace,monospace">   /* From the payload section of the HSON, get the orig section, and then get</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">JSON</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14031/1/res/res_stir_shaken.c@332">Patch Set #1, Line 332:</a> <code style="font-family:monospace,monospace">                    ast_json_object_get(json, "payload"), "orig"), "tn"));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Verify that caller_id_num is actually a value, just in case</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14031/1/res/res_stir_shaken.c@335">Patch Set #1, Line 335:</a> <code style="font-family:monospace,monospace">            ast_log(LOG_ERROR, "Failed to get private key to sign payload\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">For what callerid?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/14031/1/res/res_stir_shaken.c@370">Patch Set #1, Line 370:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The public key URL also needs to be added</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/14031">change 14031</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/14031"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I74fa41c0640ab2a64a1a80110155bd7062f13393 </div>
<div style="display:none"> Gerrit-Change-Number: 14031 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 30 Mar 2020 10:38:01 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>