[Asterisk-code-review] STIR/SHAKEN: Option split and response codes. (asterisk[16])
George Joseph
asteriskteam at digium.com
Tue Sep 28 07:34:05 CDT 2021
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: Code-Review-1
(8 comments)
File include/asterisk/res_pjsip.h:
https://gerrit.asterisk.org/c/asterisk/+/16497/comment/e5a67b4e_3a02a3ac
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 (VERIFY || ON).
https://gerrit.asterisk.org/c/asterisk/+/16497/comment/0ca6c1c6_752b2ca4
PS3, Line 852: unsigned int
This should be "enum ast_sip_stir_shaken_behavior"
File include/asterisk/res_stir_shaken.h:
https://gerrit.asterisk.org/c/asterisk/+/16497/comment/23113a58_ecb72738
PS3, Line 25: /* Response codes from RFC8224 */
: #define STIR_SHAKEN_RESPONSE_CODE_STALE_DATE 403
: #define STIR_SHAKEN_RESPONSE_CODE_USE_SUPPORTED_PASSPORT_FORMAT 428
: #define STIR_SHAKEN_RESPONSE_CODE_BAD_IDENTITY_INFO 436
: #define STIR_SHAKEN_RESPONSE_CODE_UNSUPPORTED_CREDENTIAL 437
:
I'd make this an enum with STIR_SHAKEN_RESPONSE_CODE_PASSED = 200 and STIR_SHAKEN_RESPONSE_CODE_SYSTEM_FAILED = 500.
https://gerrit.asterisk.org/c/asterisk/+/16497/comment/dbcb5765_ffbd22c9
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?
https://gerrit.asterisk.org/c/asterisk/+/16497/comment/21144754_f0989bdc
PS3, Line 94: int ast_stir_shaken_verify(const char *header, const char *payload, const char *signature, const char *algorithm,
> You have to leave the old function call, and create a whole new one. Then in your . […]
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.
File res/res_pjsip_stir_shaken.c:
https://gerrit.asterisk.org/c/asterisk/+/16497/comment/9726c3ec_817fcac2
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. 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.
File res/res_stir_shaken.c:
https://gerrit.asterisk.org/c/asterisk/+/16497/comment/8077ea9a_b33a58bb
PS3, Line 620: int ast_stir_shaken_verify
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?
https://gerrit.asterisk.org/c/asterisk/+/16497/comment/c0a012d1_58d68456
PS3, Line 746: return STIR_SHAKEN_RESPONSE_CODE_UNSUPPORTED_CREDENTIAL;
Shouldn't AST_STIR_SHAKEN_VERIFY_SIGNATURE_FAILED be returned here?
--
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: Tue, 28 Sep 2021 12:34:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
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/1c85445f/attachment-0001.html>
More information about the asterisk-code-review
mailing list