On Fri, Sep 13, Dmitry V. Levin wrote:
I'm not sure I understand the intent here.
Does it make sense to enable econf (by default) when vendordir is not enabled?
Does it make sens to enable vendordir when econf is not available?
It seems to me that these two have to be either both enabled or both
disabled, please correct me if I got it wrong.
It makes sense to enable vendordir even if you don't have econf.
econf is only used to read login.defs, but for an alternate pam.d
directory or securetty file, it's not used.
> --- a/doc/man/pam.8.xml
> +++ b/doc/man/pam.8.xml
> @@ -53,11 +53,12 @@
>
> <para>
> Vendor-supplied PAM configuration files might be installed in
> - the system directory <filename>/usr/lib/pam.d/</filename>
instead
> + the system directory <filename>/usr/lib/pam.d/</filename> or
> If no machine configuration file is found, the vendor-supplied file
> is used. All files in <filename>/etc/pam.d/</filename> override
> - files with the same name in
<filename>/usr/lib/pam.d/</filename>.
> + files with the same name in other directories.
> </para>
>
> <para>From the point of view of the system administrator, for whom this
> @@ -157,6 +158,18 @@ closing hook for modules to affect the services available to a
user.</para>
> </para>
> </listitem>
> </varlistentry>
> + <varlistentry>
> + <term><filename>%vendordir%/pam.d</filename></term>
This line adds spaces before tabs, please correct this.
> --- a/libpam/Makefile.am
> +++ b/libpam/Makefile.am
> @@ -3,10 +3,15 @@
> #
>
> AM_CFLAGS = -DDEFAULT_MODULE_PATH=\"$(SECUREDIR)/\" -DLIBPAM_COMPILE \
> - -I$(srcdir)/include $(LIBPRELUDE_CFLAGS) -DPAM_VERSION=\"$(VERSION)\"
> + -I$(srcdir)/include $(LIBPRELUDE_CFLAGS) \
> + -DPAM_VERSION=\"$(VERSION)\" $(ECONF_CFLAGS) \
I suppose ECONF_CFLAGS will never define PAM_VERSION, but let's place
$(ECONF_CFLAGS) right after $(LIBPRELUDE_CFLAGS) anyway.
> + -DSYSCONFDIR=\"$(sysconfdir)\"
> if HAVE_LIBSELINUX
> AM_CFLAGS += -D"WITH_SELINUX"
> endif
> +if HAVE_VENDORDIR
> + AM_CFLAGS += -DVENDORDIR=\"@VENDORDIR@\"
> +endif
Could you use $(VENDORDIR) instead of @VENDORDIR@ here, please?
> CLEANFILES = *~
>
> @@ -21,7 +26,7 @@ noinst_HEADERS = pam_prelude.h pam_private.h pam_tokens.h \
> pam_modutil_private.h
>
> libpam_la_LDFLAGS = -no-undefined -version-info 84:2:84
> -libpam_la_LIBADD = @LIBAUDIT@ $(LIBPRELUDE_LIBS) @LIBDL@
> +libpam_la_LIBADD = @LIBAUDIT@ $(LIBPRELUDE_LIBS) @LIBDL@ @ECONF_LIBS@
Could you use $(ECONF_LIBS) instead of @ECONF_LIBS@ here, please?
Also, let it be placed right after $(LIBPRELUDE_LIBS).
> --- a/libpam/pam_handlers.c
> +++ b/libpam/pam_handlers.c
> @@ -282,7 +282,6 @@ _pam_open_config_file(pam_handle_t *pamh
> {
> char *p;
> FILE *f;
> - int err = 0;
>
> /* Absolute path */
> if (service[0] == '/') {
> @@ -329,6 +328,23 @@ _pam_open_config_file(pam_handle_t *pamh
> *file = f;
> return PAM_SUCCESS;
> }
> +
> +#ifdef PAM_CONFIG_DIST2_D
> + /* System Configuration /<vendordir>/pam.d/ */
> + _pam_drop(p);
> + if (asprintf (&p, PAM_CONFIG_DIST2_DF, service) < 0) {
> + pam_syslog(pamh, LOG_CRIT, "asprintf failed");
> + return PAM_BUF_ERR;
> + }
> + D(("opening %s", p));
> + f = fopen(p, "r");
> + if (f != NULL) {
> + *path = p;
> + *file = f;
> + return PAM_SUCCESS;
> + }
> +#endif
> +
> _pam_drop(p);
>
> return PAM_ABORT;
This creates a third copy of the same code.
Could you turn it into a loop, please?
> --- a/libpam/pam_modutil_searchkey.c
> +++ b/libpam/pam_modutil_searchkey.c
> @@ -13,9 +13,39 @@
> #include <string.h>
> #include <stdlib.h>
> #include <ctype.h>
> +#ifdef USE_ECONF
> +#include <libeconf.h>
> +#endif
>
> #define BUF_SIZE 8192
>
> +#ifdef USE_ECONF
> +#define LOGIN_DEFS "/etc/login.defs"
> +
> +#ifndef VENDORDIR
> +#define VENDORDIR NULL
> +#endif
> +
> +static char *
> +econf_search_key (const char *name, const char *suffix, const char *key)
> +{
> + econf_file *key_file = NULL;
> + char *val;
> +
> + if (econf_readDirs (&key_file, VENDORDIR, SYSCONFDIR, name, suffix,
> + " \t", "#"))
> + return NULL;
> +
> + if (econf_getStringValue (key_file, NULL, key, &val))
> + return NULL;
> +
> + econf_free (key_file);
I'm not familiar with econf API, but this looks like a key_file leak.
No, clearly not.
> --- a/modules/pam_securetty/Makefile.am
> +++ b/modules/pam_securetty/Makefile.am
> @@ -20,6 +20,9 @@ AM_LDFLAGS = -no-undefined -avoid-version -module
> if HAVE_VERSIONING
> AM_LDFLAGS += -Wl,--version-script=$(srcdir)/../modules.map
> endif
> +if HAVE_VENDORDIR
> + AM_CFLAGS += -DVENDORDIR=\"@VENDORDIR@\"
> +endif
Could you use $(VENDORDIR) instead of @VENDORDIR@ here, ...
> securelib_LTLIBRARIES = pam_securetty.la
> pam_securetty_la_LIBADD = $(top_builddir)/libpam/libpam.la
> @@ -27,5 +30,10 @@ pam_securetty_la_LIBADD = $(top_builddir)/libpam/libpam.la
> if ENABLE_REGENERATE_MAN
> noinst_DATA = README
> README: pam_securetty.8.xml
> +if HAVE_VENDORDIR
> +XSLTPROC_CUSTOM = --stringparam vendordir $(VENDORDIR)
> +else
> +XSLTPROC_CUSTOM = --stringparam vendordir "<vendordir>"
> +endif
... like you already did here, please?
> --- a/modules/pam_securetty/pam_securetty.c
> +++ b/modules/pam_securetty/pam_securetty.c
> @@ -1,6 +1,9 @@
> /* pam_securetty module */
>
> #define SECURETTY_FILE "/etc/securetty"
> +#ifdef VENDORDIR
> +#define SECURETTY2_FILE VENDORDIR"/securetty"
> +#endif
> #define TTY_PREFIX "/dev/"
> #define CMDLINE_FILE "/proc/cmdline"
> #define CONSOLEACTIVE_FILE "/sys/class/tty/console/active"
> @@ -25,6 +28,7 @@
> #include <string.h>
> #include <ctype.h>
> #include <limits.h>
> +#include <errno.h>
>
> /*
> * here, we make a definition for the externally accessible function
> @@ -70,6 +74,7 @@ securetty_perform_check (pam_handle_t *pamh, int ctrl,
> const char *function_name)
> {
> int retval = PAM_AUTH_ERR;
> + const char *securettyfile;
> const char *username;
> const char *uttyname;
> const void *void_uttyname;
> @@ -111,10 +116,27 @@ securetty_perform_check (pam_handle_t *pamh, int ctrl,
> }
>
> if (stat(SECURETTY_FILE, &ttyfileinfo)) {
> +#ifdef VENDORDIR
> + if (errno == ENOENT) {
> + if (stat(SECURETTY2_FILE, &ttyfileinfo)) {
> + pam_syslog(pamh, LOG_NOTICE,
> + "Couldn't open %s: %m", SECURETTY2_FILE);
> + return PAM_SUCCESS; /* for compatibility with old securetty handling,
> + this needs to succeed. But we still log the
> + error. */
I wonder which of two files should be mentioned in the log message here.
If you use vendordir, the one in the vendor directory is mandatory,
the second one is optional.
Thorsten
--
Thorsten Kukuk, Distinguished Engineer, Senior Architect SLES & MicroOS
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nuernberg, Germany
Managing Director: Felix Imendoerffer (HRB 247165, AG München)