Code review comment for ~sergiodj/ubuntu/+source/memcached:sasl-configuration-wrong-path-bug1878721-focal

On Tuesday, June 02 2020, Bryce Harrington wrote:

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

That's fair enough, thanks for pointing it out.

>> > 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 will adjust and improve the test instructions to reflect that.

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

Fair enough.

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

I've just pushed it.

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

OK, I'll do that.

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

You're welcome.

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

No problem.

Since I'm changing the changelog entry and the patch description, I
think it would be a good idea to incorporate these changes into the eoan
backport as well. I noticed you approved it already (thanks!), but I
will push an updated branch there. Hopefully things will be OK.

Thanks,

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14