[Asterisk-code-review] STIR/SHAKEN: Fix certificate type and storage. (asterisk[18])
Benjamin Keith Ford
asteriskteam at digium.com
Wed Apr 21 11:23:46 CDT 2021
Benjamin Keith Ford has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/15823 )
Change subject: STIR/SHAKEN: Fix certificate type and storage.
......................................................................
STIR/SHAKEN: Fix certificate type and storage.
During OpenSIPit, we found out that the public certificates must be of
type X.509. When reading in public keys, we use the corresponding X.509
functions now.
We also discovered that we needed a better naming scheme for the
certificates since certificates with the same name would cause issues
(overwriting certs, etc.). Now when we download a public certificate, we
get the serial number from it and use that as the name of the cached
certificate.
https://wiki.asterisk.org/wiki/display/AST/OpenSIPit+2021
Change-Id: Ia00b20835f5f976e3603797f2f2fb19672d8114d
---
M configs/samples/stir_shaken.conf.sample
M res/res_stir_shaken.c
M res/res_stir_shaken/curl.c
M res/res_stir_shaken/curl.h
M res/res_stir_shaken/stir_shaken.c
M res/res_stir_shaken/stir_shaken.h
6 files changed, 203 insertions(+), 45 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/23/15823/1
diff --git a/configs/samples/stir_shaken.conf.sample b/configs/samples/stir_shaken.conf.sample
index 957fd14..d77e37a 100644
--- a/configs/samples/stir_shaken.conf.sample
+++ b/configs/samples/stir_shaken.conf.sample
@@ -35,7 +35,7 @@
;
; URL to the public key(s). Must contain variable '${CERTIFICATE}' used for
; substitution
-;public_key_url=http://mycompany.com/${CERTIFICATE}.pub
+;public_key_url=http://mycompany.com/${CERTIFICATE}.pem
;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;
@@ -45,11 +45,11 @@
; type must be "certificate"
;type=certificate
;
-; File path to a certificate
-;path=/etc/asterisk/stir/alice.crt
+; File path to a certificate. This can be RSA or ECDSA, but eventually only ECDSA will be supported.
+;path=/etc/asterisk/stir/alice.pem
;
-; URL to the public key
-;public_key_url=http://mycompany.com/alice.pub
+; URL to the public key. Must be of type X509.
+;public_key_url=http://mycompany.com/alice.pem
;
; The caller ID number to match on
;caller_id_number=1234567
diff --git a/res/res_stir_shaken.c b/res/res_stir_shaken.c
index a9f861b..f25eae3 100644
--- a/res/res_stir_shaken.c
+++ b/res/res_stir_shaken.c
@@ -559,29 +559,31 @@
* \param public_key_url The public key URL
* \param path The path to download the file to
*
- * \retval -1 on failure
- * \retval 0 on success
+ * \retval NULL on failure
+ * \retval full path filename on success
*/
-static int run_curl(const char *public_key_url, const char *path)
+static char *run_curl(const char *public_key_url, const char *path)
{
struct curl_cb_data *data;
+ char *filename;
data = curl_cb_data_create();
if (!data) {
ast_log(LOG_ERROR, "Failed to create CURL callback data\n");
- return -1;
+ return NULL;
}
- if (curl_public_key(public_key_url, path, data)) {
+ filename = curl_public_key(public_key_url, path, data);
+ if (!filename) {
ast_log(LOG_ERROR, "Could not retrieve public key for '%s'\n", public_key_url);
curl_cb_data_free(data);
- return -1;
+ return NULL;
}
set_public_key_expiration(public_key_url, data);
curl_cb_data_free(data);
- return 0;
+ return filename;
}
/*!
@@ -592,29 +594,33 @@
* \param path The path to download the file to
* \param curl Flag signaling if we have run CURL or not
*
- * \retval -1 on failure
- * \retval 0 on success
+ * \retval NULL on failure
+ * \retval full path filename on success
*/
-static int curl_and_check_expiration(const char *public_key_url, const char *path, int *curl)
+static char *curl_and_check_expiration(const char *public_key_url, const char *path, int *curl)
{
+ char *filename;
+
if (curl) {
ast_log(LOG_ERROR, "Already downloaded public key '%s'\n", path);
- return -1;
+ return NULL;
}
- if (run_curl(public_key_url, path)) {
- return -1;
+ filename = run_curl(public_key_url, path);
+ if (!filename) {
+ return NULL;
}
if (public_key_is_expired(public_key_url)) {
ast_log(LOG_ERROR, "Newly downloaded public key '%s' is expired\n", path);
- return -1;
+ ast_free(filename);
+ return NULL;
}
*curl = 1;
- add_public_key_to_astdb(public_key_url, path);
+ add_public_key_to_astdb(public_key_url, filename);
- return 0;
+ return filename;
}
struct ast_stir_shaken_payload *ast_stir_shaken_verify(const char *header, const char *payload, const char *signature,
@@ -622,9 +628,9 @@
{
struct ast_stir_shaken_payload *ret_payload;
EVP_PKEY *public_key;
- char *filename;
int curl = 0;
RAII_VAR(char *, file_path, NULL, ast_free);
+ RAII_VAR(char *, dir_path, NULL, ast_free);
RAII_VAR(char *, combined_str, NULL, ast_free);
size_t combined_size;
@@ -664,6 +670,9 @@
* The only thing that would be left to do is pull from the configuration.
*/
file_path = get_path_to_public_key(public_key_url);
+ if (ast_asprintf(&dir_path, "%s/keys/%s", ast_config_AST_DATA_DIR, STIR_SHAKEN_DIR_NAME) < 0) {
+ return NULL;
+ }
/* If we don't have an entry in AstDB, CURL from the provided URL */
if (ast_strlen_zero(file_path)) {
@@ -675,14 +684,9 @@
/* Go ahead and free file_path, in case anything was allocated above */
ast_free(file_path);
- /* Set up the default path */
- filename = basename(public_key_url);
- if (ast_asprintf(&file_path, "%s/keys/%s/%s", ast_config_AST_DATA_DIR, STIR_SHAKEN_DIR_NAME, filename) < 0) {
- return NULL;
- }
-
/* Download to the default path */
- if (run_curl(public_key_url, file_path)) {
+ file_path = run_curl(public_key_url, dir_path);
+ if (!file_path) {
return NULL;
}
@@ -703,7 +707,9 @@
remove_public_key_from_astdb(public_key_url);
/* If this fails, then there's nothing we can do */
- if (curl_and_check_expiration(public_key_url, file_path, &curl)) {
+ ast_free(file_path);
+ file_path = curl_and_check_expiration(public_key_url, dir_path, &curl);
+ if (!file_path) {
return NULL;
}
}
@@ -717,7 +723,9 @@
remove_public_key_from_astdb(public_key_url);
- if (curl_and_check_expiration(public_key_url, file_path, &curl)) {
+ ast_free(file_path);
+ file_path = curl_and_check_expiration(public_key_url, dir_path, &curl);
+ if (!file_path) {
return NULL;
}
diff --git a/res/res_stir_shaken/curl.c b/res/res_stir_shaken/curl.c
index ab29e3d..ab821b0 100644
--- a/res/res_stir_shaken/curl.c
+++ b/res/res_stir_shaken/curl.c
@@ -22,8 +22,10 @@
#include "asterisk/logger.h"
#include "curl.h"
#include "general.h"
+#include "stir_shaken.h"
#include <curl/curl.h>
+#include <sys/stat.h>
/* Used to check CURL headers */
#define MAX_HEADER_LENGTH 1023
@@ -148,9 +150,45 @@
return curl;
}
-int curl_public_key(const char *public_key_url, const char *path, struct curl_cb_data *data)
+/*!
+ * \brief Create a temporary file located at path
+ *
+ * \note This function assumes path does not end with a '/'
+ *
+ * \param path The directory path to create the file in
+ * \param filename Function allocates memory and stores full filename (including path) here
+ *
+ * \retval -1 on failure
+ * \retval file descriptor on success
+ */
+static int create_temp_file(const char *path, char **filename)
+{
+ const char *template_name = "certXXXXXX";
+ int fd;
+
+ if (ast_asprintf(filename, "%s/%s", path, template_name) < 0) {
+ ast_log(LOG_ERROR, "Failed to set up temporary file path for CURL\n");
+ return -1;
+ }
+
+ ast_mkdir(path, 0644);
+
+ if ((fd = mkstemp(*filename)) < 0) {
+ ast_log(LOG_NOTICE, "Failed to create temporary file for CURL\n");
+ ast_free(*filename);
+ return -1;
+ }
+
+ return fd;
+}
+
+char *curl_public_key(const char *public_key_url, const char *path, struct curl_cb_data *data)
{
FILE *public_key_file;
+ RAII_VAR(char *, tmp_filename, NULL, ast_free);
+ char *filename;
+ char *serial;
+ int fd;
long http_code;
CURL *curl;
char curl_errbuf[CURL_ERROR_SIZE + 1];
@@ -160,18 +198,32 @@
curl_errbuf[CURL_ERROR_SIZE] = '\0';
- public_key_file = fopen(path, "wb");
+ /* For now, it's fine to pass in path as is - it shouldn't end with a '/'. However,
+ * if we decide to change how certificates are stored in the future (configurable paths),
+ * then we will need to check to see if path ends with '/', copy everything up to the '/',
+ * and use this new variable for create_temp_file as well as for ast_asprintf below.
+ */
+ fd = create_temp_file(path, &tmp_filename);
+ if (fd == -1) {
+ ast_log(LOG_ERROR, "Failed to get temporary file descriptor for CURL\n");
+ return NULL;
+ }
+
+ public_key_file = fdopen(fd, "wb");
if (!public_key_file) {
ast_log(LOG_ERROR, "Failed to open file '%s' to write public key from '%s': %s (%d)\n",
- path, public_key_url, strerror(errno), errno);
- return -1;
+ tmp_filename, public_key_url, strerror(errno), errno);
+ close(fd);
+ remove(tmp_filename);
+ return NULL;
}
curl = get_curl_instance(data);
if (!curl) {
ast_log(LOG_ERROR, "Failed to set up CURL isntance for '%s'\n", public_key_url);
fclose(public_key_file);
- return -1;
+ remove(tmp_filename);
+ return NULL;
}
curl_easy_setopt(curl, CURLOPT_URL, public_key_url);
@@ -182,7 +234,8 @@
ast_log(LOG_ERROR, "%s\n", curl_errbuf);
curl_easy_cleanup(curl);
fclose(public_key_file);
- return -1;
+ remove(tmp_filename);
+ return NULL;
}
curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &http_code);
@@ -192,8 +245,33 @@
if (http_code / 100 != 2) {
ast_log(LOG_ERROR, "Failed to retrieve URL '%s': code %ld\n", public_key_url, http_code);
- return -1;
+ remove(tmp_filename);
+ return NULL;
}
- return 0;
+ serial = stir_shaken_get_serial_number_x509(tmp_filename);
+ if (!serial) {
+ ast_log(LOG_ERROR, "Failed to get serial from cert %s\n", tmp_filename);
+ remove(tmp_filename);
+ return NULL;
+ }
+
+ if (ast_asprintf(&filename, "%s/%s.pem", path, serial) < 0) {
+ ast_log(LOG_ERROR, "Failed to allocate memory for new filename for temporary "
+ "file %s after CURL\n", tmp_filename);
+ ast_free(serial);
+ remove(tmp_filename);
+ return NULL;
+ }
+
+ ast_free(serial);
+
+ if (rename(tmp_filename, filename)) {
+ ast_log(LOG_ERROR, "Failed to rename temporary file %s to %s after CURL\n", tmp_filename, filename);
+ ast_free(filename);
+ remove(tmp_filename);
+ return NULL;
+ }
+
+ return filename;
}
diff --git a/res/res_stir_shaken/curl.h b/res/res_stir_shaken/curl.h
index d587327..3d6927e 100644
--- a/res/res_stir_shaken/curl.h
+++ b/res/res_stir_shaken/curl.h
@@ -65,9 +65,9 @@
* \param path The path to download the file to
* \param data The curl_cb_data
*
- * \retval 1 on failure
- * \retval 0 on success
+ * \retval NULL on failure
+ * \retval full path filename on success
*/
-int curl_public_key(const char *public_key_url, const char *path, struct curl_cb_data *data);
+char *curl_public_key(const char *public_key_url, const char *path, struct curl_cb_data *data);
#endif /* _STIR_SHAKEN_CURL_H */
diff --git a/res/res_stir_shaken/stir_shaken.c b/res/res_stir_shaken/stir_shaken.c
index 0b38732..1232e2c 100644
--- a/res/res_stir_shaken/stir_shaken.c
+++ b/res/res_stir_shaken/stir_shaken.c
@@ -90,6 +90,7 @@
{
EVP_PKEY *key = NULL;
FILE *fp;
+ X509 *cert = NULL;
fp = fopen(path, "r");
if (!fp) {
@@ -97,10 +98,20 @@
return NULL;
}
+ /* If this is to get the private key, the file will be ECDSA or RSA, with the former eventually
+ * replacing the latter. For the public key, the file will be X.509.
+ */
if (priv) {
key = PEM_read_PrivateKey(fp, NULL, NULL, NULL);
} else {
- key = PEM_read_PUBKEY(fp, NULL, NULL, NULL);
+ cert = PEM_read_X509(fp, NULL, NULL, NULL);
+ if (!cert) {
+ ast_log(LOG_ERROR, "Failed to read X.509 cert from file '%s'\n", path);
+ fclose(fp);
+ return NULL;
+ }
+ key = X509_get_pubkey(cert);
+ X509_free(cert);
}
if (!key) {
@@ -109,8 +120,9 @@
return NULL;
}
- if (EVP_PKEY_id(key) != EVP_PKEY_EC) {
- ast_log(LOG_ERROR, "%s key from '%s' must be of type EVP_PKEY_EC\n", priv ? "private" : "public", path);
+ if (EVP_PKEY_id(key) != EVP_PKEY_EC && EVP_PKEY_id(key) != EVP_PKEY_RSA) {
+ ast_log(LOG_ERROR, "%s key from '%s' must be of type EVP_PKEY_EC or EVP_PKEY_RSA\n",
+ priv ? "Private" : "Public", path);
fclose(fp);
EVP_PKEY_free(key);
return NULL;
@@ -120,3 +132,53 @@
return key;
}
+
+char *stir_shaken_get_serial_number_x509(const char *path)
+{
+ FILE *fp;
+ X509 *cert;
+ ASN1_INTEGER *serial;
+ BIGNUM *bignum;
+ char *serial_hex;
+
+ fp = fopen(path, "r");
+ if (!fp) {
+ ast_log(LOG_ERROR, "Failed to open file %s\n", path);
+ return NULL;
+ }
+
+ cert = PEM_read_X509(fp, NULL, NULL, NULL);
+ if (!cert) {
+ ast_log(LOG_ERROR, "Failed to read X.509 cert from file %s\n", path);
+ fclose(fp);
+ return NULL;
+ }
+
+ serial = X509_get_serialNumber(cert);
+ if (!serial) {
+ ast_log(LOG_ERROR, "Failed to get serial number from certificate %s\n", path);
+ X509_free(cert);
+ fclose(fp);
+ return NULL;
+ }
+
+ bignum = ASN1_INTEGER_to_BN(serial, NULL);
+ if (bignum == NULL) {
+ ast_log(LOG_ERROR, "Failed to convert serial to bignum\n");
+ X509_free(cert);
+ fclose(fp);
+ return NULL;
+ }
+
+ serial_hex = BN_bn2hex(bignum);
+ X509_free(cert);
+ fclose(fp);
+ BN_free(bignum);
+
+ if (!serial_hex) {
+ ast_log(LOG_ERROR, "Failed to convert bignum to hex\n");
+ return NULL;
+ }
+
+ return serial_hex;
+}
diff --git a/res/res_stir_shaken/stir_shaken.h b/res/res_stir_shaken/stir_shaken.h
index a49050e..df87a5f 100644
--- a/res/res_stir_shaken/stir_shaken.h
+++ b/res/res_stir_shaken/stir_shaken.h
@@ -52,4 +52,14 @@
*/
EVP_PKEY *stir_shaken_read_key(const char *path, int priv);
+/*!
+ * \brief Gets the serial number in hex form from the X509 certificate at path
+ *
+ * \param path The full path of the X509 certificate
+ *
+ * \retval NULL on failure
+ * \retval serial number on success
+ */
+char *stir_shaken_get_serial_number_x509(const char *path);
+
#endif /* _STIR_SHAKEN_H */
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/15823
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 18
Gerrit-Change-Id: Ia00b20835f5f976e3603797f2f2fb19672d8114d
Gerrit-Change-Number: 15823
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/20210421/84d4a858/attachment-0001.html>
More information about the asterisk-code-review
mailing list