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

[Benjamin Keith Ford]

Benjamin Keith Ford 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:

(6 comments)

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@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?
This functionality isn't yet implemented (we've got an issue for it), but these will be for our own storage, not the ones coming in since we don't have any control over that.


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". […]
Can do.


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.
Stores are for bulk loading certificates. Certificates are just individually configured certs. Technically, a certificate object could point to a certificate in a directory that store points to. When stores get implemented, we'll need to decide how we want to handle loading certs and stores together.


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. […]
This is different from path. Path is the location of the private cert that we use to sign on outbound. The public_key_url is what we put in the Identity header to let the remote end know where to download the public certificate.


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)) {
> I expect that newly-downloaded and expired are two different things. […]
Yeah, Richard is right here. There's a chance that it could be expired on the server itself. Nothing to do with us in that case, but a valid scenario.


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? […]
This is just where we store the public keys we download - it doesn't have anything to do with the config file. A note in stir_shaken.conf on this and where the certificates will be stored would be a good idea.



-- 
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-CC: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Fri, 30 Apr 2021 20:39:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: George Joseph <gjoseph at digium.com>
Comment-In-Reply-To: Richard Mudgett <rmudgett at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210430/7e76dd4d/attachment-0001.html>