[asterisk-dev] Problems with the ASTERISK-27206 patch.
The patch for https://issues.asterisk.org/jira/browse/ASTERISK-27206 which is committed in 7385d1e017e562afe64431606e857e704f86a16d causes a configuration regression by requiring the endpoint and identifier method to agree to match the endpoint. Doing so is redundant for methods that explicitly specify which endpoints they match. The redundancy allows configuration errors that cannot be caught when the configuration is loaded. Relevant endpoint 101 configuration values: [global] ; The order by which endpoint identifier methods are given priority. ; The endpoint_identifier_order default is ;endpoint_identifier_order=ip,username,anonymous [101] type=endpoint ; The identify_by default is ;identify_by=username [identify_me] type=identify endpoint=101 ; Match by some IP address match=127.0.0.1/24 Before the patch, the 101 endpoint above would identify by either the username endpoint identifier method or the res_pjsip_endpoint_identifier_ip endpoint identifier method's [identify_me] section. After the patch, the 101 endpoint is still matched by either method. However, the identify_by default had to change to "username,ip" and the option meaning had to change slightly do it. I think this is wrong. If you went to the trouble to create an [identify_me] section which must explicitly specify the endpoint it matches then the endpoint should not need to also specify in the identify_by value that it is identified by the res_pjsip_endpoint_identifier_ip method. Doing so is redundant and will cause unnecessary configuration errors. The goal of ASTERISK-27206 is to prevent the endpoint from being identified by the username identification method so it could only be identified by the res_pjsip_endpoint_identifier_ip method. The key difference between the two methods is the username identification method has no other configuration than the endpoint's identify_by value available to specify to which endpoint the method applies. I think the previous and current identify_by documentation is a bit misleading in any case. The identify_by option historically specified which identification methods that have no other configuration requirements can match the endpoint. i.e., The username and auth_username identification methods. To satisfy ASTERISK-27206, what is needed for the identify_by option is a value to prevent methods that have no other configuration requirements from matching the endpoint. Proposed candidates for the new identify_by value are: "", "none", and "other". The "" or "none" value to indicate that none of the methods without configuration will match. The "other" value to indicate that the endpoint is matched by only other matching mechanisms with their own configuration. Looking at the some some more, I think the empty value "" is already allowable in the pjsip.conf file so the entire patch for ASTERISK-27206 would not be needed. You could have configured identify_by to an empty value and it would have satisfied the issue. However, I think database backends would have an issue with an empty value since the column is declared as an enum while the code works with it as a string of comma separated enum values. Fortunately, the first ASTERISK-27206 patch hasn't come out in a release yet. However, it is currently in the 13.19.0-rc1 version. As a result, we may have to keep the "ip" value that the first ASTERISK-27206 patch added to indicate_by as an alias and fix the database column definition. Richard -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] Problems with the ASTERISK-27206 patch.
On Tue, Jan 2, 2018, at 7:14 PM, Richard Mudgett wrote: > The patch for https://issues.asterisk.org/jira/browse/ASTERISK-27206 which > is committed in 7385d1e017e562afe64431606e857e704f86a16d causes a > configuration regression by requiring the endpoint and identifier method > to agree to match the endpoint. Doing so is redundant for methods that > explicitly specify which endpoints they match. The redundancy allows > configuration errors that cannot be caught when the configuration is > loaded. Can you clarify what the precise regression you are referring to is? Even after reading this email I'm still unclear. > > Relevant endpoint 101 configuration values: > > [global] > ; The order by which endpoint identifier methods are given priority. > ; The endpoint_identifier_order default is > ;endpoint_identifier_order=ip,username,anonymous > > [101] > type=endpoint > ; The identify_by default is > ;identify_by=username > > [identify_me] > type=identify > endpoint=101 > ; Match by some IP address > match=127.0.0.1/24 > > Before the patch, the 101 endpoint above would identify by either the > username endpoint identifier method or the > res_pjsip_endpoint_identifier_ip endpoint identifier method's > [identify_me] section. > > After the patch, the 101 endpoint is still matched by either method. > However, the identify_by default had to change to "username,ip" and the > option meaning had to change slightly do it. I think this is wrong. If > you went to the trouble to create an [identify_me] section which must > explicitly specify the endpoint it matches then the endpoint should not > need to also specify in the identify_by value that it is identified by the > res_pjsip_endpoint_identifier_ip method. Doing so is redundant and will > cause unnecessary configuration errors. I think removing the redundant requirement is a nice improvement for the future, but that comes at a cost to do it in a way that is user friendly and automatic. > The goal of ASTERISK-27206 is to prevent the endpoint from being > identified by the username identification method so it could only be > identified by the res_pjsip_endpoint_identifier_ip method. The key > difference between the two methods is the username identification method > has no other configuration than the endpoint's identify_by value available > to specify to which endpoint the method applies. > > I think the previous and current identify_by documentation is a bit > misleading in any case. The identify_by option historically specified > which identification methods that have no other configuration requirements > can match the endpoint. i.e., The username and auth_username > identification methods. To satisfy ASTERISK-27206, what is needed for the > identify_by option is a value to prevent methods that have no other > configuration requirements from matching the endpoint. The identify_by option was originally an option which listed the endpoint identifiers that an endpoint could be identified by. That was its original goal. It later evolved slightly with the addition of auth_username to also influence how. The addition of "ip" goes back to its original goal. Over all the option has muddied meaning. I disagree that whether an endpoint identifier is configurable or not has a bearing on its naming or meaning in identify_by. The configuration may imply that it should only be matched using that identifier, but that is not currently something that is done and is of a greater scope. I think having a global identify_by option which could actually mean multiple endpoint identifiers by itself is also confusing and hard to define the behavior of. Really what this just illustrates is that the option itself has evolved and hasn't been clearly defined with a strict purpose. I don't think now is the right time to solve that particular problem. If there is a defined use case which is currently broken by the change though, that is something we need to fix. -- Joshua Colp Digium, Inc. | Senior Software Developer 445 Jan Davis Drive NW - Huntsville, AL 35806 - US Check us out at: www.digium.com & www.asterisk.org -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] Problems with the ASTERISK-27206 patch.
On Tue, Jan 2, 2018 at 5:41 PM, Joshua Colp wrote: > On Tue, Jan 2, 2018, at 7:14 PM, Richard Mudgett wrote: > > The patch for https://issues.asterisk.org/jira/browse/ASTERISK-27206 > which > > is committed in 7385d1e017e562afe64431606e857e704f86a16d causes a > > configuration regression by requiring the endpoint and identifier method > > to agree to match the endpoint. Doing so is redundant for methods that > > explicitly specify which endpoints they match. The redundancy allows > > configuration errors that cannot be caught when the configuration is > > loaded. > > Can you clarify what the precise regression you are referring to is? Even > after reading this email I'm still unclear. > The regression is the new requirement for the endpoint identify_by option to list ip in order for the type=identify method to be accepted by the endpoint. This new requirement is unnecessary as the type=identify section must specify an endpoint name to know which endpoint it recognizes. More specifically, the change to res/res_pjsip_endpoint_identifier_ip.c:ip_identify() enforces this new requirement. I should have used the term complexity instead of redundancy. The new requirement adds configuration complexity because the endpoint identify_by option and the type=identify method must agree to match the endpoint. The added complexity doesn't add any value, can needlessly break existing installations regardless of the note in CHANGES saying it will, makes match by ip configuration more error prone, and the error is not recognizable on configuration load. In addition, as I pointed out, I think the entire ASTERISK-27206 patch was unnecessary. Before the patch, setting the identify_by option to an empty string would disable the username identification method from matching the endpoint. > > > > > Relevant endpoint 101 configuration values: > > > > [global] > > ; The order by which endpoint identifier methods are given priority. > > ; The endpoint_identifier_order default is > > ;endpoint_identifier_order=ip,username,anonymous > > > > [101] > > type=endpoint > > ; The identify_by default is > > ;identify_by=username > > > > [identify_me] > > type=identify > > endpoint=101 > > ; Match by some IP address > > match=127.0.0.1/24 > > > > Before the patch, the 101 endpoint above would identify by either the > > username endpoint identifier method or the > > res_pjsip_endpoint_identifier_ip endpoint identifier method's > > [identify_me] section. > > > > After the patch, the 101 endpoint is still matched by either method. > > However, the identify_by default had to change to "username,ip" and the > > option meaning had to change slightly do it. I think this is wrong. If > > you went to the trouble to create an [identify_me] section which must > > explicitly specify the endpoint it matches then the endpoint should not > > need to also specify in the identify_by value that it is identified by > the > > res_pjsip_endpoint_identifier_ip method. Doing so is redundant and will > > cause unnecessary configuration errors. > > I think removing the redundant requirement is a nice improvement for the > future, but that comes at a cost to do it in a way that is user friendly > and automatic. > > > The goal of ASTERISK-27206 is to prevent the endpoint from being > > identified by the username identification method so it could only be > > identified by the res_pjsip_endpoint_identifier_ip method. The key > > difference between the two methods is the username identification method > > has no other configuration than the endpoint's identify_by value > available > > to specify to which endpoint the method applies. > > > > I think the previous and current identify_by documentation is a bit > > misleading in any case. The identify_by option historically specified > > which identification methods that have no other configuration > requirements > > can match the endpoint. i.e., The username and auth_username > > identification methods. To satisfy ASTERISK-27206, what is needed for > the > > identify_by option is a value to prevent methods that have no other > > configuration requirements from matching the endpoint. > > The identify_by option was originally an option which listed the endpoint > identifiers that an endpoint could be identified by. That was its original > goal. It later evolved slightly with the addition of auth_username to also > influence how. The addition of "ip" goes back to its original goal. Over > all the option has muddied meaning. > Listing how an endpoint is to be identified may have been the identify_by option's original goal. That is not how it was coded to behave by the original v12.0.0 release. See https://issues.asterisk.org/jira/browse/ASTERISK-22135 (895c8e0d2c97cd04299f3f179e99d8a3873c06c6) which removed the identify_by "location" value in 2013 apparently because it wasn't used anywhere. The ASTERISK-22135 patch "redefined" identify_by to how I've described it historically behaves above. Restoring the
