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

Wed Jan 3 13:53:42 CST 2018

On Wed, Jan 3, 2018 at 5: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.
>

I accept that defining the identify_by option to list the methods the
endpoint should be identified by is easy to document and conceptually
simple.  To comply with that definition, for now, we can just add the "ip"
value, the "header" value, and any other widget name down the road as
needed.  Later we need to re-implement it to support dynamically added
widget names.  I do not accept the unnecessary requirement that the
type=identify method must be listed in the identify_by value when the user
must explicitly specify the endpoint name for the type=identify method to
even work.  The "ip" value in identify_by should just be documentation in
this case.  Adding a needless source of configuration error by requiring
the "ip" value to be listed does not help the user experience at all.  That
requirement is the regression.

The ASTERISK-27206 reporter's problem was to somehow stop the username
identification method from recognizing the endpoint.  Setting the
identify_by value to something that does not include "username" is all that
is necessary.  Adding the requirement that the type=identify method be
listed in the identify_by value to accept a match from the type=identify
method goes too far.  It goes into the negative user experience realm.

>
>
> 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.
>

The proposed "other" values are not a free for all and we are not back to
the same scenario.  It is already known which identifiers do and do not
need listing in the identify_by value.

* Adding a new identifier like "my_username" that does not have
configuration to explicitly identify a specific endpoint has to be listed
in the identify_by value.  Otherwise, there is no way to restrict which
endpoints it identifies.  i.e., Which endpoint the "my_username" identifier
matches is vague.

* Adding a new identifier like "my_ip" that does have configuration to
explicitly identify a specific endpoint does not have to be listed in the
identify_by value.  Listing "my_ip" in the identify_by value would just be
for documentation purposes.  i.e., Which endpoint the "my_ip" identifier
matches is specific.  It can match one and only one endpoint.

The "", "none", or "other" proposed value is a place holder which means not
"username", "auth_username", or any other "my_username" type identifier.
The proposed value is only needed if you explicitly did not want to
identify the endpoint by "username", "auth_username", or any other
"my_username" type identifier.  i.e., The "", "none", or "other" value
would be the only thing in the list.  The "" value actually makes that
point more explicit.

We currently can have many type=identifier identifiers match endpoint foo.
There is no restriction on that.  However, doing so does point out a
limitation in
res/res_pjsip_endpoint_identifier.c:format_ami_endpoint_identify() which
only handles the first type=identifier identifying endpoint foo.

>
> 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.
>

If you have preconceived notions of what the option means and the
documentation does not disagree then yes it is easier to understand.
However, until it supports dynamic widget names, that identify_by
definition requires a third party creating their own identifier module to
also modify Asterisk modules.

>
> >
> > >
> > > >
> > > > 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.
>

Richard
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20180103/07d91bf7/attachment-0001.html>