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

xrdp socketdir packaging cleanup #3066

Open
rowlap opened this issue May 8, 2024 · 11 comments
Open

xrdp socketdir packaging cleanup #3066

rowlap opened this issue May 8, 2024 · 11 comments

Comments

@rowlap
Copy link
Contributor

rowlap commented May 8, 2024

I'm using xrdp from the Fedora package, but in xrdp 0.10 there's a problem where the socketdir, /run/xrdp, is created at runtime and not known to the RPM package.

When switching between different versions of xrdp, this causes a problem where the change in permission scheme from 1777/drwxrwxrwt to 0755/drwxr-xr-x causes problems on downgrade, as socketdir is no longer world-writable.

Possible solutions include "owning" /run/xrdp directly from the RPM, including permission mode, or declaring a %ghost file entry for it. A workaround is to manually remove /run/xrdp after package uninstallation, which causes the next xrdp package to create it afresh with appropriate permissions.

This may be a "not our bug", if packaging and upgrade / downgrade concerns are out of scope for xrdp itself, but I thought it worth raising if the developers had any insight or opinions on the best way to handle downgrades.

I took the liberty of raising a Fedora bug for this, but I'm less familiar with other distros and packaging policies.

@matt335672
Copy link
Member

Thanks for bringing our attention to it @rowlap

I think @bsmojver and @metalefty will be interested in this discussion.

Here are some thoughts:-

  1. I don't think removing the directory is necessarily the right thing to do. In the future this could complicate upgrades when we get xrdp-sesman restartable. A better idea might be to use another directory for v0.10 if this is considered important enough.
  2. v0.10 isn't perfectly upwardly compatible with v0.9. The differences are small (see the general announcements and significant changes for packagers or developers), but some users will need to make changes to .ini files anyway.

The last point above suggests to me that this particular issue is best handled by a note in a README file. I'm not a user or a packager though, so thoughts are appreciated.

Whatever we decide, the information provided with the xrdp release may need to be updated, before the official v0.10 lands.

@rowlap
Copy link
Contributor Author

rowlap commented May 9, 2024

Thanks @matt335672; I'm sure there are aspects to handling upgrades which I've overlooked. It's hard to say what level of compatibility the packager should support in terms of going between version X and Y.

I hope we can agree at least that when xrdp is completely uninstalled, /run/xrdp should not be left behind. The rationale for doing an uninstall is that it de-risks the beta upgrade when we can quickly revert in-place.

It's getting into the weeds (and beyond my knowledge) if we consider rpm --oldpackage ... or dnf downgrade scenarios as part of the same RPM transaction; how should socketdir permissions be modified?

@matt335672
Copy link
Member

We're in agreement regarding complete uninstallation. From what I remember of scriptlets in RPM, removing the directory for a complete uninstall is easy, as the scriptlet is passed a parameter which lets it determine how many versions of the package are left when the transaction completes.

The other bit is more tricky. Again this is from memory, so could be wrong. I don't think the scriptlets have knowledge of the other package version for upgrade or downgrades, so you'd need to set the permissions correctly in a %post scriptlet of the package being installed. This would mean that we'd need to have a new version of v0.9.x built and installed just to handle the downgrade situation. This is prone to error, as we can't guarantee the user has installed the latest v0.9.x package.

@matt335672
Copy link
Member

Also, focussing on an EPEL-only solution is probably not a great idea - other distros may need to consider this too.

@bsmojver
Copy link

I can put something into the RPM spec in Fedora/EPEL, but 0.10 was not released there yet. That because it's beta (I did some copr builds only), so if an upstream solution is on the horizon, that is definitely better.

@rowlap
Copy link
Contributor Author

rowlap commented May 10, 2024

@bsmojver can this be addressed by having socketdir owned by the package, rather than created on-demand by xrdp? If so, can the package specify different permissions for 0.9 vs 0.10 to handle the announced changes?

Not sure an RPM scriptlet is the right approach here, as the package filenames / filemodes can handle ownership (I think).

@matt335672
Copy link
Member

v0.10 is imminent.

I think the simplest solution in this case (which will be in the NEWS.md for the new release) is to use --with-socketdir=/run/xrdp-socks (or similar) for the v0.10 release. That prevents the permissions for the two releases from coming into conflict.

When we get the next major release of xrdp out, we could remove the setting entirely and go for the default. That might inconvenience people upgrading two major versions but that's probably ignoreable.

@matt335672
Copy link
Member

Crossing messages!

That's a good idea @rowlap. The code in v0.10 enforces permissions but I don't think v0.9.x is quite so paranoid.

@bsmojver
Copy link

We should probably do it with: https://docs.fedoraproject.org/en-US/packaging-guidelines/Tmpfiles.d/

@rowlap
Copy link
Contributor Author

rowlap commented May 17, 2024

@bsmojver tmpfiles.d looks like the right choice for managing directories under /run.

This is extra work, but is there scope for two versions of xrdp-tmpfiles.conf corresponding to the changes in 0.10.0 (1777 vs 0755)?

I think this would be needed for transactional downgrade, otherwise we won't manage the permission change correctly. An easy workaround is non-transactional downgrade (i.e. complete removal and reinstallation), so long as no /run files are left behind.

@bsmojver
Copy link

The builds are in testing, give them a try and see what gives. As it stands, new version whacks the directory on uninstall.

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

No branches or pull requests

3 participants