[Asterisk-code-review] STIR/SHAKEN: Option split and response codes. (asterisk[16])

[George Joseph]

Attention is currently required from: Kevin Harwell, Benjamin Keith Ford.
George Joseph 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/b831241a_6a955063 
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,
             : };
             : 
> Works for me! Would the comment below be irrelevant then?
 Correct.


File include/asterisk/res_stir_shaken.h:

https://gerrit.asterisk.org/c/asterisk/+/16497/comment/45f85638_aa6e4e7c 
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 */
            : };
> 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.

Gotcha.


https://gerrit.asterisk.org/c/asterisk/+/16497/comment/43726cce_ee74dc0b 
PS3, Line 94: int ast_stir_shaken_verify(const char *header, const char *payload, const char *signature, const char *algorithm,
> 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.

Yeah, new function.


File res/res_pjsip_stir_shaken.c:

https://gerrit.asterisk.org/c/asterisk/+/16497/comment/073ed230_fa7139df 
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;
             : 	
> 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.

WFM!


File res/res_stir_shaken.c:

https://gerrit.asterisk.org/c/asterisk/+/16497/comment/3c6eca58_1a01f17e 
PS3, Line 620: int ast_stir_shaken_verify
> 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.

WFM


https://gerrit.asterisk.org/c/asterisk/+/16497/comment/831a78e4_0f919f8c 
PS3, Line 746: 		return STIR_SHAKEN_RESPONSE_CODE_UNSUPPORTED_CREDENTIAL;
> 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?

Gotcha.



-- 
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: Kevin Harwell <kharwell at digium.com>
Gerrit-Attention: Benjamin Keith Ford <bford at digium.com>
Gerrit-Comment-Date: Thu, 30 Sep 2021 13:16:47 +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/20210930/67496652/attachment-0001.html>