Add SYNO_USE_TEMP_ADMIN variable & Fix broken logic

[scruel]

Really? Do people who are willing to take the security risks of storing their passwords on their disk still exist?
Sure, sure, this year is only 2023, not 2077. 😜

1. Fix the broken logic, so that it can handle all kid of auth flows correctly. (Sorry for including this fix commit into this PR, I'm feeling quite tired and would like to go to sleep right away...)
2. Provides new method to obtain credential info, by creating a temp admin user via Synology CLI tools (so it can't work while running in the docker container) if SYNO_USE_TEMP_ADMIN is set, instead of requiring the user's own credentials which will be saved in disk.

I do really don't like to have plaintext credentials be saved in disk, and I noticed that you've spent a lot of time fighting with 2FA related stuffs, so why not just get rid of some parts of the old way. :)

[scruel]

@Eagle3386 Hi, consider I opened this PR two months ago, and haven't yet got any feedbacks, so I think it's better to mention somebody to push the progress, sorry for this if you feel disturbed.
Can you have some time to check this? Thanks!

[Eagle3386]

Mentioning me won't push progress - it's @Neilpang who owns this repo & therefore is solely in charge of accepting any PR.

Personally, I'm pretty much against your approach.
The code looks kinda "messy", e.g. it inverts the oathtool check & several comments don't really explain what you're trying to achieve.

Besides, what's the whole point of not storing the password on disk since without the TOTP secret, no external access is even possible due to no TOTP code provide upon login attempt?!
You can create a new user, prevented from login to any Synology app or service, yet granted the required rights for certificate renewal.

If ever, the whole account - name as well as its password - should be randomized & only the name should be stored, but just to be randomized upon each run again.
But yet again: what's the point? Where's the increased security? Nowhere, that's where!
TOTP secures enough, because removing the secret sufficiently prevents any attacker from a successful login.

[scruel]

@Eagle3386

The code looks kinda "messy", e.g. it inverts the oathtool check & several comments don't really explain what you're trying to achieve.

Can you add some comments by using code review tool to clarify your opinions? About the "invert?" you mentioned, I think what I did in this PR is just fixed the lack logic of your merged code... And I'd like to ask: did you even try to test all possible usages? e.g., if user have an account which don't have 2FA enabled? -- Currently it will keep requiring the OTP code even the account won't require it while logging in...

Besides, what's the whole point of not storing the password on disk since without the TOTP secret, no external access is even possible due to no TOTP code provide upon login attempt?!

To automatically renew certificates, I do think that most of the people will just set SYNO_Device_ID, rather than input OTP code manually every time when they need to do so.

You can create a new user, prevented from login to any Synology app or service, yet granted the required rights for certificate renewal.

First, the user must be created as an administrator to be able to "granted the required rights for certificate renewal.", and the admin users can easily gain root privileges via SSH, you can't prevent anything for this for now on DSM, unless you just disable SSH feature, for me, disable it will cause my own inconvenience. Secondly, I don't know what you are saying, because I don't know how to only grant the specific right to a user on DSM - without any other rights, could you please post an article to talk about "how to achieve" exactly? I really look forward to it. Thirdly, for this "new user", consider the only reason for creating it is "do the renewal job" rather than to use like a normal user, did you really want to see it in your control panel each time when you open the "user&group" tab? For me, the answer is obvious.

If ever, the whole account - name as well as its password - should be randomized & only the name should be stored, but just to be randomized upon each run again.

For above two advices you mentioned:

• "the whole account - name as well as its password - should be randomized":
  This can be easily done, but have you ever considered, if the user interrupted the execution of the script, what will happen? -- Because the "temporary admin user" cleanup thing will only be executed if the script exits on its own, so now the system will have many temporary users with the random usernames (let's consider that if the user does this by multiple times)...
  But we do can have some discussion about this, like: Which username should we use by default? Should it be customizable or not? etc.
• "only the name should be stored, but just to be randomized upon each run again.":
  Store such thing takes no side effects, so currently I think we don't need to add those few "meaningless" lines of code to test "should load/store or not", it just will make the source code of the script uglier...
  However, if the owner thinks it's necessary, then I will do that in the next commit.

But yet again: what's the point? Where's the increased security? Nowhere, that's where!
TOTP secures enough, because removing the secret sufficiently prevents any attacker from a successful login

Already replied, and if you want to have some arguments about whether it is acceptable to store your own passwords in plain text on your local storage, then you are all right, peace :)

[Eagle3386]

Can you add some comments via code review to clarify your opinions?

Nope, won't comment/review that much code - already used more time on this than initially intended.

About the "invert?" you mentioned, I think what I did in this PR is just fixed the logic you lack in your merged code...

Nope, you didn't. It's supposed to fail, because that's the previous way of using it.

And I'd like to ask: did you even try to test all possible usages? e.g., if user have an account which don't have 2FA enabled? -- Currently it will keep requiring the OTP code even the account don't require any to login...

Nope & I didn't even have to. Accounts without 2FA enabled were supposed to not be supported.
That's primarily due to the fact that Synology (almost) requires 2FA for admin accounts - which is more than absolutely correct to do so for such high privileged accounts anyway.

To automatically renew certificates, I do think that most of the people will just set SYNO_Device_ID, rather than input OTP code manually every time when they need to do so.

You basically got it wrong, right from the start - SYNO_Device_ID is set by the script, not you.
Nobody should ever fiddle with cookies to get the DID, in fact, nobody even likes that - the new code respects this & even simplifies the process: enter 1 TOTP code on 1st manual run, use automation via Task Scheduler from there on.

First, the user must be created as an administrator to be able to "granted the required rights for certificate renewal.", and the admin users can easily gain root privileges via SSH. You can't prevent anything for this for now on DSM, unless you just disable SSH feature, for me, disable it will cause my own inconvenience.

First, your inconvenience is not those of others, let alone "everybody".
Second, using the script (& following the correct initial procedure) requires no further console/shell interaction from there on, hence SSH can be safely disabled (BTW: even enhances security) up until needed in the future where one could re-enable it again.

(…) Thirdly, for this "new user", consider the only reason for creating it is "do the renewal job", did you really want to see it in your control panel each time you open the "user&group" tab? For me, the answer is obvious.

Are you serious? An entry like "CertRenewUser" bothers you?
If so, please stop talking to me. Ever again!

(…)
This can be easily done, but have you ever considered, if the user interrupted the execution of the script, what will happen? -- Because the "temporary admin user" cleanup thing will only be executed if the script exits on its own, so now the system will have many temporary users with the random usernames (let's consider that if the user does this by multiple times)...

First & foremost, "your" script should be stable enough to handle that situation.
Besides, those temporary users could be easily deleted - even via DSM's web UI - and that's because the admin(s) would still be notified about the failed script run.

But we do can have some discussion about this, like: Which username should we use by default? Should it be customizable or not? etc.

Again, I won't discuss this, as I dislike/oppose the whole purpose you're after with this PR.

Store such thing takes no side effects, so currently I think we don't need to add those few "meaningless" lines of code to test "should load/store or not", it just will make the script become uglier, but if the owner thinks it's necessary, then I will do that in the next commit...

Honestly, the script would be way uglier with your PR than with such handful lines.

Already replied, and if you want to have some arguments about either store your own passwords in the plain text is acceptable or not, then you are all right, peace :)

I don't store my passwords in plaintext, but happily that of a darn "renewal user", in a directory only accessible by root himself & nobody else.
Especially, if any such account is secured by 2FA - which pretty much every sane admin does with such users.

Besides, you seem to still just not getting it & that's why I unsubscribe from this now.

[scruel]

Nope, won't comment/review that much code - already used more time on this than initially intended.

Hmm, how nice are you.

Nope, you didn't. It's supposed to fail, because that's the previous way of using it.
Nope & I didn't even have to. Accounts without 2FA enabled were supposed to not be supported.
That's primarily due to the fact that Synology (almost) requires 2FA for admin accounts - which is more than absolutely correct to do so for such high privileged accounts anyway.
You basically got it wrong, right from the start - SYNO_Device_ID is set by the script, not you.
Nobody should ever fiddle with cookies to get the DID, in fact, nobody even likes that - the new code respects this & even simplifies the process: enter 1 TOTP code on 1st manual run, use automation via Task Scheduler from there on.
Second, using the script (& following the correct initial procedure) requires no further console/shell interaction from there on, hence SSH can be safely disabled (BTW: even enhances security) up until needed in the future where one could re-enable it again.

Then why not in your code, you didn't explain anything about your "default" failure by printing some error messages?
And let alone "nobody", if you don't want anyone to use this SYNO_Device_ID option, why you still added it? Oh, might you can say that you are not the one who added it, then why while you were changing the doc of this script, you still added it into "Optional exports"?
Your simplification ideas about this are fine and cool, but if you decided to keep providing the way, then you'd better not to consider that "nobody" will not use the way you provided...

Are you serious? An entry like "CertRenewUser" bothers you?

Hmm, what you are talking about? Looks like that you just replied even you don't know what I said, and don't want to have a normal conversation to ask...

If so, please stop talking to me. Ever again!

Hmm, how nice are you. x2

First, your inconvenience is not those of others, let alone "everybody".
Nobody should ever fiddle with cookies to get the DID, in fact, nobody even likes that - the new code respects this & even simplifies the process: enter 1 TOTP code on 1st manual run, use automation via Task Scheduler from there on.

You can do what ever you want, but when you blame someone, you'd better not to do the same thing in the same post, and hope you also can keep remembering "let alone everybody" in your mind.
Like I said before, for me, its just very inconvenience.

Again, I won't discuss this, as I dislike/oppose the whole purpose you're after with this PR.

Hmm, how nice are you. x3

Honestly, the script would be way uglier with your PR than with such handful lines.

Hmm, how nice are you. x4

I don't store my passwords in plaintext, but happily that of a darn "renewal user", in a directory only accessible by root himself & nobody else.
Especially, if any such account is secured by 2FA - which pretty much every sane admin does with such users.

Oh, I see, so you just consider that store the info (username & password) of your "renewal user" in plaintext is acceptable, but have you ever did the thing - "only granted the required rights for certificate renewal." - you mentioned before? If so, then I think your system is quite secure, even consider that the script will store SYNO_Device_ID (which we all know that this can be used for bypassing OTP) in the same plaintext file later - "SYNO_Device_ID is set by the script".

Besides, you seem to still just not getting it & that's why I unsubscribe from this now.

Neither are you. Thank you for canceling your subscription, really.

@Eagle3386 Little advice: try to be more friendly, and don't make excuses for yourself, everyone will make many mistakes in their whole life, and it doesn't matter at all, as long as they can admit them, then do the correction.

[winromulus]

While it was relatively fun and distressing reading all of this, @scruel I would very much be against this PR because it assumes this is running while installed directly on the OS and not in a docker container (as a LOT of users do). This is very much not ideal and should not be merged because it would break a lot of installations.

I kind of agree with @Eagle3386 on one point (I didn't bother to read the entire ... review fist-fight?) and that is the fact that this PR solves the problem in this particular case only with a bit of a hack (sure, it minimizes the window of opportunity for an attack, but it does leave artifacts for that user around and only works when locally installed).

Edit and PS: I would be interested in @Eagle3386 's comment that you can limit the user to cert renewals only. Reading the API docs from Synology, I haven't been able to find anything that would grant access only to that part of the API.

[scruel]

It won't break anything, if you check the code, you can see that anyone who are prefer to use docker can still use it in the docker, however, they have to use it with the old ways which will require their admin user's password.

[scruel]

Edit and PS: I would be interested in @Eagle3386 's comment that you can limit the user to cert renewals only. Reading the API docs from Synology, I haven't been able to find anything that would grant access only to that part of the API.

I don't think it's possible ATM on DSM, based on the whole researchs I've made for this.
Before this guy post something about it, I can now only think that he is bragging, however, consider that he can't be nice in this community, I don't think he will.

[Neilpang]

can anyone explain the synology dsm device to me? Or give me a link to amazon.com? so that I know what it is.

[scruel]

can anyone explain the synology dsm device to me? Or give me a link to amazon.com? so that I know what it is.

Check:
https://www.synology.com/dsm
https://hub.docker.com/r/kroese/virtual-dsm

[scruel]

Hi @Neilpang, happy Loong Year! I'd like to ask, do we have any update on this PR? Could I do anything to push the progress? Thanks!

[Neilpang]

thanks, here you go.