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