Bug 195759 - x11/slim: Fails to honour login.conf
Summary: x11/slim: Fails to honour login.conf
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-ports-bugs (Nobody)
URL:
Keywords: needs-qa, patch, patch-ready
Depends on:
Blocks:
 
Reported: 2014-12-06 20:16 UTC by Jonathan Chen
Modified: 2015-02-22 14:52 UTC (History)
3 users (show)

See Also:
koobs: maintainer-feedback+


Attachments
use login capabilities database to set up environment (3.68 KB, patch)
2014-12-16 03:23 UTC, Henry Hu
no flags Details | Diff
2nd ver (5.84 KB, patch)
2014-12-18 05:23 UTC, Henry Hu
henry.hu.sh: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Chen 2014-12-06 20:16:13 UTC
I have the set lang within /etc/login.conf:

default:\
    :lang=en_NZ.UTF-8:\
    :passwd_format=sha512:\
    ...

I've "cap_mkdb /etc/login.conf".

In my ~/.xinitrc:
  env > /tmp/j-env
  exec mate-session > .xsession-errors 2>&1

The contents of /tmp/j-env after logging in:
  VENDOR=amd
  LOGNAME=jonc
  OSTYPE=FreeBSD
  MACHTYPE=x86_64
  XAUTHORITY=/home/jonc/.Xauthority
  MAIL=/var/mail/jonc
  PATH=/sbin:/bin:/usr/sbin:/usr/bin:/usr/games:/usr/local/sbin:/usr/local/bin:$HOME/bin
  HOST=osiris.chen.org.nz
  DISPLAY=:0.0
  PWD=/home/jonc
  GROUP=core
  USER=jonc
  HOME=/home/jonc
  HOSTTYPE=FreeBSD
  SHELL=/bin/tcsh
  XDG_SESSION_COOKIE=8df97606318ac9f41ded527c53c12017-1417896465.683295-2112864763

2 things stand out:
 1. LANG is not set.
 2. PATH has literal and not evaluated "$HOME/bin" appended.

The options on my x11/slim:
  PAM : on
  UTF8 : off
Comment 1 Bugzilla Automation freebsd_committer 2014-12-06 20:16:13 UTC
Maintainer CC'd
Comment 2 Henry Hu 2014-12-14 02:07:40 UTC
I set LANG in .cshrc which works well. Because login.conf is FreeBSD-specific, slim is not reading from it, and I need to find out which program is in charge of reading from that.
For $HOME in PATH, I guess that's caused by the new default slim configuration file, which we copied the path setting from default login.conf, but slim does not resolve variables in its settings. I think that I can just remove the $HOME/bin from the default path, and if I find out which program should set the environment according to login.conf, this should also be handled by that program.
Comment 3 Jonathan Chen 2014-12-14 02:48:23 UTC
I had a quick look, and I believe that patch-switchuser.cpp should make use of setusercontext() in login_class(3); instead of just setlogin(2).
Comment 4 Henry Hu 2014-12-16 03:21:31 UTC
(In reply to Jonathan Chen from comment #3)
> I had a quick look, and I believe that patch-switchuser.cpp should make use
> of setusercontext() in login_class(3); instead of just setlogin(2).

Nice catch. It seems like that we can replace multiple calls with a single setusercontext():
setlogin -> LOGIN_SETLOGIN
initgroups, setgid -> LOGIN_SETGROUP
setuid -> LOGIN_SETUSER

and other problems are solved as well:
path -> LOGIN_SETPATH
lang -> LOGIN_SETENV

seems like that we should just use LOGIN_SETALL.

To get the login_cap_t, currently I'm using login_getpwclass(). I should use login_getuserclass() at the same time, and query the later one first, and turn to the first one if something is not present in the later one. However, setusercontext() does not support this, so just use login_getpwclass() for now.

I've created a patch for this. Please test it. I'm not sure if all these APIs are present on older systems.
Comment 5 Henry Hu 2014-12-16 03:23:01 UTC
Created attachment 150621 [details]
use login capabilities database to set up environment

It uses login_getpwclass() and setusercontext(), and falls back to the old method if that fails.
Comment 6 Jonathan Chen 2014-12-16 07:53:15 UTC
Unfortunately, the patch didn't work. env is:

VENDOR=amd
LOGNAME=jonc
OSTYPE=FreeBSD
MACHTYPE=x86_64
XAUTHORITY=/home/jonc/.Xauthority
MAIL=/var/mail/jonc
PATH=/sbin:/bin:/usr/sbin:/usr/bin:/usr/games:/usr/local/sbin:/usr/local/bin
HOST=osiris.chen.org.nz
DISPLAY=:0.0
PWD=/home/jonc
GROUP=core
USER=jonc
HOME=/home/jonc
HOSTTYPE=FreeBSD
SHELL=/bin/tcsh

The logs are too sparse to make anything out. Perhaps add more log information to see where it's failing?
Comment 7 Henry Hu 2014-12-16 16:25:58 UTC
(In reply to Jonathan Chen from comment #6)
> Unfortunately, the patch didn't work. env is:
> 
> VENDOR=amd
> LOGNAME=jonc
> OSTYPE=FreeBSD
> MACHTYPE=x86_64
> XAUTHORITY=/home/jonc/.Xauthority
> MAIL=/var/mail/jonc
> PATH=/sbin:/bin:/usr/sbin:/usr/bin:/usr/games:/usr/local/sbin:/usr/local/bin
> HOST=osiris.chen.org.nz
> DISPLAY=:0.0
> PWD=/home/jonc
> GROUP=core
> USER=jonc
> HOME=/home/jonc
> HOSTTYPE=FreeBSD
> SHELL=/bin/tcsh
> 
> The logs are too sparse to make anything out. Perhaps add more log
> information to see where it's failing?

Have you seen anything strange in /var/log/slim.log?
Comment 8 Jonathan Chen 2014-12-16 17:13:55 UTC
No. It's a pretty sparse file. Maybe put more logging in the patch to see what's failing and why?
Comment 9 Henry Hu 2014-12-18 04:30:45 UTC
Ok, I know why.

In app.cpp, slim creates child_env, and uses it as the environment of the child.
If PAM is not used, the array child_env is manually created and filled with predefined entries. Else, the array is obtained from the pam module.

So it seems hard to support all the environment variables that setusercontext() supports. Which environment variables do you really need? LANG and PATH and anything else?

setusercontext() supports these env vars:
                          path          PATH
                          manpath       MANPATH
                          lang          LANG
                          charset       MM_CHARSET
                          timezone      TZ
                          term          TERM
For PATH, because there is an option called default_path in slim's configuration file, maybe we should just ignore the login.conf.
For LANG, we can get the entry "lang" from login database and set it.
For TERM, slim sets it from its own version. Anyway, we're using slim so it's not that important.
For other variables(TZ, MM_CHARSET, MANPATH), we can do it in a similar way.

But it might be still better to use setusercontext() because it sets resource limits, priorities, umask and other things.
Comment 10 Henry Hu 2014-12-18 05:23:40 UTC
Created attachment 150706 [details]
2nd ver

Set the environment variables specified in login.conf.
setusercontext() is still called, because it sets other limitations.
Comment 11 Jonathan Chen 2014-12-18 05:52:54 UTC
It looks good. I checked the environment vars, umask, as well as resource limits.
Comment 12 Jonathan Chen 2014-12-18 17:17:52 UTC
I have noticed that about 15% of the time, my login will fail; and a retry is required. The logs do not indicate any authentication errors.
Comment 13 Henry Hu 2014-12-19 01:01:34 UTC
(In reply to Jonathan Chen from comment #12)
> I have noticed that about 15% of the time, my login will fail; and a retry
> is required. The logs do not indicate any authentication errors.

Does it happen without the patch?
Comment 14 Jonathan Chen 2014-12-19 01:05:39 UTC
Authentication is always successful with the patch (barring fumble fingers).
Comment 15 John Marino freebsd_committer 2015-02-06 17:22:34 UTC
With a quick scan of the comments, I believe the maintainer has come up with a patch that should be committed, so let's move this PR to "patch-ready" status.
Comment 16 Henry Hu 2015-02-07 03:39:55 UTC
(In reply to John Marino from comment #15)
According to user's report, the patch works. After reading setusercontext()'s code, it seems like that .login.conf is also honored.
I think that the patch needs security auditing, because it's a patch to a login manager, but I'm not sure who is responsible for this.
Comment 17 John Marino freebsd_committer 2015-02-07 09:04:28 UTC
I don't know either.  We don't have a "login manager" guru position identified.
Comment 18 Kubilay Kocak freebsd_committer freebsd_triage 2015-02-16 06:55:54 UTC
Thanks for the update Jonathan, if could include successful build output as an attachment, that will help this issue progress.

Highly preferred is:

- poudriere testport or bulk -t ouput

For instructions on how to set this up, see: https://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/testing-poudriere.html

OR

- make stage && make check-plist && make stage-qa && make package
Comment 19 Kurt Jaeger freebsd_committer 2015-02-22 14:09:49 UTC
poudriere build-testing@work
Comment 20 commit-hook freebsd_committer 2015-02-22 14:50:58 UTC
A commit references this bug:

Author: pi
Date: Sun Feb 22 14:50:27 UTC 2015
New revision: 379608
URL: https://svnweb.freebsd.org/changeset/ports/379608

Log:
  x11/slim: fix to honour login.conf, WWW

  PR:		195759
  Requested by:	Jonathan Chen <jonc@chen.org.nz>
  Approved by:	Henry Hu <henry.hu.sh@gmail.com> (maintainer)

Changes:
  head/x11/slim/Makefile
  head/x11/slim/files/patch-CMakeLists.txt
  head/x11/slim/files/patch-app.cpp
  head/x11/slim/files/patch-slim.conf
  head/x11/slim/files/patch-switchuser.cpp
  head/x11/slim/pkg-descr
Comment 21 Kurt Jaeger freebsd_committer 2015-02-22 14:52:23 UTC
build-test went fine on 10.1a, 9.3a, 8.4i, so: committed, thanks all!