diff mbox

[2/4] Make Berkeley DB a dependency of linux-pam and make sure it is selected

Message ID 1347078066-25257-2-git-send-email-golubovsky@gmail.com
State Rejected
Headers show

Commit Message

Dimitry Golubovsky Sept. 8, 2012, 4:21 a.m. UTC
The pam_userdb module is used to verify a username/password pair
against values stored in a Berkeley DB database. The database is
indexed by the username, and the data fields corresponding to the
username keys are the passwords (from man 8 pam_userdb).

Signed-off-by: Dmitry <golubovsky@gmail.com>
---
 package/linux-pam/Config.in    |    1 +
 package/linux-pam/linux-pam.mk |    2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)

Comments

Thomas Petazzoni Sept. 11, 2012, 2:32 p.m. UTC | #1
Le Sat,  8 Sep 2012 00:21:04 -0400,
Dmitry <golubovsky@gmail.com> a écrit :

> -LINUX_PAM_DEPENDENCIES = $(if $(BR2_NEEDS_GETTEXT_IF_LOCALE),gettext libintl) flex
> +LINUX_PAM_DEPENDENCIES = $(if $(BR2_NEEDS_GETTEXT_IF_LOCALE),gettext libintl) flex berkeleydb

Looking at the code, the dependency indeed looks necessary. But isn't
it possible to rework a bit linux-pam to make this dependency optional
(and then submit this upstream)? Of course, if you don't care about
making this dependency optional, then just keep it as it is.

Best regards,

Thomas
Dimitry Golubovsky Sept. 11, 2012, 2:44 p.m. UTC | #2
Thomas,

On Tue, Sep 11, 2012 at 10:32 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Le Sat,  8 Sep 2012 00:21:04 -0400,
> Dmitry <golubovsky@gmail.com> a écrit :
>
>> -LINUX_PAM_DEPENDENCIES = $(if $(BR2_NEEDS_GETTEXT_IF_LOCALE),gettext libintl) flex
>> +LINUX_PAM_DEPENDENCIES = $(if $(BR2_NEEDS_GETTEXT_IF_LOCALE),gettext libintl) flex berkeleydb
>
> Looking at the code, the dependency indeed looks necessary. But isn't
> it possible to rework a bit linux-pam to make this dependency optional
> (and then submit this upstream)? Of course, if you don't care about
> making this dependency optional, then just keep it as it is.

I don't think such change in linux-pam is really needed. I'd keep it
here given the fix is trivial, and BerkeleyDB is not a huge library.

Thanks.
Thomas Petazzoni Sept. 11, 2012, 2:45 p.m. UTC | #3
Le Tue, 11 Sep 2012 10:44:03 -0400,
Dmitry Golubovsky <golubovsky@gmail.com> a écrit :

> > Looking at the code, the dependency indeed looks necessary. But isn't
> > it possible to rework a bit linux-pam to make this dependency optional
> > (and then submit this upstream)? Of course, if you don't care about
> > making this dependency optional, then just keep it as it is.
> 
> I don't think such change in linux-pam is really needed. I'd keep it
> here given the fix is trivial, and BerkeleyDB is not a huge library.

Ok, fine with me.

Thomas
Arnout Vandecappelle Sept. 11, 2012, 7:18 p.m. UTC | #4
On 09/08/12 06:21, Dmitry wrote:
> The pam_userdb module is used to verify a username/password pair
> against values stored in a Berkeley DB database. The database is
> indexed by the username, and the data fields corresponding to the
> username keys are the passwords (from man 8 pam_userdb).

  There is a patch on the list that introduces gdbm.  Do you think that could
be an alternative for berkeleydb?

  Regards,
  Arnout
Dimitry Golubovsky Sept. 11, 2012, 7:52 p.m. UTC | #5
Arnout,

On Tue, Sep 11, 2012 at 3:18 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
> On 09/08/12 06:21, Dmitry wrote:
>>
>> The pam_userdb module is used to verify a username/password pair
>> against values stored in a Berkeley DB database. The database is
>> indexed by the username, and the data fields corresponding to the
>> username keys are the passwords (from man 8 pam_userdb).
>
>
>  There is a patch on the list that introduces gdbm.  Do you think that could
> be an alternative for berkeleydb?

I'm not sure. We'll need a choice of configurations then, whether to
use gdbm or berkeleyDB for linux-pam. What if both are selected in
menuconfig?

BerkeleyDB is much more advanced than GDBM, IMHO. One might prefer
GDBM over Berkeley DB because of license perhaps, but otherwise why?

If we want to implement it further, here's the possible logic:

PAM's configure.in shows that there may be variants in --enable-db
options. which means the logic would be:

1. See if Berkeley or GDBM is selected. If none, disable pam_userdb in
PAM --enable-db=no
2. If Berkeley is selected, enable dbm in it and specify proper option
in --enable-db=ndbm (I guess)
3. If GDBM is selected, specify proper option in --enable-db=db (again
I guess, maybe other way around)
4. If both are selected? there is an option --enable-db=yes which lets
PAM decide by itself, then dbm should be enabled in BerkeleyDB anyway
(it seems like configure only looks at headers*)

Does anybody disagree with such logic?

Plus, the database selected for PAM should become a PAM dependency.
Which we cannot tell unless we have a configure choice for the
database in PAM, but this turns the above logic upside down: PAM now
drives the database selection.

Or we can make both databases, if selected, dependencies of PAM - at
worst this only affects the building order.

And what is the status of the GDBM patch?

Thanks.

------------------------
* as in the failed autobuilder case, PAM's configure decided to build
pam_userdb if BerkeleyDB was selected (headers were present) even
though dbm was not enabled in it - linkage test was not done.
Arnout Vandecappelle Sept. 12, 2012, 5:31 a.m. UTC | #6
On 09/11/12 21:52, Dmitry Golubovsky wrote:
> Arnout,
>
> On Tue, Sep 11, 2012 at 3:18 PM, Arnout Vandecappelle<arnout@mind.be>  wrote:
>> On 09/08/12 06:21, Dmitry wrote:
>>>
>>> The pam_userdb module is used to verify a username/password pair
>>> against values stored in a Berkeley DB database. The database is
>>> indexed by the username, and the data fields corresponding to the
>>> username keys are the passwords (from man 8 pam_userdb).
>>
>>
>>   There is a patch on the list that introduces gdbm.  Do you think that could
>> be an alternative for berkeleydb?
>
> I'm not sure. We'll need a choice of configurations then, whether to
> use gdbm or berkeleyDB for linux-pam. What if both are selected in
> menuconfig?

  I meant: maybe that's a simpler dependency than a specially-configured
berkeleydb.


> BerkeleyDB is much more advanced than GDBM, IMHO. One might prefer
> GDBM over Berkeley DB because of license perhaps, but otherwise why?
>
> If we want to implement it further, here's the possible logic:
>
> PAM's configure.in shows that there may be variants in --enable-db
> options. which means the logic would be:
>
> 1. See if Berkeley or GDBM is selected. If none, disable pam_userdb in
> PAM --enable-db=no
> 2. If Berkeley is selected, enable dbm in it and specify proper option
> in --enable-db=ndbm (I guess)
> 3. If GDBM is selected, specify proper option in --enable-db=db (again
> I guess, maybe other way around)
> 4. If both are selected? there is an option --enable-db=yes which lets
> PAM decide by itself, then dbm should be enabled in BerkeleyDB anyway
> (it seems like configure only looks at headers*)
>
> Does anybody disagree with such logic?

  Looks good.  Although there should probably be a third config symbol
to simplify the logic - but we don't even have that yet for xml2/expat.


> Plus, the database selected for PAM should become a PAM dependency.
> Which we cannot tell unless we have a configure choice for the
> database in PAM, but this turns the above logic upside down: PAM now
> drives the database selection.
>
> Or we can make both databases, if selected, dependencies of PAM - at
> worst this only affects the building order.

  Yep, that's the way to do it.


> And what is the status of the GDBM patch?

  It's a bit hidden in the perl series, but I just acked it.

>
> Thanks.
>
> ------------------------
> * as in the failed autobuilder case, PAM's configure decided to build
> pam_userdb if BerkeleyDB was selected (headers were present) even
> though dbm was not enabled in it - linkage test was not done.

  Since these patches are still under review, maybe it's best to first
commit an individual patch that just does --disable-db ?  Can you do
that and check if it fixes the autobuilder issue?

  Regards,
  Arnout
Dimitry Golubovsky Sept. 12, 2012, 9:30 a.m. UTC | #7
Arnout,

On Wed, Sep 12, 2012 at 1:31 AM, Arnout Vandecappelle <arnout@mind.be> wrote:

>
>  Since these patches are still under review, maybe it's best to first
> commit an individual patch that just does --disable-db ?  Can you do
> that and check if it fixes the autobuilder issue?
>

I have just submitted such patch. Let's see how autobuilder results
change. In my test linux-pam compiled without pam_userdb.so, and
berkeleydb was selected in menuconfig.

Thanks.
diff mbox

Patch

diff --git a/package/linux-pam/Config.in b/package/linux-pam/Config.in
index 722b875..65e7b69 100644
--- a/package/linux-pam/Config.in
+++ b/package/linux-pam/Config.in
@@ -4,6 +4,7 @@  config BR2_PACKAGE_LINUX_PAM
 	select BR2_PACKAGE_GETTEXT if BR2_NEEDS_GETTEXT_IF_LOCALE
 	select BR2_PACKAGE_FLEX
 	select BR2_PACKAGE_FLEX_LIBFL
+	select BR2_PACKAGE_BERKELEYDB
 	depends on (BR2_ENABLE_LOCALE && BR2_USE_WCHAR)
 	help
 	  A Security Framework that Provides Authentication for Applications
diff --git a/package/linux-pam/linux-pam.mk b/package/linux-pam/linux-pam.mk
index 48cb073..5e0ecf0 100644
--- a/package/linux-pam/linux-pam.mk
+++ b/package/linux-pam/linux-pam.mk
@@ -15,7 +15,7 @@  LINUX_PAM_CONF_OPT = \
 	--disable-regenerate-docu \
 	--enable-securedir=/lib/security \
 	--libdir=/lib
-LINUX_PAM_DEPENDENCIES = $(if $(BR2_NEEDS_GETTEXT_IF_LOCALE),gettext libintl) flex
+LINUX_PAM_DEPENDENCIES = $(if $(BR2_NEEDS_GETTEXT_IF_LOCALE),gettext libintl) flex berkeleydb
 LINUX_PAM_AUTORECONF = YES
 LINUX_PAM_LICENSE = BSD-3c
 LINUX_PAM_LICENSE_FILES = Copyright