<p> Attention is currently required from: Kevin Harwell, Benjamin Keith Ford. </p>
<p>Patch set 3:<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/+/16497">View Change</a></p><p>8 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">File include/asterisk/res_pjsip.h:</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/+/16497/comment/e5a67b4e_3a02a3ac">Patch Set #3, Line 500:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">num ast_sip_stir_shaken_behavior {<br> /*! Don't do any STIR/SHAKEN operations */<br> AST_SIP_STIR_SHAKEN_OFF = 0,<br> /*! Only do STIR/SHAKEN attestation */<br> AST_SIP_STIR_SHAKEN_ATTEST,<br> /*! Only do STIR/SHAKEN verification */<br> AST_SIP_STIR_SHAKEN_VERIFY,<br> /*! Do STIR/SHAKEN attestation and verification */<br> AST_SIP_STIR_SHAKEN_ON,<br>};<br><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think this would be better as a bitmap because you always seem to test for (ATTEST || ON) or (VERIFY || ON).</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16497/comment/0ca6c1c6_752b2ca4">Patch Set #3, Line 852:</a> <code style="font-family:monospace,monospace">unsigned int</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should be "enum ast_sip_stir_shaken_behavior"</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File include/asterisk/res_stir_shaken.h:</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/+/16497/comment/23113a58_ecb72738">Patch Set #3, Line 25:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">/* Response codes from RFC8224 */<br>#define STIR_SHAKEN_RESPONSE_CODE_STALE_DATE 403<br>#define STIR_SHAKEN_RESPONSE_CODE_USE_SUPPORTED_PASSPORT_FORMAT 428<br>#define STIR_SHAKEN_RESPONSE_CODE_BAD_IDENTITY_INFO 436<br>#define STIR_SHAKEN_RESPONSE_CODE_UNSUPPORTED_CREDENTIAL 437<br><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'd make this an enum with STIR_SHAKEN_RESPONSE_CODE_PASSED = 200 and STIR_SHAKEN_RESPONSE_CODE_SYSTEM_FAILED = 500.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16497/comment/dbcb5765_ffbd22c9">Patch Set #3, Line 31:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">num ast_stir_shaken_verification_result {<br> AST_STIR_SHAKEN_VERIFY_NOT_PRESENT, /*! No STIR/SHAKEN information was available */<br> AST_STIR_SHAKEN_VERIFY_SIGNATURE_FAILED, /*! Signature verification failed */<br> AST_STIR_SHAKEN_VERIFY_MISMATCH, /*! Contents of the signaling and the STIR/SHAKEN payload did not match */<br> AST_STIR_SHAKEN_VERIFY_PASSED, /*! Signature verified and contents match signaling */<br>};<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Actually, can these be combined with the above or maybe expanded to include the above?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16497/comment/21144754_f0989bdc">Patch Set #3, Line 94:</a> <code style="font-family:monospace,monospace">int ast_stir_shaken_verify(const char *header, const char *payload, const char *signature, const char *algorithm,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">You have to leave the old function call, and create a whole new one. Then in your . […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Hmmmm. Is the currently released Stir/Shaken stuff actually usable in real life? If not then breaking ABI here to correct the verify function would be OK with me.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_pjsip_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/+/16497/comment/9726c3ec_817fcac2">Patch Set #3, Line 294:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> code = ast_stir_shaken_verify(header, payload, signature, algorithm, public_cert_url, &ss_payload);<br> if (code != 0) {<br> if (code > 0) {<br> /* RFC8224 states that if we can't get the credentials we need, send a 437 */<br> ast_debug(3, "STIR/SHAKEN INVITE for %s failed during verification process\n",<br> ast_sorcery_object_get_id(session->endpoint));<br> stir_shaken_inv_end_session(session, rdata, STIR_SHAKEN_RESPONSE_CODE_UNSUPPORTED_CREDENTIAL, unsupported_credential_str);<br> return 1;<br> }<br> ast_stir_shaken_add_verification(chan, caller_id, attestation, AST_STIR_SHAKEN_VERIFY_SIGNATURE_FAILED);<br> return 0;<br> <br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm a little confused. The only thing ast_stir_shaken_verify currently returns is -1 or STIR_SHAKEN_RESPONSE_CODE_UNSUPPORTED_CREDENTIAL which is hard coded below. Also, if there's a system failure like a memory allocation failure, don't we want to stop and send back a 5XX rather than just saying signature failed? Other comments in ast_stir_shaken_verify.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">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/+/16497/comment/8077ea9a_b33a58bb">Patch Set #3, Line 620:</a> <code style="font-family:monospace,monospace">int ast_stir_shaken_verify</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This function sometimes returns a -1 for error and sometimes STIR_SHAKEN_RESPONSE_CODE_UNSUPPORTED_CREDENTIAL which is a response code. You have an enum named ast_stir_shaken_verification_result. Shouldn't that be what this function returns?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16497/comment/c0a012d1_58d68456">Patch Set #3, Line 746:</a> <code style="font-family:monospace,monospace"> return STIR_SHAKEN_RESPONSE_CODE_UNSUPPORTED_CREDENTIAL;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Shouldn't AST_STIR_SHAKEN_VERIFY_SIGNATURE_FAILED be returned here?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/16497">change 16497</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/+/16497"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 16 </div>
<div style="display:none"> Gerrit-Change-Id: I4ac1ecf652cd0e336006b0ca638dc826b5b1ebf7 </div>
<div style="display:none"> Gerrit-Change-Number: 16497 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </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: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Attention: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Attention: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 28 Sep 2021 12:34:05 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Comment-In-Reply-To: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Comment-In-Reply-To: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>