Ubuntu
memcached package

debian/changelog (+18/-0)
debian/control (+2/-1)
debian/patches/fix-bug-where-sasl-will-load-config-the-wrong-path.patch (+103/-0)
debian/patches/series (+1/-0)

High

Fix Released

Link a bug report

Reviewer	Date Requested	Status
Bryce Harrington (community)	2020-05-28	Approve on 2020-06-03
Anders Kaseorg (community)		Approve on 2020-05-28
Canonical Server Core Reviewers	2020-05-28	Pending
Review via email: mp+384764@code.staging.launchpad.net

Description of the change

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

Revision history for this message

Anders Kaseorg (andersk) on 2020-05-28:

review: Approve

Revision history for this message

Bryce Harrington (bryce) wrote on 2020-06-02:

Download full text (4.5 KiB)

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

* 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:

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/sasl2/memcached.conf, but I wonder if it should also include steps to test that config stored at the alternate locations also work?

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

The bug report indicates that the problem was introduced upstream with this commit:  https://github.com/memcached/memcached/commit/39151c870c5e598f039714bdb790bd46f614856e

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.

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.

review: Needs Fixing

Revision history for this message

Anders Kaseorg (andersk) wrote on 2020-06-02:

Download full text (4.2 KiB)

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

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

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.

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 that the patch description is copied verbatim from upstream along with the patch; I didn’t write either.

cyrus-sasl has never defined SASL_CB_GETCONF, and SASL_CB_GETCONFPATH was added in 2006, so for Ubuntu’s purposes we don’t need to worry about any other cases:

https://github.com/cyrusimap/cyrus-sasl/commit/ce6608bb400180294184116ab8148a15df3bb2db

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

No, as described above, the bug is in where the config file is read, not where it’s written.  There’s no mkdir involved.  The file is only ever written manually by the admin.

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

Added to the report.

Revision history for this message

Sergio Durigan Junior (sergiodj) wrote on 2020-06-02:

Download full text (8.6 KiB)

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

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 an effort not to break existing workarounds, the patch accepts the
erroneous directory as a valid entry if it exists.

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

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

I don't quite understand this point.

As was explained, 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.

> 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/39151c870c5e598f039714bdb790bd46f614856e
>
> 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/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.

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

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

Revision history for this message

Bryce Harrington (bryce) wrote on 2020-06-02:

Download full text (8.2 KiB)

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

> 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

Revision history for this message

Sergio Durigan Junior (sergiodj) wrote on 2020-06-03:

Download full text (8.9 KiB)

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

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

Revision history for this message

Bryce Harrington (bryce) wrote on 2020-06-03:

Thanks for making the updates, this looks good.

I can sponsor the upload later this afternoon.

review: Approve

Revision history for this message

Bryce Harrington (bryce) wrote on 2020-06-04:

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

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes:

Sergio Durigan Junior

git-ubuntu developers

Ubuntumemcached package

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

Commit message

Description of the change

Preview Diff

Subscribers

Ubuntu
memcached package