Bug 13813 - idmap cache pollution with S-1-22- IDs on winbind hickup
Summary: idmap cache pollution with S-1-22- IDs on winbind hickup
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Winbind (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Christof Schmitt
QA Contact: Samba QA Contact
Depends on: 13802
  Show dependency treegraph
Reported: 2019-02-28 09:19 UTC by Volker Lendecke
Modified: 2019-04-17 23:18 UTC (History)
4 users (show)

See Also:

Patch for 4.10 (29.30 KB, patch)
2019-03-01 12:07 UTC, Volker Lendecke
no flags Details
Patch for 4.9 (31.16 KB, patch)
2019-03-01 12:08 UTC, Volker Lendecke
no flags Details
Patch for 4.8 (31.15 KB, patch)
2019-03-01 12:08 UTC, Volker Lendecke
no flags Details
Patch v2 for 4.8 (76.96 KB, patch)
2019-03-04 22:53 UTC, Christof Schmitt
no flags Details
Patch v2 for 4.9 (53.02 KB, patch)
2019-03-04 22:54 UTC, Christof Schmitt
no flags Details
Patch v2 for 4.10 (59.88 KB, patch)
2019-03-04 22:55 UTC, Christof Schmitt
no flags Details
Patch v3 for 4.8.next (79.54 KB, patch)
2019-03-06 18:40 UTC, Jeremy Allison
no flags Details
Patch v3 for 4.8.next (79.40 KB, patch)
2019-03-06 18:45 UTC, Jeremy Allison
cs: review+
ab: review+
Patch v3 for 4.9.next (55.44 KB, patch)
2019-03-06 19:00 UTC, Jeremy Allison
cs: review+
ab: review+
Patch v3 for 4.10.rcNext (53.60 KB, patch)
2019-03-06 20:07 UTC, Jeremy Allison
cs: review+
ab: review+

Note You need to log in before you can comment on or make changes to this bug.
Description Volker Lendecke 2019-02-28 09:19:05 UTC
Have patch, need bug-number
Comment 1 Volker Lendecke 2019-03-01 12:07:08 UTC
The patches to follow depend on the patches for 13802 to be applied
Comment 2 Volker Lendecke 2019-03-01 12:07:49 UTC
Created attachment 14884 [details]
Patch for 4.10
Comment 3 Volker Lendecke 2019-03-01 12:08:24 UTC
Created attachment 14885 [details]
Patch for 4.9
Comment 4 Volker Lendecke 2019-03-01 12:08:55 UTC
Created attachment 14886 [details]
Patch for 4.8
Comment 5 Christof Schmitt 2019-03-04 19:01:35 UTC
The backports fail the ABI check from --picky-developer:

[4087/4217] abi_check: bin/default/source3/libsamba-passdb.so
libsamba-passdb.so: symbol xid_to_sid has been added - please mark it _PRIVATE_ or update minor version
	signature: void (struct dom_sid *, const struct unixid *)
libsamba-passdb.so: symbol winbind_xid_to_sid has been added - please mark it _PRIVATE_ or update minor version
	signature: bool (struct dom_sid *, const struct unixid *)
Waf: Leaving directory `/home/cschmit/code/samba/bin'
Build failed: ABI for libsamba-passdb.so has changed - please fix library version then build with --abi-update
See http://wiki.samba.org/index.php/Waf#ABI_Checking for more information
If you have not changed any ABI, and your platform always gives this error, please configure with --abi-check-disable to skip this check
make: *** [all] Error 1

For master the ABI signature update is in "lib: Remove some unused code".
What is the best way to update the ABI signature for the backports?
Just keep the functions in the signature that are removed in master?
Comment 6 Volker Lendecke 2019-03-04 19:33:59 UTC
I'll find a solution by making that symbol static. I won't have time for this before next week probably, sorry for not checking with due diligence
Comment 7 Volker Lendecke 2019-03-04 19:35:55 UTC
By the way, we give 100% ABI guarantees within a release. We can NOT change *ANYTHING* user-visible. So we probably need to rewrite the whole patch series to work within the existing semantics.
Comment 8 Jeremy Allison 2019-03-04 19:41:54 UTC
Adding a symbol isn't usually considered an ABI violation (a minor version update). We already do this don't we ?
Comment 9 Jeremy Allison 2019-03-04 19:43:50 UTC
Saying "We can NOT change *ANYTHING* user-visible." isn't correct as far as I know. We can (and I think we do) add user-visible symbols so long as we update the minor version number, as existing library users will continue to work.
Comment 10 Christof Schmitt 2019-03-04 20:48:14 UTC
I think we can sort this out:

In master the ABI went from 27.1 to 27.2. This appears wrong now,
as functions have been removed, so this should have gone to 28.0.
We can simply fix that for master.

For the 4.9 backport, that only adds one functions, we create
a new signature for the 27.2 ABI.

4.8 only has passdb ABI 27.0. The change from 27.0 to 27.1 only
added create_builtin_guests: NTSTATUS (const struct dom_sid *)
We could try backporting create_builtin_guests together with
the backports to allow the ABI increase to 27.2 for the 4.8 backport.

If that sounds correct, i can look into making the additional changes.
Comment 11 Jeremy Allison 2019-03-04 20:58:28 UTC
Yeah, we should make sure that we push the major number up for our libraries for new Samba releases, that gives up minor version number space to do bugfixes with a forward compatible ABI in a previous version release stream.
Comment 12 Christof Schmitt 2019-03-04 21:41:38 UTC
I am not sure if we would want to increase the
major version for all libraries. Backporting
the new functions should usually be feasible.

is an proposed update for the passdb ABI. I will
update the backports next.
Comment 13 Christof Schmitt 2019-03-04 22:53:22 UTC
Created attachment 14897 [details]
Patch v2 for 4.8
Comment 14 Christof Schmitt 2019-03-04 22:54:36 UTC
Created attachment 14898 [details]
Patch v2 for 4.9
Comment 15 Christof Schmitt 2019-03-04 22:55:09 UTC
Created attachment 14899 [details]
Patch v2 for 4.10
Comment 16 Volker Lendecke 2019-03-05 06:59:56 UTC
Can someone explain to me what is expected from me now? I put in the bug, I have to fix it, and I am even paid to fix it. I just don't know what is expected in the form of guarantees that the Samba Team has given to all the downstream users of our passdb public library.
Comment 17 Volker Lendecke 2019-03-05 14:58:47 UTC
Closing as WONTFIX. Samba has published a fixed API that is severely broken by this kind of change.
Comment 18 Jeremy Allison 2019-03-05 18:53:24 UTC
Reopening to get feedback from Alexander before we make a final decision.
Comment 19 Jeremy Allison 2019-03-05 18:57:02 UTC
Christof, can we work with Alexander please to get a fix that works for both Samba and FreeIPA ? Adding Alexander to the CC: list for the bug.


Comment 20 Christof Schmitt 2019-03-05 19:18:52 UTC
(In reply to Jeremy Allison from comment #19)
yes, i think Alexander has to comment on what
he needs. Maybe not removing the functions in
the backports would be sufficient.

I also submitted another patch for master that
we should include here; third patch in
Comment 21 Andrew Bartlett 2019-03-06 04:54:25 UTC
(In reply to Volker Lendecke from comment #16)
(In reply to Volker Lendecke from comment #17)

The pattern we use with ldb was to say that modules (and sssd has ldb modules) must rebuild against the exact version of Samba ldb that they are run with.  

.so versions are a good thing, but the most important thing they do is assist in communication, and that is the main thing required here.  The downstream users here are not far away or uncommunicative, they are part of our community too.

Certainly the 'contracts' should be clearer, and reasonable people can disagree as to if it is better to have this in or out of our tree.  As you removed it in c5e101af2b16c2869bf5c8f8aa90876aa6450f55 I presume you prefer it out of our tree.

I will say it is unsettling to think a job is done and find new requirements, but that again is life in a software development community.
Comment 22 Volker Lendecke 2019-03-06 06:30:41 UTC
Just to point out: I removed the module because in a side-channel I was told that this version was unused by RedHat and that RedHat develops the FreeIPA module independently of Samba. Do you think that carrying code in Samba that is known to be unused because it is orphaned by the only consumer is the better choice?
Comment 23 Alexander Bokovoy 2019-03-06 07:23:27 UTC
(In reply to Volker Lendecke from comment #22)
> Just to point out: I removed the module because in a side-channel I was told
> that this version was unused by RedHat and that RedHat develops the FreeIPA
> module independently of Samba. Do you think that carrying code in Samba that
> is known to be unused because it is orphaned by the only consumer is the
> better choice?

I don't know who told you that but it is clearly not what happens in reality. Especially, given the fact that ipasam is PASSDB module, thus has to be loaded into smbd/winbindd. It cannot be developed independently of Samba. In general, there was no problem in adapting that module to Samba changes over years, as long as there were no attempts to cut off APIs deliberately.

'struct unixid' situation was forced on in 2012 by making sure some of PASSDB APIs were required to pass in 'struct unixid *' pointers. There was no public API to deal with that and we added it, stopped short of actually making gen_ndr/idmap.h a public header to avoid all this problem in the first place.

However, my proposal was more humble: do not delete the code in backports you know is used out there. That's all I ask for, especially when a presence of that code is not affecting the code you are backporting.
Comment 24 Andrew Bartlett 2019-03-06 07:31:30 UTC
(In reply to Volker Lendecke from comment #22)
No I don't.

I think the best place to maintain the module is in FreeIPA's tree, and that any copy we have will just be stale.  It is clearly impractical to keep it up to date in our tree, just as we don't carry a stale copy of debian packaging and other things.

In this I disagree with Jeremy's mail on the list here:

Where I disagree with you is in suggesting the situation is so impossible that we can't acknowledge and work to resolve user issues concurrent with also being in communion with our friends at FreeIPA.  There are practical ways forward, as I wrote.  

But our users and your fellow developers deserve better than closing the bug as WONTFIX at the first sign of difficultly.
Comment 25 Jeremy Allison 2019-03-06 18:40:55 UTC
Created attachment 14901 [details]
Patch v3 for 4.8.next

Includes updates from cs's latest fixes to master. Compiles with --without-winbind also.

Christof and Alexander please review.
Comment 26 Jeremy Allison 2019-03-06 18:45:48 UTC
Created attachment 14902 [details]
Patch v3 for 4.8.next

Sorry, missed a removed comment in the last patch in the set. This version fixes that.
Comment 27 Jeremy Allison 2019-03-06 19:00:52 UTC
Created attachment 14903 [details]
Patch v3 for 4.9.next
Comment 28 Christof Schmitt 2019-03-06 19:28:41 UTC
The backports for 4.8 and 4.9 look good to me.
Should we keep the removal patch for 4.10 or
skip it and only remove the code for master and
Comment 29 Alexander Bokovoy 2019-03-06 19:35:08 UTC
Please do not backport the removal of unixid_* functions to 4.10.

I'll do my review tomorrow by building both samba and freeipa in Fedora COPR.
Comment 30 Jeremy Allison 2019-03-06 19:41:50 UTC
I'm working on that right now. I think for 4.10.x as Alexander says we need to keep the 'removed' functions (and leave the ABI at 0.27.2) and only remove them for master - 4.11.x (or Samba5, whatever we decide to call it :-).
Comment 31 Jeremy Allison 2019-03-06 20:07:13 UTC
Created attachment 14904 [details]
Patch v3 for 4.10.rcNext

Here's the version 3 for 4.10.rcNext - keeps the same ABI as 0.27.2. Includes the changes from master to keep --without-winbind working.

With this change we now need to update the ABI number in master to 0.28.0 to match the removed functions.
Comment 32 Jeremy Allison 2019-03-06 20:08:32 UTC
Alexander, if you can RB+ these once you've tested on Fedora COPR we can get them in for 4.10.rcNext and bugfixes for 4.9.x, 4.8.x.

Thanks everyone for your help in reviewing these !
Comment 33 Jeremy Allison 2019-03-06 20:11:35 UTC
Christof, do you want to propose the ABI update patch for master to 0.28.0, or do you want me to do it ?
Comment 34 Christof Schmitt 2019-03-06 20:50:47 UTC
(In reply to Jeremy Allison from comment #33)
is the patch on samba-technical to update the ABI number.
Comment 35 Jeremy Allison 2019-03-06 21:10:18 UTC
Lukas pointed out on that thread that removing a function means a major version bump. Should we go to 1.0 instead ?
Comment 36 Christof Schmitt 2019-03-06 21:58:39 UTC
(In reply to Jeremy Allison from comment #35)
I think this is a wider question. The 0 probably
tries to indicate that this is not a stable API.
Past changes have used the second number as "major"
and the third as "minor" which is not the typical
versioning for shared libraries. I think we need
to decide:

Either stick to the older scheme of increasing
the second as "major" and then discuss how to
improve versioning for the libraries in Samba
for the future.

Or make passdb 1.0.0 now and then address the
other libraries later.

I would like to hear other opinions.
Comment 37 Alexander Bokovoy 2019-03-07 09:20:24 UTC
I've built 4.9 branch patches in https://copr.fedorainfracloud.org/coprs/abbra/test-samba-4.9-backport-bug-13813/builds/ and rebuilding freeipa succeeded without additional patches. I had to patch up Fedora 29 libldb and samba to get the patchset applied because v4-9-test requires new libldb (1.4.6 at least) and this one will not be added to Fedora after 4.9.next release.

So the patches on 4.9 backport are OK. Since 4.8 is roughly the same, the patchset should be OK. Same for 4.10.

I will work on the changes in FreeIPA to adopt to git master asap.
Comment 38 Jeremy Allison 2019-03-07 16:55:26 UTC
Re-assigning to Karolin for inclusion in 4.10.rcNext, 4.9.next, 4.8.next.

Thanks for your help everyone !
Comment 39 Karolin Seeger 2019-03-12 09:41:23 UTC
(In reply to Jeremy Allison from comment #38)
Pushed to autobuild-v4-{10,9,8}-test.
Comment 40 Karolin Seeger 2019-03-19 09:24:11 UTC
(In reply to Karolin Seeger from comment #39)
Pushed to all branches.
Closing out bug report.

Comment 41 Volker Lendecke 2019-04-15 09:55:52 UTC
As discussed in a side channel: The fixes break overlapping idmap ranges for rfc2307 "idmap ad" backend.
Comment 42 Christof Schmitt 2019-04-17 23:18:07 UTC
I would prefer to track this in a new bugzilla: Bug 13903