DEP8 failure with samba 4.15.5

[Andreas Hasenack]

[Impact]
Tests are failing with the samba version in jammy. We want people using focal be able to upgrade to newer samba and still be compatible with adsys.

[Test Plan]
1. Ensure that DEP8 tests are passing once uploaded to proposed and ready to be migrated

[Where problems could occur]
The functionalities failing are only on tests running (DEP8 or tests during package builds), but we want to be able to run our tests with newer versions of samba on focal too without regressing existing testsuite.

--------------

The DEP8 tests of adsys started to fail[1] with my samba 4.15.5 upload.

Some hints:

<didrocks> ahasenack: it seems we can’t start our smbd local daemon to simulate Active Directory smb server

<didrocks> ahasenack: yeah, I’m puzzled because on smbd start failure, we do print stderr (not stdout though): https://github.com/ubuntu/adsys/blob/main/internal/testutils/samba.go#L16
<didrocks> https://github.com/ubuntu/adsys/blob/main/internal/testutils/samba.go#L43 in particular
<didrocks> you can see here how we start it: https://github.com/ubuntu/adsys/blob/main/internal/testutils/samba.go#L21
<didrocks> and the config template is at https://github.com/ubuntu/adsys/blob/20e6f962eb87f667f5e29800be0715ab2496a10d/internal/testutils/samba.go#L55
<ahasenack> 2022/02/24 03:13:59 Setup: smbd hasn’t started successfully
<ahasenack> that's here: https://github.com/ubuntu/adsys/blob/main/internal/testutils/samba.go#L129
<ahasenack> you wait for the port to be open?
<ahasenack> which port is that, 445/tcp?
<didrocks> we wait on the port to be opened, and this one is passed as a parameter, see the template. For the argument we pass in ad tests, once sec, looking
<didrocks> 1446
<didrocks> https://github.com/ubuntu/adsys/blob/20e6f962eb87f667f5e29800be0715ab2496a10d/cmd/integration_tests/adsys_test.go#L47

<didrocks> you just need to run go test . in internal/ad/
<didrocks> the failure is as the pre-test setup

1. https://autopkgtest.ubuntu.com/results/autopkgtest-jammy/jammy/amd64/a/adsys/20220224_031434_4d453@/log.gz

[Andreas Hasenack (ahasenack)]

I capture calls to smbd:
PCOMM PID PPID RET ARGS
smbd 9605 9598 0 /usr/sbin/smbd -FS -s /tmp/adsys_smbd_3503406889/smbd.conf
smbd 9699 9460 0 /usr/sbin/smbd -FS -s /tmp/adsys_smbd_3503406889/smbd.conf

Ran them by hand:
# /usr/sbin/smbd -FS -s /tmp/adsys_smbd_3503406889/smbd.conf

Invalid option -FS: unknown option

-F still exists, but not -S apparently. The manpage confirms -S is gone. It is mentioned in the context of -i, but -S itself is no longer there.

[Andreas Hasenack (ahasenack)]

I'll check smbd to see what happened with -S, and if it was intentional.

[Andreas Hasenack (ahasenack)]

commit 3467214cf967641f4e4001a9dfea870f933fc2a3
Author: Andreas Schneider <email address hidden>
Date: Mon Jan 11 09:52:36 2021 +0100

    s3: Remove --log-stdout from daemons

    The common cmdline parser provides --debug-stdout.

    Signed-off-by: Andreas Schneider <email address hidden>
    Reviewed-by: Andrew Bartlett <email address hidden>

i.e., use --debug-stdout instead of -S

[Andreas Hasenack (ahasenack)]

I applied this simple patch, but the tests still fail. The log output is hard to parse, I'm attaching it.

--- a/internal/testutils/samba.go
+++ b/internal/testutils/samba.go
@@ -18,7 +18,7 @@ func SetupSmb(port int, sysvolDir string) func() {
        dir, cleanup := mkSmbDirWithConf(smbPort, sysvolDir)

        // #nosec:G204 - we control the directory we run smbd on (on tests)
- cmd := exec.Command("smbd", "-FS", "-s", filepath.Join(dir, "smbd.conf"))
+ cmd := exec.Command("smbd", "-F", "--debug-stdout", "-s", filepath.Join(dir, "smbd.conf"))
        stderr, err := cmd.StderrPipe()
        if err != nil {
                log.Fatalf("Setup: can’t get smb output: %v", err)

[Andreas Hasenack (ahasenack)]

My last idea is that maybe adsys got caught in the same bug as backuppc[1], and is trying to send empty username/password for anonymous authentication. I tried to catch it doing so, but couldn't find a good spot in the test for that. I think the auth is handled by https://github.com/mvo5/libsmbclient-go, and I managed to crash that standalone demo utility even.

@didrocks, could you give a hand on this one?

1. https://code.launchpad.net/~sergiodj/ubuntu/+source/backuppc/+git/backuppc/+merge/416111

[Didier Roche-Tolomelli (didrocks)]

Hum, it seems your patch makes sense and should work on most arch. I saw Michael did an upload with this: https://launchpad.net/ubuntu/+source/adsys/0.8ubuntu1
But it’s failing only on ppcel64: https://launchpadlibrarian.net/588289907/buildlog_ubuntu-jammy-ppc64el.adsys_0.8ubuntu1_BUILDING.txt.gz and passing on other architectures.

The issue may be related to the signal installed on libsmbclient: https://github.com/ubuntu/adsys/blob/main/internal/smbsafe/smbsafe.go#L4. This workaround is the only reason of smbsafe (you can’t then exec concurrently subprocesses without cleaning up those signals and the library is doing that at every smb call on your behalf).
Another idea is that stderr is not allocated for whatever reason https://github.com/ubuntu/adsys/blob/main/internal/testutils/samba.go#L38 but why only on ppcel64?

I reastically won’t have the time this week with the sprint to look at this. I hope this gives some hints if you need urgently to have it fixed before next one.

[Andreas Hasenack (ahasenack)]

Even where the build passed, the DEP8 tests are failing again (https://autopkgtest.ubuntu.com/results/autopkgtest-jammy/jammy/amd64/a/adsys/20220228_024008_ead55@/log.gz)

[Andreas Hasenack (ahasenack)]

Oh, in my link it was using samba 4.13, need to trigger it with proposed samba. I'll do that.

[Andreas Hasenack (ahasenack)]

It passed in s390x, fingers crossed about the rest: https://autopkgtest.ubuntu.com/packages/a/adsys/jammy/s390x

[Michael Hudson-Doyle (mwhudson)]

So fwiw the build failure on ppc64el is hanging on the the call to io.ReadAll(stderr) in SetupSmb (i.e. here: https://github.com/ubuntu/adsys/blob/v0.8/internal/testutils/samba.go#L38). No idea why, but my sense from poking around is that the test has already failed when the hang occurs so maybe it's some pipe buffer being full nonsense.

[Michael Hudson-Doyle (mwhudson)]

Hm no, if I apply this patch (or something like it) https://paste.ubuntu.com/p/vVTyzHMqvr/ the build succeeds. Wtf.

[Andreas Hasenack (ahasenack)]

Unassigning from me, as I don't know how to make more progress here.

[Christian Ehrhardt  (paelzer)]

Fixed in https://launchpad.net/ubuntu/+source/adsys/0.8ubuntu1 via the quick fix by Michael (thanks). AFAIU you wanted to keep the bug open for a better solution down the road?

[Andreas Hasenack (ahasenack)]

There are two bugs, which may be duplicates, or not:
- this one, which apparently became an FTBFS on ppc64el due to a test failure (the same test? Unclear to me)
- #1962510 which is an armhf failure, also due to tests failing during build

Maybe a no-change rebuild should be uploaded, to confirm what's the current status

[Michael Hudson-Doyle (mwhudson)]

Didier merged a better version of the fix I uploaded so presumably he'll upload that soon and we can see where things fall.

[Didier Roche-Tolomelli (didrocks)]

For reference, fixed in https://github.com/ubuntu/adsys/pull/289. Thanks Michael for the initial work on this!

[Launchpad Janitor (janitor)]

This bug was fixed in the package adsys - 0.8.1

---------------
adsys (0.8.1) jammy; urgency=medium

  * Change chown logic on script directory and parents to avoid potential
    vulnerability. (LP: #1961458)
  * Separate readiness from session running to avoid unrefreshed user script
    directories after a logout without any new logins.
  * pam_adsys: Fix memory leak and identation. (LP: #1961459)
  * Adapt to newer samba, while keeping backward compatilibity for CI.
    Thanks Michael. (LP: #1962170)
  * Try to stabilize configuration detection change test by calling sync() to
    sync FHS to disk, and then, hoping we get the inotify update. Seems to fix
    flakyness on armhf. (LP: #1962510)
  * Enforce closing stderr on ppcel64 in tests with new samba to avoid hangs
    in race.
  * Fix linting issues discovered by new golangci-lint.
  * Misc syntax polish.
  * Dependencies update:
    - github.com/godbus/dbus/v5
    - github.com/golangci/golangci-lint
    - gopkg.in/ini.v1

 -- Didier Roche <email address hidden> Tue, 08 Mar 2022 09:49:08 +0100

[Łukasz Zemczak (sil2100)]

Hello Andreas, or anyone else affected,

Accepted adsys into focal-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/adsys/0.9.2~20.04 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-focal to verification-done-focal. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-focal. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Jean-Baptiste Lallement (jibel)]

Verified successfully on Focal with adsys 0.9.2~20.04.

Marking as verification-done

[Chris Halse Rogers (raof)]

The verification of the Stable Release Update for adsys has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[Launchpad Janitor (janitor)]

This bug was fixed in the package adsys - 0.9.2~20.04

---------------
adsys (0.9.2~20.04) focal; urgency=medium

  * Backport to focal
    - Build with Go 1.16
    - Move debhelper compat to 12
    - Do not recommends ubuntu-advantage-desktop-daemon as it’s not available
      on focal yet.

adsys (0.9.2) kinetic; urgency=medium

  * Update generators to fix FTBFS
    - shell out to mkdir instead of go's os.Mkdir which can bypass fakeroot's
      filesystem hijacking and cause unexpected behavior
  * Update dependencies to latest:
    - github.com/golangci/golangci-lint
    - google.golang.org/protobuf

adsys (0.9.1) kinetic; urgency=medium

  [ Didier Roche ]
  [ Gabriel Nagy ]
  * Fix loading policy content from uppercase folders (LP: #1982330)
  * Add GSettings power management keys (LP: #1982349)
  * Allow parsing policy entries with empty values (LP: #1982342)
  * Allow parsing policies with unsupported types (LP: #1982343)
  * Allow parsing policy entries with no data (LP: #1982345)
  * Lowercase target name when normalizing (LP: #1982347)
  * Annotate policies that require Ubuntu Pro (LP: #1982348)
  * Update dependencies to latest:
    - github.com/spf13/cobra
    - github.com/spf13/viper
    - github.com/stretchr/testify
    - github.com/charmbracelet/bubbletea
    - github.com/charmbracelet/bubbles
    - google.golang.org/grpc
    - github.com/golangci/golangci-lint
    - github.com/sirupsen/logrus

adsys (0.9.0) kinetic; urgency=medium

  [ Jean-Baptiste Lallement ]
  [ Didier Roche ]
  [ Gabriel Nagy ]
  * Add Active Directory Watch Daemon - adwatchd: (LP: #1982351)
    - Implement a Windows daemon that watches a list of configured directories
      for changes and bumps the relevant GPT.INI files.
    - Add adsys-windows binary package which includes the Windows daemon
      executable and the admx/adml policies.
  * Config detection now includes current executable directory
  * Fixes in generator build race
  * Update dependencies to latest:
    - github.com/spf13/cobra
    - github.com/stretchr/testify
  * CI updates:
    - switch to Go setup v3
    - bump to really build with Golang 1.18

adsys (0.8.6) kinetic; urgency=medium

  * Fix new build failures on 32 bits due to libsmbclient-dev no longer sets
    the large file support cflags in libsmbclient.h.
    Update to latest libsmbclient-go.
  * Update dependencies to latest:
    - google.golang.org/grpc
    - gopkg.in/ini.v1
    - github.com/golangci/golangci-lint
    - github.com/spf13/viper
    - github.com/stretchr/testify

adsys (0.8.5) kinetic; urgency=medium

  [ Jean-Baptiste Lallement ]
  [ Didier Roche ]
  * Rename chapters to be in correct ascii order when viewed online.
    Thanks to Anton Drastrup-Fjordbak.
  * Include 22.04 in admx/adml for lts only releases. (LP: #1973745)
  * Bump embedeed dependencies minor versions for both bug fixes and minor
    security enhancements.
  * Fix dconf keys not being readable by user after applying policy.
    (LP: #1973748)
  * Ensure we can execute machine and user scripts:
    /run is now noexec on Ubuntu. Ensure that we can execute the scripts in
    /run/adsys subdirectories. The scripts mechanism has been reviewed by the...