[Asterisk-code-review] STIR/SHAKEN: Fix certificate type and storage. (asterisk[16])

[Joshua Colp]

Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/15822 )

Change subject: STIR/SHAKEN: Fix certificate type and storage.
......................................................................


Patch Set 2: Code-Review-1

(9 comments)

https://gerrit.asterisk.org/c/asterisk/+/15822/2/res/res_stir_shaken.c 
File res/res_stir_shaken.c:

https://gerrit.asterisk.org/c/asterisk/+/15822/2/res/res_stir_shaken.c@563 
PS2, Line 563:  * \retval full path filename on success
Document that the caller has to free filename


https://gerrit.asterisk.org/c/asterisk/+/15822/2/res/res_stir_shaken.c@598 
PS2, Line 598:  * \retval full path filename on success
Same here


https://gerrit.asterisk.org/c/asterisk/+/15822/2/res/res_stir_shaken/curl.h 
File res/res_stir_shaken/curl.h:

https://gerrit.asterisk.org/c/asterisk/+/15822/2/res/res_stir_shaken/curl.h@69 
PS2, Line 69:  * \retval full path filename on success
The lifetime of this should be documented, specifically that the caller has to free it


https://gerrit.asterisk.org/c/asterisk/+/15822/2/res/res_stir_shaken/curl.c 
File res/res_stir_shaken/curl.c:

https://gerrit.asterisk.org/c/asterisk/+/15822/2/res/res_stir_shaken/curl.c@197 
PS2, Line 197: 	ast_sha1_hash(hash, public_key_url);
Is this actually used any longer?


https://gerrit.asterisk.org/c/asterisk/+/15822/2/res/res_stir_shaken/stir_shaken.h 
File res/res_stir_shaken/stir_shaken.h:

https://gerrit.asterisk.org/c/asterisk/+/15822/2/res/res_stir_shaken/stir_shaken.h@61 
PS2, Line 61:  * \retval serial number on success
Caller must free it


https://gerrit.asterisk.org/c/asterisk/+/15822/2/res/res_stir_shaken/stir_shaken.c 
File res/res_stir_shaken/stir_shaken.c:

https://gerrit.asterisk.org/c/asterisk/+/15822/2/res/res_stir_shaken/stir_shaken.c@113 
PS2, Line 113: 		key = X509_get_pubkey(cert);
Is this safe to do with the cert being freed below? If so, can it be documented as such?


https://gerrit.asterisk.org/c/asterisk/+/15822/2/res/res_stir_shaken/stir_shaken.c@167 
PS2, Line 167: 		ast_log(LOG_ERROR, "Failed to convert serial to bignum\n");
This should include the certificate


https://gerrit.asterisk.org/c/asterisk/+/15822/2/res/res_stir_shaken/stir_shaken.c@173 
PS2, Line 173: 	serial_hex = BN_bn2hex(bignum);
Document how the memory for this works


https://gerrit.asterisk.org/c/asterisk/+/15822/2/res/res_stir_shaken/stir_shaken.c@179 
PS2, Line 179: 		ast_log(LOG_ERROR, "Failed to convert bignum to hex\n");
This should include the certificate



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

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Ia00b20835f5f976e3603797f2f2fb19672d8114d
Gerrit-Change-Number: 15822
Gerrit-PatchSet: 2
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Tue, 27 Apr 2021 09:54:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210427/46bdbb60/attachment-0001.html>