[Asterisk-code-review] tcptls: Use new certificate upon sip reload (asterisk[13])

[Corey Farrell]

Corey Farrell has posted comments on this change. ( https://gerrit.asterisk.org/4448 )

Change subject: tcptls: Use new certificate upon sip reload
......................................................................


Patch Set 3:

(5 comments)

https://gerrit.asterisk.org/#/c/4448/3/channels/chan_sip.c
File channels/chan_sip.c:

PS3, Line 35411: 	ast_free(sip_tls_desc.old_tls_cfg->certfile);
               : 	ast_free(sip_tls_desc.old_tls_cfg->pvtfile);
               : 	ast_free(sip_tls_desc.old_tls_cfg->cipher);
               : 	ast_free(sip_tls_desc.old_tls_cfg->cafile);
               : 	ast_free(sip_tls_desc.old_tls_cfg->capath);
               : 	ast_free(sip_tls_desc.old_tls_cfg);
               : 	sip_tls_desc.old_tls_cfg = NULL;
old_tls_cfg is allocated by ast_tcptls_server_start, so I feel that they should be freed by ast_tcptls_server_stop.

Code outside tcptls.c should not have to worry about old_tls_cfg.


https://gerrit.asterisk.org/#/c/4448/3/include/asterisk/tcptls.h
File include/asterisk/tcptls.h:

Line 148: 	struct ast_tls_config *old_tls_cfg; /*!< copy of the SSL configuration to determine whether changes have been made */
We can't add a field to the middle of a public structure, it breaks the ABI.


https://gerrit.asterisk.org/#/c/4448/3/main/tcptls.c
File main/tcptls.c:

Line 1044: 	if (desc->tls_cfg)
Please take a look at the Asterisk Coding Guidelines, especially for the if statements.  Brackets {} are required for all if statements, and the opening bracket does not go on a new line.

https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines


Line 1079: 		else if (strcmp(desc->tls_cfg->certfile, desc->old_tls_cfg->certfile) || strncmp(desc->tls_cfg->certhash, desc->old_tls_cfg->certhash, 41))
I think we only need to compare the sha1 hash.  If I point the config to a different filename with the same SHA1 hash as the old filename, isn't that still the same file?

At that point we don't need to restart TLS because the content of the "new" certificate is the same as the old.  Same applies to cafile and capath.


Line 1089: 		else if (strncmp((void*) &desc->tls_cfg->flags, (void*) &desc->old_tls_cfg->flags, sizeof(struct ast_flags)))
strncmp won't work for flags, the comparison would stop at any zero valued byte.  Need to use memcmp instead.


-- 
To view, visit https://gerrit.asterisk.org/4448
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I169e86cefc6dcd627c915134015a6a1ab1aadbe6
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Michael Kuron <m.kuron at gmx.de>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Michael Kuron <m.kuron at gmx.de>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes