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

allow unprivileged user to run 'makecache' and 'clean' with alternate config #501

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

oliverkurth
Copy link
Contributor

@oliverkurth oliverkurth commented Oct 7, 2024

Allow an unprivileged user to run tdnf makecache or tdnf clean. Print out a recommendation if there are insufficient permissions to modify the cache directory. For example, use a configuration that has a cache directory configured where access is possible.

Example:

testuser [ ~ ]$ cat tdnf.conf 
[main]
gpgcheck = 1
installonly_limit = 3
clean_requirements_on_remove = 0
repodir = /etc/yum.repos.d
cachedir = /home/testuser/cache
testuser [ ~ ]$ /build/bld-ph5/bin/tdnf -c ./tdnf.conf makecache
Refreshing metadata for: 'VMware Photon Linux 5.0 (aarch64) Updates'
photon-updates                            3570 100%
photon-updates (primary)                573775 100%
photon-updates (file lists)            2354866 100%
photon-updates (update info)              6146 100%
photon-updates (other)                  256415 100%
Metadata cache created.
testuser [ ~ ]$ 

Copy link
Contributor

@sshedi sshedi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test for both the cases.

@@ -334,7 +334,7 @@ TDNFTouchFile(
}

old_mask = umask(022);
fd = creat(pszFile, S_IRUSR | S_IRGRP | S_IROTH);
fd = creat(pszFile, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
Copy link
Contributor

@sshedi sshedi Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding this in %post of tdnf spec will be a good add-on

%post
test -d %{_var}/cache/%{name} && chmod -R 755 %{_var}/cache/%{name} || :

But assuming that /var/cache/tdnf is the configured repo dir (which holds good in most of the cases)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't be needed. When running as non-root, removing the file fails when it is read-only. But when run as root, it can be removed even when read-only. Since the system tdnf runs as root, there is no need to change permissions. makecache/clean wasn't working before the PR for non-root users, and the package is unaware of any non-root uses anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. I checked in fedora, it allows regular user to do dnf makcache. Should we be in alignment?

Copy link
Contributor Author

@oliverkurth oliverkurth Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I checked. I wasn't aware. dnf allows running makecache as an unprivileged user, but it stores the cache in /var/tmp/dnf-<user>-<random uuid>/ (like /var/tmp/dnf-okurth-50ituhym/). Not in the system wide cache - which makes sense, we don't want a regular to have write access to that.

We could consider doing something similar, in another change. This PR would be a prerequisite for that.

dwError = TDNFRefresh(pContext->hTdnf);

if (dwError == ERROR_TDNF_SYSTEM_BASE + EACCES) {
if (geteuid()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use gEuid global variable here. If not please make adjustments so that gEuid is populated properly here. I think this can be avoided.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a reason for this? geteuid() gives the same information (it's not changing), and the performance impact is negligible (if that is your concern).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make a function call when we have a global variable which is dedicated for the exact thing. Redundancy is my concern. And performance as well, even though it is negligible.

Copy link
Contributor Author

@oliverkurth oliverkurth Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I thought about this - I'll leave it as is. Performance can be ignored here (we already spent more time discussing it than we will ever save). Having a global variable is actually adding redundance, and makes it more complex. For example, if there is an issue, we'd need to figure out if the variable is properly set and so on. But thank you for your advice.

@oliverkurth oliverkurth merged commit 1bd9b71 into dev Oct 11, 2024
6 checks passed
@oliverkurth oliverkurth mentioned this pull request Oct 15, 2024
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.

2 participants