search

neothematrix / noip-renew

Auto renew (confirm) noip.com free hosts
https://hub.docker.com/r/moebiuss/noip-renew
Apache License 2.0
54 stars 20 forks source link

sudo not required and creates complications #8

Closed benyjr closed 2 years ago

benyjr commented 2 years ago

This script was designed to be run by a non privileged user. A user doesn't need sudo to edit their own crontab. sudo prompts for a password, so if cron runs this in the background after a successful execution of /usr/local/bin/noip-renew-myusername.sh, it will wait for a password and never update crontab.

peterchs commented 2 years ago

Good PR dont like the need for sudo in this script.

Couple of comments:

neothematrix commented 2 years ago

sudo is needed if you run the setup.sh script passing a "user" parameter, if you look at the code the setup.sh modifies the noip-renew-skd.sh with sed :-D

neothematrix commented 2 years ago

probably a better fix is to check if $USER is not null and different than the current user, in that case, sudo is needed and the $SUDO variable needs to be defined, otherwise the $SUDO variables can be set to null and that will just skip sudo.

Angel0ffDeath commented 2 years ago

Well, everybody is free to customize locally all scripts he is using. But here the idea is to cover as much as possible cases (understand users). So personally I think the last comment from @neothematrix is the best solution.

peterchs commented 2 years ago

Dont think it needs the user check, I dont see the scenario where the cron would run as one user and be updating another user's cron? Looks like $USER would never be null, setup.sh takes it from whoami or if running as root then it'll take the user from the argument.

Tested this branch, its broken as per my 2nd point, my cron emails output to me and I get a "/bin/sh: 1: peteakalad: not found" error when the cron runs. Likewise the initial setup.sh inserted cron is broken for same reason.

But maybe environments are different, I'm running debian based linux - does the current setup.sh or skd script cron setup actually work for anyone with $USER in the crontab entry?

Angel0ffDeath commented 2 years ago

@peteakalad Hi Peter, I think you are correct - running the script as cronjob always fails by me - I made some modification for testing to reschedule script execution each 30 min. After login (which is correct) script fails. The other test was to remotely start the script from my home automation server - again each 30 min - in this case everything seems to be ok, so definitely cronjob doesn't work at the moment. It is possible the page not to be loaded in fully - you can try my suggestion under issue about sleep times and share your results. Personally I prefer to start the script "manually" from my home automation server. And for info - by me the script is running on RPi4 with debian buster (10.x) and my home automation server is on RPi3 with debian stretch (9.x). Concerning the users - yes, after some tests it is correct - we don't need that

neothematrix commented 2 years ago

I see @peteakalad point on $USER not being needed on CRONJOB and NEWCJOB if editing the current user crontab, I don't see how it's working for anyone or more probably, there could be different version of crontab. I'm not sure anyone is using this crontab feature to be honest, I don't currently use it or have an easy way to test it.

Angel0ffDeath commented 2 years ago

I see @peteakalad point on $USER not being needed on CRONJOB and NEWCJOB if editing the current user crontab, I don't see how it's working for anyone or more probably, there could be different version of crontab. I'm not sure anyone is using this crontab feature to be honest, I don't currently use it or have an easy way to test it.

You need sudo only to check/change cronjob of another user - this means you are lazy to type 'su another_-user' or log as correct user... And if you logged with the right user - you don't need sudo. However first work of the script as cronjob should repaired - as I said by me it doesn't work

benyjr commented 2 years ago

sudo is needed if you run the setup.sh script passing a "user" parameter, if you look at the code the setup.sh modifies the noip-renew-skd.sh with sed :-D

SUDO during the attended installation is one thing.

SUDO requests during unattended cron executions is another thing. By design and security SUDO times out, if cron runs an unintended background script (the whole point of the script) that uses sudo, the script will stop waiting for a password input that the user will not be aware of. If I need to be present everytime the script is run, I might as well renew my hostname myself.

sudo should remain in the setup.sh, but should be removed from noip-renew-skd.sh. I personally changed "SUDO=sudo" to "SUDO=" in my personal version and it works properly.

Angel0ffDeath commented 2 years ago

or you can add this particular user to sudoers file and no pass will be required... But in general as I said - sudo not required to run and modify cronjob.

benyjr commented 2 years ago

I would rather not be running commands in sudo if not needed. That is a huge security issue.

benyjr commented 2 years ago

For the time being, I have disabled sudo in my local scripts as a security precaution.

Angel0ffDeath commented 2 years ago

In general I agree with you - sudo not needed ,if you are on debian. However it seems the sripts are wriiten to cover some other kind of linux with which I'm not familiar. It is possible this to requires sudo. I have tested this on my RPi (buster) and on my laptop (Linux Mint Tricia) - sudo not needed Let others confirm....

benyjr commented 2 years ago

Ok, that makes sense. I'll close this pull request then.

Angel0ffDeath commented 2 years ago

Ok, that makes sense. I'll close this pull request then.

@benyjr Just do the script for yourself... And BTW if you use cronjob also remove $USER in the definition of cronjob. Cronjob definition - a b c d e command [optional log] - this is for debian. If you have username before command - on debian will not work - it will try to start a script (username) - Peter was writing on this... And actually the scripts - doesnt matter - the most important things are in python file, so just copy the latest .py file and don't care about the scripts (after you adjusted them for you :) )

benyjr commented 2 years ago

That's a good idea. I'll probably create my own private fork for that.