A few comments:
- Add a note about the need to load TLS module first (like I just did in http_client docs) - modparam examples still have “async_http” in the docs - http_set_ssl_cert should have “tls” instead of “ssl”. SSL is bad.
- The name of the statistics/counters is not complete in the doc file. Please check
- I still think we need to discuss the way you have to run functions to set parameters for a coming function call. That’s new to Kamailio, a big change in how we do things. Usually that is handled by AVPs or XAVPs.
We should not add new ways of doing things without discussion - it’s about architecture and will affect all users and how we train people in Kamailio. Personally I don’t like running a “function” that sets options for the next function call to a particular function… What if that function doesn’t happen, will all these values be reset for the next message processed in that process?
We’re moving forward!
/O
Hi Olle,
thanks for your feedback. We're going to fix the documentation issues you spotted.
- Add a note about the need to load TLS module first (like I just did
in http_client docs)
The openssl library initialization is done by the tls module before any other mod_init(), so as far as I understand it, the load order does not matter.
To me, the real concern is that the openssl library will be initialized several times if several modules or libraries want to use openssl in the same process space (this is the case for example when using the TLS module and another module using libcurl).
- http_set_ssl_cert should have “tls” instead of “ssl”. SSL is bad.
We spoke about this one, "ssl" reflects the names int the curl API. If we totally disable SSL though, it makes sense to use "tls".
- I still think we need to discuss the way you have to run functions
to set parameters for a coming function call. (...) What if that function doesn’t happen, will all these values be reset for the next message processed in that process?
Good point, that is not addressed yet: we have to figure a way to reset all the values previously set when a new message is processed. Currently, they are reset when the HTTP query is sent.
On 04 Feb 2016, at 12:23, Camille Oudot camille.oudot@orange.com wrote:
Hi Olle,
thanks for your feedback. We're going to fix the documentation issues you spotted.
- Add a note about the need to load TLS module first (like I just did
in http_client docs)
The openssl library initialization is done by the tls module before any other mod_init(), so as far as I understand it, the load order does not matter.
To me, the real concern is that the openssl library will be initialized several times if several modules or libraries want to use openssl in the same process space (this is the case for example when using the TLS module and another module using libcurl).
It was Daniel’s recommendation to load the TLS module first since he had fixed some generic OpenSSL magic there.
- http_set_ssl_cert should have “tls” instead of “ssl”. SSL is bad.
We spoke about this one, "ssl" reflects the names int the curl API. If we totally disable SSL though, it makes sense to use "tls”.
Exactly.
- I still think we need to discuss the way you have to run functions
to set parameters for a coming function call. (...) What if that function doesn’t happen, will all these values be reset for the next message processed in that process?
Good point, that is not addressed yet: we have to figure a way to reset all the values previously set when a new message is processed. Currently, they are reset when the HTTP query is sent.
And I think you need to rethink the way you handle this. We are quite open for new ideas, but adding a totally new scheme for setting parameters for function calls is not something we need - it will just be confusing for people who learn Kamailio. Please try to use an existing framework.
Sorry for being really, really, really stubborn of this, but I’ve spent over 10 years doing Kamailio trainings… :-)
/O
Hi Olle,
On Thu, Feb 4, 2016 at 3:30 PM, Olle E. Johansson oej@edvina.net wrote:
On 04 Feb 2016, at 12:23, Camille Oudot camille.oudot@orange.com
wrote:
Hi Olle,
It was Daniel’s recommendation to load the TLS module first since he had fixed some generic OpenSSL magic there.
Will add the recommendation to the doc.
- http_set_ssl_cert should have “tls” instead of “ssl”. SSL is bad.
We spoke about this one, "ssl" reflects the names int the curl API. If we totally disable SSL though, it makes sense to use "tls”.
Exactly.
So does this mean that we are going to enforce the usage of TLS? Today it's not the case anywhere in Kamailio and, sincerely, having a parameter with "tls" in its name and then allowing the user to use ssl is misleading. I would prefer: 1) not having any mention to ssl/tls in the name (like "http_set_cert") 2) raise a warning when the user tries to use a deprecated protocol version
- I still think we need to discuss the way you have to run functions
to set parameters for a coming function call. (...) What if that function doesn’t happen, will all these values be reset for the next message processed in that process?
Good point, that is not addressed yet: we have to figure a way to reset all the values previously set when a new message is processed. Currently, they are reset when the HTTP query is sent.
And I think you need to rethink the way you handle this. We are quite open for new ideas, but adding a totally new scheme for setting parameters for function calls is not something we need - it will just be confusing for people who learn Kamailio. Please try to use an existing framework.
We will rethink this part.
Sorry for being really, really, really stubborn of this, but I’ve spent over 10 years doing Kamailio trainings… :-)
/O
No problem while the discussion is constructive, as it is in this case :)
Cheers,
Federico
On 05 Feb 2016, at 12:49, Federico Cabiddu federico.cabiddu@gmail.com wrote:
Hi Olle,
On Thu, Feb 4, 2016 at 3:30 PM, Olle E. Johansson <oej@edvina.net mailto:oej@edvina.net> wrote:
On 04 Feb 2016, at 12:23, Camille Oudot <camille.oudot@orange.com mailto:camille.oudot@orange.com> wrote:
Hi Olle,
It was Daniel’s recommendation to load the TLS module first since he had fixed some generic OpenSSL magic there.
Will add the recommendation to the doc.
- http_set_ssl_cert should have “tls” instead of “ssl”. SSL is bad.
We spoke about this one, "ssl" reflects the names int the curl API. If we totally disable SSL though, it makes sense to use "tls”.
Exactly.
So does this mean that we are going to enforce the usage of TLS? Today it's not the case anywhere in Kamailio and, sincerely, having a parameter with "tls" in its name and then allowing the user to use ssl is misleading. I would prefer:
- not having any mention to ssl/tls in the name (like "http_set_cert")
- raise a warning when the user tries to use a deprecated protocol version
I do think we’ve disabled SSL in Kamailio, if not it’s just for backwards compatibility with really old phones. TLS won’t be disabled soon. If we are selecting terminology between “SSL” and “TLS” - “TLS” is the one we should use.
And yes, if someone configures SSL I think it’s a good idea to issue a warning.
- I still think we need to discuss the way you have to run functions
to set parameters for a coming function call. (...) What if that function doesn’t happen, will all these values be reset for the next message processed in that process?
Good point, that is not addressed yet: we have to figure a way to reset all the values previously set when a new message is processed. Currently, they are reset when the HTTP query is sent.
And I think you need to rethink the way you handle this. We are quite open for new ideas, but adding a totally new scheme for setting parameters for function calls is not something we need - it will just be confusing for people who learn Kamailio. Please try to use an existing framework.
We will rethink this part.
THanks! If you want to discuss various ways to handle this we can have an IRC chat some time.
Sorry for being really, really, really stubborn of this, but I’ve spent over 10 years doing Kamailio trainings… :-)
/O
No problem while the discussion is constructive, as it is in this case :)
:-)
/O
-
Camille Oudot -
Federico Cabiddu -
Olle E. Johansson