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

💪 #492

Merged
merged 2 commits into from
Apr 25, 2024
Merged

💪 #492

merged 2 commits into from
Apr 25, 2024

Conversation

Samueru-sama
Copy link
Contributor

This first change is needed to make it easier to transition AM to use no sudo later on.

Haven't tested if this causes an issue with appman. (appman doesn't need to have its cache moved since it is always at a user owned location).

Also I did not remove the sudo and ownership checks here, As that will be on a different pr.

@Samueru-sama Samueru-sama mentioned this pull request Apr 25, 2024
@Samueru-sama
Copy link
Contributor Author

Btw @ivan-hc is the $XDG_DATA_HOME check needed inside launcher.am? If the script is always called by APP-MANAGER/appman which pass the '$DATADIR' variable then I don't think it is needed.

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 25, 2024

@Samueru-sama why mkdir -p "$AMPATH"/modules "$CACHEDIR"/am? What is "$CACHEDIR"/am?

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 25, 2024

OK, I got it... user's $HOME ~/.cache directory

@Samueru-sama
Copy link
Contributor Author

Samueru-sama commented Apr 25, 2024

@Samueru-sama why mkdir -p "$AMPATH"/modules "$CACHEDIR"/am? What is "$CACHEDIR"/am?

$CACHEDIR/am replaces $AMPATH/.cache and it uses XDG_CACHE_HOME as the location. That location is never owned by root, so the regular user will have no problems running AM do to basic functions even if it isn't owned by them.

(And also it follows the XDG_Base_Dir specification, which I know you aren't a fan of following rules lol).

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 25, 2024

Btw @ivan-hc is the $XDG_DATA_HOME check needed inside launcher.am? If the script is always called by APP-MANAGER/appman which pass the '$DATADIR' variable then I don't think it is needed.

I think that this was what remains of fisrts times you did this change... I don't remember but I think were you adding this

@Samueru-sama
Copy link
Contributor Author

Btw @ivan-hc is the $XDG_DATA_HOME check needed inside launcher.am? If the script is always called by APP-MANAGER/appman which pass the '$DATADIR' variable then I don't think it is needed.

I think that this was what remains of fisrts times you did this change... I don't remember but I think were you adding this

Yeah I would swear that I removed them from the modules as well lol. (I'm removing it now).

@ivan-hc ivan-hc merged commit 1753fec into ivan-hc:dev Apr 25, 2024
@Samueru-sama
Copy link
Contributor Author

Hey you merged this right before I removed the xdg check in launcher.am 😅

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 25, 2024

@Samueru-sama I added also the new variable "$AMBRANCH" for our tests

just replace at line 227 of APP-MANAGER

			AMREPO="https://raw.githubusercontent.com/IVAN-HC/AM/main" # Consolidating variable assignments for clarity and brevity

with

			AMREPO="https://raw.githubusercontent.com/IVAN-HC/AM/dev" # Consolidating variable assignments for clarity and brevity

and we can update/sync from "dev"

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 25, 2024

Now let improve the INSTALL script

and if you can, upload the changes in the installation scripts

@Samueru-sama
Copy link
Contributor Author

Samueru-sama commented Apr 25, 2024

Now let improve the INSTALL script

and if you can, upload the changes in the installation scripts

iirc the only change I did to INSTALL was remove the chown, which cannot be do right now as APP-MANAGER still needs sudo to update itself.

I also didn't add the $SUDOCOMMAND ./AM-updater to the module that needs it to update the apps without sudo.

In other words I don't think this will be ready today as I will see the feral gamemode repo to see how they add users to a group that doesn't need sudo without the user having to edit sudoers directly.

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 25, 2024

would you prefer that I add an AM-updater or just an "updater" script (not to be managed by -u) in the INSTALL script?

@Samueru-sama
Copy link
Contributor Author

Samueru-sama commented Apr 25, 2024

would you prefer that I add an AM-updater or just an "updater" script (not to be managed by -u) in the INSTALL script?

An AM-updater that does update APP-MANAGER and its modules would be great indeed.

It can even be similar to the way the apps update themselves, git clone the AM repo and mv --backup ./tmp ./ right?

EDIT: Nvm that would download the list of apps as well.

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 25, 2024

@Samueru-sama however, you said that it was enough to add $SUDOCOMMAND, isnt it?

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 25, 2024

@Samueru-sama however, you said that it was enough to add $SUDOCOMMAND, isnt it?

ah, I forgot.... no sudo command

@Samueru-sama
Copy link
Contributor Author

@Samueru-sama however, you said that it was enough to add $SUDOCOMMAND, isnt it?

ah, I forgot.... no sudo command

$SUDOCOMMAND is enough if the single sudoers rule is added and AM has an AM-updater for itself.

However you didn't like the idea of having to edit sudoers so I mentioned the other way with groups. (which once again I haven't taken a look at how they work, I know they do because other apps use them).

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 25, 2024

@Samueru-sama on your fork, uploads are disabled, so download/extract this https://github.com/ivan-hc/AM/archive/refs/heads/dev.zip and add the content of "modules" in your branch

@Samueru-sama
Copy link
Contributor Author

Samueru-sama commented Apr 25, 2024

@Samueru-sama on your fork, uploads are disabled, so download/extract this https://github.com/ivan-hc/AM/archive/refs/heads/dev.zip and add the content of "modules" in your branch

Btw I just hit the jackpot:

echo "username ALL=NOPASSWD: /opt/*/AM-updater" | sudo tee -a /etc/sudoers

That does the sudoers change without having to use visudo. Of course username needs to be replaced by the actual name of the user first.

This can be a function in AM that uses sudo once to allow the user that called it to be able to update without password

@Samueru-sama
Copy link
Contributor Author

@Samueru-sama on your fork, uploads are disabled, so download/extract this https://github.com/ivan-hc/AM/archive/refs/heads/dev.zip and add the content of "modules" in your branch

Oh fuck I misread this and uploaded the entire zip lol.

@Samueru-sama
Copy link
Contributor Author

@Samueru-sama on your fork, uploads are disabled, so download/extract this https://github.com/ivan-hc/AM/archive/refs/heads/dev.zip and add the content of "modules" in your branch

Done.

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 25, 2024

@Samueru-sama what about

# ALLOW PERMISSIONS ONLY TO THE CORE FILES AND DIRECTORIES OF "AM", FOR ALL USERS OF "SUDO" GROUP
find /opt/am -exec chmod 775 {} \; 2> /dev/null
find /opt/am -exec chown -R :sudo {} \; 2> /dev/null

?

it seems to work in INSTALL

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 25, 2024

sync also worked

@Samueru-sama
Copy link
Contributor Author

Samueru-sama commented Apr 25, 2024

@Samueru-sama what about

# ALLOW PERMISSIONS ONLY TO THE CORE FILES AND DIRECTORIES OF "AM", FOR ALL USERS OF "SUDO" GROUP
find /opt/am -exec chmod 775 {} \; 2> /dev/null
find /opt/am -exec chown -R :sudo {} \; 2> /dev/null

?

it seems to work in INSTALL

If it works and you don't have some weird shenanigans changing the ownership of /opt/am between users. And AM updates itself without password after running echo "username ALL=NOPASSWD: /opt/*/AM-updater" | sudo tee -a /etc/sudoers then it is good.

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 25, 2024

I've finished tests on main user, now I'm going test this on other two

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 25, 2024

I've finished the tests on non privileged user, its all ok, him cannot update anything nor can remove anything

@Samueru-sama
Copy link
Contributor Author

I've finished the tests on non privileged user, its all ok, him cannot update anything nor can remove anything

By non privileged you mean not in wheel group?

I did not test if the NOPASSWD works for users not in the wheel group but that are still added to sudoers.

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 25, 2024

Updating AM and modules with the second administrator, I get these errors:

Istantanea_2024-04-26_00-03-38

@Samueru-sama
Copy link
Contributor Author

However, Ive done a test on a local file... now that we have another directory in cache, I'll test another approach

Any updates?

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 26, 2024

I've managed to update APP-MANAGER by downloading it in $CACHEDIR/am, set owner and group and then move

@Samueru-sama
Copy link
Contributor Author

Samueru-sama commented Apr 26, 2024

I've managed to update APP-MANAGER by downloading it in $CACHEDIR/am, set owner and group and then move

Damn that was a fast response 👀

I think the issue is that AM and new modules download themselves in $CACHEDIR and that messes the permissions of the files. They should download in /opt/am/tmp like the rest of applications do and then be moved back to their respective place, making sure that only sudo can do this .

Worth mentioning that before $CACHEDIR I also tested using /tmp as the CACHEDIR but it didn't work, because /tmp is usually mounted with permissions that only let the user that created the file to delete it themselves.

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 26, 2024

@Samueru-sama see here 318b2c7

@Samueru-sama
Copy link
Contributor Author

Samueru-sama commented Apr 26, 2024

@Samueru-sama see here 318b2c7

That's great but it wouldn't be needed if the modules downloaded themselves to /opt/am/tmp (it works anyway just pointing that out)

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 26, 2024

please, remember me that I must switch this later 03b8297

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 26, 2024

now lets go testing... in my zone is 2:30 A.M., I think your zone is about 22:00/10:00 P.M., so keep testing if I fall asleep

@Samueru-sama
Copy link
Contributor Author

now lets go testing... in my zone is 2:30 A.M., I think your zone is about 22:00/10:00 P.M., so keep testing if I fall asleep

I get this error when doing am -u

chown: invalid group: ‘:sudo’ 6.6.2-2...

Also I cannot do sudo am -u as it has the function that prevents that.

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 26, 2024

this is what happens if sync is called by a non-privileged user

Istantanea_2024-04-26_02-37-49

I think it will be better to prevent normal users using update and sync

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 26, 2024

now lets go testing... in my zone is 2:30 A.M., I think your zone is about 22:00/10:00 P.M., so keep testing if I fall asleep

I get this error when doing am -u

chown: invalid group: ‘:sudo’ 6.6.2-2...

Also I cannot do sudo am -u as it has the function that prevents that.

you must remove AM

am -R am

and then download the installer from dev

wget https://raw.githubusercontent.com/ivan-hc/AM/dev/INSTALL && chmod a+x ./INSTALL

open the file with a text editor and on top change the URL from main to dev, save and run

sudo ./INSTALL

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 26, 2024

The update is gone well using the other admin

Istantanea_2024-04-26_02-44-53

@Samueru-sama
Copy link
Contributor Author

now lets go testing... in my zone is 2:30 A.M., I think your zone is about 22:00/10:00 P.M., so keep testing if I fall asleep

I get this error when doing am -u
chown: invalid group: ‘:sudo’ 6.6.2-2...
Also I cannot do sudo am -u as it has the function that prevents that.

you must remove AM

am -R am

and then download the installer from dev

wget https://raw.githubusercontent.com/ivan-hc/AM/dev/INSTALL && chmod a+x ./INSTALL

open the file with a text editor and on top change the URL from main to dev, save and run

sudo ./INSTALL

Ok but this made am be owned by me instead of root?

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 26, 2024

And back to me again, other admin

Istantanea_2024-04-26_02-47-52

no issue here

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 26, 2024

Ok but this made am be owned by me instead of root?

The first owner should be root, and in my tests I've updated and I become the owner, then I re-updated with another admin and him become the owner... and back again, updated by my user and I become owner again

for all the abow, the group "sudo" is always RW

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 26, 2024

Now we need to add a filter to update and sync, that are useless for non-sudo users.

Would you like to provide one (in APP-MANAGER)?

@Samueru-sama
Copy link
Contributor Author

Now we need to add a filter to update and sync, that are useless for non-sudo users.

Would you like to provide one (in APP-MANAGER)?

You better do it because I'm a little confused now. lol.

Will the echo "$(whoami) ALL=NOPASSWD: /opt/*/AM-updater" | sudo EDITOR="tee -a" visudo to update without password be needed?

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 26, 2024

Will the echo "$(whoami) ALL=NOPASSWD: /opt/*/AM-updater" | sudo EDITOR="tee -a" visudo to update without password be needed?

nope

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 26, 2024

@Samueru-sama is this ok as a message to prevent unprivileged users of using some options?

guest is not in the sudoers file. This incident will be reported.

@Samueru-sama
Copy link
Contributor Author

@Samueru-sama is this ok as a message to prevent unprivileged users of using some options?

guest is not in the sudoers file. This incident will be reported.

It's ok.

nope

It's a shame because with the trick above even guest should be able to update AM even when it is owned by root.

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 26, 2024

3ab26c6

It's a shame because with the trick above even guest should be able to update AM even when it is owned by root.

Nope... and I'm still looking the english version of the agent Smith in the first Matrix saying that humans should not do the job of a machine, in the first shene

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 26, 2024

It's a shame because with the trick above even guest should be able to update AM even when it is owned by root.

oh-no

ways better

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 26, 2024

@Samueru-sama is this OK? Should I merge to main?

@Samueru-sama
Copy link
Contributor Author

@Samueru-sama is this OK? Should I merge to main?

If it works and fixes the issues that the users were having recently, then yeah.

(I haven't had issues on my end if you wonder)

@Samueru-sama
Copy link
Contributor Author

Samueru-sama commented Apr 26, 2024

@ivan-hc HEY DON'T GO TO SLEEP!

I just reinstalled am now that you merged on main and it doesn't work wut:

image

This is because I am not in the root group, but I am on the wheel group:

image

Edit: Also you can see how feral gamemode works there, it has its own group that can do certain sudo tasks.

FUCK @ivan-hc Respond!


The fix is simple and it is add this to ./INSTALL:

sudo usermod -aG root $(who | awk '{print $1}')

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 26, 2024

@Samueru-sama hey, I'm here, tell me. What should I do?

@Samueru-sama
Copy link
Contributor Author

@Samueru-sama hey, I'm here, tell me. What should I do?

RIGHT NOW, would be to add this to ./INSTALL

sudo usermod -aG root $(who | awk '{print $1}')

That adds the user to the root group.

Later on we to need make a better group for this, like in the case of gamemode where you can see that it makes a gamemode group, am can make a am group.

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 26, 2024

Would you like to PR?

@Samueru-sama
Copy link
Contributor Author

Would you like to PR?

done

@ivan-hc
Copy link
Owner

ivan-hc commented Apr 26, 2024

Would you like to PR?

done

OK, good night 😴 💤

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