<p style="white-space: pre-wrap; word-wrap: break-word;">Sorry but I only just now got the time to download the patch and look at the whole thing.</p><p>Patch set 3:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/15822">View Change</a></p><p>7 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/15822/3/configs/samples/stir_shaken.conf.sample">File configs/samples/stir_shaken.conf.sample:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/15822/3/configs/samples/stir_shaken.conf.sample@4">Patch Set #3, Line 4:</a> <code style="font-family:monospace,monospace">;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">A link to the wiki page would also be good.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/15822/3/configs/samples/stir_shaken.conf.sample@25">Patch Set #3, Line 25:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">; A certificate store is used to examine, and load all certificates found in a<br>; given directory. When using this type the public key URL is generated based<br>; upon the filename, and variable substitution.<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Is the cert store used for both our own certs and those coming in?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/15822/3/configs/samples/stir_shaken.conf.sample@38">Patch Set #3, Line 38:</a> <code style="font-family:monospace,monospace">;public_key_url=http://mycompany.com/${CERTIFICATE}.pem</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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...<br>http://mycompany.com/${CERTIFICATE}/cert.pem</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/15822/3/configs/samples/stir_shaken.conf.sample@42">Patch Set #3, Line 42:</a> <code style="font-family:monospace,monospace">; Individual certificates are declared by using the certificate type.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">So does a "certificate" section override the store section or do both have to be specified.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/15822/3/configs/samples/stir_shaken.conf.sample@51">Patch Set #3, Line 51:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">; URL to the public certificate. Must be of type X509. This will be put in the identity header<br>; when signing.<br>;public_key_url=http://mycompany.com/alice.pem<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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".</p><p style="white-space: pre-wrap; word-wrap: break-word;">Same comment about the name as above.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/c/asterisk/+/15822/3/res/res_stir_shaken.c">File res/res_stir_shaken.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/15822/3/res/res_stir_shaken.c@618">Patch Set #3, Line 618:</a> <code style="font-family:monospace,monospace">      if (public_key_is_expired(public_key_url)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">How can a newly downloaded key be expired?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/15822/3/res/res_stir_shaken.c@677">Patch Set #3, Line 677:</a> <code style="font-family:monospace,monospace">     if (ast_asprintf(&dir_path, "%s/keys/%s", ast_config_AST_DATA_DIR, STIR_SHAKEN_DIR_NAME) < 0) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Shouldn't dir_path be taken from the "path" parameter in the "certificates" store config?<br>See my comment in the sample config file though.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/15822">change 15822</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/15822"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 16 </div>
<div style="display:none"> Gerrit-Change-Id: Ia00b20835f5f976e3603797f2f2fb19672d8114d </div>
<div style="display:none"> Gerrit-Change-Number: 15822 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 30 Apr 2021 14:05:58 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>