acme.sh icon indicating copy to clipboard operation
acme.sh copied to clipboard

Add docker oathtool option

Open OrpheeGT opened this issue 2 years ago • 4 comments

oathtool binary is not available in DSM6 or DSM7.

It needs to sideload binary and dependencies from debian or anything else to make it works...

As we have docker synology package available on most of Synology products, using a docker container is a good alternative.

This commit replace oathtool binary with docker run commandline. the first time the command is launched, it will take some time to download missing docker image locally.

OrpheeGT avatar Jan 28 '23 22:01 OrpheeGT

I'm not sure that straight up replacing oathtool with docker is the right choice.

  1. There are people that already have everything setup with oathtool and this will break their setup
  2. People may not want to install docker
  3. This will automatically pull the image without input from the user if they hit that code path

To me, those last 2 things are the major issue. I would instead leave the oathtool code as is, put a check to see if docker is installed, if it is and they have either pulled the image already or set some variable in the configuration, only then would I execute a command that can pull down an new image.

Also, bear in mind that this script does not have to be run on Synology host, while it is likely the primary use case, I've tried to make sure that it can be run on any networked machine that has access to the synology host. So getting oathtool may not be that big of an issue for many people.

tresni avatar Jan 30 '23 03:01 tresni

I restored oathtool binary option and added a prompt request to download image when docker is available

image

OrpheeGT avatar Jan 30 '23 10:01 OrpheeGT

I'm definitely more comfortable with that. Thanks for making the changes!

tresni avatar Jan 30 '23 21:01 tresni

send the pr to the dev branch. never ever send pr to master branch.

Neilpang avatar Jan 31 '23 08:01 Neilpang

@OrpheeGT sorry for being late to the party, but since #4646 was merged recently, there's no need for oathtool anymore.

While old setups continue to work as before, any new one uses Synology's API to get a device ID on first (manual) run, requiring just a one-time input of the TOTP code for the user account in question. Any subsequent run will then use said device ID instead of requiring yet another TOTP code or even storing the TOTP secret at all as the previous implementation did.

Therefore, IMHO, this PR is obsolete as there's neither a requirement for oathtool nor Docker at all anymore. Please, correct me if I'm wrong, though. 😅 Otherwise, this PR might be closed, reducing review workload for Neil as well. 😉

Eagle3386 avatar Sep 07 '23 20:09 Eagle3386

@OrpheeGT sorry for being late to the party, but since #4646 was merged recently, there's no need for oathtool anymore.

While old setups continue to work as before, any new one uses Synology's API to get a device ID on first (manual) run, requiring just a one-time input of the TOTP code for the user account in question. Any subsequent run will then use said device ID instead of requiring yet another TOTP code or even storing the TOTP secret at all as the previous implementation did.

Therefore, IMHO, this PR is obsolete as there's neither a requirement for oathtool nor Docker at all anymore. Please, correct me if I'm wrong, though. 😅 Otherwise, this PR might be closed, reducing review workload for Neil as well. 😉

Hello,

I guess you must be right. TBH I'm not using it at all. I have a traefik docker easier to manage/handle.

I originally made this fix/PR for a friend. and the other PR was not merged in this time...

So feel free to do what you consider the best.

OrpheeGT avatar Sep 08 '23 07:09 OrpheeGT

Well, I can't, because it's your PR & I'm only a contributor like you, not a maintainer.. 😅

Eagle3386 avatar Sep 08 '23 07:09 Eagle3386