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

[George Joseph]

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

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


Patch Set 3: Code-Review-1

(7 comments)

Sorry but I only just now got the time to download the patch and look at the whole thing.

https://gerrit.asterisk.org/c/asterisk/+/15822/3/configs/samples/stir_shaken.conf.sample 
File configs/samples/stir_shaken.conf.sample:

https://gerrit.asterisk.org/c/asterisk/+/15822/3/configs/samples/stir_shaken.conf.sample@4 
PS3, Line 4: ;
We talked about whether the settings here apply to attestation or verification or both.  I think you need a blurb about that here.  You could also define that attestation is outgoing and verification is incoming so folks understand the terms.

A link to the wiki page would also be good.


https://gerrit.asterisk.org/c/asterisk/+/15822/3/configs/samples/stir_shaken.conf.sample@25 
PS3, Line 25: ; A certificate store is used to examine, and load all certificates found in a
            : ; given directory. When using this type the public key URL is generated based
            : ; upon the filename, and variable substitution.
Is the cert store used for both our own certs and those coming in?


https://gerrit.asterisk.org/c/asterisk/+/15822/3/configs/samples/stir_shaken.conf.sample@38 
PS3, Line 38: ;public_key_url=http://mycompany.com/${CERTIFICATE}.pem
Since the type is certificate, I'd remove the "key" bit from the name and just go with "public_url".  It's not really a public key, it's a certificate stored in a public place.

I think you also need to explain the ${CERTIFICATE} variable.  How does it get set? By the name of the specific cert below if it has no cert of its own?  You could also show another example...
http://mycompany.com/${CERTIFICATE}/cert.pem


https://gerrit.asterisk.org/c/asterisk/+/15822/3/configs/samples/stir_shaken.conf.sample@42 
PS3, Line 42: ; Individual certificates are declared by using the certificate type.
So does a "certificate" section override the store section or do both have to be specified.


https://gerrit.asterisk.org/c/asterisk/+/15822/3/configs/samples/stir_shaken.conf.sample@51 
PS3, Line 51: ; URL to the public certificate. Must be of type X509. This will be put in the identity header
            : ; when signing.
            : ;public_key_url=http://mycompany.com/alice.pem
If this overrides the public_key_url in the certificates section then that should be stated.  It should also be stated that this has to be the same cert as the one specified in "path".

Same comment about the name as above.


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

https://gerrit.asterisk.org/c/asterisk/+/15822/3/res/res_stir_shaken.c@618 
PS3, Line 618: 	if (public_key_is_expired(public_key_url)) {
How can a newly downloaded key be expired?


https://gerrit.asterisk.org/c/asterisk/+/15822/3/res/res_stir_shaken.c@677 
PS3, Line 677: 	if (ast_asprintf(&dir_path, "%s/keys/%s", ast_config_AST_DATA_DIR, STIR_SHAKEN_DIR_NAME) < 0) {
Shouldn't dir_path be taken from the "path" parameter in the "certificates" store config?
See my comment in the sample config file though.



-- 
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: 3
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: Fri, 30 Apr 2021 14:05:58 +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/20210430/669c4dde/attachment-0001.html>