[Asterisk-code-review] tcptls: Enable multiple TLS certificate chains (RSA+ECC+DSA)... (asterisk[master])

[Alexander Traud]

Alexander Traud has posted comments on this change.

Change subject: tcptls: Enable multiple TLS certificate chains (RSA+ECC+DSA) for server socket.
......................................................................


Patch Set 4:

(4 comments)

https://gerrit.asterisk.org/#/c/431/4//COMMIT_MSG
Commit Message:

Line 9: When a client connects via SSL/TLS, the server uses a RSA key-pair usually.
> For better clarity, I suggest modifying this slightly:
Done


https://gerrit.asterisk.org/#/c/431/4/configs/samples/pjsip.conf.sample
File configs/samples/pjsip.conf.sample:

Line 768:                 ; a .key file must be specified via priv_key_file. Since
> For readability, the "Since PJProject 2.5 ..." should start on line 769.
Done


https://gerrit.asterisk.org/#/c/431/4/configs/samples/sip.conf.sample
File configs/samples/sip.conf.sample:

Line 566:                                         ; "asterisk_dsa.pem" and/or "asterisk_ecc.pem" are looked-up
> Just like in the pjsip.conf.sample file:
For me, "fetched" sounds like "loaded remotely". What about "loaded"? This verb is present in pjsip.conf two times already, with the same meaning. Alternatively, we could go for "to look for" which is used in sip.conf in the context of certificates already. Or for consistency, I could go for "loaded" in pjsip.conf and "to look for" in sip.conf. Please, suggest.


https://gerrit.asterisk.org/#/c/431/4/main/tcptls.c
File main/tcptls.c:

Line 761: 		if (SSL_CTX_use_certificate_chain_file(cfg->ssl_ctx, cert_file) == 0) {
        : 			ast_log(LOG_ERROR, "TLS/SSL error loading %s cert file. <%s>\n", key_type, cert_file);
        : 		} else if (SSL_CTX_use_PrivateKey_file(cfg->ssl_ctx, cert_file, SSL_FILETYPE_PEM) == 0) {
        : 			ast_log(LOG_ERROR, "TLS/SSL error loading %s cert file. <%s>\n", key_type, cert_file);
        : 		} else if (SSL_CTX_check_private_key(cfg->ssl_ctx) == 0) {
        : 			ast_log(LOG_ERROR, "TLS/SSL error loading %s cert file. <%s>\n", key_type, cert_file);
        : 		}
> I agree with Richard. It is best to avoid code duplication whenever possibl
In Asterisk, a log message is preceded by the line of code. This way, a user is able to determine which method call failed. This might help to debug the error more easily.

However, if you are asking for a dedicated error message for each case, I welcome suggestions. Alternatively, I could append "(step 1)", "(step 2)", and "(step 3)" – because sometimes users do not look-up the same revision of the code, not matching their binary/executable, and therefore mentioned line in the log message does not match the seen line of code.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iada5e00d326db5ef86e0af7069b4dfa1b979da9a
Gerrit-PatchSet: 4
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes