On Fri, 2017-05-26 at 17:17 +0300, Dmitry V. Levin wrote:
On Mon, May 22, 2017 at 01:24:13PM +0200, Tomas Mraz wrote:
> The attached patch adds to pam_access parsing files in
> /etc/security/access.d/*.conf after the main access.conf file.
>
> The files are parsed only in case the default
> /etc/security/access.conf
> file is used.
[...]
> --- a/modules/pam_access/pam_access.8.xml
> +++ b/modules/pam_access/pam_access.8.xml
[...]
> + If a config file is explicitly specified with the
> <option>accessfile</option>
> + option the files in the above directory are not parsed.
[...]
> --- a/modules/pam_access/pam_access.c
> +++ b/modules/pam_access/pam_access.c
[...]
> + if (rv == NOMATCH && strcmp(loginfo.config_file,
> PAM_ACCESS_CONFIG) != 0) {
The check should compare pointers instead of strings to match
the documentation.
I'll fix that.
> --- a/modules/pam_access/pam_access.c
> +++ b/modules/pam_access/pam_access.c
[...]
> @@ -105,6 +107,7 @@ struct login_info {
> int from_remote_host; /* If PAM_RHOST was used
> for from */
> struct addrinfo *res; /* Cached DNS resolution
> of from */
> int gai_rv; /* Cached retval of
> getaddrinfo */
> + int subfile; /* File from access.d
> subdirectory */
> };
>
> /* Parse module config arguments */
> @@ -400,7 +403,7 @@ login_access (pam_handle_t *pamh, struct
> login_info *item)
> }
> }
> (void) fclose(fp);
> - } else if (errno == ENOENT) {
> + } else if (errno == ENOENT || item->subfile) {
> /* This is no error. */
Why errors opening files in access.d subdirectory are treated
differently?
As there is higher potential for administrator mistake putting a file
in the subdirectory with incorrect access rights or SELinux context I
wanted to make the errors opening them non-fatal so that only error
message is logged but the sysadmin is not prevented from logging in
again to correct the mistake. Do you think this different handling is
not needed?
[...]
> @@ -892,6 +899,24 @@ pam_sm_authenticate (pam_handle_t *pamh, int
> flags UNUSED,
>
> rv = login_access(pamh, &loginfo);
>
> + if (rv == NOMATCH && strcmp(loginfo.config_file,
> PAM_ACCESS_CONFIG) != 0) {
> + glob_t globbuf;
> + int i, glob_rv;
> +
> + loginfo.subfile = 1;
> + glob_rv = glob(ACCESS_CONF_GLOB, GLOB_ERR, NULL,
> &globbuf);
> + if (!glob_rv) {
In two other places where we use glob(3) there is a dance with
LC_COLLATE,
we might want to repeat it here as well.
Unfortunately manipulating locale is not thread safe and pam_access can
be in auth which we attempt to keep thread safe. Perhaps I should add
comment here? Also I wrote that in the documentation that system locale
is used.
--
Tomas Mraz
No matter how far down the wrong road you've gone, turn back.
Turkish proverb
(You'll never know whether the road is wrong though.)