On Sun, 2021-11-14 at 14:08 +0100, vifino wrote:
> On Wed Nov 10, 2021 at 9:46 AM CET, Martijn van Duren wrote:
> > On Fri, 2021-10-15 at 06:13 +0000, Klemens Nanni wrote:
> > > On Sun, Oct 03, 2021 at 10:05:56AM +0000, Klemens Nanni wrote:
> > > > On Sat, Oct 02, 2021 at 07:03:21PM +0200, vifino wrote:
> > > > > On Sat Oct 2, 2021 at 6:36 PM CEST, Raf Czlonka wrote:
> > > > > > On Sat, Oct 02, 2021 at 02:15:53PM BST, vifino wrote:
> > > > > > > Index: ldapd.conf.5
> > > > > > > ===================================================================
> > > > > > > RCS file: /cvs/src/usr.sbin/ldapd/ldapd.conf.5,v
> > > > > > > retrieving revision 1.27
> > > > > > > diff -u -p -u -p -r1.27 ldapd.conf.5
> > > > > > > --- ldapd.conf.5 24 Jun 2020 07:20:47 -0000 1.27
> > > > > > > +++ ldapd.conf.5 2 Oct 2021 12:43:29 -0000
> > > > > > > @@ -270,7 +270,7 @@ Finally, the filter rule can match a bin
> > > > > > > The filter rule matches by any bind dn, including anonymous
> > > > > > > binds.
> > > > > > > .It by Ar DN
> > > > > > > The filter rule matches only if the requestor has previously
> > > > > > > performed
> > > > > > > -a bind as the specified distinguished name.
> > > > > > > +a bind as the specified distinguished name or a decendant.
> > > > > > ^^^^^^^^^
> > > > > > A spellchecker[0] would have caught that ;^)
> > > > > Ah, yes, of course. The one thing I spent zero effort on.
> > > > > I haven't quite grokked the workflow, first submitted patch and all.
> > > > > I'll certainly run `spell` next time.
> > > > >
> > > > > I'll keep this in mind for the next one. ;)
> > > > > >
> > > > > > [0] https://manpages.bsd.lv/part3-3-2.html
> > > > > >
> > > > > > Regards,
> > > > > >
> > > > > > Raf
> > > > >
> > > > > Revised patch below, not that it's necessary.
> > > > > - vifino
> > > >
> > > > The patch doesn't apply (for me) as your mail is quoted, here's your
> > > > diff in 7bit.
> > > >
> > > > Makes sense and works as expected.
> > > > OK kn if some other LDAP hacker wants to commit, otherwise I'd make sure
> > > > that this lands unless there are objections.
> > >
> > > Any takers? I plan to commit this by the end of the weekend.
> >
> > Sorry for the delay.
> >
> > The code looks good to me, however there is one edge case I think
> > doesn't occur too often in the wild, but could bite people in the
> > rear if they deploy this construct:
> >
> > Things like an organization or organizationalUnit MAY contain
> > userPassword, which implies that people might login as that particular
> > DN, which in turn might have something like posixAccount entries below
> > it. The problem now becomes that the posixAccounts get the same
> > permissions the organizationalUnit.
> Hey.
> On my first thoughts, I considered that a quite weird use of LDAP.
> After all, having seperate user accounts seems - to me - to be the point.
> Why would you bind as that?
>
> I could only think of one usecase: To allow creation of new sub-DN accounts
> from an organizational admin, limited in it's power.
> However, this could probably be solved more cleanly by a seperate rule,
> which would allow more than one admin. (Without sharing passwords.. yuck!)
>
> In my opinion, any organization which is large enough to have seperate
> admins for sub-DNs is also large enough to potentially need more than
> one account for it.
*shrugs* I don't know. But it's more than possible within the LDAP data
structures and so we should assume that people are using these features
or might be using them in the future.
It's unlikely that I would set up a setup like that, but I've been
surprised by people's setups too often that my "best practice"
understanding has been completely thrown out the window.
>
> I would love hear of a situation where this edge case gets triggered!
But if I were to let my imagination go wild: maybe it's the binddn
used by login_ldap to search for accounts.
>
> Plus, there is a simple enough workaround:
> Have more than one rule, one with a comma before the filter DN.
> It'll only match sub-DNs.
Sure, you could add a identical deny-rule with the comma after an
allow-rule, but that's creating an unintuitive pitfall. Especially
considering this will impact existing rulesets.
And now consider the semi-public account used above for the searching
of the account in login_ldap, I wouldn't want the security of my db
lying in the hands of a single leading comma...
>
> >
> > Maybe something like "by filter" would be a better fit, since it would
> > allow for an even wider functionality.
>
> I also considered this. However, I had no immediate use for something as
> fine grained as this.
> Plus, the simplicity seemed preferrable.
>
> - vifino
I agree that simplicity is preferable, but not at the expanse of
unexpected edgecases. And maybe a filter line is too complicated, but
why not add a final scope keyword (base|sub|one) and default to base?
If we use sub in the same way as filters are used it might still not be
what you want directly, but it's a lot more explicit and consistent with
existing syntax than having to work with a single comma.
Maybe I'm being too pedantic, but it's something that stands out to me
and just doesn't sit well with me.
>
> >
> > martijn@
> > >
> > >
> > > Index: auth.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.sbin/ldapd/auth.c,v
> > > retrieving revision 1.14
> > > diff -u -p -r1.14 auth.c
> > > --- auth.c 24 Oct 2019 12:39:26 -0000 1.14
> > > +++ auth.c 3 Oct 2021 09:25:10 -0000
> > > @@ -94,8 +94,13 @@ aci_matches(struct aci *aci, struct conn
> > > if (strcmp(aci->subject, "@") == 0) {
> > > if (strcmp(dn, conn->binddn) != 0)
> > > return 0;
> > > - } else if (strcmp(aci->subject, conn->binddn) != 0)
> > > - return 0;
> > > + } else {
> > > + key.size = strlen(conn->binddn);
> > > + key.data = conn->binddn;
> > > +
> > > + if (!has_suffix(&key, aci->subject))
> > > + return 0;
> > > + }
> > > }
> > >
> > > if (aci->attribute != NULL) {
> > > Index: ldapd.conf.5
> > > ===================================================================
> > > RCS file: /cvs/src/usr.sbin/ldapd/ldapd.conf.5,v
> > > retrieving revision 1.27
> > > diff -u -p -r1.27 ldapd.conf.5
> > > --- ldapd.conf.5 24 Jun 2020 07:20:47 -0000 1.27
> > > +++ ldapd.conf.5 3 Oct 2021 09:22:34 -0000
> > > @@ -270,7 +270,7 @@ Finally, the filter rule can match a bin
> > > The filter rule matches by any bind dn, including anonymous binds.
> > > .It by Ar DN
> > > The filter rule matches only if the requestor has previously performed
> > > -a bind as the specified distinguished name.
> > > +a bind as the specified distinguished name or a descendant.
> > > .It by self
> > > The filter rule matches only if the requestor has previously performed
> > > a bind as the distinguished name that is being requested.
> > >
>
