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

[Ashley Sanders]

Ashley Sanders has posted comments on this change.

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


Patch Set 4: Code-Review-1

(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:

When a client connects to a server via SSL/TLS, the server most commonly utilizes
an RSA key-pair. However, other such algorithms exist (i.e. DSA and ECDSA), and
if the server socket is configured with a certificate for either one of those, it
would lose its compatibility with RSA-only clients. 

Now, the server socket can be configured with up to one RSA, ECDSA and DSA key
each. For example, if a client is not compatible with SHA-2 hashed certificates
like Nokia mobile phones, the server socket still can use RSA/SHA-1 for legacy
clients and ECDSA/SHA-2 for everyone else.


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.

Also, the term "looked-up" sounds very awkward. I suggest changing this to "fetched" (or "retrieved" or "located", etc.).


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:

The term "looked-up" sounds very awkward. I suggest changing this to "fetched" (or "retrieved" or "located", etc.).


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);
        : 		}
> This could be coded as:
I agree with Richard. It is best to avoid code duplication whenever possible. See:
- http://en.wikipedia.org/wiki/Abstraction_principle_%28computer_programming%29
- http://en.wikipedia.org/wiki/Don%27t_repeat_yourself


-- 
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: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes