[Asterisk-code-review] STIR/SHAKEN: Option split and response codes. (asterisk[16])
Benjamin Keith Ford
asteriskteam at digium.com
Tue Sep 28 13:14:00 CDT 2021
Attention is currently required from: George Joseph, 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_pjsip.h:
https://gerrit.asterisk.org/c/asterisk/+/16497/comment/63792073_9774ea16
PS3, Line 500: num ast_sip_stir_shaken_behavior {
: /*! Don't do any STIR/SHAKEN operations */
: AST_SIP_STIR_SHAKEN_OFF = 0,
: /*! Only do STIR/SHAKEN attestation */
: AST_SIP_STIR_SHAKEN_ATTEST,
: /*! Only do STIR/SHAKEN verification */
: AST_SIP_STIR_SHAKEN_VERIFY,
: /*! Do STIR/SHAKEN attestation and verification */
: AST_SIP_STIR_SHAKEN_ON,
: };
:
> I think this would be better as a bitmap because you always seem to test for (ATTEST || ON) or (VERI […]
Works for me! Would the comment below be irrelevant then?
File include/asterisk/res_stir_shaken.h:
https://gerrit.asterisk.org/c/asterisk/+/16497/comment/28b13f6b_2868f057
PS3, Line 31: num ast_stir_shaken_verification_result {
: AST_STIR_SHAKEN_VERIFY_NOT_PRESENT, /*! No STIR/SHAKEN information was available */
: AST_STIR_SHAKEN_VERIFY_SIGNATURE_FAILED, /*! Signature verification failed */
: AST_STIR_SHAKEN_VERIFY_MISMATCH, /*! Contents of the signaling and the STIR/SHAKEN payload did not match */
: AST_STIR_SHAKEN_VERIFY_PASSED, /*! Signature verified and contents match signaling */
: };
> Actually, can these be combined with the above or maybe expanded to include the above?
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.
https://gerrit.asterisk.org/c/asterisk/+/16497/comment/67c0c991_0d23911e
PS3, Line 94: int ast_stir_shaken_verify(const char *header, const char *payload, const char *signature, const char *algorithm,
> Hmmmm. […]
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.
File res/res_pjsip_stir_shaken.c:
https://gerrit.asterisk.org/c/asterisk/+/16497/comment/89a60d33_c00fe261
PS3, Line 294: code = ast_stir_shaken_verify(header, payload, signature, algorithm, public_cert_url, &ss_payload);
: if (code != 0) {
: if (code > 0) {
: /* RFC8224 states that if we can't get the credentials we need, send a 437 */
: ast_debug(3, "STIR/SHAKEN INVITE for %s failed during verification process\n",
: ast_sorcery_object_get_id(session->endpoint));
: stir_shaken_inv_end_session(session, rdata, STIR_SHAKEN_RESPONSE_CODE_UNSUPPORTED_CREDENTIAL, unsupported_credential_str);
: return 1;
: }
: ast_stir_shaken_add_verification(chan, caller_id, attestation, AST_STIR_SHAKEN_VERIFY_SIGNATURE_FAILED);
: return 0;
:
> I'm a little confused. […]
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.
File res/res_stir_shaken.c:
https://gerrit.asterisk.org/c/asterisk/+/16497/comment/a1ed0cff_8224f668
PS3, Line 620: int ast_stir_shaken_verify
> This function sometimes returns a -1 for error and sometimes STIR_SHAKEN_RESPONSE_CODE_UNSUPPORTED_C […]
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.
https://gerrit.asterisk.org/c/asterisk/+/16497/comment/5882ddce_552a120b
PS3, Line 746: return STIR_SHAKEN_RESPONSE_CODE_UNSUPPORTED_CREDENTIAL;
> Shouldn't AST_STIR_SHAKEN_VERIFY_SIGNATURE_FAILED be returned here?
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?
--
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: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Attention: George Joseph <gjoseph at digium.com>
Gerrit-Attention: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Tue, 28 Sep 2021 18:14:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: George Joseph <gjoseph at digium.com>
Comment-In-Reply-To: Kevin Harwell <kharwell at digium.com>
Comment-In-Reply-To: Benjamin Keith Ford <bford at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210928/6c545b63/attachment-0001.html>
More information about the asterisk-code-review
mailing list