> On Monday, June 01 2020, Bryce Harrington wrote:
> > Review: Needs Fixing
> I ran update-maintainer on the focal branch.  I'll force-push the update
> soon.
>
> > Just a minor nitpick, but I noticed a typo in a comment in the patch itself:
> >
> >   ...actual configue file...
> 
> As Anders mentioned, this typo is present in the upstream patch.  I
> don't think it's worth creating a divergence here because of it, but
> please let me know if you think otherwise.

As I said, it's a minor nitpick, not something requiring a fix.  I mention it mainly because basic errors can indicate the change was inadequately peer reviewed on submission.

> > In a fresh lxc container I ran the commands verbatim as given in the SRU bug
> report, on both focal and eoan, and got the error message:
> >
> > root@triage-focal:/home/bryce# systemctl restart memcached
> > root@triage-focal:/home/bryce# memcping --servers=127.0.0.1 --binary
> --username=foo --password=bar
> > Failed to ping 127.0.0.1:11211 UNKNOWN READ FAILURE
> >
> > I experimented with some different username/password combos, and tried
> > the config file in a few different places but didn't get a different
> > result.  Could you verify that the test case works for you in lxc?
> > Maybe the setup directions are missing a step?
> 
> Anders was kind to update the testing instructions to include the -S
> flag in the test instructions.  I was able to reproduce the bug and the
> fix here using a focal VM.

I also verified the omission of the -S in the test case was indeed the error causing the failure.  Don't forget you should to make the test case cover both the bugged case and the fixed case, and show how to verify both (e.g. checking exit codes, not just lack of output).

> > I wonder if the patch and test case may deserve to be expanded on a
> > bit.  As I understand it, normally the config file should be stored in
> > a file named /etc/sasl2/memcached.conf, but the problem is that due to
> > a bug /etc/sasl2/memcached.conf/ may have inadvertently been created
> > as a subdirectory, and memcached.conf placed inside of that.  So the
> > purpose of the patch is to make memcached check the incorrect location
> > just in case.  Is that understanding correct?
> 
> As was already explained, the /etc/sasl2/memcached.conf/ directory, if
> present, will have been manually created by the user in order to
> overcome the original bug, which is that memcached was looking for the
> configuration file in the wrong directory (the one mentioned above).
> 
> As an effort not to break existing workarounds, the patch accepts the
> erroneous directory as a valid entry if it exists.

Yes, that's the level of explanation that should be present in the patch description.  You could probably also improve the bug report Impact section with some of this text too.

> > Assuming it is, I would suggest saying basically this in the patch
> > description.  It might be worth also explaining in the debian
> > changelog entry, at least mentioning the bad path by name and that
> > we're working around it.  Just saying "Fix the path" is omitting some
> > pertinent details. ;-)
> 
> I can expand both the patch description and the changelog entry, sure.

Thanks, I'll leave this as Fix needed for now, let me know when this is updated.

> cyrrus-sasl (which is what we use in Ubuntu) works
> with the concept of paths where configuration files live, so that's what
> it expects.  In other words, you shouldn't provide the full path for a
> configuration file, but rather provide the basename of it (i.e., without
> the filename present).
> 
> Given that in a "normal" world the memcached.conf file would live inside
> either /etc/sasl{,2}, these are the paths that memcached would inform
> SASL about.  However, since this bug was introduced, memcached has been
> erroneously providing "/etc/sasl2/memcached.conf" as the path, which is
> interpreted as a directory name (not a file name) by SASL, thus causing
> the issue we're dealing with.
> 
> I explain more about the reason for this bug below.
> 
> > For the test case in the SRU bug, I notice it is testing the case
> > where the config file is at /etc/sasl2/memcached.conf, but I wonder if
> > it should also include steps to test that config stored at the
> > alternate locations also work?
> 
> Anders also expanded the testing section to include information about
> the different paths.  I'll double check and add extra information if
> needed.

That does provide the scope of testing needed here, although it would be useful to express it as the specific commands to run or edits to make.  I like how you crafted the test case for lp #1875109 for example - you can see there the level of importance that the SRU team puts on the test case.

> > In looking at the patch itself, I notice that the fix only applies
> > when built with HAVE_SASL_CB_GETCONFPATH defined, and not when
> > HAVE_SASL_CB_GETCONF is defined.  Guessing that which one is defined
> > depends on what sasl library was built against...  can you explain in
> > the patch description where these are defined?  (Perhaps the reason
> > the test cases weren't working for me was because whatever defines
> > HAVE_SASL_CB_GETCONFPATH isn't being installed?)
> 
> Sure.
> 
> The HAVE_* values are defined during memcached's configure process, and
> the SASL_CB_GETCONF* values are define in <sasl/sasl.h>.
> 
> The cyrrus-sasl has only defined SASL_CB_GETCONFPATH historically.  As
> far as I have seen, SASL_CB_GETCONF was/is defined by Sun's version of
> sasl, which we don't use.
> 
> > The bug report indicates that the problem was introduced upstream with this
> commit:  https://github.com/memcached/memcached/commit/39151c870c5e598f039714b
> db790bd46f614856e
> >
> > However, I get the sense that this isn't the full story.  I'm guessing
> > there is the equivalent of a mkdir -p somewhere, that is using the
> > conf filename instead of the conf directory.  It would be interesting
> > to have an outline of steps through the code that result in the
> > failure mode.
> 
> Actually, the bug is caused by the commit referenced above, but also by
> another one:
> https://github.com/thatsafunnyname/memcached/commit/80dd99d831535ddeec73d55a0a
> dcaeaac8cb7298.
> 
> Now, the reason for the bug is interesting.  Before the commits
> mentioned above, we had a big part of the sasl_defs.c file practically
> inactive due to the "#ifdef HAVE_SASL_CB_GETCONF" check (which fails
> when compiling with cyrrus-sasl), including the definition of the
> "sasl_getconf" function, which is the source of this bug, and which
> implements the "sasl_getconfpath_t" callback used by sasl to get the
> configuration path to be used.  According to sasl_getconfpath_t(3):
> 
>   sasl_getconfpath_t is used if the application wishes to use a
>   different location for the SASL configuration files. If this callback
>   is not used SASL will either use the location in the environment
>   variable SASL_CONF_PATH (provided we are not SUID or SGID) or
>   /etc/sasl2 by default.
> 
> Which means that, before both commits existed, memcached was always
> using /etc/sasl2 as the config path.
> 
> When the commits were incorporated into the repo, memcached started
> using "sasl_getconf" for both the HAVE_SASL_CB_GETCONF and the
> HAVE_SASL_CB_GETCONFPATH cases, but the function only works is the sasl
> being used supports SASL_CB_GETCONF, because it passes the full path
> (including the filename) back to the sasl library for processing.  And
> this is the reason why the sasl library started getting
> "/etc/sasl2/memcached.conf/" as the config path.

Thanks for the additional analysis.

> > Finally, with the regression potential section of the SRU report,
> > remember not only to answer the question of "how likely is a
> > regression" but also "if a regression did occur, what would we expect
> > it to look like?"  So in this case what testers should look for would
> > be behavior changes in authentication when sasl is enabled, or
> > unexpected behaviors with memcached config file locations.
> 
> Thanks, I will also double check the regression potential section.
> 
> I hope this email clarifies all the questions you had.  I will update
> the branch in a few minutes.
> 
> Thanks for the review.

Yep, thanks ahead of time for making the requested changes.

Bryce