Merge ~sergiodj/ubuntu/+source/memcached:sasl-configuration-wrong-path-bug1878721-focal into ubuntu/+source/memcached:ubuntu/focal-devel

[Sergio Durigan Junior]

This is a simple regression that happens on memcached. The SASL configuration is being read from an erroneous file, '/etc/sasl2/memcached.conf/memcached.conf'. Upstream fixed this bug and made memcached load the configuration from the right file, '/etc/sasl2/memcached.conf', but kept the old behaviour (i.e., reading from the wrong location) just in case users have made a workaround.

The reporter was thorough and provided the debdiff along with an SRU template, so one could say I'm filing this MP for him. I kept his authorship intact.

There is a PPA with the proposed change here:

https://launchpad.net/~sergiodj/+archive/ubuntu/memcached-bug1878721

autopkgtest is still happy:

autopkgtest [08:50:52]: test client.pl: -----------------------]
autopkgtest [08:50:53]: test client.pl: - - - - - - - - - - results - - - - - - - - - -
client.pl PASS
autopkgtest [08:50:54]: @@@@@@@@@@@@@@@@@@@@ summary
daemon PASS
client.pl PASS

[Anders Kaseorg (andersk)]

review: Approve

[Bryce Harrington (bryce)]

* Changelog:
  - [√] old content and logical tag match as expected
  - [√] changelog entry correct version and targeted codename
  - [√] changelog entries correct
  - [x] update-maintainer has been run

* Actual changes:
  - [-] no upstream changes to consider
  - [-] no further upstream version to consider
  - [√] debian changes look safe

* Old Delta:
  - [-] dropped changes are ok to be dropped
  - [-] nothing else to drop
  - [√] changes forwarded upstream/debian (if appropriate)

* New Delta:
  - [-] no new patches added
  - [√] patches match what was proposed upstream
  - [√] patches correctly included in debian/patches/series
  - [√] patches have correct DEP3 metadata

* Build/Test:
  - [√] build is ok
  - [√] verified PPA package installs/uninstalls
  - [√] autopkgtest against the PPA package passes
  - [x] sanity checks test fine

A few questions / things to fix. First, the packaging work itself looks correct, except for one detail: Make sure to run update-maintainer, since this is the first ubuntu version after debian's. The eoan package is ok since it had a ubuntu version previously.

Just a minor nitpick, but I noticed a typo in a comment in the patch itself:

  ...actual configue file...

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?

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?

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

Another thing I notice in the patch is that it is also checking for if the config file was copied to /etc/sasl or /etc/sasl2. I.e. where those are supposed to be directories they've been created as files. I can see how this could be a common error, however the bug report doesn't describe this case. I wonder under what circumstances this issue was cropping up - do we have any ubuntu bug reports about this happening in the wild? Or can we point to a commit upstream that introduced the problem originally?

For the test case in the SRU bug, I notice it is testing the case where the config file is at /etc/s...

review: Needs Fixing

[Anders Kaseorg (andersk)]

> Just a minor nitpick, but I noticed a typo in a comment in the patch itself:
>
> ...actual configue file...

This is part of the upstream patch, which I copied verbatim. It’s not my typo to correct.

> 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

Ah, I had neglected to transcribe the test case step that actually enables SASL authentication:

echo '-S' >> /etc/memcached.conf

Fixed in the report, and verified in lxc.

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

No. The bug is that, no matter what state the actual filesystem is in, memcached *looks* for the incorrect /etc/sasl2/memcached.conf/memcached.conf path by default. There is no reason this path would exist, unless the admin has intentionally created it to work around this exact bug.

> Another thing I notice in the patch is that it is also checking for if the
> config file was copied to /etc/sasl or /etc/sasl2. I.e. where those are
> supposed to be directories they've been created as files.

You’re misreading the patch. Note that there are three arrays involved, one of which is old and two of which are added by the patch. The logic is:

• If /etc/sasl/memcached.conf/memcached.conf is a file, then pass the /etc/sasl/memcached.conf directory to the SASL library.
• If /etc/sasl/memcached.conf is a file, then pass the /etc/sasl directory to the SASL library.
• If /etc/sasl2/memcached.conf/memcached.conf is a file, then pass the /etc/sasl2/memcached.conf directory to the SASL library.
• If /etc/sasl2/memcached.conf is a file, then pass the /etc/sasl2 directory to the SASL library.

The SASL library expects a directory. A file at /etc/sasl or /etc/sasl2 will not work, has never worked, and is not expected to work.

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

Added to the report.

> 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?)

Keep in mind tha...

[Sergio Durigan Junior (sergiodj)]

On Monday, June 01 2020, Bryce Harrington wrote:

> Review: Needs Fixing
>
> * Changelog:
> - [√] old content and logical tag match as expected
> - [√] changelog entry correct version and targeted codename
> - [√] changelog entries correct
> - [x] update-maintainer has been run
>
> * Actual changes:
> - [-] no upstream changes to consider
> - [-] no further upstream version to consider
> - [√] debian changes look safe
>
> * Old Delta:
> - [-] dropped changes are ok to be dropped
> - [-] nothing else to drop
> - [√] changes forwarded upstream/debian (if appropriate)
>
> * New Delta:
> - [-] no new patches added
> - [√] patches match what was proposed upstream
> - [√] patches correctly included in debian/patches/series
> - [√] patches have correct DEP3 metadata
>
> * Build/Test:
> - [√] build is ok
> - [√] verified PPA package installs/uninstalls
> - [√] autopkgtest against the PPA package passes
> - [x] sanity checks test fine
>
> A few questions / things to fix. First, the packaging work itself
> looks correct, except for one detail: Make sure to run
> update-maintainer, since this is the first ubuntu version after
> debian's. The eoan package is ok since it had a ubuntu version
> previously.

Thanks for the review, Bryce.

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.

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

[Bryce Harrington (bryce)]

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

[Sergio Durigan Junior (sergiodj)]

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

[Bryce Harrington (bryce)]

Thanks for making the updates, this looks good.

I can sponsor the upload later this afternoon.

review: Approve

[Bryce Harrington (bryce)]

$ git ubuntu tag --upload
$ git push pkg upload/1.5.22-2ubuntu0.1
Enumerating objects: 21, done.
Counting objects: 100% (21/21), done.
Delta compression using up to 12 threads
Compressing objects: 100% (15/15), done.
Writing objects: 100% (15/15), 3.95 KiB | 506.00 KiB/s, done.
Total 15 (delta 10), reused 0 (delta 0)
To ssh://git.launchpad.net/ubuntu/+source/memcached
 * [new tag] upload/1.5.22-2ubuntu0.1 -> upload/1.5.22-2ubuntu0.1
$ dput ubuntu ../memcached_1.5.22-2ubuntu0.1_source.changes
Checking signature on .changes
gpg: ../memcached_1.5.22-2ubuntu0.1_source.changes: Valid signature from E603B2578FB8F0FB
Checking signature on .dsc
gpg: ../memcached_1.5.22-2ubuntu0.1.dsc: Valid signature from E603B2578FB8F0FB
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading memcached_1.5.22-2ubuntu0.1.dsc: done.
  Uploading memcached_1.5.22-2ubuntu0.1.debian.tar.xz: done.
  Uploading memcached_1.5.22-2ubuntu0.1_source.buildinfo: done.
  Uploading memcached_1.5.22-2ubuntu0.1_source.changes: done.
Successfully uploaded packages.

diff --git a/debian/changelog b/debian/changelog
index de3c327..ae93c6b 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,21 @@
+memcached (1.5.22-2ubuntu0.1) focal; urgency=medium
+
+  * d/p/fix-bug-where-sasl-will-load-config-the-wrong-path.patch:
+    Fix the path from which SASL configuration is loaded.  (LP: #1878721)
+    The bug happened because sasl expects memcached to provide a
+    path (i.e., a directory, not a filename) where the sasl
+    configuration file(s) is (are).  However, memcached was passing
+    the filename (/etc/sasl2/memcached.conf) to sasl, which was
+    interpreting it as a directory, and looking for a configuration
+    file inside it (i.e., /etc/sasl2/memcached.conf/memcached.conf).
+    Users could workaround this bug by creating a directory named
+    /etc/sasl2/memcached.conf/, and putting the configuration file
+    inside it.  This patch not only fixes this bug (by passing the
+    right directory, /etc/sasl2/, to sasl) but also supports the
+    workaround described above.
+
+ -- Anders Kaseorg <[e-mail]>  Thu, 14 May 2020 17:13:17 -0700
+
 memcached (1.5.22-2) unstable; urgency=medium
 
   * Apply a patch (merged upstream) to fix FTFBS on s390x, etc. Thanks,
diff --git a/debian/control b/debian/control
index c2b1691..caf9830 100644
--- a/debian/control
+++ b/debian/control
@@ -1,7 +1,8 @@
 Source: memcached
 Section: web
 Priority: optional
-Maintainer: Chris Lamb <[e-mail]>
+Maintainer: Ubuntu Developers <[e-mail]>
+XSBC-Original-Maintainer: Chris Lamb <[e-mail]>
 Build-Depends:
  adduser,
  debhelper-compat (= 12),
diff --git a/debian/patches/fix-bug-where-sasl-will-load-config-the-wrong-path.patch b/debian/patches/fix-bug-where-sasl-will-load-config-the-wrong-path.patch
new file mode 100644
index 0000000..3901042
--- /dev/null
+++ b/debian/patches/fix-bug-where-sasl-will-load-config-the-wrong-path.patch
@@ -0,0 +1,103 @@
+From: Zheng Gu <[e-mail]>
+Date: Fri, 22 Nov 2019 22:34:16 +0800
+Subject: fix bug where sasl will load config the wrong path
+
+/etc/sasl2/memcached.conf/memcached.conf instead of
+/etc/sasl2/memcached.conf
+
+This bug was caused by two upstream commits:
+
+  https://github.com/memcached/memcached/commit/39151c870c5e598f039714bdb790bd46f614856e
+  https://github.com/memcached/memcached/commit/80dd99d831535ddeec73d55a0adcaeaac8cb7298
+
+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.
+
+Author: Zheng Gu <[e-mail]>
+Origin: upstream, https://github.com/memcached/memcached/commit/6207330c2705fdb5f02de13b99a0d994f7c4f14a
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/memcached/+bug/1878721
+Last-Update: 2020-05-26
+Applied-Upstream: 1.6.0
+Reviewed-by: Sergio Durigan Junior <[e-mail]>
+---
+ sasl_defs.c | 29 +++++++++++++++++++++++++++++
+ 1 file changed, 29 insertions(+)
+
+diff --git a/sasl_defs.c b/sasl_defs.c
+index c60d1bf..370f947 100644
+--- a/sasl_defs.c
++++ b/sasl_defs.c
+@@ -16,6 +16,23 @@ const char * const locations[] = {
+     "/etc/sasl2/memcached.conf",
+     NULL
+ };
++
++/* If the element of locations is file, locations_dir_path stores the
++ * directory path of these elements */
++const char *const locations_dir_path[] = {
++    "/etc/sasl",
++    "/etc/sasl2",
++    NULL
++};
++
++/* If the element of locations is directory, locations_file_path stores
++ * the actual configue file which used by sasl, when GETCONFPATH is
++ * enabled */
++const char *const locations_file_path[] = {
++    "/etc/sasl/memcached.conf/memcached.conf",
++    "/etc/sasl2/memcached.conf/memcached.conf",
++    NULL
++};
+ #endif
+ 
+ #ifndef HAVE_SASL_CALLBACK_FT
+@@ -88,12 +105,24 @@ static int sasl_getconf(void *context, const char **path)
+     *path = getenv("SASL_CONF_PATH");
+ 
+     if (*path == NULL) {
++#if defined(HAVE_SASL_CB_GETCONF)
+         for (int i = 0; locations[i] != NULL; ++i) {
+             if (access(locations[i], F_OK) == 0) {
+                 *path = locations[i];
+                 break;
+             }
+         }
++#elif defined(HAVE_SASL_CB_GETCONFPATH)
++        for (int i = 0; locations[i] != NULL; ++i) {
++            if (access(locations_file_path[i], F_OK) == 0) {
++                *path = locations[i];
++                break;
++            } else if (access(locations[i], F_OK) == 0) {
++                *path = locations_dir_path[i];
++                break;
++            }
++        }
++#endif
+     }
+ 
+     if (settings.verbose) {
+-- 
+2.26.2
+
diff --git a/debian/patches/series b/debian/patches/series
index dd01f20..bf83e1a 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -4,3 +4,4 @@
 0004-Don-t-run-some-upstream-tests.patch
 0005-Source-etc-default-memcached-in-etc-init.d-memcached.patch
 stats_prefix-Remove-test-failure-due-to-non-determin.patch
+fix-bug-where-sasl-will-load-config-the-wrong-path.patch