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

Benjamin Keith Ford asteriskteam at digium.com
Tue Sep 21 11:36:25 CDT 2021


Benjamin Keith Ford has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/16525 )


Change subject: STIR/SHAKEN: Option split and response codes.
......................................................................

STIR/SHAKEN: Option split and response codes.

The stir_shaken configuration option now has 4 different choices to pick
from: off, attest, verify, and on. Off and on behave the same way they
do now. Attest will only perform attestation on the endpoint, and verify
will only perform verification on the endpoint.

Certain responses are required to be sent based on certain conditions
for STIR/SHAKEN. For example, if we get a Date header that is outside of
the time range that is considered valid, a 403 Stale Date response
should be sent. This and several other responses have been added.

Change-Id: I4ac1ecf652cd0e336006b0ca638dc826b5b1ebf7
---
A doc/UPGRADE-staging/stir_shaken_option_split.txt
M include/asterisk/res_pjsip.h
M include/asterisk/res_stir_shaken.h
M res/res_pjsip/pjsip_configuration.c
M res/res_pjsip_session.c
M res/res_pjsip_stir_shaken.c
M res/res_stir_shaken.c
7 files changed, 249 insertions(+), 69 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/25/16525/1

diff --git a/doc/UPGRADE-staging/stir_shaken_option_split.txt b/doc/UPGRADE-staging/stir_shaken_option_split.txt
new file mode 100644
index 0000000..79df214
--- /dev/null
+++ b/doc/UPGRADE-staging/stir_shaken_option_split.txt
@@ -0,0 +1,7 @@
+Subject: STIR/SHAKEN
+
+The STIR/SHAKEN configuration option has been split into
+4 different choices: off, attest, verify, and on. Off and
+on behave the same way as before. Attest will only perform
+attestation on the endpoint, and verify will only perform
+verification on the endpoint.
diff --git a/include/asterisk/res_pjsip.h b/include/asterisk/res_pjsip.h
index 351ce09..8094a32 100644
--- a/include/asterisk/res_pjsip.h
+++ b/include/asterisk/res_pjsip.h
@@ -525,6 +525,17 @@
 	AST_SIP_REDIRECT_URI_PJSIP,
 };
 
+enum 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,
+};
+
 /*!
  * \brief Incoming/Outgoing call offer/answer joint codec preference.
  *
@@ -913,7 +924,7 @@
 	unsigned int suppress_q850_reason_headers;
 	/*! Ignore 183 if no SDP is present */
 	unsigned int ignore_183_without_sdp;
-	/*! Enable STIR/SHAKEN support on this endpoint */
+	/*! Set which STIR/SHAKEN behaviors we want on this endpoint */
 	unsigned int stir_shaken;
 	/*! Should we authenticate OPTIONS requests per RFC 3261? */
 	unsigned int allow_unauthenticated_options;
diff --git a/include/asterisk/res_stir_shaken.h b/include/asterisk/res_stir_shaken.h
index 5175907..103eb17 100644
--- a/include/asterisk/res_stir_shaken.h
+++ b/include/asterisk/res_stir_shaken.h
@@ -22,6 +22,12 @@
 #define STIR_SHAKEN_PPT "shaken"
 #define STIR_SHAKEN_TYPE "passport"
 
+/* 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
+
 enum 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 */
@@ -80,12 +86,13 @@
  * \param signature The payload signature
  * \param algorithm The signature algorithm
  * \param public_cert_url The public key URL
+ * \param ss_payload A pointer to a ast_stir_shaken_payload that will get populated on success
  *
- * \retval ast_stir_shaken_payload on success
- * \retval NULL on failure
+ * \retval 0 on success
+ * \retval response code on failure (or -1 if no code to return)
  */
-struct ast_stir_shaken_payload *ast_stir_shaken_verify(const char *header, const char *payload, const char *signature,
-	const char *algorithm, const char *public_cert_url);
+int ast_stir_shaken_verify(const char *header, const char *payload, const char *signature, const char *algorithm,
+	const char *public_cert_url, struct ast_stir_shaken_payload **ss_payload);
 
 /*!
  * \brief Retrieve the stir/shaken sorcery context
diff --git a/res/res_pjsip/pjsip_configuration.c b/res/res_pjsip/pjsip_configuration.c
index 6defa7c..2c9e85d 100644
--- a/res/res_pjsip/pjsip_configuration.c
+++ b/res/res_pjsip/pjsip_configuration.c
@@ -717,6 +717,44 @@
 	return 0;
 }
 
+static int stir_shaken_handler(const struct aco_option *opt, struct ast_variable *var, void *obj)
+{
+	struct ast_sip_endpoint *endpoint = obj;
+
+	if (!strcasecmp("off", var->value)) {
+		endpoint->stir_shaken = AST_SIP_STIR_SHAKEN_OFF;
+	} else if (!strcasecmp("attest", var->value)) {
+		endpoint->stir_shaken = AST_SIP_STIR_SHAKEN_ATTEST;
+	} else if (!strcasecmp("verify", var->value)) {
+		endpoint->stir_shaken = AST_SIP_STIR_SHAKEN_VERIFY;
+	} else if (!strcasecmp("on", var->value)) {
+		endpoint->stir_shaken = AST_SIP_STIR_SHAKEN_ON;
+	} else {
+		ast_log(LOG_WARNING, "'%s' is not a valid value for option "
+			"'stir_shaken' - defaulting to 'off' for endpoint %s\n",
+			var->value, ast_sorcery_object_get_id(endpoint));
+		endpoint->stir_shaken = AST_SIP_STIR_SHAKEN_OFF;
+	}
+
+	return 0;
+}
+
+static const char *stir_shaken_map[] = {
+	[AST_SIP_STIR_SHAKEN_OFF] "off",
+	[AST_SIP_STIR_SHAKEN_ATTEST] = "attest",
+	[AST_SIP_STIR_SHAKEN_VERIFY] = "verify",
+	[AST_SIP_STIR_SHAKEN_ON] = "on",
+};
+
+static int stir_shaken_to_str(const void *obj, const intptr_t *args, char **buf)
+{
+	const struct ast_sip_endpoint *endpoint = obj;
+	if (ARRAY_IN_BOUNDS(endpoint->stir_shaken, stir_shaken_map)) {
+		*buf = ast_strdup(stir_shaken_map[endpoint->stir_shaken]);
+	}
+	return 0;
+}
+
 static int group_handler(const struct aco_option *opt,
 			 struct ast_variable *var, void *obj)
 {
@@ -2152,7 +2190,7 @@
 	ast_sorcery_object_field_register_custom(sip_sorcery, "endpoint", "codec_prefs_outgoing_answer",
 		"prefer: pending, operation: intersect, keep: all",
 		codec_prefs_handler, outgoing_answer_codec_prefs_to_str, NULL, 0, 0);
-	ast_sorcery_object_field_register(sip_sorcery, "endpoint", "stir_shaken", "no", OPT_BOOL_T, 1, FLDSET(struct ast_sip_endpoint, stir_shaken));
+	ast_sorcery_object_field_register_custom(sip_sorcery, "endpoint", "stir_shaken", "off", stir_shaken_handler, stir_shaken_to_str, NULL, 0, 0);
 	ast_sorcery_object_field_register(sip_sorcery, "endpoint", "allow_unauthenticated_options", "no", OPT_BOOL_T, 1, FLDSET(struct ast_sip_endpoint, allow_unauthenticated_options));
 
 	if (ast_sip_initialize_sorcery_transport()) {
diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index b1288b5..c72fa74 100644
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -4051,6 +4051,8 @@
 {
 	RAII_VAR(struct ast_sip_endpoint *, endpoint,
 			ast_pjsip_rdata_get_endpoint(rdata), ao2_cleanup);
+	static const pj_str_t identity_str = { "Identity", 8 };
+	const pj_str_t use_identity_header_str = { "Use Identity Header", 19 };
 	pjsip_inv_session *inv_session = NULL;
 	struct ast_sip_session *session;
 	struct new_invite invite;
@@ -4060,6 +4062,13 @@
 
 	ast_assert(endpoint != NULL);
 
+	if ((endpoint->stir_shaken == AST_SIP_STIR_SHAKEN_VERIFY ||
+		endpoint->stir_shaken == AST_SIP_STIR_SHAKEN_ON) &&
+		!ast_sip_rdata_get_header_value(rdata, identity_str)) {
+		pjsip_endpt_respond_stateless(ast_sip_get_pjsip_endpoint(), rdata, 428, &use_identity_header_str, NULL, NULL);
+		SCOPE_EXIT_RTN("No Identity header when we require one\n");
+	}
+
 	inv_session = pre_session_setup(rdata, endpoint);
 	if (!inv_session) {
 		/* pre_session_setup() returns a response on failure */
diff --git a/res/res_pjsip_stir_shaken.c b/res/res_pjsip_stir_shaken.c
index b2b2084..f1b3962 100644
--- a/res/res_pjsip_stir_shaken.c
+++ b/res/res_pjsip_stir_shaken.c
@@ -32,6 +32,9 @@
 
 #include "asterisk/res_stir_shaken.h"
 
+/*! The Date header will not be valid after this many milliseconds (60 seconds recommended) */
+#define STIR_SHAKEN_DATE_HEADER_TIMEOUT 60000
+
 /*!
  * \brief Get the attestation from the payload
  *
@@ -109,6 +112,56 @@
 	return 0;
 }
 
+static int check_date_header(pjsip_rx_data *rdata)
+{
+	static const pj_str_t date_hdr_str = { "Date", 4 };
+	char *date_hdr_val;
+	struct ast_tm date_hdr_tm;
+	struct timeval date_hdr_timeval;
+	struct timeval current_timeval;
+	char *remainder;
+	char timezone[80];
+	int64_t time_diff;
+
+	date_hdr_val = ast_sip_rdata_get_header_value(rdata, date_hdr_str);
+	if (ast_strlen_zero(date_hdr_val)) {
+		ast_log(LOG_ERROR, "Failed to get Date header from incoming INVITE for STIR/SHAKEN\n");
+		return -1;
+	}
+
+	if (!(remainder = ast_strptime(date_hdr_val, "%a, %d %b %Y %T", &date_hdr_tm))) {
+		ast_log(LOG_ERROR, "Failed to parse Date header\n");
+		return -1;
+	}
+
+	sscanf(remainder, "%79s", timezone);
+	date_hdr_timeval = ast_mktime(&date_hdr_tm, S_OR(timezone, NULL));
+	current_timeval = ast_tvnow();
+
+	time_diff = ast_tvdiff_ms(current_timeval, date_hdr_timeval);
+	if (time_diff < 0) {
+		/* An INVITE from the future! */
+		ast_log(LOG_ERROR, "STIR/SHAKEN Date header has a future date\n");
+		return -1;
+	} else if (time_diff > STIR_SHAKEN_DATE_HEADER_TIMEOUT) {
+		ast_log(LOG_ERROR, "STIR/SHAKEN Date header was outside of the allowable range (60 seconds)\n");
+		return -1;
+	}
+
+	return 0;
+}
+
+/* Send a response back and end the session */
+static void stir_shaken_inv_end_session(struct ast_sip_session *session, pjsip_rx_data *rdata, int response_code, const pj_str_t response_str)
+{
+	pjsip_tx_data *tdata;
+
+	if (pjsip_inv_end_session(session->inv_session, response_code, &response_str, &tdata) == PJ_SUCCESS) {
+		pjsip_endpt_send_response2(ast_sip_get_pjsip_endpoint(), rdata, tdata, NULL, NULL);
+	}
+	ast_hangup(session->channel);
+}
+
 /*!
  * \internal
  * \brief Session supplement callback on an incoming INVITE request
@@ -122,6 +175,10 @@
 static int stir_shaken_incoming_request(struct ast_sip_session *session, pjsip_rx_data *rdata)
 {
 	static const pj_str_t identity_str = { "Identity", 8 };
+	const pj_str_t bad_identity_info_str = { "Bad Identity Info", 17 };
+	const pj_str_t unsupported_credential_str = { "Unsupported Credential", 22 };
+	const pj_str_t stale_date_str = { "Stale Date", 10 };
+	const pj_str_t use_supported_passport_format_str = { "Use Supported PASSporT Format", 29 };
 	char *identity_hdr_val;
 	char *encoded_val;
 	struct ast_channel *chan = session->channel;
@@ -132,10 +189,21 @@
 	char *algorithm;
 	char *public_cert_url;
 	char *attestation;
+	char *ppt;
 	int mismatch = 0;
 	struct ast_stir_shaken_payload *ss_payload;
+	int code;
+	pjsip_tx_data *tdata;
+	RAII_VAR(struct ast_json *, json, NULL, ast_json_unref);
+	RAII_VAR(char *, combined_str, NULL, ast_free);
 
-	if (!session->endpoint->stir_shaken) {
+	/* Check if this is a reinvite. If it is, we don't need to do anything */
+	if (rdata->msg_info.to->tag.slen) {
+		return 0;
+	}
+
+	if (session->endpoint->stir_shaken != AST_SIP_STIR_SHAKEN_VERIFY
+		&& session->endpoint->stir_shaken != AST_SIP_STIR_SHAKEN_ON) {
 		return 0;
 	}
 
@@ -148,48 +216,88 @@
 	encoded_val = strtok_r(identity_hdr_val, ".", &identity_hdr_val);
 	header = ast_base64url_decode_string(encoded_val);
 	if (ast_strlen_zero(header)) {
-		ast_stir_shaken_add_verification(chan, caller_id, "", AST_STIR_SHAKEN_VERIFY_SIGNATURE_FAILED);
-		return 0;
+		ast_debug(3, "STIR/SHAKEN INVITE for %s is missing header\n",
+			ast_sorcery_object_get_id(session->endpoint));
+		stir_shaken_inv_end_session(session, rdata, STIR_SHAKEN_RESPONSE_CODE_BAD_IDENTITY_INFO, bad_identity_info_str);
+		return 1;
 	}
 
 	encoded_val = strtok_r(identity_hdr_val, ".", &identity_hdr_val);
 	payload = ast_base64url_decode_string(encoded_val);
 	if (ast_strlen_zero(payload)) {
-		ast_stir_shaken_add_verification(chan, caller_id, "", AST_STIR_SHAKEN_VERIFY_SIGNATURE_FAILED);
-		return 0;
+		ast_debug(3, "STIR/SHAKEN INVITE for %s is missing payload\n",
+			ast_sorcery_object_get_id(session->endpoint));
+		stir_shaken_inv_end_session(session, rdata, STIR_SHAKEN_RESPONSE_CODE_BAD_IDENTITY_INFO, bad_identity_info_str);
+		return 1;
 	}
 
 	/* It's fine to leave the signature encoded */
 	signature = strtok_r(identity_hdr_val, ";", &identity_hdr_val);
 	if (ast_strlen_zero(signature)) {
-		ast_stir_shaken_add_verification(chan, caller_id, "", AST_STIR_SHAKEN_VERIFY_SIGNATURE_FAILED);
-		return 0;
+		ast_debug(3, "STIR/SHAKEN INVITE for %s is missing signature\n",
+			ast_sorcery_object_get_id(session->endpoint));
+		stir_shaken_inv_end_session(session, rdata, STIR_SHAKEN_RESPONSE_CODE_BAD_IDENTITY_INFO, bad_identity_info_str);
+		return 1;
 	}
 
 	/* Trim "info=<" to get public cert URL */
 	strtok_r(identity_hdr_val, "<", &identity_hdr_val);
 	public_cert_url = strtok_r(identity_hdr_val, ">", &identity_hdr_val);
-	if (ast_strlen_zero(public_cert_url)) {
-		ast_stir_shaken_add_verification(chan, caller_id, "", AST_STIR_SHAKEN_VERIFY_SIGNATURE_FAILED);
-		return 0;
-	}
 
 	/* Make sure the public URL is actually a URL */
-	if (!ast_begins_with(public_cert_url, "http")) {
-		ast_stir_shaken_add_verification(chan, caller_id, "", AST_STIR_SHAKEN_VERIFY_SIGNATURE_FAILED);
-		return 0;
+	if (ast_strlen_zero(public_cert_url) || !ast_begins_with(public_cert_url, "http")) {
+		/* RFC8224 states that if we can't acquire the credentials needed
+		 * by the verification service, we should send a 436 */
+		ast_debug(3, "STIR/SHAKEN INVITE for %s did not  have valid URL (%s)\n",
+			ast_sorcery_object_get_id(session->endpoint), public_cert_url);
+		stir_shaken_inv_end_session(session, rdata, STIR_SHAKEN_RESPONSE_CODE_BAD_IDENTITY_INFO, bad_identity_info_str);
+		return 1;
 	}
 
 	algorithm = strtok_r(identity_hdr_val, ";", &identity_hdr_val);
 	if (ast_strlen_zero(algorithm)) {
-		ast_stir_shaken_add_verification(chan, caller_id, "", AST_STIR_SHAKEN_VERIFY_SIGNATURE_FAILED);
-		return 0;
+		/* RFC8224 states that if the algorithm is not specified, use ES256 */
+		algorithm = STIR_SHAKEN_ENCRYPTION_ALGORITHM;
+	} else {
+		strtok_r(algorithm, "=", &algorithm);
+		if (strcmp(algorithm, STIR_SHAKEN_ENCRYPTION_ALGORITHM)) {
+			/* RFC8224 states that if we don't support the algorithm, send a 437 */
+			ast_debug(3, "STIR/SHAKEN INVITE for %s uses an unsupported algorithm (%s)\n",
+				ast_sorcery_object_get_id(session->endpoint), algorithm);
+			stir_shaken_inv_end_session(session, rdata, STIR_SHAKEN_RESPONSE_CODE_UNSUPPORTED_CREDENTIAL, unsupported_credential_str);
+			return 1;
+		}
+	}
+
+	/* The only thing left should be ppt=shaken (which could have more values later),
+	 * unless using the compact PASSport form */
+	strtok_r(identity_hdr_val, "=", &identity_hdr_val);
+	ppt = ast_strip(identity_hdr_val);
+	if (!ast_strlen_zero(ppt) && strcmp(ppt, STIR_SHAKEN_PPT)) {
+		ast_log(LOG_ERROR, "STIR/SHAKEN INVITE for %s has unsupported ppt (%s)\n",
+			ast_sorcery_object_get_id(session->endpoint), ppt);
+		stir_shaken_inv_end_session(session, rdata, STIR_SHAKEN_RESPONSE_CODE_USE_SUPPORTED_PASSPORT_FORMAT, use_supported_passport_format_str);
+		return 1;
+	}
+
+	if (check_date_header(rdata)) {
+		ast_debug(3, "STIR/SHAKEN INVITE for %s has old Date header\n",
+			ast_sorcery_object_get_id(session->endpoint));
+		stir_shaken_inv_end_session(session, rdata, STIR_SHAKEN_RESPONSE_CODE_STALE_DATE, stale_date_str);
+		return 1;
 	}
 
 	attestation = get_attestation_from_payload(payload);
 
-	ss_payload = ast_stir_shaken_verify(header, payload, signature, algorithm, public_cert_url);
-	if (!ss_payload) {
+	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;
 	}
@@ -333,7 +441,8 @@
 
 static void stir_shaken_outgoing_request(struct ast_sip_session *session, pjsip_tx_data *tdata)
 {
-	if (!session->endpoint->stir_shaken) {
+	if (session->endpoint->stir_shaken != AST_SIP_STIR_SHAKEN_ATTEST
+		&& session->endpoint->stir_shaken != AST_SIP_STIR_SHAKEN_ON) {
 		return;
 	}
 
diff --git a/res/res_stir_shaken.c b/res/res_stir_shaken.c
index 1d8c785..053b61c 100644
--- a/res/res_stir_shaken.c
+++ b/res/res_stir_shaken.c
@@ -617,8 +617,8 @@
 	return filename;
 }
 
-struct ast_stir_shaken_payload *ast_stir_shaken_verify(const char *header, const char *payload, const char *signature,
-	const char *algorithm, const char *public_cert_url)
+int ast_stir_shaken_verify(const char *header, const char *payload, const char *signature, const char *algorithm,
+	const char *public_cert_url, struct ast_stir_shaken_payload **ss_payload)
 {
 	struct ast_stir_shaken_payload *ret_payload;
 	EVP_PKEY *public_key;
@@ -630,27 +630,27 @@
 
 	if (ast_strlen_zero(header)) {
 		ast_log(LOG_ERROR, "'header' is required for STIR/SHAKEN verification\n");
-		return NULL;
+		return -1;
 	}
 
 	if (ast_strlen_zero(payload)) {
 		ast_log(LOG_ERROR, "'payload' is required for STIR/SHAKEN verification\n");
-		return NULL;
+		return -1;
 	}
 
 	if (ast_strlen_zero(signature)) {
 		ast_log(LOG_ERROR, "'signature' is required for STIR/SHAKEN verification\n");
-		return NULL;
+		return -1;
 	}
 
 	if (ast_strlen_zero(algorithm)) {
 		ast_log(LOG_ERROR, "'algorithm' is required for STIR/SHAKEN verification\n");
-		return NULL;
+		return -1;
 	}
 
 	if (ast_strlen_zero(public_cert_url)) {
 		ast_log(LOG_ERROR, "'public_cert_url' is required for STIR/SHAKEN verification\n");
-		return NULL;
+		return -1;
 	}
 
 	/* Check to see if we have already downloaded this public cert. The reason we
@@ -665,7 +665,7 @@
 	 */
 	file_path = get_path_to_public_key(public_cert_url);
 	if (ast_asprintf(&dir_path, "%s/keys/%s", ast_config_AST_DATA_DIR, STIR_SHAKEN_DIR_NAME) < 0) {
-		return NULL;
+		return -1;
 	}
 
 	/* If we don't have an entry in AstDB, CURL from the provided URL */
@@ -681,7 +681,7 @@
 		/* Download to the default path */
 		file_path = run_curl(public_cert_url, dir_path);
 		if (!file_path) {
-			return NULL;
+			return -1;
 		}
 
 		/* Signal that we have already downloaded a new file, no reason to do it again */
@@ -704,7 +704,7 @@
 		ast_free(file_path);
 		file_path = curl_and_check_expiration(public_cert_url, dir_path, &curl);
 		if (!file_path) {
-			return NULL;
+			return STIR_SHAKEN_RESPONSE_CODE_UNSUPPORTED_CREDENTIAL;
 		}
 	}
 
@@ -720,14 +720,14 @@
 		ast_free(file_path);
 		file_path = curl_and_check_expiration(public_cert_url, dir_path, &curl);
 		if (!file_path) {
-			return NULL;
+			return STIR_SHAKEN_RESPONSE_CODE_UNSUPPORTED_CREDENTIAL;
 		}
 
 		public_key = stir_shaken_read_key(file_path, 0);
 		if (!public_key) {
 			ast_log(LOG_ERROR, "Failed to read public key from '%s'\n", file_path);
 			remove_public_key_from_astdb(public_cert_url);
-			return NULL;
+			return STIR_SHAKEN_RESPONSE_CODE_UNSUPPORTED_CREDENTIAL;
 		}
 	}
 
@@ -737,13 +737,13 @@
 	if (!combined_str) {
 		ast_log(LOG_ERROR, "Failed to allocate space for message to verify\n");
 		EVP_PKEY_free(public_key);
-		return NULL;
+		return -1;
 	}
 	snprintf(combined_str, combined_size, "%s.%s", header, payload);
 	if (stir_shaken_verify_signature(combined_str, signature, public_key)) {
 		ast_log(LOG_ERROR, "Failed to verify signature\n");
 		EVP_PKEY_free(public_key);
-		return NULL;
+		return STIR_SHAKEN_RESPONSE_CODE_UNSUPPORTED_CREDENTIAL;
 	}
 
 	/* We don't need the public key anymore */
@@ -752,28 +752,30 @@
 	ret_payload = ast_calloc(1, sizeof(*ret_payload));
 	if (!ret_payload) {
 		ast_log(LOG_ERROR, "Failed to allocate STIR/SHAKEN payload\n");
-		return NULL;
+		return -1;
 	}
 
 	ret_payload->header = ast_json_load_string(header, NULL);
 	if (!ret_payload->header) {
 		ast_log(LOG_ERROR, "Failed to create JSON from header\n");
 		ast_stir_shaken_payload_free(ret_payload);
-		return NULL;
+		return -1;
 	}
 
 	ret_payload->payload = ast_json_load_string(payload, NULL);
 	if (!ret_payload->payload) {
 		ast_log(LOG_ERROR, "Failed to create JSON from payload\n");
 		ast_stir_shaken_payload_free(ret_payload);
-		return NULL;
+		return -1;
 	}
 
 	ret_payload->signature = (unsigned char *)ast_strdup(signature);
 	ret_payload->algorithm = ast_strdup(algorithm);
 	ret_payload->public_cert_url = ast_strdup(public_cert_url);
 
-	return ret_payload;
+	*ss_payload = ret_payload;
+
+	return 0;
 }
 
 /*!
@@ -834,15 +836,11 @@
 		goto cleanup;
 	}
 
-	/* Check the alg value for "ES256" */
+	/* Check to see if there is a value for alg */
 	val = ast_json_string_get(ast_json_object_get(obj, "alg"));
-	if (ast_strlen_zero(val)) {
-		ast_log(LOG_ERROR, "STIR/SHAKEN JWT did not have required field 'alg'\n");
-		goto cleanup;
-	}
-	if (strcmp(val, STIR_SHAKEN_ENCRYPTION_ALGORITHM)) {
-		ast_log(LOG_ERROR, "STIR/SHAKEN JWT field 'alg' did not have "
-			"required value '%s' (was '%s')\n", STIR_SHAKEN_ENCRYPTION_ALGORITHM, val);
+	if (!ast_strlen_zero(val) && !strcmp(val, STIR_SHAKEN_ENCRYPTION_ALGORITHM)) {
+		/* If alg is not present that's fine; if it is and is not ES256, cleanup */
+		ast_log(LOG_ERROR, "STIR/SHAKEN JWT did not have supported type for field 'alg' (was %s)\n", val);
 		goto cleanup;
 	}
 
@@ -1494,6 +1492,7 @@
 	char *public_cert_url = "http://testing123";
 	char *header;
 	char *payload;
+	int ret;
 	struct ast_json *tmp_json;
 	char public_path[] = "/tmp/stir_shaken_public.XXXXXX";
 	char private_path[] = "/tmp/stir_shaken_public.XXXXXX";
@@ -1539,45 +1538,45 @@
 	payload = ast_json_dump_string(tmp_json);
 
 	/* Test empty header parameter */
-	returned_payload = ast_stir_shaken_verify("", payload, (const char *)signed_payload->signature,
-		STIR_SHAKEN_ENCRYPTION_ALGORITHM, public_cert_url);
-	if (returned_payload) {
+	ret = ast_stir_shaken_verify("", payload, (const char *)signed_payload->signature,
+		STIR_SHAKEN_ENCRYPTION_ALGORITHM, public_cert_url, &returned_payload);
+	if (ret == 0) {
 		ast_test_status_update(test, "Verified a signature with missing 'header'\n");
 		test_stir_shaken_cleanup_cert(caller_id_number);
 		return AST_TEST_FAIL;
 	}
 
 	/* Test empty payload parameter */
-	returned_payload = ast_stir_shaken_verify(header, "", (const char *)signed_payload->signature,
-		STIR_SHAKEN_ENCRYPTION_ALGORITHM, public_cert_url);
-	if (returned_payload) {
+	ret = ast_stir_shaken_verify(header, "", (const char *)signed_payload->signature,
+		STIR_SHAKEN_ENCRYPTION_ALGORITHM, public_cert_url, &returned_payload);
+	if (ret == 0) {
 		ast_test_status_update(test, "Verified a signature with missing 'payload'\n");
 		test_stir_shaken_cleanup_cert(caller_id_number);
 		return AST_TEST_FAIL;
 	}
 
 	/* Test empty signature parameter */
-	returned_payload = ast_stir_shaken_verify(header, payload, "",
-		STIR_SHAKEN_ENCRYPTION_ALGORITHM, public_cert_url);
-	if (returned_payload) {
+	ret = ast_stir_shaken_verify(header, payload, "",
+		STIR_SHAKEN_ENCRYPTION_ALGORITHM, public_cert_url, &returned_payload);
+	if (ret == 0) {
 		ast_test_status_update(test, "Verified a signature with missing 'signature'\n");
 		test_stir_shaken_cleanup_cert(caller_id_number);
 		return AST_TEST_FAIL;
 	}
 
 	/* Test empty algorithm parameter */
-	returned_payload = ast_stir_shaken_verify(header, payload, (const char *)signed_payload->signature,
-		"", public_cert_url);
-	if (returned_payload) {
+	ret = ast_stir_shaken_verify(header, payload, (const char *)signed_payload->signature,
+		"", public_cert_url, &returned_payload);
+	if (ret == 0) {
 		ast_test_status_update(test, "Verified a signature with missing 'algorithm'\n");
 		test_stir_shaken_cleanup_cert(caller_id_number);
 		return AST_TEST_FAIL;
 	}
 
 	/* Test empty public key URL */
-	returned_payload = ast_stir_shaken_verify(header, payload, (const char *)signed_payload->signature,
-		STIR_SHAKEN_ENCRYPTION_ALGORITHM, "");
-	if (returned_payload) {
+	ret = ast_stir_shaken_verify(header, payload, (const char *)signed_payload->signature,
+		STIR_SHAKEN_ENCRYPTION_ALGORITHM, "", &returned_payload);
+	if (ret == 0) {
 		ast_test_status_update(test, "Verified a signature with missing 'public key URL'\n");
 		test_stir_shaken_cleanup_cert(caller_id_number);
 		return AST_TEST_FAIL;
@@ -1587,9 +1586,9 @@
 	test_stir_shaken_add_fake_astdb_entry(public_cert_url, public_path);
 
 	/* Verify a valid signature */
-	returned_payload = ast_stir_shaken_verify(header, payload, (const char *)signed_payload->signature,
-		STIR_SHAKEN_ENCRYPTION_ALGORITHM, public_cert_url);
-	if (!returned_payload) {
+	ret = ast_stir_shaken_verify(header, payload, (const char *)signed_payload->signature,
+		STIR_SHAKEN_ENCRYPTION_ALGORITHM, public_cert_url, &returned_payload);
+	if (ret != 0) {
 		ast_test_status_update(test, "Failed to verify a valid signature\n");
 		remove_public_key_from_astdb(public_cert_url);
 		test_stir_shaken_cleanup_cert(caller_id_number);

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/16525
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 19
Gerrit-Change-Id: I4ac1ecf652cd0e336006b0ca638dc826b5b1ebf7
Gerrit-Change-Number: 16525
Gerrit-PatchSet: 1
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210921/09041ac4/attachment-0001.html>


More information about the asterisk-code-review mailing list