Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vncserver: recognize environment variables in configuration files #1664

Closed
wants to merge 1 commit into from

Conversation

casantos
Copy link
Contributor

@casantos casantos commented Aug 31, 2023

Since commit c67a5f2 (Start the sessions using xinit), vncserver runs Xvnc via exec(), instead of via system(). Before that, it was possible to enable PAM authentication for the user who owned the Xvnc process by adding this to /etc/tigervnc/vncserver-config-defaults:

SecurityTypes=TLSPlain
PlainUsers=$USER

This does not work anymore because with exec() the command line is not parsed by the user shell.

A simple workaround would be to use "PlainUsers=*" but this creates a security vulnerability, since it allows any user to have access to the VNC session, not only the owner of the Xvnc process.

Another workaround is to create a per-user ~/.vnc/config containing

PlainUsers=<actual-user-name>

but this is inconvenient when there is a large number of VNC users to configure.

This commits partially restores the old behavior by replacing any $FOO in a parameter value by the contents of the corresponding environment variable.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2233204

Since commit c67a5f2 (Start the sessions using xinit), vncserver runs
Xvnc via exec(), instead of via system(). Before that, it was possible
to enable PAM authentication for the user who owned the Xvnc process by
adding this to /etc/tigervnc/vncserver-config-defaults:

    SecurityTypes=TLSPlain
    PlainUsers=$USER

This does not work anymore because with exec() the command line is not
parsed by the user shell.

A simple workaround would be to use "PlainUsers=*" but this creates a
security vulnerability, since it allows any user to have access to the
VNC session, not only the owner of the Xvnc process.

Another workaround is to create a per-user ~/.vnc/config containing

   PlainUsers=<actual-user-name>

but this is inconvenient when there is a large number of VNC users to
configure.

This commits partially restores the old behavior by replacing any $FOO
in a parameter value by the contents of the corresponding environment
variable.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2233204
Signed-off-by: Carlos Santos <casantos@redhat.com>
@CendioOssman
Copy link
Member

I understand the use case here, but supporting environment variables is a big messy thing. The changes suggested here are very incomplete as there are variations to the syntax, and you also need to handle escaping. I'd also consider environment variables in config files to be very non-standard, and may surprise users.

So I would suggest opening a feature request for your scenario instead, and we can discuss alternatives there.

I'd also very much like to understand your use case. PAM usually requires running things as root, so I'm a bit surprised by your setup.

The upstream report also suggests that this is a regression. I don't understand how, as I can't find any previous code like this. Perhaps it was a Red Hat specific patch? @grulja?

@CendioOssman
Copy link
Member

Ah, the regression confusion was discussed further down in the upstream ticket. It seems that was an unintentional feature.

@casantos
Copy link
Contributor Author

casantos commented Sep 8, 2023

PAM does not require root access on RHEL. Maybe we could try a different approach, making Xvnc recognize $LOGNAMEor $USER, as in this patch.

0001-Support-using-LOGNAME-and-USER-environment-variables.patch.txt

@CendioOssman
Copy link
Member

PAM does not require root access on RHEL.

That can't be generally true? As far as I know, RHEL still uses pam_unix for local users, which requires root access.

An approach like that is probably better. But it should be under the principle of "magical markers" in this field, just like * has special meaning. We don't want general environment variable support. That is something typically reserved to the shells.

As for the syntax for such markers. I'd ideally see we use something that is common elsewhere. Do you know of any examples of other systems that have a special syntax to mean "current user"? Using $ is probably not ideal as it might give the impression that it is general environment variable support.

@casantos
Copy link
Contributor Author

casantos commented Sep 8, 2023

PAM does not require root access on RHEL.

That can't be generally true? As far as I know, RHEL still uses pam_unix for local users, which requires root access.

It does not work with the PAM configuration file included in the sources (unix/vncserver/tigervnc.pam) but works if I use the login PAM config:

# rm -f /etc/pam.d/vnc; ln -s login /etc/pam.d/vnc

An approach like that is probably better. But it should be under the principle of "magical markers" in this field, just like * has special meaning. We don't want general environment variable support. That is something typically reserved to the shells.

As for the syntax for such markers. I'd ideally see we use something that is common elsewhere. Do you know of any examples of other systems that have a special syntax to mean "current user"? Using $ is probably not ideal as it might give the impression that it is general environment variable support.

What about %u, as used by systemd in the unit files?

@casantos
Copy link
Contributor Author

I just submitter a follow-up PR: #1671

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants