Bug 10300 - fail authentication if user isn't member of *any* require_membership_of specified groups
Summary: fail authentication if user isn't member of *any* require_membership_of speci...
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: Winbind (show other bugs)
Version: unspecified
Hardware: All All
: P2 critical
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-29 12:06 UTC by Noel Power
Modified: 2014-02-03 01:24 UTC (History)
4 users (show)

See Also:


Attachments
patch file (1.15 KB, patch)
2013-11-29 12:06 UTC, Noel Power
ddiss: review+
Details
Patch from master, applies to 3.6, 4.0 and 4.1 (1.42 KB, patch)
2013-11-29 15:23 UTC, David Disseldorp
no flags Details
Patch from master with fixed bug ID, applies to 3.6, 4.0 and 4.1 (1.37 KB, patch)
2013-12-03 10:44 UTC, David Disseldorp
ddiss: review+
asn: review+
jra: review+
abartlet: review+
Details
Patch from master with fixed bug ID and adjusted path, applies to 3.3 (1.40 KB, patch)
2013-12-05 11:40 UTC, David Disseldorp
asn: review+
Details
Patch from master with fixed bug ID, applies to 3.4 (1.37 KB, patch)
2013-12-05 11:40 UTC, David Disseldorp
asn: review+
Details
Patch from master with fixed bug ID, applies to 3.5 (1.37 KB, patch)
2013-12-05 11:41 UTC, David Disseldorp
asn: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Noel Power 2013-11-29 12:06:16 UTC
Created attachment 9495 [details]
patch file

If for example you set

    require_membership_of specified=bogus

where bogus ( like it hints is a non existent group ) then
you will be happily authenticated. This imho wrong and dangerous as you
easily might not notice a typo when entering that field, it would be
better to fail in this case ( and force the administrator to investigate )

see mailing list thread https://lists.samba.org/archive/samba-technical/2013-November/096411.html (with associated patch ack) and additionally the patch attached to this bug
Comment 1 David Disseldorp 2013-11-29 12:24:22 UTC
Comment on attachment 9495 [details]
patch file

This patch is on it's way onto the master branch following review from Andreas and I.

@Noel, can you please confirm the patch cleanly applies and is suitable for 4.0 and 4.1. Feel free to attach new patches for the maintenance branches if needed.
Comment 2 Noel Power 2013-11-29 12:54:16 UTC
(In reply to comment #1)

> @Noel, can you please confirm the patch cleanly applies and is suitable for 4.0
> and 4.1
git am succeeds for latest pulled v3.6-stable, v4.0-stable & v4.1-stable
Comment 3 David Disseldorp 2013-11-29 12:56:49 UTC
(In reply to comment #2)
> (In reply to comment #1)
> 
> > @Noel, can you please confirm the patch cleanly applies and is suitable for 4.0
> > and 4.1
> git am succeeds for latest pulled v3.6-stable, v4.0-stable & v4.1-stable

Great, I'll attach the patch which has the reviewed-buy and bug# attributes attached.
Comment 4 David Disseldorp 2013-11-29 15:23:57 UTC
Created attachment 9496 [details]
Patch from master, applies to 3.6, 4.0 and 4.1
Comment 5 David Disseldorp 2013-12-03 10:07:13 UTC
(In reply to comment #4)
> Created attachment 9496 [details]
> Patch from master, applies to 3.6, 4.0 and 4.1

Gah, I (Noel is innocent) pasted the wrong Bugzilla URL in the master commit. I'll fix it for the backport and resubmit.
Comment 6 David Disseldorp 2013-12-03 10:44:40 UTC
Created attachment 9499 [details]
Patch from master with fixed bug ID, applies to 3.6, 4.0 and 4.1
Comment 7 Noel Power 2013-12-03 10:53:03 UTC
Comment on attachment 9499 [details]
Patch from master with fixed bug ID, applies to 3.6, 4.0 and 4.1

+1
Comment 8 David Disseldorp 2013-12-03 11:08:34 UTC
Karolin, please merge.
Comment 9 Karolin Seeger 2013-12-04 11:38:19 UTC
(In reply to comment #8)
> Karolin, please merge.

Sorry, I need a second review first.
Volunteers?
Comment 10 David Disseldorp 2013-12-04 12:14:39 UTC
Comment on attachment 9499 [details]
Patch from master with fixed bug ID, applies to 3.6, 4.0 and 4.1

The master commit was reviewed by Andreas and myself. This is the same patch, with the bugzilla ID fixed.
Comment 11 Karolin Seeger 2013-12-04 12:21:21 UTC
(In reply to comment #10)
> Comment on attachment 9499 [details]
> Patch from master with fixed bug ID, applies to 3.6, 4.0 and 4.1
> 
> The master commit was reviewed by Andreas and myself. This is the same patch,
> with the bugzilla ID fixed.

Ok, thanks, but I need explicit review flags (or comments) in the bug report, not only in the commit message.
To be picky, I need it for each version separately...
Comment 12 David Disseldorp 2013-12-04 12:33:19 UTC
(In reply to comment #11)
> (In reply to comment #10)
> > Comment on attachment 9499 [details] [details]
> > Patch from master with fixed bug ID, applies to 3.6, 4.0 and 4.1
> > 
> > The master commit was reviewed by Andreas and myself. This is the same patch,
> > with the bugzilla ID fixed.
> 
> Ok, thanks, but I need explicit review flags (or comments) in the bug report,
> not only in the commit message.

Fair enough :)
I've requested review from Jeremy and Andreas.

> To be picky, I need it for each version separately...

Does that mean you want separate versions of the (same) patch attached for 3.6, 4.0 and 4.1?
Comment 13 Karolin Seeger 2013-12-04 12:35:26 UTC
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > Comment on attachment 9499 [details] [details] [details]
> > > Patch from master with fixed bug ID, applies to 3.6, 4.0 and 4.1
> > > 
> > > The master commit was reviewed by Andreas and myself. This is the same patch,
> > > with the bugzilla ID fixed.
> > 
> > Ok, thanks, but I need explicit review flags (or comments) in the bug report,
> > not only in the commit message.
> 
> Fair enough :)
> I've requested review from Jeremy and Andreas.

Thanks!

> > To be picky, I need it for each version separately...
> 
> Does that mean you want separate versions of the (same) patch attached for 3.6,
> 4.0 and 4.1?

Some team members would prefer that, yes, but we have never agreed on the team list, so that's not 100% clear.
Comment 14 Jeremy Allison 2013-12-04 18:10:38 UTC
Comment on attachment 9499 [details]
Patch from master with fixed bug ID, applies to 3.6, 4.0 and 4.1

LGTM.
Comment 15 David Disseldorp 2013-12-05 11:40:11 UTC
Created attachment 9505 [details]
Patch from master with fixed bug ID and adjusted path, applies to 3.3
Comment 16 David Disseldorp 2013-12-05 11:40:57 UTC
Created attachment 9506 [details]
Patch from master with fixed bug ID, applies to 3.4
Comment 17 David Disseldorp 2013-12-05 11:41:24 UTC
Created attachment 9507 [details]
Patch from master with fixed bug ID, applies to 3.5
Comment 18 Andrew Bartlett 2013-12-06 18:12:15 UTC
Comment on attachment 9499 [details]
Patch from master with fixed bug ID, applies to 3.6, 4.0 and 4.1

Garming and reproduced and tested this yesterday, and confirm it fixes the issue.
Comment 19 Karolin Seeger 2013-12-09 06:16:35 UTC
Patch already in master.
Pushed to v4-1-test, v4-0-test and v3-6-stable.

Closing out bug report.

Thanks!