Re: [PATCH] ddns.cgi: Drop static provider list for token based auth.

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 5321 bytes --]

Hi,

Alternatives I could come up with are:

* Create a command to have ddns list all those providers. That way that list is in ddns where it probably belongs best.

* Update the UI and add a third field for the token. That is however very bad UI because either the token field or the username/password field are a waste of space.

Actually, we need to work towards having token only. Anything else is just dangerous because the whole account can be managed if this data leaks.

-Michael

> On 2 Dec 2020, at 16:02, Stefan Schantl <stefan.schantl(a)ipfire.org> wrote:
> 
> Mhm,
> 
> you are right this will only move the problem from the developers (us)
> to the users.
> 
> This is a really hard problem because there is no easy solution for
> this issue.
> 
> We simple can take care about the users input and verify if the
> providers supports what has been configured or we even do not.
> 
> If we decide to safe the users from themself, we need something to
> verify against and that would be something like such a crappy static
> list.
> 
> Do I miss something or think to narrow?
> 
> @List followers, what are your opinions?
> 
> Best regards,
> 
> -Stefan
>> 
>> You are moving the problem from the developer to the user.
>> 
>> I think that might be more messy.
>> 
>> I am also not sure how this will affect existing setups?
>> 
>> -Michael
>> 
>>> On 2 Dec 2020, at 15:35, Stefan Schantl <stefan.schantl(a)ipfire.org>
>>> wrote:
>>> 
>>> Hello Michael,
>>> 
>>> the intension of this patch was, that a supported provider of DDNS
>>> has
>>> changed it's API to support now token based authentication. The
>>> code
>>> already has been changed so here we are safe by now.
>>> 
>>> Also in case a new dynamic DNS provider will be added to DDNS, we
>>> do
>>> not have to do anything, because the GUI automatically will be
>>> filled
>>> with the new provider.
>>> 
>>> But there is one big exception in this:
>>> 
>>> If a new or existing provider has supports authentication tokens,
>>> this
>>> provider has to be added to this list, which is not very intuitive.
>>> 
>>> We also have to ship the GUI each time for new, if this happened.
>>> 
>>> (This was the former reason, why we created the "ddns list-
>>> providers"
>>> command to prevent from this.)
>>> 
>>> My change simplifies, the check if token based authentication
>>> should be
>>> used or not. It removes the check against the static list of
>>> providers
>>> which supports token based authentications. (This is the list,
>>> which
>>> needs to be keep up to date by hand.)
>>> 
>>> For the user nothing changes, because if he speciefies "token" as
>>> username, still the token base auth will be used for the choosen
>>> provider.
>>> 
>>> The only way an user may be affected is, if he configures a token
>>> based
>>> auth for a provider which does not supports it. This now would be
>>> possible and will result in an authentication error against the
>>> provider - But Hey, if he fills in incorrect data the same will be
>>> happen.
>>> 
>>> The only bigger problem would be if the valid username of a user is
>>> "token". In this case he will be affected in the same way and this
>>> could not be fixed in an easy way.
>>> 
>>> IMHO, the benefits are bigger for us, because the maintain work
>>> will
>>> become much easier and one stupid error source will get eliminated.
>>> 
>>> Best regards,
>>> 
>>> -Stefan 
>>>> Hello,
>>>> 
>>>> I do not understand exactly what this patch is trying to achieve.
>>>> 
>>>> Unfortunately we have no choice than doing it this way with the
>>>> current UI.
>>>> 
>>>> I do not think it is worth altering the UI for this, and I do not
>>>> know how we could do it without having a list again?
>>>> 
>>>> -Michael
>>>> 
>>>>> On 2 Dec 2020, at 11:30, Stefan Schantl <
>>>>> stefan.schantl(a)ipfire.org>
>>>>> wrote:
>>>>> 
>>>>> This is really hard to maintain when adding new or altering
>>>>> existing
>>>>> providers.
>>>>> 
>>>>> Reference #12415.
>>>>> 
>>>>> Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
>>>>> ---
>>>>> html/cgi-bin/ddns.cgi | 8 ++++----
>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>> 
>>>>> diff --git a/html/cgi-bin/ddns.cgi b/html/cgi-bin/ddns.cgi
>>>>> index 715c37290..024eaf7f6 100644
>>>>> --- a/html/cgi-bin/ddns.cgi
>>>>> +++ b/html/cgi-bin/ddns.cgi
>>>>> @@ -665,13 +665,13 @@ sub GenerateDDNSConfigFile {
>>>>> 
>>>>> 		my $use_token = 0;
>>>>> 
>>>>> -		# Handle token based auth for various
>>>>> providers.
>>>>> -		if ($provider ~~ ["dns.lightningwirelabs.com",
>>>>> "entrydns.net", "regfish.com",
>>>>> -				  "spdns.de", "zzzz.io"] &&
>>>>> $username
>>>>> eq "token") {
>>>>> +		# Check if token based auth is configured.
>>>>> +		if ($username eq "token") {
>>>>> 			$use_token = 1;
>>>>> +		}
>>>>> 
>>>>> 		# Handle token auth for freedns.afraid.org and
>>>>> regfish.com.
>>>>> -		} elsif ($provider ~~ ["freedns.afraid.org",
>>>>> "regfish.com"] && $password eq "") {
>>>>> +		if ($provider ~~ ["freedns.afraid.org",
>>>>> "regfish.com"]
>>>>> && $password eq "") {
>>>>> 			$use_token = 1;
>>>>> 			$password = $username;
>>>>> 
>>>>> -- 
>>>>> 2.20.1
>>>>>