<p> Attention is currently required from: Kevin Harwell, Benjamin Keith Ford. </p>
<p><a href="https://gerrit.asterisk.org/c/asterisk/+/16497">View Change</a></p><p>6 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/b831241a_6a955063">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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">Works for me! Would the comment below be irrelevant then?<br> Correct.</pre></blockquote></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/45f85638_aa6e4e7c">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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">These are separate from response codes. They were created without the response codes in mind, as there wasn't anything set in stone at the time of creating the spec. Maybe this could be renamed to 'ast_stir_shaken_verification_dialplan_result' or something? It could still be misleading with that name. But the intention was never to have these tied together with the response codes.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Gotcha.</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/43726cce_ee74dc0b">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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">It seems the consensus is to leave the function definition intact. I'm kinda in the middle on it where I could go either way, since this is technically unfinished code.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Yeah, new function.</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/073ed230_fa7139df">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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">This is going away for sure, back to how it was originally, but with a response that will be sent when certain conditions are met according to RFC8224. That way, we can send 5XX responses too, and if the signature ACTUALLY fails the verification process, we can be aware of that and set the verification on the channel.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">WFM!</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/3c6eca58_1a01f17e">Patch Set #3, Line 620:</a> <code style="font-family:monospace,monospace">int ast_stir_shaken_verify</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">ast_stir_shaken_verification_result is for dialplan logic. There are some things (such as missing identity header, a mismatch in the INVITE when comparing timestamps, etc.) that wouldn't make sense in the scope of this function. I think it is going to revert to returning a ast_stir_shaken_payload, and then another function that does essentially the same thing will have a structure that it populates with additional information on failure (ast_stir_shaken_response, or something similar). That seems to make the most sense with the current implementation, but am open to changing it.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">WFM</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/831a78e4_0f919f8c">Patch Set #3, Line 746:</a> <code style="font-family:monospace,monospace"> return STIR_SHAKEN_RESPONSE_CODE_UNSUPPORTED_CREDENTIAL;</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">On local, I've changed this back to returning NULL and populating a response with 4xx and Unsupported Credential. AST_STIR_SHAKEN_VERIFY_SIGNATURE_FAILED gets set in res_pjsip_stir_shaken since it's just dialplan related. Perhaps that enum should be renamed?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Gotcha.</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: Thu, 30 Sep 2021 13:16:47 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: George Joseph <gjoseph@digium.com> </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>