Bug 266532 - x11/lightdm: Login sessions do not respect login.conf
Summary: x11/lightdm: Login sessions do not respect 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 Only Me
Assignee: freebsd-desktop (Team)
URL:
Keywords: needs-patch, needs-qa
Depends on:
Blocks: 273720 275885
  Show dependency treegraph
 
Reported: 2022-09-21 12:01 UTC by Daniel O'Connor
Modified: 2023-12-29 18:28 UTC (History)
8 users (show)

See Also:
madpilot: maintainer-feedback+


Attachments
pre logon (807 bytes, application/x-shellscript)
2023-12-22 15:28 UTC, Ivan Rozhuk
no flags Details
post logon (629 bytes, application/x-shellscript)
2023-12-22 15:29 UTC, Ivan Rozhuk
no flags Details
patch v1 (19.04 KB, patch)
2023-12-23 13:07 UTC, Guido Falsi
no flags Details | Diff
patch v2 (19.05 KB, patch)
2023-12-24 11:34 UTC, Guido Falsi
no flags Details | Diff
patch v3 (19.05 KB, patch)
2023-12-24 17:13 UTC, Guido Falsi
no flags Details | Diff
patch v3.1 (19.37 KB, patch)
2023-12-24 18:59 UTC, Guido Falsi
no flags Details | Diff
patch v3.2 (19.39 KB, patch)
2023-12-27 09:12 UTC, Guido Falsi
no flags Details | Diff
patch (1.37 KB, patch)
2023-12-28 04:11 UTC, Ivan Rozhuk
no flags Details | Diff
patch v4 (19.65 KB, patch)
2023-12-28 23:48 UTC, Guido Falsi
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel O'Connor 2022-09-21 12:01:04 UTC
I have created a ~/.login.conf to set LANG but when I login via lightdm it has no effect (works on SSH though).

I think this could be fixed by patching lightdm to add a call to setusercontext(), ie graft the code from xdm: https://github.com/freedesktop/xorg-xdm/blob/master/xdm/session.c#L669

into session-child.c around line 540:
https://github.com/canonical/lightdm/blob/653809c387c0a4e4d96f7999db3106d89970d4c7/src/session-child.c#L540

I plan on trying it soon but would appreciate some feedback since it's security related.
Comment 1 Guido Falsi freebsd_committer freebsd_triage 2022-09-23 06:38:10 UTC
Hi!

Thanks for reporting this.

I'm unable to properly review your proposal, but since this is not FreeBSD specific I think you really should create the patch you propose and submit it upstream.

The lightdm guys are the most suitable people to review your proposal.

Have you tried implementing this and running it locally? Does it work for you?
Comment 2 Daniel O'Connor 2022-09-23 07:13:53 UTC
I did try it but was not successful, it was quite odd - I added debugging and I could see it set the variables etc but for some reason it did not propagate to the user session.

I'll try filing a lightdm bug - thanks.
Comment 3 Guido Falsi freebsd_committer freebsd_triage 2022-09-23 07:24:50 UTC
(In reply to Daniel O'Connor from comment #2)

Looking at it, I suspect the line you suggest for adding the call to setusercontext() is actually running elevated, not in the context of the user being logged in, so that environment is discarded when the session is actually started dropping to user privilege.

This is just a quick guess, not an educated comment though.
Comment 4 Daniel O'Connor 2022-09-23 08:43:51 UTC
I just figured out why..
When session_child_run calls execve it passes in the env parameter with the list generated by PAM (pam_getenvlist). This overwrites the environmental variables set by setusercontext.

My work around is to reset the environment to pam_getenvlist() just before setusercontext is called.

Here is my patch so far (need to set HAVE_SETUSERCONTEXT to 1):
https://gist.github.com/DanielO/178a3131cc1ae646e703d59e5d9c712d
Comment 5 Daniel O'Connor 2022-09-23 09:26:37 UTC
I filed a lightdm issue: https://github.com/canonical/lightdm/issues/269
Comment 6 Olivier Duchateau 2022-09-24 19:23:06 UTC
To have support of localization for desktop environment. You can also create file in /var/db/AccountsService/users/. Name of such file is your login.

For example for the Xfce desktop

[User]
Language= ← here value of LANG variable
XSession=xfce
SystemAccount=false

Value of XSession is desktop file found in /usr/local/share/xsession/

It even works on Linux! Below on my system

> gdbus call --system --dest org.freedesktop.Accounts --object-path /org/freedesktop/Accounts/User`id -u` --method org.freedesktop.DBus.Properties.Get org.freedesktop.Accounts.User Language
> (<'fr_FR.UTF-8'>,)

> gdbus call --system --dest org.freedesktop.Accounts --object-path /org/freedesktop/Accounts/User`id -u` --method org.freedesktop.DBus.Properties.Get org.freedesktop.Accounts.User XSession
> (<'pantheon'>,)
Comment 7 Daniel O'Connor 2022-09-25 07:12:11 UTC
(In reply to Olivier Duchateau from comment #6)
Thanks, that is useful to know.

I think lightdm should still call setusercontext() though - that way it matches the behaviour of other login types.
Comment 8 Daniel Tameling 2023-09-22 05:24:41 UTC
Given that there was no reaction from upstream, would it be possible to add the patch to ports? That would also address bug #273720.
Comment 9 Ivan Rozhuk 2023-12-22 15:28:45 UTC
Created attachment 247199 [details]
pre logon
Comment 10 Ivan Rozhuk 2023-12-22 15:29:25 UTC
Created attachment 247200 [details]
post logon
Comment 11 Ivan Rozhuk 2023-12-22 15:48:19 UTC
This issue also actual for slim and probably other logon managers and tools that do user logon.

As far I understand FreeBSD logon process a bit more complex than linux, at least it requires additional syscalls to handle login classes.


To fix this I use "su -l ${USER}" in 2 wrapper scripts:
 1. logon_pre.sh: it save env to XDG_RUNTIME_DIR ("/var/run/user/`id -ur ${USER} | tr -d '\n'`") and run: /usr/bin/su -l "${USER}" -c "env XDG_RUNTIME_DIR=${XDG_RUNTIME_DIR} ${THIS_SCRIPT_DIR}/logon_post.sh ${@}"

 2. logon_post.sh: it save its own env, restore env from firs script, restore its own env and run Xsession.

To use this you need:
1. save both scripts to /usr/local/etc/lightdm
2. chmod +x them
3. in file /usr/local/etc/lightdm/lightdm.conf replace:
session-wrapper=/usr/local/etc/lightdm/Xsession
with
session-wrapper=/usr/local/etc/lightdm/logon_pre.sh


Another cool hack:
xmir-command=/usr/bin/nice -n -15 /usr/local/bin/Xmir
xserver-command=/usr/bin/nice -n -15 /usr/local/bin/X
this increase xorg priority and fix some frizes.


For slim I use for a while:
login_cmd		/usr/bin/su -l ${USER} -c "env XAUTHORITY=${XAUTHORITY} XDG_SESSION_COOKIE=${XDG_SESSION_COOKIE} DISPLAY=${DISPLAY} ~/.xinitrc %session"

This is a bit insecure since XDG_SESSION_COOKIE visible via "ps axd".

Ti increase xorg priority:
default_xserver		/usr/bin/nice
xserver_arguments	-n -15 /usr/local/bin/X -nolisten tcp vt09


Hope lightdm and slim maintainers will assimilate these scripts and config hacks into ports tree.
Comment 12 Guido Falsi freebsd_committer freebsd_triage 2023-12-22 16:07:01 UTC
(In reply to Daniel O'Connor from comment #5)

Hi,

I admit I forgot about this.

I see you filed a p[atch upstream, but dumping a diff file in a github gist is problematic, nobody is going to really look at it and makes reviewing changes a pain.

You should really file this as a Pull Request with the upstream code to get someone to look at it.

Also, the lightdm project is under the canonical umbrella and they have strict requirements for contributors. You have to sign documents. Unless you do that, they will not look at your patches, I guess.

Anyway, I could take a look and evaluate including that in the port but I also need clarifications. Reviewing in a gist is complicated.

Not sure what could be the best way to do this, maybe if I cloned lightdm github repo you could file your patch as a pull request with my fork so we can perform the review there?

Because I see a lot of changes, some I don't understand and I really need to understand them thoroughly before including.
Comment 13 Guido Falsi freebsd_committer freebsd_triage 2023-12-22 16:12:00 UTC
(In reply to Guido Falsi from comment #12)

Hi,

I Think this kind of customizations should be left to the end user/administator. Adding wrapper scripts like this make the default setup complex, make it diverge strongly from upstream code and practice.

Also, who would be responsible for maintaining these wrapper scripts and fix them in case they need updating?

We can't drop this responsibility on desktop@

ALso changing Xorg priority is not something the OS should enforce on users, and definitely not something a display manager should do unilaterally, this should at least be discussed with the Xorg maintainers and evaluated as a policy for all the ports tree.

I really would not feel comfortable adding such opinionated custom scripts to the port. I'm also not really able to give a reasonable security evaluation of these scripts.

In the default port we need to try to be as neutral as possible.
Comment 14 Guido Falsi freebsd_committer freebsd_triage 2023-12-22 16:12:36 UTC
(In reply to Guido Falsi from comment #13)

Oops, this was meant to be a reply to comment #11
Comment 15 Ivan Rozhuk 2023-12-22 17:40:15 UTC
(In reply to Guido Falsi from comment #13)

> Adding wrapper scripts like this make the default setup complex, make it diverge strongly from upstream code and practice.

Port already add its own Xsession script, it distributed in ports tree.
You can rename pre script to Xsession, merge post script with current Xsession and this will be transparent migration for users.


> Also, who would be responsible for maintaining these wrapper scripts and fix them in case they need updating?

You can CC me on any one can do this, this is very simple small scripts that have comments.


> ALso changing Xorg priority is not something the OS should enforce on users

Whatever, enjoy with freezes :)


> I really would not feel comfortable adding such opinionated custom scripts to the port.

Ok, leave it broken. :)


> I'm also not really able to give a reasonable security evaluation of these scripts.

install, env, su - is so hard to understand. )


Anyway, feel free to ignore this work, I just finish with this in my environment and share results of few days research.


PS: Also these vars nice to have in init chain:
# https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html
if [ -z "${XDG_DATA_HOME}" ]; then
	export XDG_DATA_HOME="${HOME}/.local/share"
	mkdir -p "${XDG_DATA_HOME}"
fi
if [ -z "${XDG_CONFIG_HOME}" ]; then
	export XDG_CONFIG_HOME="${HOME}/.config"
	mkdir -p "${XDG_CONFIG_HOME}"
fi
if [ -z "${XDG_STATE_HOME}" ]; then
	export XDG_STATE_HOME="${HOME}/.local/state"
	mkdir -p "${XDG_STATE_HOME}"
fi
if [ -z "${XDG_CACHE_HOME}" ]; then
	export XDG_CACHE_HOME="${HOME}/.cache"
	mkdir -p "${XDG_CACHE_HOME}"
fi
if [ -z "${XDG_RUNTIME_DIR}" ] && [ -d '/var/run/user' ]; then
	export XDG_RUNTIME_DIR="/var/run/user/`id -ur ${USER} | tr -d '\n'`"
	install -d -o "${USER}" -g "${USER}" -m 0700 "${XDG_RUNTIME_DIR}"
fi
# XAuth staff.
if [ -z "${XAUTHORITY}" ]; then
	if [ -n "${XDG_RUNTIME_DIR}" ]; then
		export XAUTHORITY="${XDG_RUNTIME_DIR}/.Xauthority"
	else
		export XAUTHORITY="${HOME}/.Xauthority"
	fi
fi
Comment 16 Guido Falsi freebsd_committer freebsd_triage 2023-12-22 18:15:01 UTC
(In reply to Ivan Rozhuk from comment #15)

> Port already add its own Xsession script, it distributed in ports tree.

And that's part of the problem. People could have their configuration around that, and changing it one sided could break things.

And what if some user does not like these changes? What about POLA?

> You can CC me on any one can do this, this is very simple small scripts that have comments.

This is not the4e point. Not all users will report issues, not all issues will be reported to desktop@. You also assume all people like doing things the way you do, which is not the case.

> Whatever, enjoy with freezes :)

You assume my systems are behaving exactly like yours. Maybe these freezes and other issues are specific to your machines?

The point is, this kind of customizations are just that and should not be imposed as defaults by ports. We cannot assume everybody would like things done in a certain way, or that some other people workloads would not be harmed by such changes.

> install, env, su - is so hard to understand. )

The problem is such commands act during a complex process of user login via display manager, which I don't know the details. The system already does it's privilege changes, so, for example, the use of su looks inappropriate. Things that need to run as the user should be ran in another script that is already ran as the user.

But such review is beyond my knowledger about Xorg and its login process and I simply cannot do it.


But The project has other committers. This PR is about a lightdm issue with login.conf.

Your patches have a much wider scope, if you really want to pursue such changes you should start a discussion on the x11@ mailing list.
Comment 17 Ivan Rozhuk 2023-12-22 18:45:14 UTC
(In reply to Guido Falsi from comment #16)
> And what if some user does not like these changes? What about POLA?

Is POLA include bugs? )
Xsession may become as sample and will not overwrite user changes.
Or these 2 scripts may be distributed separately and sample config may use it by default.

Even proper fix for lightdm to start work like su -l may break some users with strange scripts.


> Maybe these freezes and other issues are specific to your machines?

sw cursor + heavy load like llvm/chromium compilation.
This is very simple change to improve user experience without any risk of side effects.


> Your patches have a much wider scope, if you really want to pursue such changes you should start a discussion on the x11@ mailing list.

My issue with proper logon is solved.
This is quick workaround to avoid dig inside su.c, login.c, pam_unix.c and move some code to lightdm + waste of time to promoting these changes to upstream.
Comment 18 Guido Falsi freebsd_committer freebsd_triage 2023-12-22 19:16:08 UTC
(In reply to Ivan Rozhuk from comment #17)

> Is POLA include bugs? )

Well actually it could, but that's not the point here.

There is no bug. It looks to me you have a very personal definition of bug and of how to "solve" them.

> Or these 2 scripts may be distributed separately and sample config may use it by default.

Great idea, why don't you file a bug report for  anew port which includes these two scripts and all the glue needed to install them? Maybe someone would pick it up (not me, obviously)

> sw cursor + heavy load like llvm/chromium compilation.
> This is very simple change to improve user experience without any risk of side effects.

The point is that changing Xorg priority is not a bugfix, but a local workaround that each user can choose to apply locally, and is not something that we should impose from the official ports.

Personally I don't consider what you describe as a bug or even an annoyance. But that's me.


> This is very simple change to improve user experience without any risk of side effects.

This is your opinion, but is against port rules. Any patch included in the ports tree should also be sent upstream [1] so your proposal of including a patch that is not good enough to be upstreamed is not acceptable by the ports rules.

Also not upstreaming patches is a big problem with the ports tree. Differences with upstream accumulate and we are handling ports here, not forks.

So yes, if upstreeam has a bug, the bug is expected to also exist in the port. It can be fixed in the port before the upstream but in such a way to be upstreamed.

Anyway I'm not considering these scripts for inclusion. I would not feel comfortable committing them. But this will not stop you from writing to the mailing lists or seeking other committer's interest in them.


[1] https://docs.freebsd.org/en/articles/contributing/#_maintainer_responsibilities
Comment 19 Guido Falsi freebsd_committer freebsd_triage 2023-12-23 13:07:17 UTC
Created attachment 247213 [details]
patch v1

Hi,

I am sending a cumulative patch to try to address this bug and also bug #273720 and bug #275885 integrating patches from these tree bugs too.

Please test this and report if it actually fixes the issues and does what it says.

Thanks in advance.

Some notes about the patch:

- Imported patch suggested by darius@dons.net.au to use setusercontext (should solve login.conf issues), upstreamable patch at [1]. Requires USES=autoreconf
- Add patch to implement new configuration value "smart-xsession-errors" (disabled by default) to move .xsession-errors from home directory to ~/.cache (or ploaces indicated by XDG variables if present) inspiured from: [2] upstreamable patch at [3], inspired by bug #266532
- Move some LIB_DEPENDS that really are BUILD_DEPENDS
- Added USES=autoreconf to allow patches to work, adjust DOC dependencies. During autoreconf gtkdoc and yelp are unconditionally required
- Due to autoreconf usage, the build system insisted on installing apparmor files, which are not supported in FreeBSD patched Makefile.am to skip those.
- Unconditionally disable libaudit, I found no support in FreeBSD
- Add QT5 option to properly compile and install qt5 pars. Disabled by default since it is unused at present by other ports
- Install pam files as samples, to try not to overwrite customized ones
- Use ETCDIR in post-install
- Pet portclippy/portfmt

[1] https://github.com/canonical/lightdm/compare/main...madpilot78:lightdm:use_setusercontext

[2] https://github.com/canonical/lightdm/pull/287

[3] https://github.com/canonical/lightdm/compare/main...madpilot78:lightdm:xsession_errors_location_configure
Comment 20 Daniel Tameling 2023-12-24 06:00:43 UTC
Thanks for taking care of this.

I tested this and it still doesn't use the setusercontext function. The reason for this is that the corresponding check fail during configure. From my config.log:

configure:17573: checking for setusercontext
configure:17573: clang -o conftest -O2 -pipe  -Wno-error=incompatible-function-pointer-types -fstack-protector-strong -isystem /usr/local/include -fno-strict-aliasing  -isystem /usr/local/include  -fstack-protector-strong  conftest.c -L/usr/local/lib >&5
ld: error: undefined symbol: setusercontext
>>> referenced by conftest.c
>>>               /tmp/conftest-408de2.o:(main)
clang: error: linker command failed with exit code 1 (use -v to see invocation)

configure needs to add -lutil so that the library with the symbol gets linked in. This flag also needs to get added to the Makefile.

There are also some trailing whitespace issues with the posted patch. You might want to clean this up before committing.
Comment 21 Guido Falsi freebsd_committer freebsd_triage 2023-12-24 11:34:54 UTC
Created attachment 247226 [details]
patch v2

@Daniel Tameling

Thanks for testing the patch.

I forgot about the -lutil part and clearly did not verify the build output thoroughly enough.

Anyway I've updated the patch. Adding LIBS=-lutil should be enough to make everything work

Can you test again with this new patch?

Thanks in advance!
Comment 22 Daniel Tameling 2023-12-24 15:50:14 UTC
There is one minor issue left: you didn't remove the original execve call, so that the new one with environ is never reached. When I delete the first execve, it works as expected.

Another thing to think about is that files/patch-src_session-child.c patches the PATH:
-            pam_putenv (pam_handle, "PATH=/usr/local/bin:/usr/bin:/bin");
+            pam_putenv (pam_handle, "PATH=/sbin:/bin:/usr/sbin:/usr/bin:/usr/local/sbin:/usr/local/bin:~/bin");
This doesn't have any effect since we call setusercontext later, so one could completely drop it. At the very least I would do something about the ~/bin, since this won't get expanded and is a literal ~. If one wants to keep the line, one could just delete the last component of the PATH or expand the ~ manually with the HOME directory we get a few lines later.

There are also some trailing whitespaces in the patch:
patch_v2:122: trailing whitespace.

patch_v2:132: trailing whitespace.
@@ -48,7 +48,7 @@ AC_CHECK_HEADERS(gcrypt.h, [], AC_MSG_ERROR(libgcrypt
patch_v2:133: trailing whitespace.

patch_v2:135: trailing whitespace.

patch_v2:138: trailing whitespace.

warning: squelched 8 whitespace errors
warning: 13 lines add whitespace errors.
Comment 23 Guido Falsi freebsd_committer freebsd_triage 2023-12-24 16:39:31 UTC
(In reply to Daniel Tameling from comment #22)

The execve thing was a mistake on my part. Thanks for reporting!

Regarding the path, I personally would remove the ~/bin one anyway, it really does not belong there.

The whitespace changes you see are all in diff files, those are expected and part of the diff, cannot and should not be removed. The diffs are generated automatically. So those don't need fixing.

Attaching an updated patch shortly (but the whitespace in the patch files will be still there as explained above)
Comment 24 Guido Falsi freebsd_committer freebsd_triage 2023-12-24 17:13:19 UTC
Created attachment 247227 [details]
patch v3

- Removed extra call to execve
- Removed ~/bin from path
Comment 25 Daniel Tameling 2023-12-24 18:37:47 UTC
Thanks for the explanation. Did you upload the correct patch? v3 and v2 look identical to me.
Comment 26 Guido Falsi freebsd_committer freebsd_triage 2023-12-24 18:59:20 UTC
Created attachment 247228 [details]
patch v3.1

Here is the correct file.

I must have grabbed some stale file without noticing.

Sorry for the mistake.
Comment 27 Daniel Tameling 2023-12-24 20:52:07 UTC
It works now as intended. Many thanks!
Comment 28 Ivan Rozhuk 2023-12-26 04:11:39 UTC
(In reply to Guido Falsi from comment #26)

1. Logon work fine, env vars same as for "su -l".

2. Can you please reduce these build deps, I do not build DOCS:
===>  Cleaning for yelp-tools-42.0
===>  Cleaning for py39-lxml-4.9.3
===>  Cleaning for py39-cython-0.29.37
===>  Cleaning for yelp-xsl-42.0
===>  Cleaning for gtk-doc-1.33.2_2
===>  Cleaning for py39-pygments-2.17.2
===>  Cleaning for py39-hatchling-1.21.0
===>  Cleaning for py39-editables-0.5
===>  Cleaning for py39-pathspec-0.12.1
===>  Cleaning for py39-pluggy-1.3.0
===>  Cleaning for py39-setuptools_scm-6.4.2
===>  Cleaning for py39-trove-classifiers-2023.11.29
===>  Cleaning for py39-calver-2022.6.26_1


3. "smart-xsession-errors" - work. Probably better set it to true in config, since it commented=false.
I do not test it with "XDG_*" in login.conf.


Thanks!
Comment 29 Guido Falsi freebsd_committer freebsd_triage 2023-12-26 09:34:13 UTC
(In reply to Ivan Rozhuk from comment #28)

I intentionally made it so that it defaults to false. The commented line in the config file reflects the default, the other line with the description of the option describes what happens when the option is turned on.

I do not want to change the default location of .xsession-errors for all users.

Such a decision is beyond the scope of the single lightdm port. Changing the default location of the .xession-errors file should be discussed on the mailing lists and a consensus be reached there for the change to be applied to all interested ports.

I'm not even sure how upstream will react to this change, anyway I'm going to publish PRs on github for these changes later today or tomorrow with upstream.
Comment 30 Guido Falsi freebsd_committer freebsd_triage 2023-12-26 09:35:24 UTC
(In reply to Ivan Rozhuk from comment #28)

Forgot to mention, thanks for testing and the feedback, this is really appreciated!
Comment 31 Ivan Rozhuk 2023-12-26 13:03:25 UTC
In login.c and su.c:
1. setusercontext() called twice: before for and after
2. first arg (login_class) never set NULL.
3. setusercontext() called before export/set env vars.

I am not saying that patch is wrong, but it uses setusercontext() a bit different than system apps.
Comment 32 Daniel Tameling 2023-12-27 07:09:00 UTC
(In reply to Ivan Rozhuk from comment #31)

2. If the first argument is NULL, the first thing setusercontext does is that it gets the argument itself by calling login_getpwclass(pwd).
Comment 33 Guido Falsi freebsd_committer freebsd_triage 2023-12-27 08:30:42 UTC
(In reply to Ivan Rozhuk from comment #31)

1 - the call before the fork is done with the "LOGIN_SETGROUP" argument and the comment on it clearly states the reason.
Here we are calling it once with "LOGIN_SETALL", which includes that too, in one go. Not sure why this is being done in two steps in su and login.

2 - I confirm Daniel analysis, you can see at [1] that setusercontext will grab the correct login class by itself.

3 - The first call to setusercontext is performed with the LOGIN_SETGROUP argument, which causes it to only configure groups membership for the user, I don't think that makes any difference in relation to env variables. The comment there makes it clear this is simply done because PAM could add group memberships to the user.

in general:

The patch logic is anyway an improvement on what lightdm was doing (that is a simple setuid/setgid). We are keeping the same basic semantics though. Looks like login.c is building the environment one piece at the time, but replicating that logic looks unnecessarily complicated based on the information we have.

This code can be improved in the future if issues arise.


BTW I noticed now I made an indent mistake in the patch I need to fix.


[1] https://cgit.freebsd.org/src/tree/lib/libutil/login_class.c#n437
Comment 34 Guido Falsi freebsd_committer freebsd_triage 2023-12-27 09:03:50 UTC
Filed the setusercontext and .xsession-errors patches upstream:

https://github.com/canonical/lightdm/pull/334

https://github.com/canonical/lightdm/pull/335
Comment 35 Guido Falsi freebsd_committer freebsd_triage 2023-12-27 09:04:40 UTC
Removing bug #273720 from See also, as it is already set in "blocks"
Comment 36 Guido Falsi freebsd_committer freebsd_triage 2023-12-27 09:12:13 UTC
Created attachment 247282 [details]
patch v3.2

This new patch fixes an indentation glitch.

Only white space changes included, no functional change.
Comment 37 Anton Saietskii 2023-12-27 14:07:14 UTC
(In reply to Guido Falsi from comment #36)

Guido, I'm getting some whitespace warnings with this patch:
> git apply /tmp/patch.txt
/tmp/patch.txt:117: trailing whitespace.

/tmp/patch.txt:124: trailing whitespace.
@@ -48,7 +48,7 @@ AC_CHECK_HEADERS(gcrypt.h, [], AC_MSG_ERROR(libgcrypt
/tmp/patch.txt:125: trailing whitespace.

/tmp/patch.txt:127: trailing whitespace.

/tmp/patch.txt:130: trailing whitespace.

warning: squelched 8 whitespace errors
warning: 13 lines add whitespace errors.
>

Perhaps remaking via "git format-patch" would be better?
Comment 38 Guido Falsi freebsd_committer freebsd_triage 2023-12-27 14:45:12 UTC
(In reply to Anton Saietskii from comment #37)

The whitespaces are in the patch files. These are actually part of the diff format, I cannot remove them, otherwise the patches would not apply.

A quick test with "git format-patch" shows the same exact whitespace false positives. This makes sense, the trailing whitespaces are in the patch files, which are generated using "make makepatch" and are part of the expected output.

BTW "git format-patch" creates multiple patch files, since I have a branch with multiple commits here to work on this patch. I'm doing it like this because I am merging multiple changes, but in this way I can track each one in its own commit. But having multiple patch files in the bug report would be a problem.

I already explained this in comment #23
Comment 39 Gleb Popov freebsd_committer freebsd_triage 2023-12-27 16:15:41 UTC
Just a note, SDDM was patched to use setusercontext long ago: https://github.com/sddm/sddm/blob/develop/src/helper/Backend.cpp#L71
Comment 40 Ivan Rozhuk 2023-12-27 21:35:37 UTC
(In reply to Guido Falsi from comment #36)

> priv->log_filename = g_build_filename (g_getenv ("XDG_STATE_HOME"), ".xsession-errors", NULL);

This does not work, even if XDG_STATE_HOME set in login.conf.
Looks like session_init() executed before setusercontext() staff.
Comment 41 Ivan Rozhuk 2023-12-28 01:45:07 UTC
(In reply to Ivan Rozhuk from comment #40)
Sorry, according to logs all ok:

> [+116.31s] DEBUG: Session pid=29553: Logging to ~/.local/state/.xsession-errors
Comment 42 Ivan Rozhuk 2023-12-28 04:11:31 UTC
Created attachment 247307 [details]
patch

This patch move ~/.Xauthority to ${XDG_RUNTIME_DIR} if XDG_RUNTIME_DIR is set.

Set user-authority-in-system-dir=false in config.
Tested with ConsoleKit2. Not sure that will work with logind without additional code.
Comment 43 Guido Falsi freebsd_committer freebsd_triage 2023-12-28 08:19:11 UTC
(In reply to Ivan Rozhuk from comment #42)

Hi,

Could you please file this as a separate bug report, and also explain why you want to move the ".Xauthority" file? Is it not working properly where it is? Is there a standard suggesting another location as an alternative (maybe a preferred one)? Or some other reason I cannot imagine, maybe.

I'd also rather not add code to this already complex patch, that is already in an advanced state of testing.

Also, this port is already diverging too much from upstream for my tastes, so I'd rather have a very strong reason for adding this option and change its behaviour further or ask for the patch to at least be filed upstream before inclusion. [1]


BTW your patch cannot work as is because you also need code in src/lightdm.c telling lightdm to actually read that variable from the configuration file.


[1] considering upstream policy for contributions, since you created this patch and can claim full copyright for it, the only way for them to accept it would be for you to file it on github as a pull request and sign their contributor agreement. Apart from that their main platform is linux, so you should give them some assurance this has no ill effects on linux systems and would not cause any issues there.
Comment 44 Guido Falsi freebsd_committer freebsd_triage 2023-12-28 08:27:16 UTC
(In reply to Ivan Rozhuk from comment #41)

The variables defined in login.conf should all work. But since those environment variables could be defined in various stages of the login process by different entities (login.conf via setusercontext, via pam modules, via various scripts), so I guess it could happen for some to be defined too late for lightdm to notice them.

I have no solution to this though, except to suggest to make sure to define them as early as possible.
Comment 45 Guido Falsi freebsd_committer freebsd_triage 2023-12-28 23:48:14 UTC
Created attachment 247324 [details]
patch v4

I modified the patch to not unconditionally depend on yelp-tools and gtk-doc.

I get to this with some replacement in configure.ac and Makefile.ac files.

This was suggested by Anton Saietskii in bug #273720


I'm planning to commit this before the end of the years, so it can be part fof the next quarterly. There will be little chance for further improvements.

Any testing appreciated.

I obviously am testing it myself and everything looks fine IMHO.
Comment 46 Ivan Rozhuk 2023-12-29 00:02:38 UTC
(In reply to Guido Falsi from comment #43)

> [1] considering upstream policy for contributions

There is too many requirements for free (non sponsored) work in open source.
https://github.com/canonical/lightdm/pull/338



> why you want to move the ".Xauthority" file?

Because I do not want to have crap in home dir (user-authority-in-system-dir=false) and I do not like have crap in var (user-authority-in-system-dir=true).
ConsoleKit2 will create /var/run/user/$UID dir and promote it as XDG_RUNTIME_DIR in any way.
user-authority-in-system-dir=true mean that lightdm will create /var/run/$USER to keep xauthority.

I try switch to /var/run/$USER by setting $XDG_RUNTIME_DIR in login.conf and later in .profile but gnupg (pgp-agent) have hard coded places to guess and not use $XDG_RUNTIME_DIR.
It has more ugly code than lightdm to patch it.
Also ConsoleKit2 do additional management for this like create/delete and without patching lightdm it will do this job even if I do not use it.


> BTW your patch cannot work as is because you also need code in src/lightdm.c telling lightdm to actually read that variable from the configuration file.

It can.
Simple hack: depend on state of user-authority-in-system-dir string inside x_authority_filename var will end with ".Xauthority" or with "xauthority".



PS: I found that /etc/profile is not handled. Is it missed?
Comment 47 Ivan Rozhuk 2023-12-29 00:20:11 UTC
(In reply to Guido Falsi from comment #45)

It build and work.
No unneeded deps installed during build.

Thanks!
Comment 48 Ivan Rozhuk 2023-12-29 00:56:01 UTC
Comment on attachment 247307 [details]
patch

Moved to https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=275989
Comment 49 commit-hook freebsd_committer freebsd_triage 2023-12-29 18:26:18 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=23a47f28de20c42fb2c3a7286c815797013df6ea

commit 23a47f28de20c42fb2c3a7286c815797013df6ea
Author:     Guido Falsi <madpilot@FreeBSD.org>
AuthorDate: 2023-12-29 18:23:10 +0000
Commit:     Guido Falsi <madpilot@FreeBSD.org>
CommitDate: 2023-12-29 18:25:15 +0000

    x11/lightdm: Fix login.conf issues, import various improvements

    - Add patch to use setusercontext(3) to setup user environment, so it respects login.conf among other things [1]
    - Use autoreconf, since patch requires regenerating configure script to check for setusercontext(3) [2]
    - Explicitly disable libaudit support, not supported in FreeBSD [3]
    - Add QT5 options, disabled by default, to control linking against qt5 [4]
    - Import patch adding option to enable alternative location for .xsession-errors file [5]
    - Correctly define runtime dependencies
    - Forcibly disable installation of apparmor files
    - Install PAM configuration files as samples, so in the future they are not overwritten if customized
    - Pet portclippy/portfmt
    - Regenerate patches

    Upstreaming:

    [1] https://github.com/canonical/lightdm/pull/334

    [5] https://github.com/canonical/lightdm/pull/335

    Many thanks to all people involved!

    PR:             266532 [1] [2],
                    273720 [1],
                    275885 [3] [4] [5]
    Tested by:      Ivan Rozhuk <rozhuk.im@gmail.com>,
                    Daniel Tameling <tamelingdaniel@gmail.com> (provided setusercontext patch),
                    Anton Saietskii <vsasjason@gmail.com>

 x11/lightdm/Makefile                               | 71 ++++++++++++--------
 .../files/patch-common_configuration.c (new)       | 10 +++
 x11/lightdm/files/patch-configure.ac (new)         | 11 ++++
 x11/lightdm/files/patch-data_Makefile.am (new)     | 29 +++++++++
 x11/lightdm/files/patch-data_lightdm.conf          | 23 +++++--
 x11/lightdm/files/patch-data_users.conf            |  4 +-
 .../files/patch-liblightdm-gobject_language.c      | 10 +--
 x11/lightdm/files/patch-src_lightdm.c              | 11 +++-
 x11/lightdm/files/patch-src_session-child.c        | 75 +++++++++++++++++++---
 x11/lightdm/files/patch-src_session.c (new)        | 21 ++++++
 x11/lightdm/pkg-plist                              | 19 +++++-
 11 files changed, 235 insertions(+), 49 deletions(-)
Comment 50 Guido Falsi freebsd_committer freebsd_triage 2023-12-29 18:28:41 UTC
Patch committed.

Thanks for all the help, suggestions and support!