[asterisk-dev] Problems with the ASTERISK-27206 patch.

Wed Jan 3 07:40:05 CST 2018

On Wed, Jan 3, 2018 at 4:08 AM, Joshua Colp <jcolp at digium.com> wrote:

> On Wed, Jan 3, 2018, at 12:03 AM, Richard Mudgett wrote:
> > On Tue, Jan 2, 2018 at 5:41 PM, Joshua Colp <jcolp at digium.com> 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.
>
> I disagree that those are a regression. Those are certainly valid findings
> and comments, but I don't see a specific regression based on what you've
> said. If you are trying to state that the regression is "we now require ip
> to be listed in identify_by in order to match using the IP endpoint
> identifier" then that was the purpose of the patch itself and is not a
> regression. I also don't think that the option to set it to an empty string
> would have been user friendly, that's more of a side effect of how it was
> coded. Having a specific option for IP only based matching is explicit.
>
> Having a non-explicit option like you mentioned previously for identify_by
> becomes a free for all on the other endpoint identifiers. If we encounter a
> scenario where two "other" endpoint identifiers could match but only want
> to match one then boom - we're back to the same scenario. You've just
> enforced that two endpoint identifiers that are configurable in some way
> can never match the same request even if for some reason they did if they
> fall under the "other" option. If the answer is "don't configure things
> them that way" then perhaps it isn't possible, we don't know. As well even
> with your "none" or "other" you'd still need to list it in the identify_by
> list, so I don't see how that's different than requiring "ip" to be there
> except being less specific.
>
> Approaching this from an end-user perspective who knows NOTHING of this
> stuff having "ip" in the identify_by also makes logical sense. It's
> explicit, you can understand what it means and you can understand what it
> does. The use of "other" is vague and requires more thought and
> investigation. The use of "none" is just downright confusing at first
> glance unless you dive deeper into things.
>

In the end, this is what it comes down to for me.  The end-user perspective
is the most important perspective.

>
> >
> > >
> > > >
> > > > 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 original definition and actually implementing that behavior
> > correctly is the huge scope creep we wish to avoid at this time.
> Partially
> > implementing it as the ASTERISK-27206 patch attempts will require changes
> > to res_pjsip to add a new widgit identification method to identify_by.
> In
> > fact https://issues.asterisk.org/jira/browse/ASTERISK-27491 would need
> to
> > do just that if the original definition is "restored" by the
> ASTERISK-27206
> > patch.  That was something I know we tried to avoid in the beginning.  We
> > were attempting to add or extend functionality by just loading modules.
>
> It is only through actual usage that we find where simply loading a module
> to extend functionality works and where things need to further adapt.
>
> >
> >
> > >
> > > 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.
> > >
> >
> > Agree or not, that is how it was coded when v12 was originally
> released.  I
> > am not expanding the scope.  I am preserving how the option actually
> > behaves; not redefining it.
> >
> > What global identify_by option?  I did not mention such an option.  The
> > identify_by option is per endpoint as it is part of the endpoint
> definition
> > config section.
>
> I meant an identify_by option which covers multiple endpoint identifiers,
> like your "none" or "other".
>
> >
> >
> > >
> > > 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.
> > >
> >
> > The ASTERISK-27206 patch incompletely attempts to restore the originally
> > intended option behavior and actually is not needed to fix the issue.
>
> --
> 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
>

-- 
George Joseph
Digium, Inc. | Software Developer
445 Jan Davis Drive NW - Huntsville, AL 35806 - US
Check us out at: www.digium.com & www.asterisk.org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20180103/f05d4919/attachment-0001.html>